From 322116c510e52955885d4ec3b8803ed0ec2fb4cb Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 11 Aug 2023 13:30:10 +0100 Subject: [PATCH 1/2] chore: fix revive lint for golangci-lint 1.54.0 grep `FIXME` for additional tech-debt / issues found through the revive linting. Signed-off-by: Dave Dykstra <2129743+DrDaveD@users.noreply.github.com> --- cmd/internal/cli/key_pull.go | 2 +- cmd/internal/cli/pull.go | 6 +++--- internal/app/apptainer/keyserver_list.go | 3 +++ internal/pkg/build/apps/apps.go | 11 ++++++----- internal/pkg/build/sources/conveyorPacker_arch.go | 8 +++++--- internal/pkg/build/sources/conveyorPacker_busybox.go | 2 ++ internal/pkg/build/sources/conveyorPacker_yum.go | 2 ++ internal/pkg/build/sources/conveyorPacker_zypper.go | 2 ++ internal/pkg/client/library/pull.go | 5 +---- internal/pkg/client/net/pull.go | 2 +- internal/pkg/client/oras/oras.go | 6 ++++++ internal/pkg/client/oras/pull.go | 2 +- internal/pkg/client/shub/pull.go | 2 +- internal/pkg/image/packer/gocryptfs.go | 5 +---- internal/pkg/image/unpacker/squashfs_test.go | 8 ++++++-- internal/pkg/remote/credential/login_handler.go | 3 +-- internal/pkg/remote/remote_test.go | 6 +++--- .../pkg/runtime/engine/apptainer/container_linux.go | 4 ++-- .../pkg/runtime/engine/apptainer/process_linux.go | 8 ++++---- internal/pkg/sypgp/sypgp.go | 2 +- internal/pkg/sypgp/sypgp_test.go | 2 +- pkg/util/capabilities/config_test.go | 6 +++--- 22 files changed, 56 insertions(+), 41 deletions(-) diff --git a/cmd/internal/cli/key_pull.go b/cmd/internal/cli/key_pull.go index ef05a99b88..029b0ef08e 100644 --- a/cmd/internal/cli/key_pull.go +++ b/cmd/internal/cli/key_pull.go @@ -62,7 +62,7 @@ func doKeyPullCmd(ctx context.Context, fingerprint string, co ...client.Option) keyring := sypgp.NewHandle(path, opts...) // get matching keyring - el, err := sypgp.FetchPubkey(ctx, fingerprint, false, co...) + el, err := sypgp.FetchPubkey(ctx, fingerprint, co...) if err != nil { return fmt.Errorf("unable to pull key from server: %v", err) } diff --git a/cmd/internal/cli/pull.go b/cmd/internal/cli/pull.go index b29c82cf71..68674a5afe 100644 --- a/cmd/internal/cli/pull.go +++ b/cmd/internal/cli/pull.go @@ -259,7 +259,7 @@ func pullRun(cmd *cobra.Command, args []string) { sylog.Warningf("Skipping container verification") } case ShubProtocol: - _, err := shub.PullToFile(ctx, imgCache, pullTo, pullFrom, tmpDir, noHTTPS) + _, err := shub.PullToFile(ctx, imgCache, pullTo, pullFrom, noHTTPS) if err != nil { sylog.Fatalf("While pulling shub image: %v\n", err) } @@ -269,12 +269,12 @@ func pullRun(cmd *cobra.Command, args []string) { sylog.Fatalf("Unable to make docker oci credentials: %s", err) } - _, err = oras.PullToFile(ctx, imgCache, pullTo, pullFrom, tmpDir, ociAuth, noHTTPS, reqAuthFile) + _, err = oras.PullToFile(ctx, imgCache, pullTo, pullFrom, ociAuth, noHTTPS, reqAuthFile) if err != nil { sylog.Fatalf("While pulling image from oci registry: %v", err) } case HTTPProtocol, HTTPSProtocol: - _, err := net.PullToFile(ctx, imgCache, pullTo, pullFrom, tmpDir) + _, err := net.PullToFile(ctx, imgCache, pullTo, pullFrom) if err != nil { sylog.Fatalf("While pulling from image from http(s): %v\n", err) } diff --git a/internal/app/apptainer/keyserver_list.go b/internal/app/apptainer/keyserver_list.go index e76f7c19ac..9e816e910a 100644 --- a/internal/app/apptainer/keyserver_list.go +++ b/internal/app/apptainer/keyserver_list.go @@ -22,6 +22,9 @@ import ( ) // KeyserverList prints information about remote configurations +// FIXME - remoteName is not being honored +// +//nolint:revive func KeyserverList(remoteName string, usrConfigFile string) (err error) { c := &remote.Config{} diff --git a/internal/pkg/build/apps/apps.go b/internal/pkg/build/apps/apps.go index 8784a97887..3decc7a537 100644 --- a/internal/pkg/build/apps/apps.go +++ b/internal/pkg/build/apps/apps.go @@ -397,7 +397,7 @@ func copyFiles(b *types.Bundle, a *App) error { dst = splitLine[1] } - if err := copyFile(src, filepath.Join(appBase, dst)); err != nil { + if err := copyWithfLr(src, filepath.Join(appBase, dst)); err != nil { return err } } @@ -457,17 +457,18 @@ func appData(b *types.Bundle, a *App) string { return filepath.Join(b.RootfsPath, "/scif/data/", a.Name) } -func copyFile(src, dst string) error { +// FIXME: Replace with Go, or existing util function? +func copyWithfLr(src, dst string) error { cp, err := bin.FindBin("cp") if err != nil { return err } var stderr bytes.Buffer - copyCommand := exec.Command(cp, "-fLr", src, dst) - copyCommand.Stderr = &stderr + copyCmd := exec.Command(cp, "-fLr", src, dst) + copyCmd.Stderr = &stderr sylog.Debugf("Copying %v to %v", src, dst) - if err := copyCommand.Run(); err != nil { + if err := copyCmd.Run(); err != nil { return fmt.Errorf("while copying %v to %v: %v: %v", src, dst, err, stderr.String()) } diff --git a/internal/pkg/build/sources/conveyorPacker_arch.go b/internal/pkg/build/sources/conveyorPacker_arch.go index 8e98e73044..817c89fae2 100644 --- a/internal/pkg/build/sources/conveyorPacker_arch.go +++ b/internal/pkg/build/sources/conveyorPacker_arch.go @@ -45,7 +45,7 @@ type ArchConveyorPacker struct { // prepareFakerootEnv prepares a build environment to // make fakeroot working with pacstrap. -func (cp *ArchConveyorPacker) prepareFakerootEnv(_ context.Context) (func(), error) { +func (cp *ArchConveyorPacker) prepareFakerootEnv() (func(), error) { truePath, err := bin.FindBin("true") if err != nil { return nil, fmt.Errorf("while searching true command: %s", err) @@ -113,7 +113,9 @@ func (cp *ArchConveyorPacker) prepareFakerootEnv(_ context.Context) (func(), err } // Get just stores the source -func (cp *ArchConveyorPacker) Get(ctx context.Context, b *types.Bundle) (err error) { +// +// FIXME: use context for cancellation. +func (cp *ArchConveyorPacker) Get(_ context.Context, b *types.Bundle) (err error) { cp.b = b err = cp.getBootstrapOptions() @@ -139,7 +141,7 @@ func (cp *ArchConveyorPacker) Get(ctx context.Context, b *types.Bundle) (err err insideUserNs, setgroupsAllowed := namespaces.IsInsideUserNamespace(os.Getpid()) if insideUserNs && setgroupsAllowed { - umountFn, err := cp.prepareFakerootEnv(ctx) + umountFn, err := cp.prepareFakerootEnv() if umountFn != nil { defer umountFn() } diff --git a/internal/pkg/build/sources/conveyorPacker_busybox.go b/internal/pkg/build/sources/conveyorPacker_busybox.go index 097b072178..66e5c0a2d1 100644 --- a/internal/pkg/build/sources/conveyorPacker_busybox.go +++ b/internal/pkg/build/sources/conveyorPacker_busybox.go @@ -34,6 +34,8 @@ type BusyBoxConveyorPacker struct { } // Get just stores the source +// +// FIXME: use context for cancellation. func (c *BusyBoxConveyor) Get(_ context.Context, b *types.Bundle) (err error) { c.b = b diff --git a/internal/pkg/build/sources/conveyorPacker_yum.go b/internal/pkg/build/sources/conveyorPacker_yum.go index e17dd270f6..c9b4387984 100644 --- a/internal/pkg/build/sources/conveyorPacker_yum.go +++ b/internal/pkg/build/sources/conveyorPacker_yum.go @@ -48,6 +48,8 @@ type YumConveyorPacker struct { } // Get downloads container information from the specified source +// +// FIXME: use context for cancellation. func (c *YumConveyor) Get(_ context.Context, b *types.Bundle) (err error) { c.b = b diff --git a/internal/pkg/build/sources/conveyorPacker_zypper.go b/internal/pkg/build/sources/conveyorPacker_zypper.go index 07cff373d3..efc4d37334 100644 --- a/internal/pkg/build/sources/conveyorPacker_zypper.go +++ b/internal/pkg/build/sources/conveyorPacker_zypper.go @@ -60,6 +60,8 @@ func machine() (string, error) { // Get downloads container information from the specified source // +// FIXME: use context for cancellation. +// //nolint:maintidx func (cp *ZypperConveyorPacker) Get(_ context.Context, b *types.Bundle) error { var suseconnectProduct, suseconnectModver string diff --git a/internal/pkg/client/library/pull.go b/internal/pkg/client/library/pull.go index b4a9c64388..f496c2ae10 100644 --- a/internal/pkg/client/library/pull.go +++ b/internal/pkg/client/library/pull.go @@ -100,10 +100,7 @@ func downloadWrapper(ctx context.Context, c *libClient.Client, imagePath, arch s } }(time.Now()) - if err := DownloadImage(ctx, c, imagePath, arch, libraryRef, pb); err != nil { - return err - } - return nil + return DownloadImage(ctx, c, imagePath, arch, libraryRef, pb) } // Pull will pull a library image to the cache or direct to a temporary file if cache is disabled diff --git a/internal/pkg/client/net/pull.go b/internal/pkg/client/net/pull.go index d3eed00365..a593180aeb 100644 --- a/internal/pkg/client/net/pull.go +++ b/internal/pkg/client/net/pull.go @@ -193,7 +193,7 @@ func Pull(ctx context.Context, imgCache *cache.Handle, pullFrom string, tmpDir s } // PullToFile will pull an http(s) image to the specified location, through the cache, or directly if cache is disabled -func PullToFile(ctx context.Context, imgCache *cache.Handle, pullTo, pullFrom, _ string) (imagePath string, err error) { +func PullToFile(ctx context.Context, imgCache *cache.Handle, pullTo, pullFrom string) (imagePath string, err error) { directTo := "" if imgCache.IsDisabled() { directTo = pullTo diff --git a/internal/pkg/client/oras/oras.go b/internal/pkg/client/oras/oras.go index f603e36090..b0c2afa251 100644 --- a/internal/pkg/client/oras/oras.go +++ b/internal/pkg/client/oras/oras.go @@ -34,6 +34,8 @@ import ( ) // DownloadImage downloads a SIF image specified by an oci reference to a file using the included credentials +// +// FIXME: use context for cancellation. func DownloadImage(ctx context.Context, path, ref string, ociAuth *authn.AuthConfig, noHTTPS bool, reqAuthFile string) error { rt := client.NewRoundTripper(ctx, nil) im, err := remoteImage(ref, ociAuth, noHTTPS, rt, reqAuthFile) @@ -115,6 +117,8 @@ func DownloadImage(ctx context.Context, path, ref string, ociAuth *authn.AuthCon // UploadImage uploads the image specified by path and pushes it to the provided oci reference, // it will use credentials if supplied +// +// FIXME: use context for cancellation. func UploadImage(_ context.Context, path, ref string, ociAuth *authn.AuthConfig, noHTTPS bool, reqAuthFile string) error { // ensure that are uploading a SIF if err := ensureSIF(path); err != nil { @@ -187,6 +191,8 @@ func ensureSIF(filepath string) error { } // RefHash returns the digest of the SIF layer of the OCI manifest for supplied ref +// +// FIXME: use context for cancellation. func RefHash(_ context.Context, ref string, ociAuth *authn.AuthConfig, noHTTPS bool, reqAuthFile string) (v1.Hash, error) { im, err := remoteImage(ref, ociAuth, noHTTPS, nil, reqAuthFile) if err != nil { diff --git a/internal/pkg/client/oras/pull.go b/internal/pkg/client/oras/pull.go index e3a84f0749..4eba940466 100644 --- a/internal/pkg/client/oras/pull.go +++ b/internal/pkg/client/oras/pull.go @@ -83,7 +83,7 @@ func Pull(ctx context.Context, imgCache *cache.Handle, pullFrom, tmpDir string, } // PullToFile will pull an oras image to the specified location, through the cache, or directly if cache is disabled -func PullToFile(ctx context.Context, imgCache *cache.Handle, pullTo, pullFrom, _ string, ociAuth *authn.AuthConfig, noHTTPS bool, reqAuthFile string) (imagePath string, err error) { +func PullToFile(ctx context.Context, imgCache *cache.Handle, pullTo, pullFrom string, ociAuth *authn.AuthConfig, noHTTPS bool, reqAuthFile string) (imagePath string, err error) { directTo := "" if imgCache.IsDisabled() { directTo = pullTo diff --git a/internal/pkg/client/shub/pull.go b/internal/pkg/client/shub/pull.go index 7af0b7c743..2c31426337 100644 --- a/internal/pkg/client/shub/pull.go +++ b/internal/pkg/client/shub/pull.go @@ -196,7 +196,7 @@ func Pull(ctx context.Context, imgCache *cache.Handle, pullFrom, tmpDir string, } // PullToFile will pull a shub image to the specified location, through the cache, or directly if cache is disabled -func PullToFile(ctx context.Context, imgCache *cache.Handle, pullTo, pullFrom, _ string, noHTTPS bool) (imagePath string, err error) { +func PullToFile(ctx context.Context, imgCache *cache.Handle, pullTo, pullFrom string, noHTTPS bool) (imagePath string, err error) { directTo := "" if imgCache.IsDisabled() { directTo = pullTo diff --git a/internal/pkg/image/packer/gocryptfs.go b/internal/pkg/image/packer/gocryptfs.go index 5e552d8d8e..a790edc2dc 100644 --- a/internal/pkg/image/packer/gocryptfs.go +++ b/internal/pkg/image/packer/gocryptfs.go @@ -195,8 +195,5 @@ func cmdRun(commands []string, stdout, stderr io.Writer) error { if stderr != nil { cmd.Stderr = stderr } - if err := cmd.Run(); err != nil { - return err - } - return nil + return cmd.Run() } diff --git a/internal/pkg/image/unpacker/squashfs_test.go b/internal/pkg/image/unpacker/squashfs_test.go index 0c4b1ae722..d3c5dfa419 100644 --- a/internal/pkg/image/unpacker/squashfs_test.go +++ b/internal/pkg/image/unpacker/squashfs_test.go @@ -53,14 +53,18 @@ func TestSquashfs(t *testing.T) { }) } -func testSquashfs(t *testing.T, _ string) { +func testSquashfs(t *testing.T, tmpParent string) { s := NewSquashfs() if !s.HasUnsquashfs() { t.Skip("unsquashfs not found") } - dir := t.TempDir() + dir, err := os.MkdirTemp(tmpParent, "test-squashfs-") + if err != nil { + t.Fatalf("while creating tmpdir: %v", err) + } + defer os.RemoveAll(dir) // create archive with files present in this directory archive := createArchive(t) diff --git a/internal/pkg/remote/credential/login_handler.go b/internal/pkg/remote/credential/login_handler.go index ff4725c402..e1dac637ad 100644 --- a/internal/pkg/remote/credential/login_handler.go +++ b/internal/pkg/remote/credential/login_handler.go @@ -172,7 +172,6 @@ func (h *keyserverHandler) login(u *url.URL, username, password string, insecure }, nil } -//nolint:revive,nolintlint -func (h *keyserverHandler) logout(u *url.URL, reqAuthFile string) error { +func (h *keyserverHandler) logout(_ *url.URL, _ string) error { return nil } diff --git a/internal/pkg/remote/remote_test.go b/internal/pkg/remote/remote_test.go index 58c076f7e2..52f2a56272 100644 --- a/internal/pkg/remote/remote_test.go +++ b/internal/pkg/remote/remote_test.go @@ -93,13 +93,13 @@ func TestWriteToReadFrom(t *testing.T) { test.c.WriteTo(&r) - item, err := ReadFrom(&r) + newConfig, err := ReadFrom(&r) if err != nil { t.Errorf("unexpected failure running %s test: %s", test.name, err) } - if !reflect.DeepEqual(test.c, *item) { - t.Errorf("failed to read/write config:\n\thave: %v\n\twant: %v", test.c, *item) + if !reflect.DeepEqual(test.c, *newConfig) { + t.Errorf("failed to read/write config:\n\thave: %v\n\twant: %v", test.c, *newConfig) } }) } diff --git a/internal/pkg/runtime/engine/apptainer/container_linux.go b/internal/pkg/runtime/engine/apptainer/container_linux.go index 53b6b4dc98..a5d0ddd0aa 100644 --- a/internal/pkg/runtime/engine/apptainer/container_linux.go +++ b/internal/pkg/runtime/engine/apptainer/container_linux.go @@ -1945,7 +1945,7 @@ func (c *container) addHomeLayer(system *mount.System, source, dest string) erro // addHomeNoLayer is responsible for staging the home directory and adding the base // directory of the staged home into the container when overlay/underlay are unavailable -func (c *container) addHomeNoLayer(system *mount.System, _, dest string) error { +func (c *container) addHomeNoLayer(system *mount.System, dest string) error { flags := uintptr(syscall.MS_BIND | syscall.MS_REC) homeBase := fs.RootDir(dest) @@ -1999,7 +1999,7 @@ func (c *container) addHomeMount(system *mount.System) error { sessionLayer := c.engine.EngineConfig.GetSessionLayer() sylog.Debugf("Adding home directory mount [%v:%v] to list using layer: %s\n", stagingDir, dest, sessionLayer) if !c.isLayerEnabled() { - return c.addHomeNoLayer(system, stagingDir, dest) + return c.addHomeNoLayer(system, dest) } return c.addHomeLayer(system, stagingDir, dest) } diff --git a/internal/pkg/runtime/engine/apptainer/process_linux.go b/internal/pkg/runtime/engine/apptainer/process_linux.go index d443b2445e..11bd19b158 100644 --- a/internal/pkg/runtime/engine/apptainer/process_linux.go +++ b/internal/pkg/runtime/engine/apptainer/process_linux.go @@ -717,7 +717,7 @@ func injectEnvHandler(senv map[string]string, noEval bool) interpreter.OpenHandl } } -func runtimeVarsHandler(_ map[string]string) interpreter.OpenHandler { +func runtimeVarsHandler() interpreter.OpenHandler { var once sync.Once return func(_ string, _ int, _ os.FileMode) (io.ReadWriteCloser, error) { @@ -752,7 +752,7 @@ func sylogBuiltin(_ context.Context, argv []string) error { } // getAllEnvBuiltin display all exported variables in the form KEY=VALUE. -func getAllEnvBuiltin(_ *interpreter.Shell) interpreter.ShellBuiltin { +func getAllEnvBuiltin() interpreter.ShellBuiltin { return func(ctx context.Context, _ []string) error { hc := interp.HandlerCtx(ctx) @@ -892,10 +892,10 @@ func runActionScript(engineConfig *apptainerConfig.EngineConfig) ([]string, []st senv := engineConfig.GetApptainerEnv() shell.RegisterOpenHandler("/.inject-apptainer-env.sh", injectEnvHandler(senv, engineConfig.GetNoEval())) - shell.RegisterOpenHandler("/.singularity.d/env/99-runtimevars.sh", runtimeVarsHandler(senv)) + shell.RegisterOpenHandler("/.singularity.d/env/99-runtimevars.sh", runtimeVarsHandler()) // register few builtin - shell.RegisterShellBuiltin("getallenv", getAllEnvBuiltin(shell)) + shell.RegisterShellBuiltin("getallenv", getAllEnvBuiltin()) shell.RegisterShellBuiltin("sylog", sylogBuiltin) shell.RegisterShellBuiltin("fixpath", fixPathBuiltin) shell.RegisterShellBuiltin("hash", hashBuiltin) diff --git a/internal/pkg/sypgp/sypgp.go b/internal/pkg/sypgp/sypgp.go index ea4629c0b9..8b77e5e945 100644 --- a/internal/pkg/sypgp/sypgp.go +++ b/internal/pkg/sypgp/sypgp.go @@ -902,7 +902,7 @@ func date(s string) string { } // FetchPubkey pulls a public key from the Key Service. -func FetchPubkey(ctx context.Context, fingerprint string, _ bool, opts ...client.Option) (openpgp.EntityList, error) { +func FetchPubkey(ctx context.Context, fingerprint string, opts ...client.Option) (openpgp.EntityList, error) { // Decode fingerprint and ensure proper length. var fp []byte fp, err := hex.DecodeString(fingerprint) diff --git a/internal/pkg/sypgp/sypgp_test.go b/internal/pkg/sypgp/sypgp_test.go index 6aa0135037..bf1897fa48 100644 --- a/internal/pkg/sypgp/sypgp_test.go +++ b/internal/pkg/sypgp/sypgp_test.go @@ -136,7 +136,7 @@ func TestFetchPubkey(t *testing.T) { client.OptHTTPClient(srv.Client()), } - el, err := FetchPubkey(context.Background(), tt.fingerprint, false, opts...) + el, err := FetchPubkey(context.Background(), tt.fingerprint, opts...) if (err != nil) != tt.wantErr { t.Fatalf("unexpected error: %v", err) return diff --git a/pkg/util/capabilities/config_test.go b/pkg/util/capabilities/config_test.go index 3a51ddaf0d..36ece3f07d 100644 --- a/pkg/util/capabilities/config_test.go +++ b/pkg/util/capabilities/config_test.go @@ -50,13 +50,13 @@ func TestReadFromWriteTo(t *testing.T) { test.c.WriteTo(&r) - item, err := ReadFrom(&r) + newConfig, err := ReadFrom(&r) if err != nil { t.Errorf("unexpected failure running %s test: %s", test.name, err) } - if !reflect.DeepEqual(test.c, *item) { - t.Errorf("failed to read/write config:\n\thave: %v\n\twant: %v", test.c, *item) + if !reflect.DeepEqual(test.c, *newConfig) { + t.Errorf("failed to read/write config:\n\thave: %v\n\twant: %v", test.c, *newConfig) } }) } From 705b51bfea2f697a5ec35b56a21e31d358349ab4 Mon Sep 17 00:00:00 2001 From: Dave Dykstra <2129743+DrDaveD@users.noreply.github.com> Date: Fri, 13 Sep 2024 17:13:46 -0500 Subject: [PATCH 2/2] remove unnecessary nolint:revive Signed-off-by: Dave Dykstra <2129743+DrDaveD@users.noreply.github.com> --- internal/app/apptainer/keyserver_list.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/app/apptainer/keyserver_list.go b/internal/app/apptainer/keyserver_list.go index 9e816e910a..394e4ea14b 100644 --- a/internal/app/apptainer/keyserver_list.go +++ b/internal/app/apptainer/keyserver_list.go @@ -23,8 +23,6 @@ import ( // KeyserverList prints information about remote configurations // FIXME - remoteName is not being honored -// -//nolint:revive func KeyserverList(remoteName string, usrConfigFile string) (err error) { c := &remote.Config{}