Conversation
austinvazquez
requested review from
AkihiroSuda,
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
kolyshkin,
mrunalp,
thaJeztah,
tianon and
vbatts
as code owners
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM (assuming the version format is correct)
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
arf; looks like some changes are needed (see failures)
austinvazquez
force-pushed
the
add-go-1.20
branch
3 times, most recently
from
028536f to
8e9adf2
Compare
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
left a comment
tianon
left a comment
There was a problem hiding this comment.
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. |
|
Why not use go.mod ? |
I can make this happen. IIRC other specs had them. |
austinvazquez
force-pushed
the
add-go-1.20
branch
from
8e9adf2 to
917fe22
Compare
go.mod
Outdated
| @@ -0,0 +1,10 @@ | |||
| module github.com/opencontainers/runtime-spec | |||
There was a problem hiding this comment.
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")?
There was a problem hiding this comment.
image-spec and distribution-spec already have go.mod:
- https://github.com/opencontainers/image-spec/blob/main/go.mod
- https://github.com/opencontainers/distribution-spec/blob/main/specs-go/go.mod
And these struct definitions are unlikely to have breaking changes (until the spec itself has breaking changes)
There was a problem hiding this comment.
@austinvazquez @thaJeztah
Do we need to split go.mod to another PR?
There was a problem hiding this comment.
I would consider moving it separate yes, as it's not directly related to updating / updating CI to test against a newer go version
There was a problem hiding this comment.
Okay, thanks for the feedback. Let me split and push.
austinvazquez
force-pushed
the
add-go-1.20
branch
from
917fe22 to
2434407
Compare
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Changes look good overall, but they need to be structured.
- Fix using deprecated ioutil.
- Switch to using golangci-lint.
- 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.
|
@austinvazquez ↑ PTAL |
Signed-off-by: Austin Vazquez <[email protected]>
@kolyshkin, let me draft up what golangci-lint through github action would look like and you can lmk your thoughts. |
austinvazquez
force-pushed
the
add-go-1.20
branch
from
2434407 to
6a5c9eb
Compare
|
@kolyshkin, I have split commits and changed linting to use github action. PTAL and let me know your thoughts. |
|
CI is failing |
Signed-off-by: Austin Vazquez <[email protected]>
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
force-pushed
the
add-go-1.20
branch
from
6a5c9eb to
167ffb4
Compare
Oof I've fallen victim to the go.mod issue. Moved the hack up to resolve. |
5 participants
Signed-off-by: Austin Vazquez [email protected]