Add GitHub actions for test and linting #62

thaJeztah · 2023-01-13T16:32:14Z

No description provided.

.github/workflows/validate.yml

.github/workflows/test.yml

thaJeztah · 2023-01-13T16:56:07Z

@jeffwidman @dims @pacoxu @aboch PTAL

A test-run of these changes can be found on my fork, where I opened a draft-PR from this branch; [testing] Add GitHub actions for test and linting thaJeztah/netns#1

jeffwidman

Thanks!

.github/workflows/test.yml

.github/workflows/validate.yml

jeffwidman · 2023-01-13T17:59:41Z

Since this adds github actions that are versioned, we should probably also add a stanza to https://github.com/vishvananda/netns/blob/master/.github/dependabot.yml to monitor/bump the actions.

Feel free to include as part of this PR (albeit in a separate commit), but if you're not sure how to do that, I can handle it in a separate PR.

thaJeztah · 2023-01-13T19:21:52Z

Ah, interesting; looks like go1.12 is actually an issue with the updated golang.org/x/sys.

Error: ../../../go/pkg/mod/golang.org/x/sys@v0.2.0/unix/syscall.go:83:16: undefined: unsafe.Slice
Error: ../../../go/pkg/mod/golang.org/x/sys@v0.2.0/unix/syscall_linux.go:2256:9: undefined: unsafe.Slice
Error: ../../../go/pkg/mod/golang.org/x/sys@v0.2.0/unix/syscall_unix.go:118:7: undefined: unsafe.Slice
Error: ../../../go/pkg/mod/golang.org/x/sys@v0.2.0/unix/sysvshm_unix.go:33:7: undefined: unsafe.Slice

Maybe it's time now to update to something newer (I think I saw some projects are still on go1.15, let me try if that works)

thaJeztah · 2023-01-13T19:38:24Z

I opened #64 to update the minimum go version 👍 (I'll rebase this PR once that's merged to remove the extra commit; CI is happy on thaJeztah#1)

thaJeztah · 2023-01-14T11:15:43Z

Alright; rebased; this one should be ready for review (thaJeztah#1 is still happy)

thaJeztah · 2023-01-14T11:16:50Z

Feel free to include as part of this PR (albeit in a separate commit), but if you're not sure how to do that, I can handle it in a separate PR.

Oh! I forgot about that one; I'd have to look up how to set that up, so if you have time to handle that, that'd be appreciated 🤗 ❤️

.github/workflows/validate.yml

jeffwidman

A few small suggestions/questions

jeffwidman · 2023-01-16T20:43:18Z

.github/workflows/test.yml

+        # using newer versions than that, so we can drop the old version if
+        # it becomes too much of a burden.
+        go-version: [1.17.x, 1.19.x]
+        os: [ubuntu-20.04, ubuntu-22.04]


Should we also test on windows? To ensure no runtime errors in the future?

I realize today there's no tests that will run on windows, but still seems best to set it in the matrix proactively for if/when there might be some down the road... it'll also catch compile errors even w/o any tests I think??

jeffwidman · 2023-01-16T20:46:47Z

.github/workflows/test.yml

+        # currently not much of a burden. Most projects using this module are
+        # using newer versions than that, so we can drop the old version if
+        # it becomes too much of a burden.
+        go-version: [1.17.x, 1.19.x]


Suggested change

go-version: [1.17.x, 1.19.x]

go-version: [1.17.x, 'stable']

This lets us always default to the latest release: https://github.com/actions/setup-go#using-stableoldstable-aliases

Given the extremely low activity on this repo, I'd much rather use stable than having to bump the pins, because we're more likely to catch issues that way...

Interestingly, they also support picking up the version from go.mod, but doesn't like like it's possibly to cleanly do this as part of a matrix: https://github.com/actions/setup-go#getting-go-version-from-the-gomod-file

jeffwidman · 2023-01-16T20:48:18Z

.github/workflows/validate.yml

+      matrix:
+        # We only run on the latest version of go, as some linters may be
+        # version-dependent (for example gofmt can change between releases).
+        go-version: [1.19.x]


Suggested change

go-version: [1.19.x]

go-version: ['stable']

Again, I realize it's non-deterministic, but I think for this particular repo it's probably the better way to go...

jeffwidman · 2023-01-16T21:07:05Z

Feel free to include as part of this PR (albeit in a separate commit), but if you're not sure how to do that, I can handle it in a separate PR.

Oh! I forgot about that one; I'd have to look up how to set that up, so if you have time to handle that, that'd be appreciated 🤗 ❤️

Handled in:

Make Dependabot watch github actions #66

jeffwidman · 2023-01-18T19:35:38Z

@thaJeztah I left some minor feedback, I know we're both involved with a lot of projects, so any chance of getting this across the line while it's still fresh in our minds?

thaJeztah · 2023-01-18T20:07:51Z

Doh! Saw your comments, and then forgot about them. Thanks for the ping; yes, let me look at those 👍

Test against the "oldest" supported version and the current version of go. Go 1.17 is kept in this matrix as it is the minimum version specified in go.mod, and maintaining compatibility with go 1.17 is currently not much of a burden. Most projects using this module are using newer versions than that, so we can drop the old version if it becomes too much of a burden. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Linting is disabled on Windows, as the current build-tags do not properly exclude non-unix platforms; level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 1.0348ms" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:28:18: Stat_t not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:29:17: Fstat not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:32:17: Fstat not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:43:13: Stat_t not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:44:17: Fstat not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:56:13: Stat_t not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:57:17: Fstat not declared by package unix" Error: level=error msg="[linters_context] typechecking error: D:\\a\\netns\\netns\\netns.go:71:17: Close not declared by package unix" Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-01-18T20:17:52Z

.github/workflows/test.yml

+        git diff --exit-code
+    - name: Test
+      run: |
+        go test -exec "sudo -n" -v ./...


Hm. Somewhat surprised this worked on Windows (with the sudo), but perhaps because there's currently no tests. Looks like Ci is happy (thaJeztah#1), so worst case, it'll explode if we would add non-linux tests 😂

thaJeztah · 2023-01-18T20:21:01Z

Okay; updated. I think this should be ready for another review 👍

jeffwidman

Looks great, thanks!

thaJeztah · 2023-01-18T21:14:09Z

Thanks!

thaJeztah changed the title ~~[testing] Add GitHub actions for test and linting~~ Add GitHub actions for test and linting Jan 13, 2023

thaJeztah mentioned this pull request Jan 13, 2023

[testing] Add GitHub actions for test and linting thaJeztah/netns#1

Closed

thaJeztah force-pushed the enable_gha branch 4 times, most recently from c7fbc03 to 6a4e2bc Compare January 13, 2023 16:48

thaJeztah commented Jan 13, 2023

View reviewed changes

.github/workflows/validate.yml Outdated Show resolved Hide resolved

.github/workflows/validate.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

thaJeztah marked this pull request as ready for review January 13, 2023 16:51

jeffwidman reviewed Jan 13, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

.github/workflows/validate.yml Outdated Show resolved Hide resolved

.github/workflows/validate.yml Outdated Show resolved Hide resolved

jeffwidman mentioned this pull request Jan 13, 2023

Bump golang.org/x/sys from 0.2.0 to 0.4.0 #63

Open

thaJeztah force-pushed the enable_gha branch from 6a4e2bc to 0d8d464 Compare January 13, 2023 19:16

thaJeztah force-pushed the enable_gha branch 2 times, most recently from b178c5d to 54471b1 Compare January 13, 2023 19:34

thaJeztah force-pushed the enable_gha branch from 54471b1 to 008fbd2 Compare January 14, 2023 11:13

mmorel-35 reviewed Jan 14, 2023

View reviewed changes

.github/workflows/validate.yml Outdated Show resolved Hide resolved

thaJeztah force-pushed the enable_gha branch from 008fbd2 to 93425eb Compare January 14, 2023 15:19

jeffwidman reviewed Jan 16, 2023

View reviewed changes

thaJeztah force-pushed the enable_gha branch from 93425eb to 203d57f Compare January 18, 2023 20:11

thaJeztah added 2 commits January 18, 2023 21:12

thaJeztah force-pushed the enable_gha branch from 203d57f to f0a0fd9 Compare January 18, 2023 20:13

thaJeztah commented Jan 18, 2023

View reviewed changes

jeffwidman approved these changes Jan 18, 2023

View reviewed changes

jeffwidman merged commit 1104d96 into vishvananda:master Jan 18, 2023

thaJeztah deleted the enable_gha branch January 18, 2023 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub actions for test and linting #62

Add GitHub actions for test and linting #62

thaJeztah commented Jan 13, 2023

thaJeztah commented Jan 13, 2023

jeffwidman left a comment

jeffwidman commented Jan 13, 2023

thaJeztah commented Jan 13, 2023

thaJeztah commented Jan 13, 2023

thaJeztah commented Jan 14, 2023

thaJeztah commented Jan 14, 2023

jeffwidman left a comment

jeffwidman Jan 16, 2023 •

edited

jeffwidman Jan 16, 2023

jeffwidman Jan 16, 2023

jeffwidman Jan 16, 2023

jeffwidman commented Jan 16, 2023 •

edited

jeffwidman commented Jan 18, 2023 •

edited

thaJeztah commented Jan 18, 2023

thaJeztah Jan 18, 2023

thaJeztah commented Jan 18, 2023

jeffwidman left a comment

thaJeztah commented Jan 18, 2023

Add GitHub actions for test and linting #62

Add GitHub actions for test and linting #62

Conversation

thaJeztah commented Jan 13, 2023

thaJeztah commented Jan 13, 2023

jeffwidman left a comment

Choose a reason for hiding this comment

jeffwidman commented Jan 13, 2023

thaJeztah commented Jan 13, 2023

thaJeztah commented Jan 13, 2023

thaJeztah commented Jan 14, 2023

thaJeztah commented Jan 14, 2023

jeffwidman left a comment

Choose a reason for hiding this comment

jeffwidman Jan 16, 2023 • edited

Choose a reason for hiding this comment

jeffwidman Jan 16, 2023

Choose a reason for hiding this comment

jeffwidman Jan 16, 2023

Choose a reason for hiding this comment

jeffwidman Jan 16, 2023

Choose a reason for hiding this comment

jeffwidman commented Jan 16, 2023 • edited

jeffwidman commented Jan 18, 2023 • edited

thaJeztah commented Jan 18, 2023

thaJeztah Jan 18, 2023

Choose a reason for hiding this comment

thaJeztah commented Jan 18, 2023

jeffwidman left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 18, 2023

jeffwidman Jan 16, 2023 •

edited

jeffwidman commented Jan 16, 2023 •

edited

jeffwidman commented Jan 18, 2023 •

edited