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

Upgrade golang to v1.16 #3233

Merged
merged 5 commits into from
Mar 16, 2021
Merged

Upgrade golang to v1.16 #3233

merged 5 commits into from
Mar 16, 2021

Conversation

mctofu
Copy link
Contributor

@mctofu mctofu commented Mar 4, 2021

This allows use of the new embed package added in 1.16.

There's also a few notable updates for modules:

The go mod vendor and go mod tidy subcommands now accept the -e flag, which instructs them to proceed despite errors in resolving missing packages. (noted here)

Module-aware mode is enabled by default, regardless of whether a go.mod file is present in the current working directory or a parent directory. More precisely, the GO111MODULE environment variable now defaults to on.

We could omit GO111MODULE=on from the helper scripts.

@mctofu mctofu requested a review from a team as a code owner March 4, 2021 23:03
@xlgmokha
Copy link
Contributor

xlgmokha commented Mar 4, 2021

#3235 should help fix the failing tests.

@mctofu
Copy link
Contributor Author

mctofu commented Mar 5, 2021

The tests are showing a behavior change when there is a reference to a package that has been renamed. In the test we are expecting that go mod tidy would complain about this but we are now seeing it fail prior to that at the go get -d step.

$ go get -d
github.com/dependabot/vgotest imports
        github.com/googleapis/gnostic/OpenAPIv2: cannot find module providing package github.com/googleapis/gnostic/OpenAPIv2

@dependabot dependabot deleted a comment from lovemybabygirl42000 Mar 5, 2021
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

🎉
Aside: https://github.com/thepwagner/action-update-dockerurl supports the Dockerfile syntax we're using for this dependency, and https://github.com/thepwagner/action-update-brewformula/blob/main/brew/golang.go is gnarly, but handles parsing https://golang.org/dl/?mode=json - we could automate these bumps!

I'd dig some tests that cover the new retract directive too, e.g. to ensure Dependabot won't propose an update to a retracted version. As noted, there are a lot of module goodies in 1.16.

mctofu added 3 commits March 8, 2021 11:50
Allows use of the new embed feature.
Previously this error was only occuring with `go mod tidy` and was being
ignored. With go 1.16 the error is triggered during `go get` so we need to
handle it again.

Distinguishing the error from GitDependenciesNotReachable also took some
extra effort. The error message has changed from:
github.com/dependabot/vgotest imports
	github.com/googleapis/gnostic/OpenAPIv2: module github.com/googleapis/gnostic@latest found (v0.5.1), but does not contain package github.com/googleapis/gnostic/OpenAPIv2

to:
github.com/dependabot/vgotest imports
        github.com/googleapis/gnostic/OpenAPIv2: cannot find module providing package github.com/googleapis/gnostic/OpenAPIv2

We now use `go list` to check that github.com/googleapis/gnostic is a
reachable repo/module.
@mctofu mctofu force-pushed the mctofu/go-v1.16 branch from 74c33ec to 1d72d70 Compare March 8, 2021 19:51
@mctofu
Copy link
Contributor Author

mctofu commented Mar 8, 2021

I pushed an update to fix the failing test that is worth re-reviewing: f94051a.

The issue is that go get -d now fails if the code references a package that can't be found so I've reverted to our previous behavior of raising DependencyFileNotResolvable (#2627). However, since the new error message doesn't differentiate between the module not existing vs existing but not providing the package I also had to make some adjustments to prevent the error from raising GitDependenciesNotReachable instead. I found that running go list -m -versions #{repo_path} was a workable way to test that a module's repo was reachable and was potentially lighter weight than using go get since it doesn't download the module's dependencies.

@mctofu
Copy link
Contributor Author

mctofu commented Mar 8, 2021

I'd dig some tests that cover the new retract directive too, e.g. to ensure Dependabot won't propose an update to a retracted version.

@thepwagner: I added a test that we won't strip the retract directive from a go.mod. Avoiding retracted versions isn't working yet. I think we need to update the extracted go module code to get the latest behavior updates there.

@mctofu mctofu requested review from thepwagner and xlgmokha March 8, 2021 23:12
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

I think we need to update the extracted go module code to get the latest behavior updates there.

I assumed since 0.4.1 included golang/mod@c0d644d#diff-320fe6a1bc7a6321d706e6c7fc95d457cd5ea1b13425b320d56897c78074b350 , we'd have the necessary bits to update the helpers.

Dockerfile Outdated Show resolved Hide resolved
@mctofu
Copy link
Contributor Author

mctofu commented Mar 12, 2021

I assumed since 0.4.1 included golang/mod@c0d644d#diff-320fe6a1bc7a6321d706e6c7fc95d457cd5ea1b13425b320d56897c78074b350 , we'd have the necessary bits to update the helpers.

Aha! I had been hoping that modfetch.Lookup("direct", args.Dependency.Name) would have already filtered retracted versions in 1.16. That doesn't seem to be the case so we will need to filter on our end.

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