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 to Go 1.18 #3200

Merged
merged 4 commits into from
Jul 26, 2022
Merged

Upgrade to Go 1.18 #3200

merged 4 commits into from
Jul 26, 2022

Conversation

epk
Copy link
Contributor

@epk epk commented Jul 22, 2022

Signed-off-by: Aditya Sharma git@adi.run

Description of your changes

  • Upgrades Go to 1.18
  • Fixes some minor lint errors

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

epk added 3 commits July 21, 2022 21:34
Signed-off-by: Aditya Sharma <git@adi.run>
Signed-off-by: Aditya Sharma <git@adi.run>
Signed-off-by: Aditya Sharma <git@adi.run>
@@ -265,7 +265,7 @@ func (c *installProviderCmd) Run(k *kong.Context, logger logging.Logger) error {
}

func warnIfNotFound(err error) error {
serr, ok := err.(*apierrors.StatusError)
serr, ok := err.(*apierrors.StatusError) //nolint:errorlint // we need to be able to extract the underlying typed error
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we need the nolint here. Could you elaborate?

Copy link
Contributor Author

@epk epk Jul 22, 2022

Choose a reason for hiding this comment

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

It was a lint error asking to use errors.As but that only returns a bool and we'd still have to cast it to *apierrors.StatusError later on

cmd/crank/install.go:268:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
	serr, ok := err.(*apierrors.StatusError)

@@ -264,10 +264,10 @@ metadata:
name: vault-default
spec:
type: Vault
# defaultScope used for scoping secrets for cluster scoped resources.
# For example, secrets for MRs will land under
# defaultScope used for scoping secrets for cluster scoped resources.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these unrelated changes? Can we discard them?

github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=
github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=
github.com/coreos/go-systemd v0.0.0-20190620071333-e64a0ec8b42a/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=
github.com/coreos/go-systemd/v22 v22.0.0/go.mod h1:xO0FLkIi5MaZafQlIrOotqXZ90ih+1atmu1JpKERPPk=
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why so many removed in this file?

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.18 has module graph pruning:

If the main module is at go 1.17 or higher, the module graph used for minimal version selection includes only the immediate requirements for each module dependency that specifies go 1.17 or higher in its own go.mod file, unless that version of the module is also (transitively) required by some other dependency at go 1.16 or below. (The transitive dependencies of go 1.17 dependencies are pruned out of the module graph.)

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for the explanation!

@@ -1,6 +1,6 @@
module github.com/crossplane/crossplane

go 1.17
go 1.18

require (
Copy link
Member

Choose a reason for hiding this comment

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

We can update crossplane-runtime version, too. It'd be great if you can run Crossplane with that change and see if it crashes or has anything unexpected in the logs 🙂

Signed-off-by: Aditya Sharma <git@adi.run>
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Thanks @epk!

@muvaf muvaf merged commit 2015c64 into crossplane:master Jul 26, 2022
@epk epk deleted the epk/go1-18 branch July 26, 2022 09:08
@epk epk mentioned this pull request Aug 22, 2022
6 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.

None yet

2 participants