Skip to content

Fix CI failures, upgrade to Go 1.14+ #3187

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

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

jdolitsky
Copy link
Contributor

@jdolitsky jdolitsky commented Jun 26, 2020

  • Modify Travis manifest to use Go 1.14.x
  • Hardcode version of golangci-lint in setup script
  • Hardcode version of go-md2man in setup script

@jdolitsky jdolitsky changed the title Fix golanglint-ci issue in Travis Fix golangci-lint issue in Travis Jun 26, 2020
- Modify Travis manifest to use Go 1.14.x
- Hardcode version of golangci-lint in setup script
- Hardcode version of go-md2man in setup script

Signed-off-by: jdolitsky <393494+jdolitsky@users.noreply.github.com>
@jdolitsky jdolitsky changed the title Fix golangci-lint issue in Travis Fix CI failures, upgrade to Go 1.14+ Jun 26, 2020
@jdolitsky
Copy link
Contributor Author

@thaJeztah regarding your comments-

Perhaps inline this for the lines that need this instead of exporting if we only want it for golangci-lint?

The remainder of the script is just go get's so it was intentional to apply it globally.

Regarding why the -u was removed - this is actually what was causing the failure. The -u does not prevent go from grabbing the latest/master branch why may be broken in some way. I found this explanation in the golangci-lint website: https://golangci-lint.run/usage/install/#ci-installation

Note: such go get installation aren't guaranteed to work. We recommend using binary installation.

Why?
go get installation isn't recommended because of the following points:

some users use -u flag for go get, which upgrades our dependencies. Resulting configuration wasn't tested and isn't guaranteed to work.
go.mod replacement directive doesn't apply. It means a user will be using patched version of golangci-lint if we use such replacements.
it's stability depends on a user's Go version (e.g. on this compiler Go <= 1.12 bug).
we've encountered a lot of issues with Go modules hashes.
it allows installation from master branch which can't be considered stable.
it's slower than binary installation

I figured this same logic applies to go-md2man as well. If you think we should instead use golanglint-ci's recommended installation procedure (curl to bash), I'm happy to make just that modification.

Additionally, the latest version of golangci-lint (v1.27.0) appears to require Go 1.14, so this was upgraded as well. Let me know if you prefer any changes after this explanantion.

@thaJeztah
Copy link
Member

The remainder of the script is just go get's so it was intentional to apply it globally.

Oh! I see I commented before oyu made the final changes, yes, makes sense

Regarding why the -u was removed - this is actually what was causing the failure.

Ah, gotcha, thanks!

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

ping @manishtomar @dmcgowan PTAL

Copy link
Contributor

@manishtomar manishtomar left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 62d0fd4 into distribution:master Jun 29, 2020
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