-
Notifications
You must be signed in to change notification settings - Fork 248
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
Update golang to 1.20.7 #2884
Update golang to 1.20.7 #2884
Conversation
Use the cdi builder which is configured with golang 1.20.7 Updated some libraries to the latest version, specifically excluding k8s and runtime-controller due to issues that need to be resolved outside of this commit. Signed-off-by: Alexander Wels <awels@redhat.com>
Fix generate verify issue Signed-off-by: Alexander Wels <awels@redhat.com>
Signed-off-by: Alexander Wels <awels@redhat.com>
Signed-off-by: Alexander Wels <awels@redhat.com>
9bd161d
to
0c770da
Compare
@@ -14,7 +14,7 @@ import ( | |||
// DefaultResponseMimeType is DEPRECATED, use DefaultResponseContentType(mime) | |||
var DefaultResponseMimeType string | |||
|
|||
// PrettyPrintResponses controls the indentation feature of XML and JSON serialization | |||
//PrettyPrintResponses controls the indentation feature of XML and JSON serialization |
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.
I suppose this fix is related to your pre-commit hook, right? Is removing that whitespace the expected thing? Seems odd since it's present in all other comments.
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.
So the vendor does not have the white space, and every time I saved it would add it. Which would break the module verification because something is different between the way I saved and and the way it gets pulled. We shouldn't affect the vendor directory at all IMO, so I kept it as we get it from a go mod vendor.
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.
Gotcha, looks good to me then. However, what do you mean by
k8s and runtime-controller due to issues that need to be resolved
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.
So there is a bug in the controller-runtime library from version 0.15 and newer that is not fixed yet kubernetes-sigs/controller-runtime#2425 is the fix, but it is not merged. So until that is merged we cannot upgrade. For kubernetes a bunch of function arguments changed and some behavior that I didn't want to mess with in this PR.
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.
for future ref this is how this (and similar other issues) are overcome in 0.15 https://github.com/kubevirt/hyperconverged-cluster-operator/pull/2333/files#r1200311040
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akalenyu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Use the cdi builder which is configured with golang 1.20.7 Updated some libraries to the latest version, specifically excluding k8s and runtime-controller due to issues that need to be resolved outside of this commit.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: