Switch to GHA CI #728

kolyshkin · 2021-10-15T01:59:59Z

~~This PR builds on and currently includes PR #727, thus the draft status.~~

This removes Travis CI config and adds GHA CI. You can see how it looks like at https://github.com/kolyshkin/runtime-tools/actions/runs/1361288633 and kolyshkin#1

Also:

fix many linter warnings from the default golangci-lint config
(alas not all of them from errcheck -- there are way too many 😿, I gave up)
add golangci-lint, fix all its warnings
re-add commit subject length validation

Addresses: #726

tianon

This all looks sane to me -- do you think it would be useful for me to try pushing your branch to a new branch here on the origin so the PR can actually run the actions?

(I don't personally have a strong preference, but other folks might.)

kolyshkin · 2021-10-15T18:37:16Z

This all looks sane to me -- do you think it would be useful for me to try pushing your branch to a new branch here on the origin so the PR can actually run the actions?

As noted in description, I have those ran at https://github.com/kolyshkin/runtime-tools/actions/runs/1344345504, don't think it makes any difference which repo it is.

This is still WIP; currently working on enabling golangci-lint and it's overwhelming :( Guess I'll have to disable errcheck linter :(

I'll let you know once it's ready @tianon, thanks for looking!

tianon · 2021-10-15T18:58:02Z

Totally fair -- figured it was worth offering in case you thought it'd be helpful! Your work is absolutely appreciated (here and elsewhere).

kolyshkin · 2021-10-15T21:06:14Z

This PR builds on and currently includes PR #727, thus the draft status. Will rebase once #727 is merged.

kolyshkin · 2021-10-15T21:20:27Z

Latest run: https://github.com/kolyshkin/runtime-tools/runs/3910388378?check_suite_focus=true

kolyshkin · 2021-10-19T02:04:11Z

Can we merge #727 so we can merge this one, and (re)enable CI for this repo? I have more changes that are piling up but this one is a blocker as we can't review PRs without CI.

tianon · 2021-10-19T02:19:36Z

Thanks to Travis it's going to require a repository administrator (which excludes me 😅).

@vbatts is that you? @caniszczyk?

IMO both PRs look great and merging this which contains both seems like a simpler path, but I don't have any issue with any solution that unblocks Kir and gets this repo able to progress again. 😁

vbatts · 2021-10-19T15:34:01Z

not I. I think it's got to be cra

caniszczyk · 2021-10-19T15:35:29Z

GHA shouldn't require anything on my end, IMHO let's drop TravisCI fast

caniszczyk · 2021-10-19T15:42:08Z

I truly don't know how to disable the Travis CI check

I pulled the webhook and couldn't find any other integrations

kolyshkin · 2021-10-19T16:22:16Z

@caniszczyk this is not about disabling Travis, this is about removing travis (and, maybe, pullapprove, too) from the list of jobs required to merge a PR. This is under Settings -> Branches -> Branch protection rules -> master -> edit -> Status checks that are required. Need to remove travis-ci from that list (and later add GHA CI jobs added by this PR).

caniszczyk · 2021-10-19T16:26:20Z

Got it, it's in Branches and that's why I couldn't find it Done.

…

On Tue, Oct 19, 2021 at 11:22 AM Kir Kolyshkin ***@***.***> wrote: @caniszczyk <https://github.com/caniszczyk> this is not about disabling Travis, this is about removing travis (and, maybe, pullapprove, too) from the list of jobs required to merge a PR. This is under Settings -> Branches -> Branch protection rules -> master -> edit -> Status checks that are required. Need to remove travis-ci from that list (and later add GHA CI jobs added by this PR). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#728 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPSIL7XCXCCCO6I62ATCDUHWLMTANCNFSM5GA6EQMA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Cheers, Chris Aniszczyk https://aniszczyk.org

1. Remove .gofmt and .golint targets -- they are to be replaced by golangci-lint a bit later. 2. Simplify .gotest target, add support for TESTFLAGS Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

1. Move the common go build flags to BUILD_FLAGS. Add an ability to specify more build flags via EXTRA_FLAGS. 2. Add STATIC_BUILD_FLAGS, use it for runtimetest static build. Commit 95617cb added CGO_ENABLED=0 in order to produce a static build, which was the way things worked back in the day. Now we can use netgo, and we have to specify "-extldflags -static" in -ldflags in order to have a static build. 3. Enable to build binaries with -race from CI. Signed-off-by: Kir Kolyshkin <[email protected]>

Now, the commit ID will contain the closest tag, the number of commits since the tag, and the (abbreviated) git commit sha (see example below). This tag is still unique and can be used instead of bare sha for all git commands. Before: $ ./runtimetest -version runtimetest version 0.9.0, commit: ea2948e After: $ ./runtimetest -version runtimetest version 0.9.0, commit: v0.9.0-28-ge91e387 This means that - the closest tag is v0.9.0 - there were 28 commits after the tag - the abbreviated sha is e91e387 Nice, isn't it? While at it, - allow to set commit id via environment (might be needed for distros); - remove unneeded "|| true" stance. Signed-off-by: Kir Kolyshkin <[email protected]>

Fix the following warnings by removing the unused stuff. > generate/seccomp/consts.go:7:2: `kill` is unused (deadcode) > kill = "kill" > ^ > generate/seccomp/consts.go:8:2: `trap` is unused (deadcode) > trap = "trap" > ^ > generate/seccomp/consts.go:9:2: `trace` is unused (deadcode) > trace = "trace" > ^ > generate/seccomp/consts.go:10:2: `allow` is unused (deadcode) > allow = "allow" > ^ > generate/seccomp/consts.go:11:2: `errno` is unused (deadcode) > errno = "errno" > ^ > generate/seccomp/syscall_compare.go:95:6: `identicalExceptAction` is unused (deadcode) > func identicalExceptAction(config1, config2 *rspec.LinuxSyscall) bool { > ^ > generate/seccomp/syscall_compare.go:103:6: `identicalExceptArgs` is unused (deadcode) > func identicalExceptArgs(config1, config2 *rspec.LinuxSyscall) bool { > ^ All of the above was added by commit 60473d2 but never ever used. > generate/generate.go:1567:6: `strPtr` is unused (deadcode) > func strPtr(s string) *string { return &s } > ^ This was added by commit 80e63b3, but later in afc8d35 all its uses were removed. Signed-off-by: Kir Kolyshkin <[email protected]>

These ones: generate/generate.go:1563:2: S1023: redundant `return` statement (gosimple) return ^ validation/util/linux_resources_devices.go:29:6: S1002: should omit comparison to bool constant, can be simplified to `device.Allow` (gosimple) if device.Allow == true { ^ cmd/runtimetest/main.go:985:94: S1039: unnecessary use of fmt.Sprintf (gosimple) rfcError, err := c.Ok(actual == expected, specerror.LinuxProcOomScoreAdjSet, spec.Version, fmt.Sprintf("has expected OOM score adjustment")) ^ validation/hostname/hostname.go:34:13: S1039: unnecessary use of fmt.Sprintf (gosimple) t.Skip(1, fmt.Sprintf("linux-specific namespace test")) ^ validation/linux_ns_itype/linux_ns_itype.go:122:13: S1039: unnecessary use of fmt.Sprintf (gosimple) t.Skip(1, fmt.Sprintf("linux-specific namespace test")) ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Fix deprecation warnings related to commit 84a62c6 from 2016. In particular, these ones: validation/util/test.go:299:15: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) if err := f(g.Spec(), t, &state); err != nil { ^ validation/config_updates_without_affect/config_updates_without_affect.go:71:2: SA1019: g.SetSpec is deprecated: Replace with: (staticcheck) g.SetSpec(spec) ^ validation/delete_resources/delete_resources.go:62:44: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) if err := util.ValidateLinuxResourcesPids(g.Spec(), t, &state); err != nil { ^ validation/hooks/hooks.go:30:40: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) output = filepath.Join(r.BundleDir, g.Spec().Root.Path, "output") ^ validation/killsig/killsig.go:37:38: SA1019: sigConfig.Spec is deprecated: Replace with generator.Config. (staticcheck) rootDir := filepath.Join(bundleDir, sigConfig.Spec().Root.Path) ^ validation/misc_props/misc_props.go:65:24: SA1019: annotationConfig.Spec is deprecated: Replace with generator.Config. (staticcheck) {extendedSpec{Spec: *annotationConfig.Spec()}, util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete, true, specerror.NewError(specerror.AnnotationsKeyIgnoreUnknown, fmt.Errorf("implementations that are reading/processing this configuration file MUST NOT generate an error if they encounter an unknown annotation key"), rspecs.Version)}, ^ validation/misc_props/misc_props.go:66:24: SA1019: basicConfig.Spec is deprecated: Replace with generator.Config. (staticcheck) {extendedSpec{Spec: *basicConfig.Spec(), Unknown: "unknown"}, util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete, true, specerror.NewError(specerror.ExtensibilityIgnoreUnknownProp, fmt.Errorf("runtimes that are reading or processing this configuration file MUST NOT generate an error if they encounter an unknown property"), rspecs.Version)}, ^ validation/misc_props/misc_props.go:67:24: SA1019: invalidConfig.Spec is deprecated: Replace with generator.Config. (staticcheck) {extendedSpec{Spec: *invalidConfig.Spec()}, util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete, false, specerror.NewError(specerror.ValidValues, fmt.Errorf("runtimes that are reading or processing this configuration file MUST generate an error when invalid or unsupported values are encountered"), rspecs.Version)}, ^ validation/prestart/prestart.go:31:40: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) output = filepath.Join(r.BundleDir, g.Spec().Root.Path, "output") ^ validation/prestart/prestart.go:33:38: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) Path: filepath.Join(r.BundleDir, g.Spec().Root.Path, "/bin/sh"), ^ validation/prestart_fail/prestart_fail.go:30:34: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/false"), ^ validation/poststop_fail/poststop_fail.go:31:37: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) output := filepath.Join(bundleDir, g.Spec().Root.Path, "output") ^ validation/poststop_fail/poststop_fail.go:33:34: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/false"), ^ validation/poststop_fail/poststop_fail.go:38:34: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck) Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/sh"), ^ ...and so on (too many to list). Signed-off-by: Kir Kolyshkin <[email protected]>

This code was added by commits 9b1de8c, fc0fc84, and f5e59a3. Since these new struct members are not pointers, no initialization is needed. Remove the initializers, call the parent initializers from the users. This fixes the following warnings: generate/config.go:191:5: SA4022: the address of a variable cannot be nil (staticcheck) if &g.Config.VM.Hypervisor == nil { ^ generate/config.go:198:5: SA4022: the address of a variable cannot be nil (staticcheck) if &g.Config.VM.Kernel == nil { ^ generate/config.go:205:5: SA4022: the address of a variable cannot be nil (staticcheck) if &g.Config.VM.Image == nil { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Commit 20a71e4 implemented a number of improvements, including a modification to validatePosixMounts, which results in the following linter warning: > cmd/runtimetest/main.go:1179:4: SA4006: this value of `rfcError` is never used (staticcheck) > rfcError, err = c.Ok(false, specerror.MountsInOrder, spec.Version, fmt.Sprintf("mounts[%d] (%s) found", i, configMount.Destination)) > ^ It seems that c.harness.YAML() call was mistakenly placed into the "else" code block. Move it out. Signed-off-by: Kir Kolyshkin <[email protected]>

Fix the following warning: > validate/validate.go:134:2: SA4006: this value of `err` is never used (staticcheck) > configRenamedToConfigSchemaVersion, err := semver.Parse("1.0.0-rc2") // config.json became config-schema.json in 1.0.0-rc2 > ^ As the error is unexpected here, ignore it. Signed-off-by: Kir Kolyshkin <[email protected]>

Motivated by this linter warning: > validation/util/container.go:116:3: SA4006: this value of `err` is never used (staticcheck) > err = err2 > ^ Rather than fixing it, let's use bytes.Buffer (rather than files) for stdout and stderr, simplifying the whole thing. Signed-off-by: Kir Kolyshkin <[email protected]>

It is only called from defer, so it does not make sense to return an error. So, log errors to stderr instead of returning them. Also, log errors from Kill and WaitingForStatus. Signed-off-by: Kir Kolyshkin <[email protected]>

We do not expect any errors from YAML function, so explicitly ignore those to silence the errcheck linter. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

These functions never return errors, so let's simplify them and their callers. This fixes a bunch of errcheck linter warning, as some callers of these methods were not checking for errors. Signed-off-by: Kir Kolyshkin <[email protected]>

Had to disable errcheck linter, as there are too many warnings yet to be fixed. Signed-off-by: Kir Kolyshkin <[email protected]>

To replace the one removed by commit 5432bc4. Signed-off-by: Kir Kolyshkin <[email protected]>

jdolitsky · 2021-10-20T14:26:56Z

validation/util/container.go

-func (r *Runtime) Clean(removeBundle bool, forceRemoveBundle bool) error {
-	r.Kill("KILL")
-	WaitingForStatus(*r, LifecycleStatusStopped, time.Second*10, time.Second/10)
+func (r *Runtime) Clean(removeBundle bool, forceRemoveBundle bool) {


not familiar enough with this codebase.. is this a API-breaking change? Does it matter? (comment applies to other spots too)

I was literally about to merge.
Should I wait?

@vbatts - if @kolyshkin made the change, I am assuming nothing to worry about. Plus this repo is pre-1.0. Press that button!

Here's a copy/paste from the relevant commit message:

validation: fix Clean

It is only called from defer, so it does not make sense to return an
error. So, log errors to stderr instead of returning them.

Also, log errors from Kill and WaitingForStatus.

The API concern is valid though, but apparently validation/util is de-facto internal package, only used by runtime-tools (checked via https://sourcegraph.com/search?q=context:global+opencontainers/runtime-tools/validation/util&patternType=literal)

Re-checked with pkg.go.dev -- same thing, it is only imported internally (and by forks) https://pkg.go.dev/github.com/opencontainers/runtime-tools/validation/util?tab=importedby

Ideally, such packages should be moved to under internal/ but it's out of scope for this PR (which is already big enough).

jdolitsky

👍

caniszczyk · 2021-10-20T18:01:44Z

NICE, great work all

kolyshkin force-pushed the gha-ci branch 2 times, most recently from 1ec79b2 to 4857041 Compare October 15, 2021 02:34

tianon approved these changes Oct 15, 2021

View reviewed changes

kolyshkin changed the title ~~Switch to GHA CI~~ [WIP] Switch to GHA CI Oct 15, 2021

kolyshkin changed the title ~~[WIP] Switch to GHA CI~~ Switch to GHA CI Oct 15, 2021

kolyshkin mentioned this pull request Oct 15, 2021

Change /dev to be mounted by default with /noexec #725

Merged

kolyshkin mentioned this pull request Oct 19, 2021

Fork opencontainers/runtime-tools to allow us more control containers/common#800

Closed

kolyshkin added 12 commits October 19, 2021 15:50

Makefile: rm gofmt and golint, simplify gotest

fab664e

1. Remove .gofmt and .golint targets -- they are to be replaced by golangci-lint a bit later. 2. Simplify .gotest target, add support for TESTFLAGS Signed-off-by: Kir Kolyshkin <[email protected]>

ci: replace travis with gha ci

5432bc4

Signed-off-by: Kir Kolyshkin <[email protected]>

validation: fix Clean

fec9c3c

It is only called from defer, so it does not make sense to return an error. So, log errors to stderr instead of returning them. Also, log errors from Kill and WaitingForStatus. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin added 5 commits October 19, 2021 15:51

Explicitly ignore errors from YAML

9505f16

We do not expect any errors from YAML function, so explicitly ignore those to silence the errcheck linter. Signed-off-by: Kir Kolyshkin <[email protected]>

runtimetest: silence errlint on unix.Unmount

c0037c9

Signed-off-by: Kir Kolyshkin <[email protected]>

Add*Hook: do not return errors

a7cecde

These functions never return errors, so let's simplify them and their callers. This fixes a bunch of errcheck linter warning, as some callers of these methods were not checking for errors. Signed-off-by: Kir Kolyshkin <[email protected]>

ci: add golangci-lint run

a22a894

Had to disable errcheck linter, as there are too many warnings yet to be fixed. Signed-off-by: Kir Kolyshkin <[email protected]>

ci: re-add commit subject length validation

10c865d

To replace the one removed by commit 5432bc4. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the gha-ci branch from c8f904e to 10c865d Compare October 19, 2021 22:55

kolyshkin marked this pull request as ready for review October 19, 2021 23:10

kolyshkin mentioned this pull request Oct 19, 2021

Add maintainers as repo admins #729

Closed

jdolitsky reviewed Oct 20, 2021

View reviewed changes

vbatts approved these changes Oct 20, 2021

View reviewed changes

jdolitsky approved these changes Oct 20, 2021

View reviewed changes

vbatts merged commit 17ac4c5 into opencontainers:master Oct 20, 2021

jdolitsky mentioned this pull request Oct 20, 2021

Move all internal packages to internal/ #730

Open

kolyshkin mentioned this pull request Oct 22, 2021

🚨 Overhaul CI steps #726

Closed

4 tasks

flouthoc mentioned this pull request Oct 25, 2021

ci: verify oci-valiation containers/crun#760

Closed

kolyshkin mentioned this pull request Oct 25, 2021

MAINTAINERS: add @kolyshkin #736

Merged

kolyshkin mentioned this pull request Jan 5, 2022

Followup fixes to validator #742

Merged

Switch to GHA CI #728

Switch to GHA CI #728

Uh oh!

Conversation

kolyshkin commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Oct 15, 2021

Uh oh!

tianon commented Oct 15, 2021 via email

Uh oh!

kolyshkin commented Oct 15, 2021

Uh oh!

kolyshkin commented Oct 15, 2021

Uh oh!

kolyshkin commented Oct 19, 2021

Uh oh!

tianon commented Oct 19, 2021

Uh oh!

vbatts commented Oct 19, 2021 via email

Uh oh!

caniszczyk commented Oct 19, 2021

Uh oh!

caniszczyk commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Oct 19, 2021

Uh oh!

caniszczyk commented Oct 19, 2021 via email

Uh oh!

jdolitsky Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

vbatts Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

jdolitsky Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

jdolitsky left a comment

Choose a reason for hiding this comment

Uh oh!

caniszczyk commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin commented Oct 15, 2021 •

edited

Loading

caniszczyk commented Oct 19, 2021 •

edited

Loading