From 63d479d07ee2a40c40d216cdceeeba2748d57603 Mon Sep 17 00:00:00 2001 From: Ahmet Alp Balkan Date: Mon, 9 Dec 2019 19:15:42 -0800 Subject: [PATCH] Disallow fmt.Errorf usage through code validation Came across during a code review, new contributors can end up using fmt.Errorf which does not create stack trace at the error site --and also doesn't preserve stack trace if used to wrap an error. Signed-off-by: Ahmet Alp Balkan --- cmd/krew/cmd/root.go | 2 +- cmd/validate-krew-manifest/main.go | 2 +- hack/verify-code-patterns.sh | 8 ++++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/krew/cmd/root.go b/cmd/krew/cmd/root.go index 755a8bba..1679eb49 100644 --- a/cmd/krew/cmd/root.go +++ b/cmd/krew/cmd/root.go @@ -94,7 +94,7 @@ func preRun(cmd *cobra.Command, _ []string) error { } if !isMigrated && cmd.Use != "receipts-upgrade" { fmt.Fprintln(os.Stderr, "You need to perform a migration to continue using krew.\nPlease run `kubectl krew system receipts-upgrade`") - return fmt.Errorf("krew home outdated") + return errors.New("krew home outdated") } if installation.IsWindows() { diff --git a/cmd/validate-krew-manifest/main.go b/cmd/validate-krew-manifest/main.go index 53c9f998..0f86a49e 100644 --- a/cmd/validate-krew-manifest/main.go +++ b/cmd/validate-krew-manifest/main.go @@ -70,7 +70,7 @@ func validateManifestFile(path string) error { filename := filepath.Base(path) manifestExtension := filepath.Ext(filename) if manifestExtension != constants.ManifestExtension { - return fmt.Errorf("expected manifest extension %q but found %q", constants.ManifestExtension, manifestExtension) + return errors.Errorf("expected manifest extension %q but found %q", constants.ManifestExtension, manifestExtension) } pluginNameFromFileName := strings.TrimSuffix(filename, manifestExtension) klog.V(4).Infof("inferred plugin name as %s", pluginNameFromFileName) diff --git a/hack/verify-code-patterns.sh b/hack/verify-code-patterns.sh index 5d77e913..453f4ecf 100755 --- a/hack/verify-code-patterns.sh +++ b/hack/verify-code-patterns.sh @@ -44,6 +44,14 @@ if [[ -n "$out" ]]; then exit 1 fi +# Do not use fmt.Errorf as it does not start a stacktrace at error site +out="$(grep --include '*.go' -EIrn 'fmt\.Errorf?' || true)" +if [[ -n "$out" ]]; then + echo >&2 "You used fmt.Errorf; use pkg/errors.Errorf instead to preserve stack traces:" + echo >&2 "$out" + exit 1 +fi + # Do not initialize index.{Plugin,Platform} structs in test code. out="$(grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '[^]](index\.)(Plugin|Platform){' || true)" if [[ -n "$out" ]]; then