Merged
Conversation
Multiple changes were made to pass linting. Some Go built-in names are being used for variables (e.g., min). This happens in the Go source itself including the Go standard library and is not always a bad practice. To handle allowing some built-in names to be used the linter config is updated to allow (via opt-in) some names to pass. This allows us to still check for re-use of Go built-in names and opt-in to any new uses. There were also several cases where a value was checked for nil before checking its length when this is already handled by len() or the types default value. These were cleaned up. The license validation was updated because it was checking everything in the .git directory including all remote content that was local. The previous vendor directory was from a time prior to Go modules when Helm handled dependencies differently. It was no longer needed. Signed-off-by: Matt Farina <matt.farina@suse.com>
Member
|
Linting is failing. |
335c2b0 to
face4b2
Compare
Signed-off-by: Matt Farina <matt.farina@suse.com>
face4b2 to
66f84e5
Compare
robertsirc
approved these changes
Dec 19, 2024
Member
robertsirc
left a comment
There was a problem hiding this comment.
The vast majority of this is to upgrade us to Go 1.23, I do have a question about changes to some conditional if statements.
| case *batchv1.Job: | ||
| selector, err = metav1.LabelSelectorAsSelector(t.Spec.Selector) | ||
| case *corev1.Service: | ||
| if t.Spec.Selector == nil || len(t.Spec.Selector) == 0 { |
Member
There was a problem hiding this comment.
Are we ok with removing the check if spec selector is nil? will this break functionality?
Collaborator
Author
There was a problem hiding this comment.
t.Spec.Selector is of type map[string]string. When there is nothing in the slice (the default value) len should return 0.
The linter catching these is the reason I changed them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multiple changes were made to pass linting. Some Go built-in names are being used for variables (e.g., min). This happens in the Go source itself including the Go standard library and is not always a bad practice.
To handle allowing some built-in names to be used the linter config is updated to allow (via opt-in) some names to pass. This allows us to still check for re-use of Go built-in names and opt-in to any new uses.
There were also several cases where a value was checked for nil before checking its length when this is already handled by len() or the types default value. These were cleaned up.
The license validation was updated because it was checking everything in the .git directory including all remote content that was local. The previous vendor directory was from a time prior to Go modules when Helm handled dependencies differently. It was no longer needed.
What this PR does / why we need it: This is needed to update to the latest k8s packages as they require Go 1.23
Special notes for your reviewer: Most of the changes are to handling new linting messages when Go was updated.
If applicable:
docs neededlabel should be applied if so)