From 0020c9125202c407b812f281d53f1a3e298e6c90 Mon Sep 17 00:00:00 2001 From: apostasie Date: Sun, 2 Mar 2025 19:11:24 -0800 Subject: [PATCH 1/4] Makefile fixes - enable freebsd tasks - use ORG_PREFIXES consistently Signed-off-by: apostasie --- mod/tigron/Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mod/tigron/Makefile b/mod/tigron/Makefile index 4e6b13ce06a..681dbc3f920 100644 --- a/mod/tigron/Makefile +++ b/mod/tigron/Makefile @@ -75,6 +75,7 @@ lint-go-all: $(call title, $@) @cd $(MAKEFILE_DIR) \ && GOOS=darwin make lint-go \ + && GOOS=freebsd make lint-go \ && GOOS=linux make lint-go \ && GOOS=windows make lint-go $(call footer, $@) @@ -82,7 +83,7 @@ lint-go-all: lint-imports: $(call title, $@) @cd $(MAKEFILE_DIR) \ - && goimports-reviser -recursive -list-diff -set-exit-status -output stdout -company-prefixes "github.com/containerd" ./... + && goimports-reviser -recursive -list-diff -set-exit-status -output stdout -company-prefixes "$(ORG_PREFIXES)" ./... $(call footer, $@) lint-yaml: @@ -124,6 +125,7 @@ lint-licenses-all: $(call title, $@) @cd $(MAKEFILE_DIR) \ && GOOS=darwin make lint-licenses \ + && GOOS=freebsd make lint-licenses \ && GOOS=linux make lint-licenses \ && GOOS=windows make lint-licenses $(call footer, $@) @@ -141,6 +143,7 @@ fix-go-all: $(call title, $@) @cd $(MAKEFILE_DIR) \ && GOOS=darwin make fix-go \ + && GOOS=freebsd make fix-go \ && GOOS=linux make fix-go \ && GOOS=windows make fix-go $(call footer, $@) From 7f04990549af1f750a960234ac2035d42714ad99 Mon Sep 17 00:00:00 2001 From: apostasie Date: Sun, 2 Mar 2025 19:13:33 -0800 Subject: [PATCH 2/4] Move to creack pty Signed-off-by: apostasie --- go.mod | 1 + go.sum | 4 +- mod/tigron/go.mod | 1 + mod/tigron/go.sum | 2 + mod/tigron/test/internal/pty/pty.go | 28 ++++++++- mod/tigron/test/internal/pty/pty_darwin.go | 25 -------- mod/tigron/test/internal/pty/pty_freebsd.go | 25 -------- mod/tigron/test/internal/pty/pty_linux.go | 69 --------------------- mod/tigron/test/internal/pty/pty_windows.go | 25 -------- 9 files changed, 31 insertions(+), 149 deletions(-) delete mode 100644 mod/tigron/test/internal/pty/pty_darwin.go delete mode 100644 mod/tigron/test/internal/pty/pty_freebsd.go delete mode 100644 mod/tigron/test/internal/pty/pty_linux.go delete mode 100644 mod/tigron/test/internal/pty/pty_windows.go diff --git a/go.mod b/go.mod index d3e9d9bf7e3..8c3d9323603 100644 --- a/go.mod +++ b/go.mod @@ -81,6 +81,7 @@ require ( github.com/containerd/plugin v1.0.0 // indirect github.com/containerd/ttrpc v1.2.7 // indirect github.com/containers/ocicrypt v1.2.1 // indirect + github.com/creack/pty v1.1.24 // indirect github.com/djherbis/times v1.6.0 // indirect github.com/docker/docker-credential-helpers v0.8.2 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect diff --git a/go.sum b/go.sum index 3de86add4af..7439d3b68bc 100644 --- a/go.sum +++ b/go.sum @@ -76,8 +76,8 @@ github.com/coreos/go-iptables v0.8.0/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFE github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8iXXhfZs= github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= -github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/cyphar/filepath-securejoin v0.4.1 h1:JyxxyPEaktOD+GAnqIqTf9A8tHyAG22rowi7HkoSU1s= github.com/cyphar/filepath-securejoin v0.4.1/go.mod h1:Sdj7gXlvMcPZsbhwhQ33GguGLDGQL7h7bg04C/+u9jI= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/mod/tigron/go.mod b/mod/tigron/go.mod index 78efedc51b8..e03295efb48 100644 --- a/mod/tigron/go.mod +++ b/mod/tigron/go.mod @@ -3,6 +3,7 @@ module github.com/containerd/nerdctl/mod/tigron go 1.23 require ( + github.com/creack/pty v1.1.24 golang.org/x/sync v0.11.0 golang.org/x/term v0.29.0 gotest.tools/v3 v3.5.2 diff --git a/mod/tigron/go.sum b/mod/tigron/go.sum index fa70cdd035e..e8489e88ada 100644 --- a/mod/tigron/go.sum +++ b/mod/tigron/go.sum @@ -1,3 +1,5 @@ +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= diff --git a/mod/tigron/test/internal/pty/pty.go b/mod/tigron/test/internal/pty/pty.go index 7fa117cf541..9aa32f3e047 100644 --- a/mod/tigron/test/internal/pty/pty.go +++ b/mod/tigron/test/internal/pty/pty.go @@ -14,11 +14,33 @@ limitations under the License. */ +// Package pty. +// Note that creack is MIT licensed, making it better to depend on it rather than using derived code here. +// Underlying creack implementation is OK though they have more (unnecessary to us) features and do not follow the +// same coding standards. package pty -import "errors" +import ( + "errors" + "os" + + creack "github.com/creack/pty" +) var ( - ErrPTYFailure = errors.New("pty failure") - ErrPTYUnsupportedPlatform = errors.New("pty not supported on this platform") + ErrFailure = errors.New("pty failure") + ErrUnsupportedPlatform = errors.New("pty not supported on this platform") ) + +func Open() (*os.File, *os.File, error) { + pty, tty, err := creack.Open() + if err != nil { + if errors.Is(err, creack.ErrUnsupported) { + err = errors.Join(ErrUnsupportedPlatform, err) + } else { + err = errors.Join(ErrFailure, err) + } + } + + return pty, tty, err +} diff --git a/mod/tigron/test/internal/pty/pty_darwin.go b/mod/tigron/test/internal/pty/pty_darwin.go deleted file mode 100644 index 07f96e3212f..00000000000 --- a/mod/tigron/test/internal/pty/pty_darwin.go +++ /dev/null @@ -1,25 +0,0 @@ -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package pty - -import ( - "os" -) - -func Open() (pty, tty *os.File, err error) { - return nil, nil, ErrPTYUnsupportedPlatform -} diff --git a/mod/tigron/test/internal/pty/pty_freebsd.go b/mod/tigron/test/internal/pty/pty_freebsd.go deleted file mode 100644 index 07f96e3212f..00000000000 --- a/mod/tigron/test/internal/pty/pty_freebsd.go +++ /dev/null @@ -1,25 +0,0 @@ -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package pty - -import ( - "os" -) - -func Open() (pty, tty *os.File, err error) { - return nil, nil, ErrPTYUnsupportedPlatform -} diff --git a/mod/tigron/test/internal/pty/pty_linux.go b/mod/tigron/test/internal/pty/pty_linux.go deleted file mode 100644 index 44855f8f111..00000000000 --- a/mod/tigron/test/internal/pty/pty_linux.go +++ /dev/null @@ -1,69 +0,0 @@ -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package pty - -import ( - "errors" - "os" - "strconv" - "syscall" - "unsafe" -) - -// Inspiration from https://github.com/creack/pty/tree/2cde18bfb702199728dd43bf10a6c15c7336da0a - -func Open() (pty, tty *os.File, err error) { - // Wrap errors - defer func() { - if err != nil && pty != nil { - err = errors.Join(pty.Close(), err) - } - if err != nil { - err = errors.Join(ErrPTYFailure, err) - } - }() - - // Open the pty - pty, err = os.OpenFile("/dev/ptmx", os.O_RDWR, 0) - if err != nil { - return nil, nil, err - } - - // Get the slave unit number - var n uint32 - _, _, e := syscall.Syscall(syscall.SYS_IOCTL, pty.Fd(), syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))) - if e != 0 { - return nil, nil, e - } - - sname := "/dev/pts/" + strconv.Itoa(int(n)) - - // Unlock - var u int32 - _, _, e = syscall.Syscall(syscall.SYS_IOCTL, pty.Fd(), syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))) - if e != 0 { - return nil, nil, e - } - - // Open the slave, preventing it from becoming the controlling terminal - tty, err = os.OpenFile(sname, os.O_RDWR|syscall.O_NOCTTY, 0) - if err != nil { - return nil, nil, err - } - - return pty, tty, nil -} diff --git a/mod/tigron/test/internal/pty/pty_windows.go b/mod/tigron/test/internal/pty/pty_windows.go deleted file mode 100644 index 07f96e3212f..00000000000 --- a/mod/tigron/test/internal/pty/pty_windows.go +++ /dev/null @@ -1,25 +0,0 @@ -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package pty - -import ( - "os" -) - -func Open() (pty, tty *os.File, err error) { - return nil, nil, ErrPTYUnsupportedPlatform -} From 077558c89715c582f1296143a0121df816fa8c4b Mon Sep 17 00:00:00 2001 From: apostasie Date: Sun, 2 Mar 2025 19:17:33 -0800 Subject: [PATCH 3/4] Lint fixes Signed-off-by: apostasie --- mod/tigron/require/requirement.go | 24 +++++---- mod/tigron/test/case.go | 40 ++++++++++---- mod/tigron/test/command.go | 88 ++++++++++++++++++++----------- mod/tigron/test/config.go | 15 +++++- mod/tigron/test/data.go | 40 +++++++++++--- mod/tigron/test/expected.go | 8 +-- mod/tigron/test/helpers.go | 30 +++++++---- mod/tigron/test/interfaces.go | 2 +- mod/tigron/test/test.go | 7 +-- mod/tigron/utils/utilities.go | 18 ++++--- 10 files changed, 188 insertions(+), 84 deletions(-) diff --git a/mod/tigron/require/requirement.go b/mod/tigron/require/requirement.go index 15c1d0bb526..4f110a5bf3f 100644 --- a/mod/tigron/require/requirement.go +++ b/mod/tigron/require/requirement.go @@ -26,7 +26,7 @@ import ( func Binary(name string) *test.Requirement { return &test.Requirement{ - Check: func(data test.Data, helpers test.Helpers) (bool, string) { + Check: func(_ test.Data, _ test.Helpers) (bool, string) { mess := fmt.Sprintf("executable %q has been found in PATH", name) ret := true if _, err := exec.LookPath(name); err != nil { @@ -41,7 +41,7 @@ func Binary(name string) *test.Requirement { func OS(os string) *test.Requirement { return &test.Requirement{ - Check: func(data test.Data, helpers test.Helpers) (bool, string) { + Check: func(_ test.Data, _ test.Helpers) (bool, string) { mess := fmt.Sprintf("current operating system is %q", runtime.GOOS) ret := true if runtime.GOOS != os { @@ -55,7 +55,7 @@ func OS(os string) *test.Requirement { func Arch(arch string) *test.Requirement { return &test.Requirement{ - Check: func(data test.Data, helpers test.Helpers) (bool, string) { + Check: func(_ test.Data, _ test.Helpers) (bool, string) { mess := fmt.Sprintf("current architecture is %q", runtime.GOARCH) ret := true if runtime.GOARCH != arch { @@ -67,18 +67,22 @@ func Arch(arch string) *test.Requirement { } } -var Amd64 = Arch("amd64") -var Arm64 = Arch("arm64") -var Windows = OS("windows") -var Linux = OS("linux") -var Darwin = OS("darwin") +//nolint:gochecknoglobals +var ( + Amd64 = Arch("amd64") + Arm64 = Arch("arm64") + Windows = OS("windows") + Linux = OS("linux") + Darwin = OS("darwin") +) -// NOTE: Not will always lose setups and cleanups... +// NOTE: Not will always ignore any setup and cleanup inside the wrapped requirement. func Not(requirement *test.Requirement) *test.Requirement { return &test.Requirement{ Check: func(data test.Data, helpers test.Helpers) (bool, string) { ret, mess := requirement.Check(data, helpers) + return !ret, mess }, } @@ -93,10 +97,12 @@ func All(requirements ...*test.Requirement) *test.Requirement { for _, requirement := range requirements { ret, subMess = requirement.Check(data, helpers) mess += "\n" + subMess + if !ret { return ret, mess } } + return ret, mess }, Setup: func(data test.Data, helpers test.Helpers) { diff --git a/mod/tigron/test/case.go b/mod/tigron/test/case.go index 46c02f2f4f6..99f8101cc56 100644 --- a/mod/tigron/test/case.go +++ b/mod/tigron/test/case.go @@ -59,10 +59,13 @@ type Case struct { parent *Case } -// Run prepares and executes the test, and any possible subtests +// Run prepares and executes the test, and any possible subtests. +// +//nolint:gocognit func (test *Case) Run(t *testing.T) { t.Helper() // Run the test + //nolint:thelper testRun := func(subT *testing.T) { subT.Helper() @@ -81,10 +84,13 @@ func (test *Case) Run(t *testing.T) { // If we have a parent, get parent env, data and config var parentData Data + var parentConfig Config + if test.parent != nil { parentData = test.parent.Data parentConfig = test.parent.Config + for k, v := range test.parent.Env { if _, ok := test.Env[k]; !ok { test.Env[k] = v @@ -96,23 +102,22 @@ func (test *Case) Run(t *testing.T) { test.Data = configureData(test.t, test.Data, parentData) test.Config = configureConfig(test.Config, parentConfig) - var b CustomizableCommand + var custCom CustomizableCommand if registeredTestable == nil { - b = &GenericCommand{} + custCom = &GenericCommand{} } else { - b = registeredTestable.CustomCommand(test, test.t) + custCom = registeredTestable.CustomCommand(test, test.t) } - b.WithCwd(test.Data.TempDir()) - - b.withT(test.t) - b.withTempDir(test.Data.TempDir()) - b.withEnv(test.Env) - b.withConfig(test.Config) + custCom.WithCwd(test.Data.TempDir()) + custCom.withT(test.t) + custCom.withTempDir(test.Data.TempDir()) + custCom.withEnv(test.Env) + custCom.withConfig(test.Config) // Attach the base command, and t test.helpers = &helpersInternal{ - cmdInternal: b, + cmdInternal: custCom, t: test.t, } @@ -125,9 +130,11 @@ func (test *Case) Run(t *testing.T) { if !shouldRun { test.t.Skipf("test skipped as: %s", message) } + if test.Require.Setup != nil { setups = append(setups, test.Require.Setup) } + if test.Require.Cleanup != nil { cleanups = append(cleanups, test.Require.Cleanup) } @@ -154,35 +161,46 @@ func (test *Case) Run(t *testing.T) { } // Execute cleanups now + test.t.Log("") test.t.Log("======================== Pre-test cleanup ========================") + for _, cleanup := range cleanups { cleanup(test.Data, test.helpers) } // Register the cleanups, in reverse test.t.Cleanup(func() { + test.t.Log("") test.t.Log("======================== Post-test cleanup ========================") + slices.Reverse(cleanups) + for _, cleanup := range cleanups { cleanup(test.Data, test.helpers) } }) // Run the setups + test.t.Log("") test.t.Log("======================== Test setup ========================") + for _, setup := range setups { setup(test.Data, test.helpers) } // Run the command if any, with expectations // Note: if we have a command, we already know we DO have Expected + test.t.Log("") test.t.Log("======================== Test Run ========================") + if test.Command != nil { test.Command(test.Data, test.helpers).Run(test.Expected(test.Data, test.helpers)) } // Now go for the subtests + test.t.Log("") test.t.Log("======================== Processing subtests ========================") + for _, subTest := range test.SubTests { subTest.parent = test subTest.Run(test.t) diff --git a/mod/tigron/test/command.go b/mod/tigron/test/command.go index b3eaf5947fb..cca0771237c 100644 --- a/mod/tigron/test/command.go +++ b/mod/tigron/test/command.go @@ -65,7 +65,7 @@ type CustomizableCommand interface { read(key ConfigKey) ConfigValue } -// GenericCommand is a concrete Command implementation +// GenericCommand is a concrete Command implementation. type GenericCommand struct { Config Config TempDir string @@ -122,15 +122,20 @@ func (gc *GenericCommand) Run(expect *Expected) { gc.t.Helper() } - var result *icmd.Result - var env []string + var ( + result *icmd.Result + env []string + tty *os.File + psty *os.File + ) + output := &bytes.Buffer{} stdout := "" copyGroup := &errgroup.Group{} - var tty *os.File - var psty *os.File + if !gc.async { iCmdCmd := gc.boot() + if gc.pty { psty, tty, _ = pty.Open() _, _ = term.MakeRaw(int(tty.Fd())) @@ -141,6 +146,7 @@ func (gc *GenericCommand) Run(expect *Expected) { // Copy from the master copyGroup.Go(func() error { _, _ = io.Copy(output, psty) + return nil }) @@ -195,25 +201,29 @@ func (gc *GenericCommand) Run(expect *Expected) { if expect != nil { // Build the debug string - additionally attach the env (which iCmd does not do) debug := result.String() + "Env:\n" + strings.Join(env, "\n") + // ExitCode goes first - if expect.ExitCode == internal.ExitCodeNoCheck { //nolint:revive + switch expect.ExitCode { + case internal.ExitCodeNoCheck: // ExitCodeNoCheck means we do not care at all about exit code. It can be a failure, a success, or a timeout. - } else if expect.ExitCode == internal.ExitCodeGenericFail { + case internal.ExitCodeGenericFail: // ExitCodeGenericFail means we expect an error (excluding timeout). assert.Assert(gc.t, result.ExitCode != 0, - "Command succeeded while we were expecting an error\n"+debug) - } else if result.Timeout { + "Expected exit code to be different than 0\n"+debug) + case internal.ExitCodeTimeout: assert.Assert(gc.t, expect.ExitCode == internal.ExitCodeTimeout, "Command unexpectedly timed-out\n"+debug) - } else { + default: assert.Assert(gc.t, expect.ExitCode == result.ExitCode, fmt.Sprintf("Expected exit code: %d\n", expect.ExitCode)+debug) } + // Range through the expected errors and confirm they are seen on stderr for _, expectErr := range expect.Errors { assert.Assert(gc.t, strings.Contains(gc.rawStdErr, expectErr.Error()), fmt.Sprintf("Expected error: %q to be found in stderr\n", expectErr.Error())+debug) } + // Finally, check the output if we are asked to if expect.Output != nil { expect.Output(stdout, debug, gc.t) @@ -228,19 +238,22 @@ func (gc *GenericCommand) Stderr() string { func (gc *GenericCommand) Background(timeout time.Duration) { // Run it gc.async = true + i := gc.boot() + gc.timeout = timeout gc.result = icmd.StartCmd(i) } func (gc *GenericCommand) Signal(sig os.Signal) error { - return gc.result.Cmd.Process.Signal(sig) + return gc.result.Cmd.Process.Signal(sig) //nolint:wrapcheck } func (gc *GenericCommand) withEnv(env map[string]string) { if gc.Env == nil { gc.Env = map[string]string{} } + for k, v := range env { gc.Env[k] = v } @@ -262,43 +275,48 @@ func (gc *GenericCommand) PrependArgs(args ...string) { gc.prependArgs = append(gc.prependArgs, args...) } +//nolint:ireturn func (gc *GenericCommand) Clone() TestableCommand { // Copy the command and return a new one - with almost everything from the parent command - cc := *gc - cc.result = nil - cc.stdin = nil - cc.timeout = 0 - cc.rawStdErr = "" + com := *gc + com.result = nil + com.stdin = nil + com.timeout = 0 + com.rawStdErr = "" // Clone Env - cc.Env = make(map[string]string, len(gc.Env)) + com.Env = make(map[string]string, len(gc.Env)) for k, v := range gc.Env { - cc.Env[k] = v + com.Env[k] = v } - return &cc + + return &com } func (gc *GenericCommand) T() *testing.T { return gc.t } +//nolint:ireturn func (gc *GenericCommand) clear() TestableCommand { - cc := *gc - cc.mainBinary = "" - cc.helperBinary = "" - cc.mainArgs = []string{} - cc.prependArgs = []string{} - cc.helperArgs = []string{} + com := *gc + com.mainBinary = "" + com.helperBinary = "" + com.mainArgs = []string{} + com.prependArgs = []string{} + com.helperArgs = []string{} // Clone Env - cc.Env = make(map[string]string, len(gc.Env)) + com.Env = make(map[string]string, len(gc.Env)) // Reset configuration - cc.Config = &config{} + com.Config = &config{} for k, v := range gc.Env { - cc.Env[k] = v + com.Env[k] = v } - return &cc + + return &com } func (gc *GenericCommand) withT(t *testing.T) { + t.Helper() gc.t = t } @@ -317,7 +335,9 @@ func (gc *GenericCommand) boot() icmd.Cmd { } binary := gc.mainBinary + //nolint:gocritic args := append(gc.prependArgs, gc.mainArgs...) + if gc.helperBinary != "" { args = append([]string{binary}, args...) args = append(gc.helperArgs, args...) @@ -330,16 +350,20 @@ func (gc *GenericCommand) boot() icmd.Cmd { iCmdCmd := icmd.Command(binary, args...) iCmdCmd.Env = []string{} - for _, v := range os.Environ() { + + for _, envValue := range os.Environ() { add := true + for _, b := range gc.envBlackList { - if strings.HasPrefix(v, b+"=") { + if strings.HasPrefix(envValue, b+"=") { add = false + break } } + if add { - iCmdCmd.Env = append(iCmdCmd.Env, v) + iCmdCmd.Env = append(iCmdCmd.Env, envValue) } } diff --git a/mod/tigron/test/config.go b/mod/tigron/test/config.go index 6cf5ceb0c25..88001fec439 100644 --- a/mod/tigron/test/config.go +++ b/mod/tigron/test/config.go @@ -16,25 +16,32 @@ package test -// WithConfig returns a config object with a certain config property set +// WithConfig returns a config object with a certain config property set. +// +//nolint:ireturn func WithConfig(key ConfigKey, value ConfigValue) Config { cfg := &config{} cfg.Write(key, value) + return cfg } // Contains the implementation of the Config interface +//nolint:ireturn func configureConfig(cfg Config, parent Config) Config { if cfg == nil { cfg = &config{ config: make(map[ConfigKey]ConfigValue), } } + if parent != nil { // Note: implementation dependent + //nolint:forcetypeassert cfg.(*config).adopt(parent) } + return cfg } @@ -42,11 +49,14 @@ type config struct { config map[ConfigKey]ConfigValue } +//nolint:ireturn func (cfg *config) Write(key ConfigKey, value ConfigValue) Config { if cfg.config == nil { cfg.config = make(map[ConfigKey]ConfigValue) } + cfg.config[key] = value + return cfg } @@ -54,14 +64,17 @@ func (cfg *config) Read(key ConfigKey) ConfigValue { if cfg.config == nil { cfg.config = make(map[ConfigKey]ConfigValue) } + if val, ok := cfg.config[key]; ok { return val } + return "" } func (cfg *config) adopt(parent Config) { // Note: implementation dependent + //nolint:forcetypeassert for k, v := range parent.(*config).config { // Only copy keys that are not set already if _, ok := cfg.config[k]; !ok { diff --git a/mod/tigron/test/data.go b/mod/tigron/test/data.go index 97c7659c6d5..28e020b8d77 100644 --- a/mod/tigron/test/data.go +++ b/mod/tigron/test/data.go @@ -24,31 +24,46 @@ import ( "testing" ) -// WithData returns a data object with a certain key value set +const ( + identifierMaxLength = 76 +) + +// WithData returns a data object with a certain key value set. +// +//nolint:ireturn func WithData(key string, value string) Data { dat := &data{} dat.Set(key, value) + return dat } // Contains the implementation of the Data interface +//nolint:ireturn func configureData(t *testing.T, seedData Data, parent Data) Data { + t.Helper() + if seedData == nil { seedData = &data{} } + + //nolint:forcetypeassert dat := &data{ // Note: implementation dependent labels: seedData.(*data).labels, tempDir: t.TempDir(), testID: func(suffix ...string) string { suffix = append([]string{t.Name()}, suffix...) + return defaultIdentifierHashing(suffix...) }, } + if parent != nil { dat.adopt(parent) } + return dat } @@ -62,11 +77,14 @@ func (dt *data) Get(key string) string { return dt.labels[key] } +//nolint:ireturn func (dt *data) Set(key string, value string) Data { if dt.labels == nil { dt.labels = map[string]string{} } + dt.labels[key] = value + return dt } @@ -80,6 +98,7 @@ func (dt *data) TempDir() string { func (dt *data) adopt(parent Data) { // Note: implementation dependent + //nolint:forcetypeassert for k, v := range parent.(*data).labels { // Only copy keys that are not set already if _, ok := dt.labels[k]; !ok { @@ -93,19 +112,26 @@ func defaultIdentifierHashing(names ...string) string { // So, the rules are stringent on what it can contain. replaceWith := []byte("-") name := strings.ToLower(strings.Join(names, string(replaceWith))) - // Ensure we have a unique identifier despite characters replacements (well, as unique as the names collection being passed) + // Ensure we have a unique identifier despite characters replacements + // (well, as unique as the names collection being passed) signature := fmt.Sprintf("%x", sha256.Sum256([]byte(name)))[0:8] // Make sure we do not use any unsafe characters safeName := regexp.MustCompile(`[^a-z0-9-]+`) // And we avoid repeats of the separator noRepeat := regexp.MustCompile(fmt.Sprintf(`[%s]{2,}`, replaceWith)) - sn := safeName.ReplaceAll([]byte(name), replaceWith) - sn = noRepeat.ReplaceAll(sn, replaceWith) + escapedName := safeName.ReplaceAll([]byte(name), replaceWith) + escapedName = noRepeat.ReplaceAll(escapedName, replaceWith) // Do not allow trailing or leading dash (as that may stutter) - name = strings.Trim(string(sn), string(replaceWith)) + name = strings.Trim(string(escapedName), string(replaceWith)) + // Ensure we will never go above 76 characters in length (with signature) - if len(name) > 67 { + if len(name) > (identifierMaxLength - len(signature)) { name = name[0:67] } - return name + "-" + signature + + if name[len(name)-1:] != "-" { + signature = "-" + signature + } + + return name + signature } diff --git a/mod/tigron/test/expected.go b/mod/tigron/test/expected.go index 3ef181a7009..67d8bfa80f3 100644 --- a/mod/tigron/test/expected.go +++ b/mod/tigron/test/expected.go @@ -16,14 +16,16 @@ package test -// RunCommand is the simplest way to express a test.TestableCommand for very basic cases when access to test data is not necessary +// Command is the simplest way to express a test.TestableCommand for very basic cases +// where access to test data is not necessary. func Command(args ...string) Executor { - return func(data Data, helpers Helpers) TestableCommand { + return func(_ Data, helpers Helpers) TestableCommand { return helpers.Command(args...) } } -// Expects is provided as a simple helper covering "expectations" for simple use-cases where access to the test data is not necessary +// Expects is provided as a simple helper covering "expectations" for simple use-cases +// where access to the test data is not necessary. func Expects(exitCode int, errors []error, output Comparator) Manager { return func(_ Data, _ Helpers) *Expected { return &Expected{ diff --git a/mod/tigron/test/helpers.go b/mod/tigron/test/helpers.go index d09443b3f0b..3cdfdc2ebea 100644 --- a/mod/tigron/test/helpers.go +++ b/mod/tigron/test/helpers.go @@ -30,56 +30,68 @@ type helpersInternal struct { t *testing.T } -// Ensure will run a command and make sure it is successful +// Ensure will run a command and make sure it is successful. func (help *helpersInternal) Ensure(args ...string) { help.Command(args...).Run(&Expected{ - ExitCode: 0, + ExitCode: internal.ExitCodeSuccess, }) } -// Anyhow will run a command regardless of outcome (may or may not fail) +// Anyhow will run a command regardless of outcome (may or may not fail). func (help *helpersInternal) Anyhow(args ...string) { - help.Command(args...).Run(nil) + help.Command(args...).Run(&Expected{ + ExitCode: internal.ExitCodeNoCheck, + }) } -// Fail will run a command and make sure it does fail +// Fail will run a command and make sure it does fail. func (help *helpersInternal) Fail(args ...string) { help.Command(args...).Run(&Expected{ ExitCode: internal.ExitCodeGenericFail, }) } -// Capture will run a command, ensure it is successful and return stdout +// Capture will run a command, ensure it is successful and return stdout. func (help *helpersInternal) Capture(args ...string) string { var ret string + help.Command(args...).Run(&Expected{ - Output: func(stdout string, info string, t *testing.T) { + //nolint:thelper + Output: func(stdout string, _ string, _ *testing.T) { ret = stdout }, }) + return ret } -// Err will run a command, ensure it is successful and return stdout +// Err will run a command with no expectation and return Stderr. func (help *helpersInternal) Err(args ...string) string { cmd := help.Command(args...) cmd.Run(nil) + return cmd.Stderr() } -// Command will return a clone of your base command without running it +// Command will return a clone of your base command without running it. +// +//nolint:ireturn func (help *helpersInternal) Command(args ...string) TestableCommand { cc := help.cmdInternal.Clone() cc.WithArgs(args...) + return cc } // Custom will return a command for the requested binary and args, with the environment of your test // (eg: Env, Cwd, etc.) +// +//nolint:ireturn func (help *helpersInternal) Custom(binary string, args ...string) TestableCommand { cc := help.cmdInternal.clear() cc.WithBinary(binary) cc.WithArgs(args...) + return cc } diff --git a/mod/tigron/test/interfaces.go b/mod/tigron/test/interfaces.go index d83b3117887..70f11007911 100644 --- a/mod/tigron/test/interfaces.go +++ b/mod/tigron/test/interfaces.go @@ -74,7 +74,7 @@ type Helpers interface { // A TestableCommand can be used as a Case Command obviously, but also as part of a Setup or Cleanup routine, // and as the basis of any type of helper. // For more powerful use-cases outside of test cases, see below CustomizableCommand. -type TestableCommand interface { +type TestableCommand interface { //nolint:interfacebloat // WithBinary specifies what binary to execute WithBinary(binary string) // WithArgs specifies the args to pass to the binary. Note that WithArgs can be used multiple times and is additive. diff --git a/mod/tigron/test/test.go b/mod/tigron/test/test.go index d1d2490e05a..00cf7b708fc 100644 --- a/mod/tigron/test/test.go +++ b/mod/tigron/test/test.go @@ -25,9 +25,10 @@ type Testable interface { AmbientRequirements(testCase *Case, t *testing.T) } -var ( - registeredTestable Testable -) +// FIXME +// +//nolint:gochecknoglobals +var registeredTestable Testable func Customize(testable Testable) { registeredTestable = testable diff --git a/mod/tigron/utils/utilities.go b/mod/tigron/utils/utilities.go index ea8fa458bc1..c11c9fed8ab 100644 --- a/mod/tigron/utils/utilities.go +++ b/mod/tigron/utils/utilities.go @@ -19,18 +19,20 @@ package utils import ( "crypto/rand" "encoding/base64" - "fmt" ) -// RandomStringBase64 generates a base64 encoded random string -func RandomStringBase64(n int) string { - b := make([]byte, n) - l, err := rand.Read(b) +// RandomStringBase64 generates a base64 encoded random string. +func RandomStringBase64(desiredLength int) string { + randomBytes := make([]byte, desiredLength) + + randomLength, err := rand.Read(randomBytes) if err != nil { panic(err) } - if l != n { - panic(fmt.Errorf("expected %d bytes, got %d bytes", n, l)) + + if randomLength != desiredLength { + panic("rand failing") } - return base64.URLEncoding.EncodeToString(b) + + return base64.URLEncoding.EncodeToString(randomBytes) } From 62c1565bf8a4369038abb408497edf6c2c007991 Mon Sep 17 00:00:00 2001 From: apostasie Date: Sun, 2 Mar 2025 19:18:06 -0800 Subject: [PATCH 4/4] Add golangci and yamllint to tigron Signed-off-by: apostasie --- mod/tigron/.golangci.yml | 54 ++++++++++++++++++++++++++++++++++++++++ mod/tigron/.yamllint | 13 ++++++++++ 2 files changed, 67 insertions(+) create mode 100644 mod/tigron/.golangci.yml create mode 100644 mod/tigron/.yamllint diff --git a/mod/tigron/.golangci.yml b/mod/tigron/.golangci.yml new file mode 100644 index 00000000000..66e0b8d2ade --- /dev/null +++ b/mod/tigron/.golangci.yml @@ -0,0 +1,54 @@ +--- +output: + sort-results: true + +issues: + max-issues-per-linter: 0 + max-same-issues: 0 + +run: + concurrency: 0 + timeout: 5m + issues-exit-code: 2 + tests: true + modules-download-mode: readonly + allow-parallel-runners: true + allow-serial-runners: true + +linters: + disable-all: false + enable-all: true + disable: + # Opting-out + - nonamedreturns # named returns are occasionally useful + - exhaustruct # does not serve much of a purpose + - funlen # not interested + - cyclop # not interested much + - godox # having these are useful + # Duplicating + - gci # we use go-imports instead + # Deprecated + - tenv + # TODO: Temporarily out until we wrap up all of them +# - wrapcheck + +linters-settings: + staticcheck: + checks: + - "all" + + depguard: + rules: + main: + files: + - "$all" + allow: + - $gostd + - "github.com/containerd/nerdctl/mod/tigron" + # WATCHOUT! https://github.com/OpenPeeDeeP/depguard/issues/108 + # Currently, depguard will fail detecting any dependency starting with a standard package name as third-party. + # Thus, the following three are allowed provisionally, though currently not "necessary". + - "golang.org/x/sync" + - "golang.org/x/term" + - "gotest.tools" + - "github.com/creack/pty" diff --git a/mod/tigron/.yamllint b/mod/tigron/.yamllint new file mode 100644 index 00000000000..17ba29ddcfd --- /dev/null +++ b/mod/tigron/.yamllint @@ -0,0 +1,13 @@ +--- + +extends: default + +rules: + indentation: + spaces: 2 + indent-sequences: consistent + truthy: + allowed-values: ['true', 'false', 'on', 'off'] + comments-indentation: disable + document-start: disable + line-length: disable