-
Notifications
You must be signed in to change notification settings - Fork 276
Upgrade Go version 1.23 to 1.25 #1234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // TimeWithinRange fails the test if act is not after lowerBound or not before upperBound | ||
| func TimeWithinRange(tb testing.TB, act time.Time, lowerBound time.Time, upperBound time.Time) { | ||
| if !(act.After(lowerBound) && act.Before(upperBound)) { | ||
| if !act.After(lowerBound) || !act.Before(upperBound) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious about changes like the above and also in few files like modifying the corev1 to v1, is it because of lint failures or for 1.25 version you need these exactly like how you modified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every change that I made was due to the lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, then looks good to me
| return nil | ||
| } else if err != nil { | ||
| return fmt.Errorf("Encountered an error while checking if the node is unschedulable. Not setting an uncordon label: %w", err) | ||
| return fmt.Errorf("encountered an error while checking if the node is unschedulable. not setting an uncordon label: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: semicolon or leaving it will be better having a period and then followed by small letter doesn't look good in error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make that into a semicolon. We can't have capitalized letters or the lint will fail.
| asgLaunchHandler := launch.New(interruptionEventStore, *node, nthConfig, metrics, recorder, clientset) | ||
| drainCordonHander := draincordon.New(interruptionEventStore, *node, nthConfig, nodeMetadata, metrics, recorder) | ||
|
|
||
| InterruptionLoop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing :) now it makes sense like what you mentioned earlier about loop not exiting.
LikithaVemulapalli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Description of changes:
Upgraded GO version to 1.25
Updated golangci-lint-action version from v3 -> v7 for the validation step - Lint Eastwood. v3 uses go version 1.24, so cannot run the test.
How you tested your changes:
Environment (Linux / Windows): make unit-test
Notes:
All code changes don't modify the logic, except node-termination-handler.go. In the file, the existing break statement doesn't do anything. It only exits the select statement, which is the same with not having the break statement. Referring to the comment, it seems like the author wanted to exit the outer loop. Please take a careful look into this part when reviewing.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.