-
Notifications
You must be signed in to change notification settings - Fork 324
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
Bump to go 1.17 #878
Bump to go 1.17 #878
Conversation
@@ -1,3 +1,4 @@ | |||
//go:build enterprise |
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'm sure there is a reason but why do we need the duplicate enterprise build declaration? Is the old one left for backward compatibility?
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.
This stack overflow was good context on how they interplay-- https://stackoverflow.com/questions/68360688/whats-the-difference-between-gobuild-and-build-directives?noredirect=1&lq=1
We can leave both which in theory would be backwards compatible but we have bumped the min go version to 1.17 anyways so we could probably just remove the old // +build
lines. I think that's probably the cleanest thing to do, does that sound good?
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 the info and link!!
I was thinking that if we're bumping the min go version there would be no need for back compatibility lines? If its too much work there's very little to be gained from it, but might be cleaner. Will defer to to your judgement!
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.
yup I agree!
) | ||
|
||
require ( |
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.
Could we merge these require blocks?
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 1.17 groups the require blocks as indirect and direct, and since this is generated I think we should leave it as it was generated via go mod tidy
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 explaining!
) | ||
|
||
require ( |
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.
Could we merge these require blocks?
) | ||
|
||
go 1.16 | ||
require ( |
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.
Here too, please?
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.
Looks great! I just added the same note three times about merging require blocks. I don't know if there is a reason they should be separate. If so, feel free to ignore.
…o 1.17 environment
The control-plane module go fmt with 1.17 will error if you only ran go fmt with 1.16, so the minimum working version should be bumped to 1.17. The other 2 modules were bumped to 1.17 for consistency. The go.mod files themselves changed because in go 1.17 the indirect imports are in a separate section.
Since the minimum supported go version is 1.17, we can remove build directives compatible with older versions. For more info see: https://stackoverflow.com/questions/68360688/whats-the-difference-between-gobuild-and-build-directives?noredirect=1&lq=1
2a4e02f
to
2ca1261
Compare
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.
Great stuff, nothing blocking from me, thanks for taking care of this one!
Does it make sense to also update the CONTRIBUTNG.md? It currently says go-1.11.4+
:-)
yup definitely, thanks!!! |
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: