From 433f401c83bb4bbde9e5a378b9e750df088ac73a Mon Sep 17 00:00:00 2001 From: "Lennart Kats (databricks)" Date: Sun, 30 Jul 2023 14:44:33 +0200 Subject: [PATCH] Add validation for Git settings in bundles (#578) ## Changes This checks whether the Git settings are consistent with the actual Git state of a source directory. (This PR adds to https://github.com/databricks/cli/pull/577.) Previously, we would silently let users configure their Git branch to e.g. `main` and deploy with that metadata even if they were actually on a different branch. With these changes, the following config would result in an error when deployed from any other branch than `main`: ``` bundle: name: example workspace: git: branch: main environments: ... ``` > not on the right Git branch: > expected according to configuration: main > actual: my-feature-branch It's not very useful to set the same branch for all environments, though. For development, it's better to just let the CLI auto-detect the right branch. Therefore, it's now possible to set the branch just for a single environment: ``` bundle: name: example 2 environments: development: default: true production: # production can only be deployed from the 'main' branch git: branch: main ``` Adding to that, the `mode: production` option actually checks that users explicitly set the Git branch as seen above. Setting that branch helps avoid mistakes, where someone accidentally deploys to production from the wrong branch. (I could see us offering an escape hatch for that in the future.) # Testing Manual testing to validate the experience and error messages. Automated unit tests. --------- Co-authored-by: Fabian Jakobs --- bundle/config/bundle.go | 3 + bundle/config/environment.go | 2 + bundle/config/git.go | 6 ++ bundle/config/mutator/load_git_details.go | 2 + .../mutator/process_environment_mode.go | 5 ++ .../mutator/process_environment_mode_test.go | 15 +++++ bundle/config/mutator/validate_git_details.go | 29 +++++++++ .../mutator/validate_git_details_test.go | 65 +++++++++++++++++++ bundle/config/root.go | 12 ++++ bundle/phases/deploy.go | 2 + bundle/tests/autoload_git/databricks.yml | 11 +++- bundle/tests/autoload_git_test.go | 15 +++-- cmd/bundle/deploy.go | 10 +-- cmd/bundle/destroy.go | 4 +- 14 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 bundle/config/mutator/validate_git_details.go create mode 100644 bundle/config/mutator/validate_git_details_test.go diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index cf386477..f3401477 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -25,6 +25,9 @@ type Bundle struct { // Lock configures locking behavior on deployment. Lock Lock `json:"lock" bundle:"readonly"` + // Force-override Git branch validation. + Force bool `json:"force" bundle:"readonly"` + // Contains Git information like current commit, current branch and // origin url. Automatically loaded by reading .git directory if not specified Git Git `json:"git,omitempty"` diff --git a/bundle/config/environment.go b/bundle/config/environment.go index c1f4f4ad..7152f791 100644 --- a/bundle/config/environment.go +++ b/bundle/config/environment.go @@ -29,6 +29,8 @@ type Environment struct { // Does not permit defining new variables or redefining existing ones // in the scope of an environment Variables map[string]string `json:"variables,omitempty"` + + Git Git `json:"git,omitempty"` } const ( diff --git a/bundle/config/git.go b/bundle/config/git.go index 7ada8dfb..760134a8 100644 --- a/bundle/config/git.go +++ b/bundle/config/git.go @@ -4,4 +4,10 @@ type Git struct { Branch string `json:"branch,omitempty"` OriginURL string `json:"origin_url,omitempty"` Commit string `json:"commit,omitempty" bundle:"readonly"` + + // Inferred is set to true if the Git details were inferred and weren't set explicitly + Inferred bool `json:"-" bundle:"readonly"` + + // The actual branch according to Git (may be different from the configured branch) + ActualBranch string `json:"-" bundle:"readonly"` } diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 121924c6..f22aafe0 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -31,6 +31,8 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) error { log.Warnf(ctx, "failed to load current branch: %s", err) } else { b.Config.Bundle.Git.Branch = branch + b.Config.Bundle.Git.ActualBranch = branch + b.Config.Bundle.Git.Inferred = true } } // load commit hash if undefined diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 65d8a689..d2030234 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -109,6 +109,11 @@ func findIncorrectPath(b *bundle.Bundle, mode config.Mode) string { } func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUsed bool) error { + if b.Config.Bundle.Git.Inferred { + env := b.Config.Bundle.Environment + return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.%s.git' configuration", env) + } + r := b.Config.Resources for i := range r.Pipelines { if r.Pipelines[i].Development { diff --git a/bundle/config/mutator/process_environment_mode_test.go b/bundle/config/mutator/process_environment_mode_test.go index 6f53abd8..36e0396e 100644 --- a/bundle/config/mutator/process_environment_mode_test.go +++ b/bundle/config/mutator/process_environment_mode_test.go @@ -22,6 +22,10 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Config: config.Root{ Bundle: config.Bundle{ Mode: mode, + Git: config.Git{ + OriginURL: "http://origin", + Branch: "main", + }, }, Workspace: config.Workspace{ CurrentUser: &config.User{ @@ -114,6 +118,17 @@ func TestProcessEnvironmentModeProduction(t *testing.T) { assert.False(t, bundle.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) } +func TestProcessEnvironmentModeProductionGit(t *testing.T) { + bundle := mockBundle(config.Production) + + // Pretend the user didn't set Git configuration explicitly + bundle.Config.Bundle.Git.Inferred = true + + err := validateProductionMode(context.Background(), bundle, false) + require.ErrorContains(t, err, "git") + bundle.Config.Bundle.Git.Inferred = false +} + func TestProcessEnvironmentModeProductionOkForPrincipal(t *testing.T) { bundle := mockBundle(config.Production) diff --git a/bundle/config/mutator/validate_git_details.go b/bundle/config/mutator/validate_git_details.go new file mode 100644 index 00000000..116498bf --- /dev/null +++ b/bundle/config/mutator/validate_git_details.go @@ -0,0 +1,29 @@ +package mutator + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" +) + +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) error { + 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 fmt.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 new file mode 100644 index 00000000..252964ee --- /dev/null +++ b/bundle/config/mutator/validate_git_details_test.go @@ -0,0 +1,65 @@ +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) { + bundle := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Git: config.Git{ + Branch: "main", + ActualBranch: "main", + }, + }, + }, + } + + m := ValidateGitDetails() + err := m.Apply(context.Background(), bundle) + + assert.NoError(t, err) +} + +func TestValidateGitDetailsNonMatchingBranches(t *testing.T) { + bundle := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Git: config.Git{ + Branch: "main", + ActualBranch: "feature", + }, + }, + }, + } + + m := ValidateGitDetails() + err := m.Apply(context.Background(), bundle) + + expectedError := "not on the right Git branch:\n expected according to configuration: main\n actual: feature\nuse --force to override" + assert.EqualError(t, err, expectedError) +} + +func TestValidateGitDetailsNotUsingGit(t *testing.T) { + bundle := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Git: config.Git{ + Branch: "main", + ActualBranch: "", + }, + }, + }, + } + + m := ValidateGitDetails() + err := m.Apply(context.Background(), bundle) + + assert.NoError(t, err) +} diff --git a/bundle/config/root.go b/bundle/config/root.go index f5a4f00d..52f88737 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -225,5 +225,17 @@ func (r *Root) MergeEnvironment(env *Environment) error { r.Bundle.ComputeID = env.ComputeID } + git := &r.Bundle.Git + if env.Git.Branch != "" { + git.Branch = env.Git.Branch + git.Inferred = false + } + if env.Git.Commit != "" { + git.Commit = env.Git.Commit + } + if env.Git.OriginURL != "" { + git.OriginURL = env.Git.OriginURL + } + return nil } diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 8b53273c..011bb4b2 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -3,6 +3,7 @@ package phases import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/artifacts" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy/files" "github.com/databricks/cli/bundle/deploy/lock" "github.com/databricks/cli/bundle/deploy/terraform" @@ -15,6 +16,7 @@ func Deploy() bundle.Mutator { lock.Acquire(), bundle.Defer( bundle.Seq( + mutator.ValidateGitDetails(), files.Upload(), libraries.MatchWithArtifacts(), artifacts.CleanUp(), diff --git a/bundle/tests/autoload_git/databricks.yml b/bundle/tests/autoload_git/databricks.yml index d0e1de60..ba4785ae 100644 --- a/bundle/tests/autoload_git/databricks.yml +++ b/bundle/tests/autoload_git/databricks.yml @@ -1,4 +1,11 @@ bundle: name: autoload git config test - git: - branch: foo + +environments: + development: + default: true + + production: + # production can only be deployed from the 'main' branch + git: + branch: main diff --git a/bundle/tests/autoload_git_test.go b/bundle/tests/autoload_git_test.go index 87c7180e..a1075198 100644 --- a/bundle/tests/autoload_git_test.go +++ b/bundle/tests/autoload_git_test.go @@ -6,10 +6,15 @@ import ( "github.com/stretchr/testify/assert" ) -func TestGitConfig(t *testing.T) { +func TestAutoLoad(t *testing.T) { b := load(t, "./autoload_git") - assert.Equal(t, "foo", b.Config.Bundle.Git.Branch) - sshUrl := "git@github.com:databricks/cli.git" - httpsUrl := "https://github.com/databricks/cli" - assert.Contains(t, []string{sshUrl, httpsUrl}, b.Config.Bundle.Git.OriginURL) + assert.True(t, b.Config.Bundle.Git.Inferred) + assert.Contains(t, b.Config.Bundle.Git.OriginURL, "/cli") +} + +func TestManuallySetBranch(t *testing.T) { + b := loadEnvironment(t, "./autoload_git", "production") + assert.False(t, b.Config.Bundle.Git.Inferred) + assert.Equal(t, "main", b.Config.Bundle.Git.Branch) + assert.Contains(t, b.Config.Bundle.Git.OriginURL, "/cli") } diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index a39f1996..807bb982 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -13,16 +13,18 @@ func newDeployCommand() *cobra.Command { PreRunE: ConfigureBundleWithVariables, } - var forceDeploy bool + var force bool + var forceLock bool var computeID string - cmd.Flags().BoolVar(&forceDeploy, "force", false, "Force acquisition of deployment lock.") + cmd.Flags().BoolVar(&force, "force", false, "Force-override Git branch validation.") + cmd.Flags().BoolVar(&forceLock, "force-deploy", false, "Force acquisition of deployment lock.") cmd.Flags().StringVarP(&computeID, "compute-id", "c", "", "Override compute in the deployment with the given compute ID.") cmd.RunE = func(cmd *cobra.Command, args []string) error { b := bundle.Get(cmd.Context()) - // If `--force` is specified, force acquisition of the deployment lock. - b.Config.Bundle.Lock.Force = forceDeploy + b.Config.Bundle.Force = force + b.Config.Bundle.Lock.Force = forceLock b.Config.Bundle.ComputeID = computeID return bundle.Apply(cmd.Context(), b, bundle.Seq( diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index 82d82144..22d998ab 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -23,13 +23,13 @@ func newDestroyCommand() *cobra.Command { var autoApprove bool var forceDestroy bool cmd.Flags().BoolVar(&autoApprove, "auto-approve", false, "Skip interactive approvals for deleting resources and files") - cmd.Flags().BoolVar(&forceDestroy, "force", false, "Force acquisition of deployment lock.") + cmd.Flags().BoolVar(&forceDestroy, "force-lock", false, "Force acquisition of deployment lock.") cmd.RunE = func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() b := bundle.Get(ctx) - // If `--force` is specified, force acquisition of the deployment lock. + // If `--force-lock` is specified, force acquisition of the deployment lock. b.Config.Bundle.Lock.Force = forceDestroy // If `--auto-approve`` is specified, we skip confirmation checks