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

Add Go 1.20 support #1179

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Conversation

austinvazquez
Copy link
Contributor

@austinvazquez austinvazquez commented Feb 2, 2023

  • Add Go 1.20 to GitHub actions matrix
  • Updates actions/setup-go from v1 to v3. (Resolves deprecation warnings in GitHub actions around Node 12. Reference here)

Signed-off-by: Austin Vazquez macedonv@amazon.com

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 (assuming the version format is correct)

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.

arf; looks like some changes are needed (see failures)

@austinvazquez austinvazquez force-pushed the add-go-1.20 branch 3 times, most recently from 028536f to 8e9adf2 Compare February 2, 2023 23:35
@austinvazquez
Copy link
Contributor Author

arf; looks like some changes are needed (see failures)

Added a little extra scope to resolve the errors. Took the opportunity to update the lint tooling from golint to golangci-lint and resolved new errors. Included here since we have the hood open but could split this into a seperate commit/PR if that helps.

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Adding go.mod explicitly to .gitignore is a little odd when I don't think anything here generates it (right?), but I don't care about that enough to block this.

The other combined CI fixes/updates seem fine to include here IMO 👍

@austinvazquez
Copy link
Contributor Author

Adding go.mod explicitly to .gitignore is a little odd when I don't think anything here generates it (right?), but I don't care about that enough to block this.

The other combined CI fixes/updates seem fine to include here IMO 👍

Good call out I could have explained that better. CI generates them here and just wanted to be sure I didn't accidentally push those.

@AkihiroSuda
Copy link
Member

Why not use go.mod ?

@austinvazquez
Copy link
Contributor Author

Why not use go.mod ?

I can make this happen. IIRC other specs had them.

@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Feb 8, 2023
go.mod Outdated
@@ -0,0 +1,10 @@
module github.com/opencontainers/runtime-spec
Copy link
Member

@thaJeztah thaJeztah Feb 8, 2023

Choose a reason for hiding this comment

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

This has a potential wider impact than updating go versions in CI; do we still need to discuss;

(or was there a concensus on that yet w.r.t. "SemVer from a Go perspective vs Spec perspective")?

Copy link
Member

Choose a reason for hiding this comment

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

image-spec and distribution-spec already have go.mod:

And these struct definitions are unlikely to have breaking changes (until the spec itself has breaking changes)

Copy link
Member

Choose a reason for hiding this comment

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

@thaJeztah WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@austinvazquez @thaJeztah
Do we need to split go.mod to another PR?

Copy link
Member

Choose a reason for hiding this comment

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

I would consider moving it separate yes, as it's not directly related to updating / updating CI to test against a newer go version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks for the feedback. Let me split and push.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Changes look good overall, but they need to be structured.

  1. Fix using deprecated ioutil.
  2. Switch to using golangci-lint.
  3. Modify ci to use matrix of go versions

One other thing, for golangci-lint it is better to use their action directly from .github/workflows rather than through make.

@AkihiroSuda
Copy link
Member

@austinvazquez ↑ PTAL

Signed-off-by: Austin Vazquez <macedonv@amazon.com>
@austinvazquez
Copy link
Contributor Author

Changes look good overall, but they need to be structured.

  1. Fix using deprecated ioutil.
  2. Switch to using golangci-lint.
  3. Modify ci to use matrix of go versions

One other thing, for golangci-lint it is better to use their action directly from .github/workflows rather than through make.

@kolyshkin, let me draft up what golangci-lint through github action would look like and you can lmk your thoughts.

@austinvazquez
Copy link
Contributor Author

@kolyshkin, I have split commits and changed linting to use github action. PTAL and let me know your thoughts.

@AkihiroSuda
Copy link
Member

CI is failing

Signed-off-by: Austin Vazquez <macedonv@amazon.com>
Adds a Go compiler matrix to CI for testing of latest Go versions.
Updates and pins to major version GitHub actions packages.

Signed-off-by: Austin Vazquez <macedonv@amazon.com>
@austinvazquez
Copy link
Contributor Author

CI is failing

Oof I've fallen victim to the go.mod issue. Moved the hack up to resolve.

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

@AkihiroSuda AkihiroSuda merged commit 9067ce6 into opencontainers:main Mar 17, 2023
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 28, 2023
@austinvazquez austinvazquez deleted the add-go-1.20 branch April 13, 2023 17:30
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
12 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.

5 participants