From 23169cdd87894b541c765a707e287af4d649deaa Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 4 Dec 2024 09:51:37 +0100 Subject: [PATCH 1/8] 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:") } From fa3f4cde7760900f8c5bf46b473e6171f08067fa Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 16:21:12 +0100 Subject: [PATCH 2/8] fix tests --- .../config/mutator/load_git_details_test.go | 53 ++++++++++++------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/bundle/config/mutator/load_git_details_test.go b/bundle/config/mutator/load_git_details_test.go index eb990269d..d24d058d7 100644 --- a/bundle/config/mutator/load_git_details_test.go +++ b/bundle/config/mutator/load_git_details_test.go @@ -1,4 +1,4 @@ -package mutator_test +package mutator import ( "testing" @@ -8,54 +8,67 @@ import ( "github.com/stretchr/testify/require" ) -func TestCheckMatch(t *testing.T, force bool) { +func TestCheckMatch(t *testing.T) { type test struct { - Fetched string - Configured string + Fetched string + ConfiguredBefore string + ConfiguredAfter string } tests := []test{ { - Fetched: "main", - Configured: "main", + Fetched: "main", + ConfiguredBefore: "main", + ConfiguredAfter: "main", }, { - Fetched: "main", - Configured: "", + Fetched: "main", + ConfiguredBefore: "", + ConfiguredAfter: "main", }, { - Fetched: "", - Configured: "main", + Fetched: "", + ConfiguredBefore: "main", + ConfiguredAfter: "main", }, { - Fetched: "", - Configured: "main", + Fetched: "", + ConfiguredBefore: "main", + ConfiguredAfter: "main", }, } - for test := range tests { - name := "CheckMatch " + test.Fetched + " " + test.Configured + for _, test := range tests { + name := "CheckMatch " + test.Fetched + " " + test.ConfiguredBefore + " " + test.ConfiguredAfter t.Run(name, func(t *testing.T) { - diags := checkMatch("", test.Fetched, testConfigured, false) + configValue := test.ConfiguredBefore + diags := checkMatch("", test.Fetched, &configValue, false) assert.Nil(t, diags) + assert.Equal(t, test.ConfiguredAfter, configValue) }) t.Run(name+" force", func(t *testing.T) { - diags := checkMatch("", test.Fetched, testConfigured, true) + configValue := test.ConfiguredBefore + diags := checkMatch("", test.Fetched, &configValue, true) assert.Nil(t, diags) + assert.Equal(t, test.ConfiguredAfter, configValue) }) } } -func TestCheckWarning(t *testing.T, force bool) { - diags := checkMatch("Git branch", "feature", "main", true) +func TestCheckWarning(t *testing.T) { + configValue := "main" + diags := checkMatch("Git branch", "feature", &configValue, 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") + assert.Equal(t, "main", configValue) } -func TestCheckError(t *testing.T, force bool) { - diags := checkMatch("Git branch", "feature", "main", false) +func TestCheckError(t *testing.T) { + configValue := "main" + diags := checkMatch("Git branch", "feature", &configValue, 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") + assert.Equal(t, "main", configValue) } From e5a85e9e6dfb9ac2b15dd4ce409fea16d3728780 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 16:37:05 +0100 Subject: [PATCH 3/8] tmp - debuggin issue on CI --- bundle/config/mutator/load_git_details.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 5d447f968..02332bf5b 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -22,6 +22,8 @@ func (m *loadGitDetails) Name() string { } func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + fmt.Printf("running loadGitDetails config=%v\n", b.Config.Bundle.Git) + var diags diag.Diagnostics info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot.Native(), b.WorkspaceClient()) if err != nil { @@ -38,7 +40,10 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn config.ActualBranch = info.CurrentBranch if config.Branch == "" && info.CurrentBranch != "" { + fmt.Printf("setting inferred config=%v info=%v\n", *config, info) config.Inferred = true + } else { + fmt.Printf("NOT setting inferred config=%v info=%v\n", *config, info) } // The value from config takes precedence; however, we always warn if configValue and fetchedValue disagree. @@ -59,6 +64,7 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn } func checkMatch(field, fetchedValue string, configValue *string, allowedToMismatch bool) []diag.Diagnostic { + fmt.Printf("checkMatch %s %s %s %v\n", field, fetchedValue, *configValue, allowedToMismatch) if fetchedValue == "" { return nil } From 3b19d8fc40f792fbe4bf230e6d721740d9a6311f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 16:41:39 +0100 Subject: [PATCH 4/8] restore original Inferred logic. On CI info.CurrentBranch is "" but according to original logic it is still Inferred because it only checks if Config value is empty string. --- bundle/config/mutator/load_git_details.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 02332bf5b..e03a629ed 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -39,7 +39,7 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn config := &b.Config.Bundle.Git config.ActualBranch = info.CurrentBranch - if config.Branch == "" && info.CurrentBranch != "" { + if config.Branch == "" { fmt.Printf("setting inferred config=%v info=%v\n", *config, info) config.Inferred = true } else { From bf0ef9eabf657988960dfeb4e57e4d2f713d65dd Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 16:49:45 +0100 Subject: [PATCH 5/8] Revert "tmp - debuggin issue on CI" This reverts commit e5a85e9e6dfb9ac2b15dd4ce409fea16d3728780. --- bundle/config/mutator/load_git_details.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index e03a629ed..5a63f15ab 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -22,8 +22,6 @@ func (m *loadGitDetails) Name() string { } func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - fmt.Printf("running loadGitDetails config=%v\n", b.Config.Bundle.Git) - var diags diag.Diagnostics info, err := git.FetchRepositoryInfo(ctx, b.BundleRoot.Native(), b.WorkspaceClient()) if err != nil { @@ -40,10 +38,7 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn config.ActualBranch = info.CurrentBranch if config.Branch == "" { - fmt.Printf("setting inferred config=%v info=%v\n", *config, info) config.Inferred = true - } else { - fmt.Printf("NOT setting inferred config=%v info=%v\n", *config, info) } // The value from config takes precedence; however, we always warn if configValue and fetchedValue disagree. @@ -64,7 +59,6 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn } func checkMatch(field, fetchedValue string, configValue *string, allowedToMismatch bool) []diag.Diagnostic { - fmt.Printf("checkMatch %s %s %s %v\n", field, fetchedValue, *configValue, allowedToMismatch) if fetchedValue == "" { return nil } From d4f30604f909b2e02e3335233c84b7c725c81019 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 17:42:42 +0100 Subject: [PATCH 6/8] clean up duplicate test case --- bundle/config/mutator/load_git_details_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/bundle/config/mutator/load_git_details_test.go b/bundle/config/mutator/load_git_details_test.go index d24d058d7..ead5d5fdf 100644 --- a/bundle/config/mutator/load_git_details_test.go +++ b/bundle/config/mutator/load_git_details_test.go @@ -31,11 +31,6 @@ func TestCheckMatch(t *testing.T) { ConfiguredBefore: "main", ConfiguredAfter: "main", }, - { - Fetched: "", - ConfiguredBefore: "main", - ConfiguredAfter: "main", - }, } for _, test := range tests { From 0322733914bfcf245289c05473a12455e466ef36 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 17:54:48 +0100 Subject: [PATCH 7/8] Update bundle/config/mutator/load_git_details.go Co-authored-by: Pieter Noordhuis --- bundle/config/mutator/load_git_details.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 5a63f15ab..a95e1bd41 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -45,7 +45,7 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn // 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)...) + diags = append(diags, checkMatch("Git origin URL", info.OriginURL, &config.OriginURL, true)...) relBundlePath, err := filepath.Rel(b.WorktreeRoot.Native(), b.BundleRoot.Native()) if err != nil { From 48e6b9215e4d908926dc2117bba11f187dbe9fd3 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 9 Dec 2024 17:57:27 +0100 Subject: [PATCH 8/8] rm duplicate line --- bundle/config/mutator/load_git_details.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index a95e1bd41..e491fd367 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -51,10 +51,9 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn if err != nil { diags = append(diags, diag.FromErr(err)...) } else { - b.Config.Bundle.Git.BundleRootPath = filepath.ToSlash(relBundlePath) + config.BundleRootPath = filepath.ToSlash(relBundlePath) } - config.BundleRootPath = filepath.ToSlash(relBundlePath) return diags }