refactor(test): add lint rule for messages starting with the component

Signed-off-by: Laurentiu Niculae <[email protected]>
project-zot · Nov 21, 2023 · e89b043 · e89b043
1 parent 09b6985
commit e89b043
Show file tree

Hide file tree

Showing 39 changed files with 360 additions and 266 deletions.
diff --git a/.github/workflows/golangci-lint.yaml b/.github/workflows/golangci-lint.yaml
@@ -48,10 +48,28 @@ jobs:
       - name: Run log linter
         run: |
           set +e
+          
+          function lintLogStartingWithUpperCase {
+              grep --with-filename -n "Msg[f]\?(\"[A-Z]" $1 | grep -v -E "Msg[f]?\(\"HTTP|OpenID|OAuth|TLS"
+          }
 
-          # log messages should never start upper-cased
-          find . -name '*.go' | grep -v '_test.go' | xargs grep -n "Msg(\"[A-Z]" | grep -v -E "Msg\(\"HTTP|OpenID|OAuth|TLS"
-          if [ $? -eq 0 ]; then
-            exit 1
-          fi
+          # log messages should not start with component "component:"
+          function lintLogStartingWithComponent {
+            grep --with-filename -n -E "(Errorf|errors.New|Msg[f]?)\(\"[a-zA-Z-]+:" $1
+          }
+
+          files=$(find . -name '*.go' | grep -v '_test.go')
+
+          for file in $files
+          do
+              lintLogStartingWithUpperCase "$file"
+              if [ $? -eq 0 ]; then
+                  exit 1
+              fi
+              lintLogStartingWithComponent "$file"
+              if [ $? -eq 0 ]; then
+                  exit 1
+              fi
+          done
+          
           exit 0
diff --git a/LINT.sh b/LINT.sh
@@ -0,0 +1,24 @@
+#!/bin/bash
+
+function lintLogStartingWithUpperCase {
+    grep --with-filename -n "Msg[f]\?(\"[A-Z]" $1 | grep -v -E "Msg[f]?\(\"HTTP|OpenID|OAuth|TLS"
+}
+
+# log messages should not start with component "component:"
+function lintLogStartingWithComponent {
+  grep --with-filename -n -E "(Errorf|errors.New|Msg[f]?)\(\"[a-zA-Z-]+:" $1
+}
+
+files=$(find . -name '*.go' | grep -v '_test.go')
+
+for file in $files
+do
+    lintLogStartingWithUpperCase "$file"
+    if [ $? -eq 0 ]; then
+        exit 1
+    fi
+    lintLogStartingWithComponent "$file"
+    if [ $? -eq 0 ]; then
+        exit 1
+    fi
+done
diff --git a/Makefile b/Makefile
@@ -310,6 +310,34 @@ $(GOLINTER):
 	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(TOOLSDIR)/bin $(GOLINTER_VERSION)
 	$(GOLINTER) version
 
+define lintLogStartingWithUpperCase
+	grep --with-filename -n "Msg[f]\?(\"[A-Z]" $(1) | grep -v -E "Msg[f]?\(\"HTTP|OpenID|OAuth|TLS"
+endef
+
+define lintLogStartingWithComponent
+	grep --with-filename -n -E "(errors.New|Msg[f]?)\(\"[a-zA-Z-]+:" $(1)
+endef
+
+GO_FILES=$(shell find . -name '*.go' | grep -v '_test.go')
+found_failure="false"
+
+.PHONY: check-logs
+check-logs: 
+	echo "START: $(found_failure)"
+	@for file in $(GO_FILES); do \
+		$(call lintLogStartingWithUpperCase,$$file); \
+		if [ $$? -eq 0 ]; then \
+		$(eval found_failure="true") \
+			echo "MID: $(found_failure)"; \
+		fi; \
+	done; \
+	echo "End: $(found_failure)"; \
+	if [ $(found_failure) = "true" ]; then \
+		exit 1; \
+	else \
+		exit 0; \
+	fi
+
 .PHONY: check
 check: $(if $(findstring ui,$(BUILD_LABELS)), ui)
 check: ./golangcilint.yaml $(GOLINTER)

diff --git a/errors/errors.go b/errors/errors.go
diff --git a/pkg/api/authn.go b/pkg/api/authn.go
@@ -790,13 +790,15 @@ func OAuth2Callback(ctlr *Controller, w http.ResponseWriter, r *http.Request, st
 
 	stateOrigin, ok := stateCookie.Values["state"].(string)
 	if !ok {
-		ctlr.Log.Error().Err(zerr.ErrInvalidStateCookie).Msg("openID: unable to get 'state' cookie from request")
+		ctlr.Log.Error().Err(zerr.ErrInvalidStateCookie).Str("component", "openID").
+			Msg(": unable to get 'state' cookie from request")
 
 		return "", zerr.ErrInvalidStateCookie
 	}
 
 	if stateOrigin != state {
-		ctlr.Log.Error().Err(zerr.ErrInvalidStateCookie).Msg("openID: 'state' cookie differs from the actual one")
+		ctlr.Log.Error().Err(zerr.ErrInvalidStateCookie).Str("component", "openID").
+			Msg(": 'state' cookie differs from the actual one")
 
 		return "", zerr.ErrInvalidStateCookie
 	}

diff --git a/pkg/cli/client/config_cmd_test.go b/pkg/cli/client/config_cmd_test.go
@@ -221,7 +221,7 @@ func TestConfigCmdMain(t *testing.T) {
 		cmd.SetArgs(args)
 		err := cmd.Execute()
 		So(err, ShouldNotBeNil)
-		So(buff.String(), ShouldContainSubstring, "invalid config")
+		So(buff.String(), ShouldContainSubstring, "invalid server config")
 	})
 
 	Convey("Test remove config bad permissions", t, func() {

diff --git a/pkg/cli/client/cve_cmd_test.go b/pkg/cli/client/cve_cmd_test.go
@@ -369,7 +369,7 @@ func TestServerCVEResponse(t *testing.T) {
 		So(err, ShouldNotBeNil)
 	})
 
-	Convey("Test CVE by image name - GQL - invalid output format", t, func() {
+	Convey("Test CVE by image name - GQL - invalid cli output format", t, func() {
 		args := []string{"list", "zot-cve-test:0.0.1", "-f", "random", "--config", "cvetest"}
 		configPath := makeConfigFile(fmt.Sprintf(`{"configs":[{"_name":"cvetest","url":"%s","showspinner":false}]}`, url))
 		defer os.Remove(configPath)
@@ -380,7 +380,7 @@ func TestServerCVEResponse(t *testing.T) {
 		cveCmd.SetArgs(args)
 		err = cveCmd.Execute()
 		So(err, ShouldNotBeNil)
-		So(buff.String(), ShouldContainSubstring, "invalid output format")
+		So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 	})
 
 	Convey("Test images by CVE ID - GQL - positive", t, func() {
@@ -417,7 +417,7 @@ func TestServerCVEResponse(t *testing.T) {
 		So(str, ShouldNotContainSubstring, "REPOSITORY TAG OS/ARCH DIGEST SIGNED SIZE")
 	})
 
-	Convey("Test images by CVE ID - GQL - invalid output format", t, func() {
+	Convey("Test images by CVE ID - GQL - invalid cli output format", t, func() {
 		args := []string{"affected", "CVE-2019-9923", "-f", "random", "--config", "cvetest"}
 		configPath := makeConfigFile(fmt.Sprintf(`{"configs":[{"_name":"cvetest","url":"%s","showspinner":false}]}`, url))
 		defer os.Remove(configPath)
@@ -428,7 +428,7 @@ func TestServerCVEResponse(t *testing.T) {
 		cveCmd.SetArgs(args)
 		err = cveCmd.Execute()
 		So(err, ShouldNotBeNil)
-		So(buff.String(), ShouldContainSubstring, "invalid output format")
+		So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 	})
 
 	Convey("Test fixed tags by image name and CVE ID - GQL - positive", t, func() {
@@ -532,7 +532,7 @@ func TestServerCVEResponse(t *testing.T) {
 		So(strings.TrimSpace(str), ShouldNotContainSubstring, "REPOSITORY TAG OS/ARCH SIGNED SIZE")
 	})
 
-	Convey("Test CVE by name and CVE ID - GQL - invalid output format", t, func() {
+	Convey("Test CVE by name and CVE ID - GQL - invalid cli output format", t, func() {
 		args := []string{"affected", "CVE-2019-9923", "--repo", "zot-cve-test", "-f", "random", "--config", "cvetest"}
 		configPath := makeConfigFile(fmt.Sprintf(`{"configs":[{"_name":"cvetest","url":"%s","showspinner":false}]}`, url))
 		defer os.Remove(configPath)
@@ -543,7 +543,7 @@ func TestServerCVEResponse(t *testing.T) {
 		cveCmd.SetArgs(args)
 		err = cveCmd.Execute()
 		So(err, ShouldNotBeNil)
-		So(buff.String(), ShouldContainSubstring, "invalid output format")
+		So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 	})
 }
 

diff --git a/pkg/cli/client/image_cmd_internal_test.go b/pkg/cli/client/image_cmd_internal_test.go
@@ -470,7 +470,7 @@ func TestOutputFormat(t *testing.T) {
 		cmd.SetArgs(args)
 		err := cmd.Execute()
 		So(err, ShouldNotBeNil)
-		So(buff.String(), ShouldContainSubstring, "invalid output format")
+		So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 	})
 }
 

diff --git a/pkg/cli/client/image_cmd_test.go b/pkg/cli/client/image_cmd_test.go
@@ -500,7 +500,7 @@ func TestOutputFormatGQL(t *testing.T) {
 			cmd.SetArgs(args)
 			err := cmd.Execute()
 			So(err, ShouldNotBeNil)
-			So(buff.String(), ShouldContainSubstring, "invalid output format")
+			So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 		})
 	})
 }
@@ -554,7 +554,7 @@ func TestServerResponseGQL(t *testing.T) {
 				cmd.SetArgs(args)
 				err := cmd.Execute()
 				So(err, ShouldNotBeNil)
-				So(buff.String(), ShouldContainSubstring, "invalid output format")
+				So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 			})
 		})
 
@@ -632,7 +632,7 @@ func TestServerResponseGQL(t *testing.T) {
 				cmd.SetArgs(args)
 				err := cmd.Execute()
 				So(err, ShouldNotBeNil)
-				So(buff.String(), ShouldContainSubstring, "invalid output format")
+				So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 			})
 		})
 
@@ -683,7 +683,7 @@ func TestServerResponseGQL(t *testing.T) {
 				cmd.SetArgs(args)
 				err := cmd.Execute()
 				So(err, ShouldNotBeNil)
-				So(buff.String(), ShouldContainSubstring, "invalid output format")
+				So(buff.String(), ShouldContainSubstring, "invalid cli output format")
 			})
 		})
 

diff --git a/pkg/cli/client/repo_test.go b/pkg/cli/client/repo_test.go
@@ -95,7 +95,7 @@ func TestSuggestions(t *testing.T) {
 		suggestion := client.ShowSuggestionsIfUnknownCommand(
 			client.NewRepoCommand(client.NewSearchService()), []string{"bad-command"})
 		str := space.ReplaceAllString(suggestion.Error(), " ")
-		So(str, ShouldContainSubstring, "unknown subcommand")
+		So(str, ShouldContainSubstring, "unknown cli subcommand")
 
 		suggestion = client.ShowSuggestionsIfUnknownCommand(
 			client.NewRepoCommand(client.NewSearchService()), []string{"listt"})

diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go
@@ -559,15 +559,15 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, log z
 
 				if config.Extensions.Search.CVE.Trivy.DBRepository == "" {
 					defaultDBDownloadURL := "ghcr.io/aquasecurity/trivy-db"
-					log.Info().Str("url", defaultDBDownloadURL).
-						Msg("config: using default trivy-db download URL.")
+					log.Info().Str("url", defaultDBDownloadURL).Str("component", "config").
+						Msg("using default trivy-db download URL.")
 					config.Extensions.Search.CVE.Trivy.DBRepository = defaultDBDownloadURL
 				}
 
 				if config.Extensions.Search.CVE.Trivy.JavaDBRepository == "" {
 					defaultJavaDBDownloadURL := "ghcr.io/aquasecurity/trivy-java-db"
-					log.Info().Str("url", defaultJavaDBDownloadURL).
-						Msg("config: using default trivy-java-db download URL.")
+					log.Info().Str("url", defaultJavaDBDownloadURL).Str("component", "config").
+						Msg("using default trivy-java-db download URL.")
 					config.Extensions.Search.CVE.Trivy.JavaDBRepository = defaultJavaDBDownloadURL
 				}
 			}
@@ -643,7 +643,7 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, log z
 		cachePath := path.Join(cacheDir, storageConstants.BoltdbName+storageConstants.DBExtensionName)
 
 		if _, err := os.Stat(cachePath); err == nil {
-			log.Info().Msg("config: dedupe set to false for s3 driver but used to be true.")
+			log.Info().Str("component", "config").Msg("dedupe set to false for s3 driver but used to be true.")
 			log.Info().Str("cache path", cachePath).Msg("found cache database")
 
 			config.Storage.RemoteCache = false
@@ -664,7 +664,7 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, log z
 			subpathCachePath := path.Join(subpathCacheDir, storageConstants.BoltdbName+storageConstants.DBExtensionName)
 
 			if _, err := os.Stat(subpathCachePath); err == nil {
-				log.Info().Msg("config: dedupe set to false for s3 driver but used to be true. ")
+				log.Info().Str("component", "config").Msg("dedupe set to false for s3 driver but used to be true. ")
 				log.Info().Str("cache path", subpathCachePath).Msg("found cache database")
 
 				storageConfig.RemoteCache = false

diff --git a/pkg/extensions/extension_mgmt.go b/pkg/extensions/extension_mgmt.go
@@ -122,7 +122,7 @@ func (mgmt *Mgmt) HandleGetConfig(w http.ResponseWriter, r *http.Request) {
 
 	buf, err := zcommon.MarshalThroughStruct(sanitizedConfig, &StrippedConfig{})
 	if err != nil {
-		mgmt.Log.Error().Err(err).Msg("mgmt: couldn't marshal config response")
+		mgmt.Log.Error().Err(err).Str("component", "mgmt").Msg("couldn't marshal config response")
 		w.WriteHeader(http.StatusInternalServerError)
 	}
 

diff --git a/pkg/extensions/imagetrust/image_trust.go b/pkg/extensions/imagetrust/image_trust.go
@@ -167,7 +167,7 @@ func (imgTrustStore *ImageTrustStore) VerifySignature(
 	}
 
 	if manifestDigest.String() == "" {
-		return "", time.Time{}, false, zerr.ErrBadManifestDigest
+		return "", time.Time{}, false, zerr.ErrBadSignatureManifestDigest
 	}
 
 	switch signatureType {

diff --git a/pkg/extensions/imagetrust/image_trust_test.go b/pkg/extensions/imagetrust/image_trust_test.go
@@ -156,7 +156,7 @@ func TestVerifySignatures(t *testing.T) {
 		imgTrustStore := &imagetrust.ImageTrustStore{}
 		_, _, _, err := imgTrustStore.VerifySignature("", []byte(""), "", "", image.AsImageMeta(), "repo")
 		So(err, ShouldNotBeNil)
-		So(err, ShouldEqual, zerr.ErrBadManifestDigest)
+		So(err, ShouldEqual, zerr.ErrBadSignatureManifestDigest)
 	})
 
 	Convey("wrong signature type", t, func() {

diff --git a/pkg/extensions/lint/lint.go b/pkg/extensions/lint/lint.go
@@ -43,15 +43,15 @@ func (linter *Linter) CheckMandatoryAnnotations(repo string, manifestDigest godi
 
 	content, err := imgStore.GetBlobContent(repo, manifestDigest)
 	if err != nil {
-		linter.log.Error().Err(err).Msg("linter: unable to get image manifest")
+		linter.log.Error().Err(err).Str("component", "linter").Msg("unable to get image manifest")
 
 		return false, err
 	}
 
 	var manifest ispec.Manifest
 
 	if err := json.Unmarshal(content, &manifest); err != nil {
-		linter.log.Error().Err(err).Msg("linter: couldn't unmarshal manifest JSON")
+		linter.log.Error().Err(err).Str("component", "linter").Msg("couldn't unmarshal manifest JSON")
 
 		return false, err
 	}
@@ -78,15 +78,15 @@ func (linter *Linter) CheckMandatoryAnnotations(repo string, manifestDigest godi
 
 	content, err = imgStore.GetBlobContent(repo, configDigest)
 	if err != nil {
-		linter.log.Error().Err(err).Msg("linter: couldn't get config JSON " +
+		linter.log.Error().Err(err).Str("component", "linter").Msg("couldn't get config JSON " +
 			configDigest.String())
 
 		return false, err
 	}
 
 	var imageConfig ispec.Image
 	if err := json.Unmarshal(content, &imageConfig); err != nil {
-		linter.log.Error().Err(err).Msg("linter: couldn't unmarshal config JSON " + configDigest.String())
+		linter.log.Error().Err(err).Str("component", "linter").Msg("couldn't unmarshal config JSON " + configDigest.String())
 
 		return false, err
 	}

diff --git a/pkg/extensions/monitoring/monitoring_test.go b/pkg/extensions/monitoring/monitoring_test.go
@@ -476,11 +476,11 @@ func TestPopulateStorageMetrics(t *testing.T) {
 
 		// Wait for storage metrics to update
 		found, err := test.ReadLogFileAndSearchString(logPath,
-			"monitoring: computed storage usage for repo alpine", time.Minute)
+			"computed storage usage for repo alpine", time.Minute)
 		So(err, ShouldBeNil)
 		So(found, ShouldBeTrue)
 		found, err = test.ReadLogFileAndSearchString(logPath,
-			"monitoring: computed storage usage for repo busybox", time.Minute)
+			"computed storage usage for repo busybox", time.Minute)
 		So(err, ShouldBeNil)
 		So(found, ShouldBeTrue)
 

diff --git a/pkg/extensions/scrub/scrub.go b/pkg/extensions/scrub/scrub.go
@@ -33,15 +33,17 @@ func RunScrubRepo(ctx context.Context, imgStore storageTypes.ImageStore, repo st
 				Str("image", result.ImageName).
 				Str("tag", result.Tag).
 				Str("status", result.Status).
-				Msg("scrub: blobs/manifest ok")
+				Str("component", "scrub").
+				Msg("blobs/manifest ok")
 		} else {
 			log.Warn().
 				Str("image", result.ImageName).
 				Str("tag", result.Tag).
 				Str("status", result.Status).
 				Str("affected blob", result.AffectedBlob).
 				Str("error", result.Error).
-				Msg("scrub: blobs/manifest affected")
+				Str("component", "scrub").
+				Msg("blobs/manifest affected")
 		}
 	}
 

diff --git a/pkg/extensions/scrub/scrub_test.go b/pkg/extensions/scrub/scrub_test.go
@@ -76,7 +76,7 @@ func TestScrubExtension(t *testing.T) {
 
 		data, err := os.ReadFile(logFile.Name())
 		So(err, ShouldBeNil)
-		So(string(data), ShouldContainSubstring, "scrub: blobs/manifest ok")
+		So(string(data), ShouldContainSubstring, "blobs/manifest ok")
 	})
 
 	Convey("Blobs integrity affected", t, func(c C) {
@@ -128,7 +128,7 @@ func TestScrubExtension(t *testing.T) {
 
 		data, err := os.ReadFile(logFile.Name())
 		So(err, ShouldBeNil)
-		So(string(data), ShouldContainSubstring, "scrub: blobs/manifest affected")
+		So(string(data), ShouldContainSubstring, "blobs/manifest affected")
 	})
 
 	Convey("Generator error - not enough permissions to access root directory", t, func(c C) {
@@ -215,7 +215,7 @@ func TestRunScrubRepo(t *testing.T) {
 
 		data, err := os.ReadFile(logFile.Name())
 		So(err, ShouldBeNil)
-		So(string(data), ShouldContainSubstring, "scrub: blobs/manifest ok")
+		So(string(data), ShouldContainSubstring, "blobs/manifest ok")
 	})
 
 	Convey("Blobs integrity affected", t, func(c C) {
@@ -258,7 +258,7 @@ func TestRunScrubRepo(t *testing.T) {
 
 		data, err := os.ReadFile(logFile.Name())
 		So(err, ShouldBeNil)
-		So(string(data), ShouldContainSubstring, "scrub: blobs/manifest affected")
+		So(string(data), ShouldContainSubstring, "blobs/manifest affected")
 	})
 
 	Convey("CheckRepo error - not enough permissions to access root directory", t, func(c C) {