From 23169cdd87894b541c765a707e287af4d649deaa Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 09:51:37 +0100 Subject: [PATCH] Improve git config section validation Previously we checked if ActualBranch (from .git) and Branch disagreed and issued an error unless --force was passed. We still do that, but also: - if --force is passed, we now log this as a warning (previously it went silent) - we do the same logic for commit (previously unchecked) - we do similar logic for originURL except it always logs warning and never prevent the deployment I removed separate mutator (validate_git_details.go) and put everything into load_git_details.go. --- bundle/config/mutator/load_git_details.go | 57 ++++++++++++----- .../config/mutator/load_git_details_test.go | 61 ++++++++++++++++++ bundle/config/mutator/validate_git_details.go | 29 --------- .../mutator/validate_git_details_test.go | 63 ------------------- bundle/phases/deploy.go | 2 - bundle/tests/git_test.go | 3 +- 6 files changed, 105 insertions(+), 110 deletions(-) create mode 100644 bundle/config/mutator/load_git_details_test.go delete mode 100644 bundle/config/mutator/validate_git_details.go delete mode 100644 bundle/config/mutator/validate_git_details_test.go diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 82255552a..5d447f968 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -2,6 +2,7 @@ package mutator import ( "context" + "fmt" "path/filepath" "github.com/databricks/cli/bundle" @@ -33,22 +34,18 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn b.WorktreeRoot = vfs.MustNew(info.WorktreeRoot) } - b.Config.Bundle.Git.ActualBranch = info.CurrentBranch - if b.Config.Bundle.Git.Branch == "" { - // Only load branch if there's no user defined value - b.Config.Bundle.Git.Inferred = true - b.Config.Bundle.Git.Branch = info.CurrentBranch + config := &b.Config.Bundle.Git + + config.ActualBranch = info.CurrentBranch + if config.Branch == "" && info.CurrentBranch != "" { + config.Inferred = true } - // load commit hash if undefined - if b.Config.Bundle.Git.Commit == "" { - b.Config.Bundle.Git.Commit = info.LatestCommit - } - - // load origin url if undefined - if b.Config.Bundle.Git.OriginURL == "" { - b.Config.Bundle.Git.OriginURL = info.OriginURL - } + // The value from config takes precedence; however, we always warn if configValue and fetchedValue disagree. + // In case of branch and commit and absence of --force we raise severity to Error + diags = append(diags, checkMatch("Git branch", info.CurrentBranch, &config.Branch, b.Config.Bundle.Force)...) + diags = append(diags, checkMatch("Git commit", info.LatestCommit, &config.Commit, b.Config.Bundle.Force)...) + diags = append(diags, checkMatch("Git originURL", info.OriginURL, &config.OriginURL, true)...) relBundlePath, err := filepath.Rel(b.WorktreeRoot.Native(), b.BundleRoot.Native()) if err != nil { @@ -56,5 +53,37 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn } else { b.Config.Bundle.Git.BundleRootPath = filepath.ToSlash(relBundlePath) } + + config.BundleRootPath = filepath.ToSlash(relBundlePath) return diags } + +func checkMatch(field, fetchedValue string, configValue *string, allowedToMismatch bool) []diag.Diagnostic { + if fetchedValue == "" { + return nil + } + + if *configValue == "" { + *configValue = fetchedValue + return nil + } + + if *configValue != fetchedValue { + tmpl := "not on the right %s:\n expected according to configuration: %s\n actual: %s%s" + extra := "" + var severity diag.Severity + if allowedToMismatch { + severity = diag.Warning + } else { + severity = diag.Error + extra = "\nuse --force to override" + } + + return []diag.Diagnostic{{ + Severity: severity, + Summary: fmt.Sprintf(tmpl, field, *configValue, fetchedValue, extra), + }} + } + + return nil +} diff --git a/bundle/config/mutator/load_git_details_test.go b/bundle/config/mutator/load_git_details_test.go new file mode 100644 index 000000000..eb990269d --- /dev/null +++ b/bundle/config/mutator/load_git_details_test.go @@ -0,0 +1,61 @@ +package mutator_test + +import ( + "testing" + + "github.com/databricks/cli/libs/diag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheckMatch(t *testing.T, force bool) { + type test struct { + Fetched string + Configured string + } + + tests := []test{ + { + Fetched: "main", + Configured: "main", + }, + { + Fetched: "main", + Configured: "", + }, + { + Fetched: "", + Configured: "main", + }, + { + Fetched: "", + Configured: "main", + }, + } + + for test := range tests { + name := "CheckMatch " + test.Fetched + " " + test.Configured + t.Run(name, func(t *testing.T) { + diags := checkMatch("", test.Fetched, testConfigured, false) + assert.Nil(t, diags) + }) + t.Run(name+" force", func(t *testing.T) { + diags := checkMatch("", test.Fetched, testConfigured, true) + assert.Nil(t, diags) + }) + } +} + +func TestCheckWarning(t *testing.T, force bool) { + diags := checkMatch("Git branch", "feature", "main", true) + require.Len(t, diags, 1) + assert.Equal(t, diags[0].Severity, diag.Warning) + assert.Equal(t, diags[0].Summary, "not on the right Git branch:\n expected according to configuration: main\n actual: feature") +} + +func TestCheckError(t *testing.T, force bool) { + diags := checkMatch("Git branch", "feature", "main", false) + require.Len(t, diags, 1) + assert.Equal(t, diags[0].Severity, diag.Error) + assert.Equal(t, diags[0].Summary, "not on the right Git branch:\n expected according to configuration: main\n actual: feature\nuse --force to override") +} diff --git a/bundle/config/mutator/validate_git_details.go b/bundle/config/mutator/validate_git_details.go deleted file mode 100644 index 69a4221fd..000000000 --- a/bundle/config/mutator/validate_git_details.go +++ /dev/null @@ -1,29 +0,0 @@ -package mutator - -import ( - "context" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/diag" -) - -type validateGitDetails struct{} - -func ValidateGitDetails() *validateGitDetails { - return &validateGitDetails{} -} - -func (m *validateGitDetails) Name() string { - return "ValidateGitDetails" -} - -func (m *validateGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - if b.Config.Bundle.Git.Branch == "" || b.Config.Bundle.Git.ActualBranch == "" { - return nil - } - - if b.Config.Bundle.Git.Branch != b.Config.Bundle.Git.ActualBranch && !b.Config.Bundle.Force { - return diag.Errorf("not on the right Git branch:\n expected according to configuration: %s\n actual: %s\nuse --force to override", b.Config.Bundle.Git.Branch, b.Config.Bundle.Git.ActualBranch) - } - return nil -} diff --git a/bundle/config/mutator/validate_git_details_test.go b/bundle/config/mutator/validate_git_details_test.go deleted file mode 100644 index 952e0b572..000000000 --- a/bundle/config/mutator/validate_git_details_test.go +++ /dev/null @@ -1,63 +0,0 @@ -package mutator - -import ( - "context" - "testing" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" - "github.com/stretchr/testify/assert" -) - -func TestValidateGitDetailsMatchingBranches(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Bundle: config.Bundle{ - Git: config.Git{ - Branch: "main", - ActualBranch: "main", - }, - }, - }, - } - - m := ValidateGitDetails() - diags := bundle.Apply(context.Background(), b, m) - assert.NoError(t, diags.Error()) -} - -func TestValidateGitDetailsNonMatchingBranches(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Bundle: config.Bundle{ - Git: config.Git{ - Branch: "main", - ActualBranch: "feature", - }, - }, - }, - } - - m := ValidateGitDetails() - diags := bundle.Apply(context.Background(), b, m) - - expectedError := "not on the right Git branch:\n expected according to configuration: main\n actual: feature\nuse --force to override" - assert.EqualError(t, diags.Error(), expectedError) -} - -func TestValidateGitDetailsNotUsingGit(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Bundle: config.Bundle{ - Git: config.Git{ - Branch: "main", - ActualBranch: "", - }, - }, - }, - } - - m := ValidateGitDetails() - diags := bundle.Apply(context.Background(), b, m) - assert.NoError(t, diags.Error()) -} diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 2dc9623bd..b51c1d277 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -7,7 +7,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/artifacts" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy" "github.com/databricks/cli/bundle/deploy/files" "github.com/databricks/cli/bundle/deploy/lock" @@ -149,7 +148,6 @@ func Deploy(outputHandler sync.OutputHandler) bundle.Mutator { terraform.StatePull(), terraform.CheckDashboardsModifiedRemotely(), deploy.StatePull(), - mutator.ValidateGitDetails(), artifacts.CleanUp(), libraries.ExpandGlobReferences(), libraries.Upload(), diff --git a/bundle/tests/git_test.go b/bundle/tests/git_test.go index dec6c268a..819563dfd 100644 --- a/bundle/tests/git_test.go +++ b/bundle/tests/git_test.go @@ -36,11 +36,10 @@ func TestGitBundleBranchValidation(t *testing.T) { }) b := load(t, "./git_branch_validation") - bundle.Apply(context.Background(), b, mutator.LoadGitDetails()) + diags := bundle.Apply(context.Background(), b, mutator.LoadGitDetails()) assert.False(t, b.Config.Bundle.Git.Inferred) assert.Equal(t, "feature-a", b.Config.Bundle.Git.Branch) assert.Equal(t, "feature-b", b.Config.Bundle.Git.ActualBranch) - diags := bundle.Apply(context.Background(), b, mutator.ValidateGitDetails()) assert.ErrorContains(t, diags.Error(), "not on the right Git branch:") }