Skip to content
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

Merged
merged 5 commits into from
Nov 30, 2021
Merged

Bump to go 1.17 #878

merged 5 commits into from
Nov 30, 2021

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Nov 24, 2021

Changes proposed in this PR:

How I've tested this PR:

  • The pipeline build steps and acceptance tests pass

How I expect reviewers to test this PR:

  • review

Checklist:

  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@ndhanushkodi ndhanushkodi marked this pull request as ready for review November 29, 2021 17:22
@ndhanushkodi ndhanushkodi requested review from a team, kschoche and t-eckert and removed request for a team November 29, 2021 17:22
@@ -1,3 +1,4 @@
//go:build enterprise
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup I agree!

Comment on lines 14 to +16
)

require (
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining!

Comment on lines 23 to +25
)

require (
Copy link
Contributor

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?

Comment on lines 28 to +30
)

go 1.16
require (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too, please?

Copy link
Contributor

@t-eckert t-eckert left a 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.

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
Copy link
Contributor

@kschoche kschoche left a 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+ :-)

@ndhanushkodi
Copy link
Contributor Author

Does it make sense to also update the CONTRIBUTNG.md? It currently says go-1.11.4+ :-)

yup definitely, thanks!!!

@ndhanushkodi ndhanushkodi merged commit 503d7d8 into main Nov 30, 2021
@ndhanushkodi ndhanushkodi deleted the bump-to-go-1.17 branch November 30, 2021 01:28
@jmurret jmurret mentioned this pull request Jul 5, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants