fix: miscellaneous fixes for ai-reported suggestions (#4101)

* test(cli): replace panic with t.Fatalf in deprecated config tests

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

* fix(trivy): keep sbom generation failures non-fatal in runTrivy

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

* docs(meta): fix typos in hooks comments

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

* refactor(requestcontext): align package name and godoc comments

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

* test(gc): factor metrics setup helper and fix typo

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

* test(trivy): cover non-fatal SBOM generation failures

- add runTrivy test ensuring scan succeeds when SBOM generation fails

- inject artifact runner constructor for deterministic internal testing

- fix matchesRepo doc comment to be action-agnostic

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

---------

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

Author: rchincha

pkg/cli/client/config_cmd_deprecated_test.go

@@ -82,7 +82,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) {
 
 		actual, err := os.ReadFile(configPath)
 		if err != nil {
-			panic(err)
+			t.Fatalf("failed to read config file %s: %v", configPath, err)
 		}
 		actualStr := string(actual)
 		So(actualStr, ShouldContainSubstring, "configtest1")
@@ -405,7 +405,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) {
 
 		actual, err := os.ReadFile(configPath)
 		if err != nil {
-			panic(err)
+			t.Fatalf("failed to read config file %s: %v", configPath, err)
 		}
 		actualStr := string(actual)
 		So(actualStr, ShouldContainSubstring, "https://test-url.com")
@@ -450,7 +450,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) {
 
 		actual, err := os.ReadFile(configPath)
 		if err != nil {
-			panic(err)
+			t.Fatalf("failed to read config file %s: %v", configPath, err)
 		}
 		actualStr := string(actual)
 		So(actualStr, ShouldContainSubstring, `https://new-url.com`)
@@ -477,7 +477,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) {
 
 		actual, err := os.ReadFile(configPath)
 		if err != nil {
-			panic(err)
+			t.Fatalf("failed to read config file %s: %v", configPath, err)
 		}
 		actualStr := string(actual)
 		So(actualStr, ShouldNotContainSubstring, "showspinner")

pkg/extensions/search/cve/trivy/scanner.go

@@ -57,6 +57,8 @@ const (
 
 var errImageStoreNotFound = errors.New("image store not found")
 
+var newArtifactRunner = artifact.NewRunner //nolint:gochecknoglobals // test seam for deterministic runner injection
+
 // getNewScanOptions sets trivy configuration values for our scans and returns them as
 // a trivy Options structure.
 func getNewScanOptions(dir string, dbRepositoryRef, javaDBRepositoryRef name.Reference,
@@ -330,7 +332,7 @@ func (scanner Scanner) runTrivy(ctx context.Context, opts flag.Options) (types.R
 	var sbom *generatedSBOM
 
 	err = scanner.withTempDir(func() error {
-		runner, err := artifact.NewRunner(ctx, opts, artifact.TargetContainerImage)
+		runner, err := newArtifactRunner(ctx, opts, artifact.TargetContainerImage)
 		if err != nil {
 			return err
 		}
@@ -347,13 +349,14 @@ func (scanner Scanner) runTrivy(ctx context.Context, opts flag.Options) (types.R
 		}
 
 		if scanner.sbomOptions.enabled {
-			sbom, err = scanner.generateSBOM(ctx, runner, opts, report)
-			if err != nil {
-				scanner.log.Warn().Err(err).Str("image", opts.ImageOptions.Input).Msg("failed to generate sbom")
+			var sbomErr error
+			sbom, sbomErr = scanner.generateSBOM(ctx, runner, opts, report)
+			if sbomErr != nil {
+				scanner.log.Warn().Err(sbomErr).Str("image", opts.ImageOptions.Input).Msg("failed to generate sbom")
 			}
 		}
 
-		return err
+		return nil
 	})
 
 	return report, sbom, err

pkg/extensions/search/cve/trivy/scanner_internal_test.go

@@ -5,11 +5,13 @@ package trivy
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"os"
 	"path"
 	"testing"
 	"time"
 
+	"github.com/aquasecurity/trivy-db/pkg/metadata"
 	dbTypes "github.com/aquasecurity/trivy-db/pkg/types"
 	"github.com/aquasecurity/trivy/pkg/commands/artifact"
 	"github.com/aquasecurity/trivy/pkg/flag"
@@ -132,6 +134,54 @@ func TestGenerateSBOM(t *testing.T) {
 	})
 }
 
+func TestRunTrivySBOMGenerationFailureIsNonFatal(t *testing.T) {
+	Convey("runTrivy should return report and nil error when SBOM generation fails", t, func() {
+		logger := log.NewTestLogger()
+		rootDir := t.TempDir()
+
+		dbDir := path.Join(rootDir, "_trivy", "db")
+		err := os.MkdirAll(dbDir, 0o755)
+		So(err, ShouldBeNil)
+		err = os.WriteFile(metadata.Path(dbDir), []byte(`{"Version":2}`), 0o600)
+		So(err, ShouldBeNil)
+
+		store := local.NewImageStore(rootDir, false, false, logger, monitoring.NewMetricsServer(false, logger), nil, nil, nil, nil)
+		storeController := storage.StoreController{DefaultStore: store}
+
+		scanner := Scanner{
+			log:             logger,
+			storeController: storeController,
+			sbomOptions: sbomOptions{
+				enabled:      true,
+				reportFormat: trivyTypes.FormatSPDXJSON,
+			},
+		}
+
+		sbomErr := errors.New("sbom generation failed")
+		oldNewArtifactRunner := newArtifactRunner
+		newArtifactRunner = func(ctx context.Context, opts flag.Options, target artifact.TargetKind,
+			runnerOpts ...artifact.RunnerOption,
+		) (artifact.Runner, error) {
+			return fakeArtifactRunner{
+				reportFn: func(ctx context.Context, opts flag.Options, report trivyTypes.Report) error {
+					return sbomErr
+				},
+			}, nil
+		}
+		defer func() {
+			newArtifactRunner = oldNewArtifactRunner
+		}()
+
+		report, generated, err := scanner.runTrivy(context.Background(), flag.Options{
+			ImageOptions: flag.ImageOptions{Input: "repo:tag"},
+		})
+
+		So(err, ShouldBeNil)
+		So(report, ShouldResemble, trivyTypes.Report{})
+		So(generated, ShouldBeNil)
+	})
+}
+
 func TestMultipleStoragePath(t *testing.T) {
 	Convey("Test multiple storage path", t, func() {
 		// Create temporary directory

pkg/meta/hooks.go

@@ -143,7 +143,7 @@ func rollbackDigestManifestTags(ctx context.Context, repo string, tags, appliedM
 }
 
 // OnUpdateManifest is called when a new manifest is added. It updates metadb according to the type
-// of image pushed(normal images, signatues, etc.). In care of any errors, it makes sure to keep
+// of image pushed(normal images, signatures, etc.). In case of any errors, it makes sure to keep
 // consistency between metadb and the image store.
 func OnUpdateManifest(ctx context.Context, repo, reference, mediaType string, digest godigest.Digest, body []byte,
 	storeController storage.StoreController, metaDB mTypes.MetaDB, log log.Logger,
@@ -213,7 +213,7 @@ func OnUpdateManifestDigestTags(ctx context.Context, repo string, tags []string,
 }
 
 // OnDeleteManifest is called when a manifest is deleted. It updates metadb according to the type
-// of image pushed(normal images, signatues, etc.). In care of any errors, it makes sure to keep
+// of image pushed(normal images, signatures, etc.). In case of any errors, it makes sure to keep
 // consistency between metadb and the image store.
 func OnDeleteManifest(repo, reference, mediaType string, digest godigest.Digest, manifestBlob []byte,
 	storeController storage.StoreController, metaDB mTypes.MetaDB, log log.Logger,
@@ -271,7 +271,7 @@ func OnDeleteManifest(repo, reference, mediaType string, digest godigest.Digest,
 	return nil
 }
 
-// OnGetManifest is called when a manifest is downloaded. It increments the download couter on that manifest.
+// OnGetManifest is called when a manifest is downloaded. It increments the download counter on that manifest.
 func OnGetManifest(name, reference, mediaType string, body []byte,
 	storeController storage.StoreController, metaDB mTypes.MetaDB, log log.Logger,
 ) error {

pkg/requestcontext/authn.go

@@ -1,4 +1,4 @@
-package uac
+package requestcontext
 
 import (
 	"context"

pkg/requestcontext/user_access_control.go

@@ -1,4 +1,4 @@
-package uac
+package requestcontext
 
 import (
 	"context"
@@ -167,9 +167,8 @@ func (uac *UserAccessControl) SetGlobPatterns(action string, patterns map[string
 	uac.authzInfo.globPatterns[action] = patterns
 }
 
-/*
-Can returns whether or not the user/anonymous who made the request has 'action' permission on 'repository'.
-*/
+// Can returns whether or not the user/anonymous who made the request has
+// action permission on repository.
 func (uac *UserAccessControl) Can(action, repository string) bool {
 	var defaultRet bool
 	if uac.isBehaviourAction(action) {
@@ -205,10 +204,8 @@ func (uac *UserAccessControl) areGlobPatternsSet() bool {
 	return !notSet
 }
 
-/*
-returns whether or not 'repository' can be found in the list of patterns
-on which the user who made the request has read permission on.
-*/
+// matchesRepo returns whether repository matches the provided action's glob patterns
+// and is allowed by the longest matching pattern.
 func (uac *UserAccessControl) matchesRepo(globPatterns map[string]bool, repository string) bool {
 	var longestMatchedPattern string
 

pkg/storage/gc/gc_test.go

@@ -58,12 +58,20 @@ var testCases = []struct {
 	},
 }
 
+func newTestMetricsServer(t *testing.T, log zlog.Logger) monitoring.MetricServer {
+	t.Helper()
+
+	metrics := monitoring.NewMetricsServer(false, log)
+	t.Cleanup(metrics.Stop)
+
+	return metrics
+}
+
 func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 	log := zlog.NewTestLogger()
 	audit := zlog.NewAuditLogger("debug", "/dev/null")
 
-	metrics := monitoring.NewMetricsServer(false, log)
-	defer metrics.Stop() // Clean up metrics server to prevent resource leaks
+	metrics := newTestMetricsServer(t, log)
 
 	trueVal := true
 
@@ -1242,7 +1250,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					So(err, ShouldBeNil)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(uploads, ShouldEqual, []string{})
 					} else {
@@ -1253,7 +1261,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					isPresent, _, _, err := imgStore.StatBlobUpload(repoName, blobUploadID)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(err, ShouldNotBeNil)
 						So(isPresent, ShouldBeFalse)
@@ -1271,7 +1279,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					So(err, ShouldBeNil)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(uploads, ShouldEqual, []string{})
 					} else {
@@ -1282,7 +1290,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) {
 					isPresent, _, _, err = imgStore.StatBlobUpload(repoName, blobUploadID)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(err, ShouldNotBeNil)
 						So(isPresent, ShouldBeFalse)
@@ -1316,8 +1324,7 @@ func TestGarbageCollectDeletion(t *testing.T) {
 		log := zlog.NewTestLogger()
 		audit := zlog.NewAuditLogger("debug", "/dev/null")
 
-		metrics := monitoring.NewMetricsServer(false, log)
-		defer metrics.Stop() // Clean up metrics server to prevent resource leaks
+		metrics := newTestMetricsServer(t, log)
 
 		trueVal := true
 		falseVal := false
@@ -1755,8 +1762,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) {
 	log := zlog.NewTestLogger()
 	audit := zlog.NewAuditLogger("debug", "/dev/null")
 
-	metrics := monitoring.NewMetricsServer(false, log)
-	defer metrics.Stop() // Clean up metrics server to prevent resource leaks
+	metrics := newTestMetricsServer(t, log)
 
 	trueVal := true
 
@@ -2566,7 +2572,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) {
 					So(err, ShouldBeNil)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(uploads, ShouldEqual, []string{})
 					} else {
@@ -2577,7 +2583,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) {
 					isPresent, _, _, err := imgStore.StatBlobUpload(repoName, blobUploadID)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(err, ShouldNotBeNil)
 						So(isPresent, ShouldBeFalse)
@@ -2595,7 +2601,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) {
 					So(err, ShouldBeNil)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(uploads, ShouldEqual, []string{})
 					} else {
@@ -2606,7 +2612,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) {
 					isPresent, _, _, err = imgStore.StatBlobUpload(repoName, blobUploadID)
 
 					if testcase.testCaseName == s3TestName {
-						// Remote sorage is written to only after the blob upload is finished,
+						// Remote storage is written to only after the blob upload is finished,
 						// there should be no space used by blob uploads
 						So(err, ShouldNotBeNil)
 						So(isPresent, ShouldBeFalse)