From 592474880d7834a4be3bf2333d0e8cea1ddeb887 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 11 Dec 2024 17:42:03 +0100 Subject: [PATCH] Enable 'govet' linter; expand log/diag with non-f functions (#1996) ## Changes Fix all the govet-found issues and enable govet linter. This prompts adding non-formatting variants of logging functions (Errorf -> Error). ## Tests Existing tests. --- .golangci.yaml | 7 ++++- bundle/artifacts/whl/autodetect.go | 2 +- bundle/deferred_test.go | 2 +- bundle/deploy/terraform/plan.go | 3 +- bundle/libraries/filer_volume_test.go | 2 +- bundle/run/job.go | 4 +-- bundle/run/pipeline.go | 4 +-- cmd/labs/project/entrypoint.go | 3 -- cmd/root/auth_test.go | 3 +- libs/log/logger.go | 45 +++++++++++++++++++++++++++ libs/template/renderer.go | 2 +- 11 files changed, 62 insertions(+), 15 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 6ab8bb2fef..7598646d35 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -4,12 +4,17 @@ linters: - bodyclose - errcheck - gosimple - #- govet + - govet - ineffassign - staticcheck - unused - gofmt linters-settings: + govet: + enable-all: true + disable: + - fieldalignment + - shadow gofmt: rewrite-rules: - pattern: 'a[b:len(a)]' diff --git a/bundle/artifacts/whl/autodetect.go b/bundle/artifacts/whl/autodetect.go index 88dc742c12..3d17a698c0 100644 --- a/bundle/artifacts/whl/autodetect.go +++ b/bundle/artifacts/whl/autodetect.go @@ -42,7 +42,7 @@ func (m *detectPkg) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return nil } - log.Infof(ctx, fmt.Sprintf("Found Python wheel project at %s", b.BundleRootPath)) + log.Infof(ctx, "Found Python wheel project at %s", b.BundleRootPath) module := extractModuleName(setupPy) if b.Config.Artifacts == nil { diff --git a/bundle/deferred_test.go b/bundle/deferred_test.go index 3abc4aa102..ea3df17c44 100644 --- a/bundle/deferred_test.go +++ b/bundle/deferred_test.go @@ -19,7 +19,7 @@ func (t *mutatorWithError) Name() string { func (t *mutatorWithError) Apply(_ context.Context, b *Bundle) diag.Diagnostics { t.applyCalled++ - return diag.Errorf(t.errorMsg) + return diag.Errorf(t.errorMsg) // nolint:govet } func TestDeferredMutatorWhenAllMutatorsSucceed(t *testing.T) { diff --git a/bundle/deploy/terraform/plan.go b/bundle/deploy/terraform/plan.go index 72f0b49a89..7f7473efab 100644 --- a/bundle/deploy/terraform/plan.go +++ b/bundle/deploy/terraform/plan.go @@ -2,7 +2,6 @@ package terraform import ( "context" - "fmt" "path/filepath" "github.com/databricks/cli/bundle" @@ -57,7 +56,7 @@ func (p *plan) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { IsEmpty: !notEmpty, } - log.Debugf(ctx, fmt.Sprintf("Planning complete and persisted at %s\n", planPath)) + log.Debugf(ctx, "Planning complete and persisted at %s\n", planPath) return nil } diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index 0d886824d9..870dce97cc 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -173,7 +173,7 @@ func TestFilerForVolumeNotFoundAndInBundle(t *testing.T) { { Severity: diag.Error, Summary: "volume /Volumes/main/my_schema/my_volume does not exist: error from API", - Locations: []dyn.Location{{"config.yml", 1, 2}, {"volume.yml", 1, 2}}, + Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}, {File: "volume.yml", Line: 1, Column: 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, Detail: `You are using a volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please first deploy diff --git a/bundle/run/job.go b/bundle/run/job.go index 340af961cf..35a6ef4ffc 100644 --- a/bundle/run/job.go +++ b/bundle/run/job.go @@ -143,7 +143,7 @@ func logProgressCallback(ctx context.Context, progressLogger *cmdio.Logger) func progressLogger.Log(event) // log progress events in using the default logger - log.Infof(ctx, event.String()) + log.Info(ctx, event.String()) } } @@ -203,7 +203,7 @@ func (r *jobRunner) Run(ctx context.Context, opts *Options) (output.RunOutput, e logDebug(r) logProgress(r) }).GetWithTimeout(jobRunTimeout) - if err != nil && runId != nil { + if err != nil { r.logFailedTasks(ctx, *runId) } if err != nil { diff --git a/bundle/run/pipeline.go b/bundle/run/pipeline.go index ffe0128430..6db80a71ba 100644 --- a/bundle/run/pipeline.go +++ b/bundle/run/pipeline.go @@ -37,7 +37,7 @@ func (r *pipelineRunner) logEvent(ctx context.Context, event pipelines.PipelineE } } if logString != "" { - log.Errorf(ctx, fmt.Sprintf("[%s] %s", event.EventType, logString)) + log.Errorf(ctx, "[%s] %s", event.EventType, logString) } } @@ -132,7 +132,7 @@ func (r *pipelineRunner) Run(ctx context.Context, opts *Options) (output.RunOutp } for _, event := range events { progressLogger.Log(&event) - log.Infof(ctx, event.String()) + log.Info(ctx, event.String()) } update, err := w.Pipelines.GetUpdateByPipelineIdAndUpdateId(ctx, pipelineID, updateID) diff --git a/cmd/labs/project/entrypoint.go b/cmd/labs/project/entrypoint.go index 99edf83c8c..5210c85f95 100644 --- a/cmd/labs/project/entrypoint.go +++ b/cmd/labs/project/entrypoint.go @@ -190,9 +190,6 @@ func (e *Entrypoint) getLoginConfig(cmd *cobra.Command) (*loginConfig, *config.C if isNoLoginConfig && !e.IsBundleAware { return nil, nil, ErrNoLoginConfig } - if !isNoLoginConfig && err != nil { - return nil, nil, fmt.Errorf("load: %w", err) - } if e.IsAccountLevel { log.Debugf(ctx, "Using account-level login profile: %s", lc.AccountProfile) cfg, err := e.envAwareConfigWithProfile(ctx, lc.AccountProfile) diff --git a/cmd/root/auth_test.go b/cmd/root/auth_test.go index 9ba2a8fa99..92f028514e 100644 --- a/cmd/root/auth_test.go +++ b/cmd/root/auth_test.go @@ -15,7 +15,8 @@ import ( ) func TestEmptyHttpRequest(t *testing.T) { - ctx, _ := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() req := emptyHttpRequest(ctx) assert.Equal(t, req.Context(), ctx) } diff --git a/libs/log/logger.go b/libs/log/logger.go index 43a30e92bf..794a7e8655 100644 --- a/libs/log/logger.go +++ b/libs/log/logger.go @@ -31,6 +31,51 @@ func log(logger *slog.Logger, ctx context.Context, level slog.Level, msg string) _ = logger.Handler().Handle(ctx, r) } +// Trace logs a string using the context-local or global logger. +func Trace(ctx context.Context, msg string) { + logger := GetLogger(ctx) + if !logger.Enabled(ctx, LevelTrace) { + return + } + log(logger, ctx, LevelTrace, msg) +} + +// Debug logs a string using the context-local or global logger. +func Debug(ctx context.Context, msg string) { + logger := GetLogger(ctx) + if !logger.Enabled(ctx, LevelDebug) { + return + } + log(logger, ctx, LevelDebug, msg) +} + +// Info logs a string using the context-local or global logger. +func Info(ctx context.Context, msg string) { + logger := GetLogger(ctx) + if !logger.Enabled(ctx, LevelInfo) { + return + } + log(logger, ctx, LevelInfo, msg) +} + +// Warn logs a string using the context-local or global logger. +func Warn(ctx context.Context, msg string) { + logger := GetLogger(ctx) + if !logger.Enabled(ctx, LevelWarn) { + return + } + log(logger, ctx, LevelWarn, msg) +} + +// Error logs a string using the context-local or global logger. +func Error(ctx context.Context, msg string) { + logger := GetLogger(ctx) + if !logger.Enabled(ctx, LevelError) { + return + } + log(logger, ctx, LevelError, msg) +} + // Tracef logs a formatted string using the context-local or global logger. func Tracef(ctx context.Context, format string, v ...any) { logger := GetLogger(ctx) diff --git a/libs/template/renderer.go b/libs/template/renderer.go index 0f30a67d0e..5030cd9df3 100644 --- a/libs/template/renderer.go +++ b/libs/template/renderer.go @@ -310,7 +310,7 @@ func (r *renderer) persistToDisk(ctx context.Context, out filer.Filer) error { if err == nil { return fmt.Errorf("failed to initialize template, one or more files already exist: %s", path) } - if err != nil && !errors.Is(err, fs.ErrNotExist) { + if !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("error while verifying file %s does not already exist: %w", path, err) } }