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

@@ -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
+	config := &b.Config.Bundle.Git
-	if b.Config.Bundle.Git.Branch == "" {
+
-		// Only load branch if there's no user defined value
+	config.ActualBranch = info.CurrentBranch
-		b.Config.Bundle.Git.Inferred = true
+	if config.Branch == "" && info.CurrentBranch != "" {
-		b.Config.Bundle.Git.Branch = info.CurrentBranch
+		config.Inferred = true
 	}
 
-	// load commit hash if undefined
+	// The value from config takes precedence; however, we always warn if configValue and fetchedValue disagree.
-	if b.Config.Bundle.Git.Commit == "" {
+	// In case of branch and commit and absence of --force we raise severity to Error
-		b.Config.Bundle.Git.Commit = info.LatestCommit
+	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)...)
-	// load origin url if undefined
-	if b.Config.Bundle.Git.OriginURL == "" {
-		b.Config.Bundle.Git.OriginURL = info.OriginURL
-	}
 
 	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
+}

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")
+}

bundle/config/mutator/validate_git_details.go

@@ -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
-}

bundle/config/mutator/validate_git_details_test.go

@@ -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())
-}

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(),

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:")
 }