-
Notifications
You must be signed in to change notification settings - Fork 256
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 Go module version to 1.17 #1222
Conversation
Is there some reason we can't just move everything to go 1.17? |
@katiewasnothere What's the reason for wanting to if this is sufficient? Allows a broader range of go versions to build this project. |
lgtm 👍🏼 |
What do we need a broader range for? |
@katiewasnothere We don't need a broader range, but it may be annoying if someone was working on this to pull the latest from master and find they may need to upgrade their go install to build certain bits because we want to adopt only 1.17 build tags. If we're not using any 1.17 features, and this approach works fine then this seems alright. ctrd and k8s have done the same so far, although k8s lists 1.16 as the minimum version but iirc it only builds on 1.17 😂 |
Although it would be nice to up our required version to 1.16 at least so we can use the io/fs stuff.. |
what's the status here @dcantah? |
@katiewasnothere Was waiting for a second sign-off, but now it's behind the times. I can rebase |
I mean if we're thinking of updating to min 1.16 so we can use the io/fs package, why not update to 1.17 too? |
@katiewasnothere Was mentioning it'd be nice but ehh, I don't really mind anymore on upgrading to 1.17 but I think it makes more sense to just update it to the version that it absolutely needs to be on, and we don't use any 1.17 features as of now (we don't use any 1.16 features either but I can think of a few places that we could change). All of the projects I've seen that have made the jump to 1.17 however just ran gofmt on 1.17 and had it add the new build tags however. No one seems to have gotten rid of the old syntax https://cs.github.com/?q=%22%2F%2Fgo%3Abuild%22 |
@dcantah can you update this and then I'll rereview? |
@katiewasnothere Yep, sorry Hamza had messaged the same but I've been swamped today. I'll just update us to 1.17 at this point. |
This change runs gofmt on Go 1.17 in /internal, /cmd, /hcn, and /test. Go 1.17 added a new syntax for build tags that gofmt automatically applies. The new format will just be ignored on older builds and 1.17 and up will prefer the new syntax (which is much nicer if I may add), so this should be harmless. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
This change updates the go directive from 1.13 to 1.17 Signed-off-by: Daniel Canter <dcanter@microsoft.com>
Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@katiewasnothere Done |
@katiewasnothere @helsaawy Does this look alright? |
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
Update Go module version to 1.17 This change does three things: 1. Runs gofmt on Go 1.17 in /internal, /cmd, /hcn, and /test. Go 1.17 added a new syntax for build tags that gofmt automatically applies. The new format will just be ignored on older builds and 1.17 and up will prefer the new syntax (which is much nicer if I may add), so this should be harmless. 2. Updates the go directive in our go.mod to be 1.17. We don't currently make use of any 1.17 features but we plan to (as well as the 1.16 io/fs additions). 3. Runs go mod vendor in the root of the repo and in /test for the new module graph pruning (https://go.dev/ref/mod#graph-pruning) Signed-off-by: Daniel Canter <dcanter@microsoft.com>
Update Go module version to 1.17 This change does three things: 1. Runs gofmt on Go 1.17 in /internal, /cmd, /hcn, and /test. Go 1.17 added a new syntax for build tags that gofmt automatically applies. The new format will just be ignored on older builds and 1.17 and up will prefer the new syntax (which is much nicer if I may add), so this should be harmless. 2. Updates the go directive in our go.mod to be 1.17. We don't currently make use of any 1.17 features but we plan to (as well as the 1.16 io/fs additions). 3. Runs go mod vendor in the root of the repo and in /test for the new module graph pruning (https://go.dev/ref/mod#graph-pruning) Signed-off-by: Daniel Canter <dcanter@microsoft.com>
This change does three things:
Runs gofmt on Go 1.17 in /internal, /cmd, /hcn, and /test. Go 1.17 added a new syntax for build tags that gofmt automatically applies. The new format will just be ignored on older builds and 1.17 and up will prefer the new syntax (which is much nicer if I may add), so this should be harmless.
Updates the
go
directive in our go.mod to be 1.17. We don't currently make use of any 1.17 features but we plan to (as well as the 1.16 io/fs additions).Runs go mod vendor in the root of the repo and in /test for the new module graph pruning (https://go.dev/ref/mod#graph-pruning)