-
Notifications
You must be signed in to change notification settings - Fork 69
ci(pvc): upgrade Go to 1.24 for pvcviewer-controller #734
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
base: notebooks-v1
Are you sure you want to change the base?
ci(pvc): upgrade Go to 1.24 for pvcviewer-controller #734
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
a9a367a to
b794c32
Compare
- Update CI workflow pvcviewer_controller_unit_test.yaml to use go-version 1.24 - Update go.mod to go 1.24 - Update Dockerfile GOLANG_VERSION to 1.24 Fixes kubeflow#723 Signed-off-by: Hen Schwartz <[email protected]>
b794c32 to
77c656d
Compare
|
Thanks Hen, I reviewed the code and was able to deploy the component and create a volume. I also verified that there are no changes to One small comment: please add /lgtm |
|
Hey @henschwartz, following my earlier review, I noticed one more thing that could be improved. It’s not mandatory, but it would make the code cleaner.
I checked our environment and the module cache is writable, so We can remove these flags, though it’s not required for the PR approval. |
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.
Hey @henschwartz - thanks for this contribution 💯
While I have tested the code in my own Kind cluster and feel confident about the changes - I'm hoping we can use this PR (opportunity) to improve the flexibility of the unit test configuration to make it less redundant .
If you can code and verify the simple suggestion I provide below - I will feel good about lgtming the PR quickly afterwards.
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: "1.22.2" | ||
| go-version: "1.24" |
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.
| go-version: "1.24" | |
| go-version-file: 'components/pvcviewer-controller/go.mod' |
Given we already need to adjust this code block - I think we would be better served to change it more substantially to avoid having to hardcode (and couple) this go-version input with the related go.mod file
You can see this pattern employed in notebooks-v2 code - and I think we should have notebooks-v1 follow suit! Will make any future updates easier - as we only need to modify go.mod and the GH workflow will then automatically pick up the right version.
- https://github.com/kubeflow/notebooks/blob/notebooks-v2/.github/workflows/ws-backend-test.yml#L31
- note: for now lets NOT include the
cacherelated key here - even though it is present innotebooks-v2
- note: for now lets NOT include the
|
/ok-to-test |






[TASK] Upgrade Go to 1.24 - pvcviewer-controller component #723