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.

Sign up for GitHub

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

Jump to bottom

Add Go 1.20 support #1179

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Add Go 1.20 support #1179

merged 3 commits into from
Mar 17, 2023

Conversation

austinvazquez
Copy link
Contributor

@austinvazquez austinvazquez commented Feb 2, 2023 •
edited
Loading

  • 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 [email protected]

thaJeztah
thaJeztah approved these changes Feb 2, 2023
View reviewed changes
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)

thaJeztah
thaJeztah requested changes Feb 2, 2023
View reviewed changes
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.

tianon
tianon approved these changes Feb 3, 2023
View reviewed changes
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
thaJeztah
thaJeztah reviewed Feb 8, 2023
View reviewed changes
go.mod Outdated
@@ -0,0 +1,10 @@
module github.com/opencontainers/runtime-spec
Copy link
Member

@thaJeztah thaJeztah Feb 8, 2023 •
edited
Loading

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.

kolyshkin
kolyshkin requested changes Mar 9, 2023
View reviewed changes
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

@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

@austinvazquez
Switch Go linting to use golangci-lint
15d2a5a 
Signed-off-by: Austin Vazquez <[email protected]>
@austinvazquez
Add Go 1.20 support to CI
167ffb4 
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 <[email protected]>
@austinvazquez
Copy link
Contributor Author

CI is failing

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

thaJeztah
thaJeztah approved these changes Mar 16, 2023
View reviewed changes
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
Merged
@austinvazquez austinvazquez deleted the add-go-1.20 branch April 13, 2023 17:30
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin requested changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tianon tianon tianon approved these changes

@crosbymichael crosbymichael Awaiting requested review from crosbymichael crosbymichael is a code owner

@cyphar cyphar Awaiting requested review from cyphar cyphar is a code owner

@dqminh dqminh Awaiting requested review from dqminh dqminh is a code owner

@giuseppe giuseppe Awaiting requested review from giuseppe giuseppe is a code owner

@hqhq hqhq Awaiting requested review from hqhq hqhq is a code owner

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@vbatts vbatts Awaiting requested review from vbatts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@austinvazquez @AkihiroSuda @tianon @thaJeztah @kolyshkin