From 837a48c561d03688b83633394ab734bde2e8b232 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 15 Jul 2023 17:12:34 +0200 Subject: [PATCH 01/17] Extend deployment modes --- bundle/config/environment.go | 10 +- .../mutator/expand_workspace_root_test.go | 18 ++- .../config/mutator/populate_current_user.go | 21 ++- .../mutator/populate_current_user_test.go | 39 ++++- .../mutator/process_environment_mode.go | 124 ++++++++++++-- .../mutator/process_environment_mode_test.go | 152 ++++++++++++++---- bundle/config/workspace.go | 9 +- bundle/deploy/files/sync.go | 7 +- bundle/tests/job_and_pipeline/databricks.yml | 1 + internal/sync_test.go | 10 +- libs/sync/path.go | 11 +- libs/sync/sync.go | 5 +- 12 files changed, 347 insertions(+), 60 deletions(-) diff --git a/bundle/config/environment.go b/bundle/config/environment.go index 06a8d8909e..3e66977e52 100644 --- a/bundle/config/environment.go +++ b/bundle/config/environment.go @@ -32,7 +32,13 @@ type Environment struct { } const ( - // Right now, we just have a default / "" mode and a "development" mode. - // Additional modes are expected to come for pull-requests and production. + // Development mode: deployments done purely for running things in development. + // Any deployed resources will be marked as "dev" and might hidden or cleaned up. Development Mode = "development" + + // Production mode: deployments done for production purposes. + // Any deployed resources will not be changed but this mode will enable + // various strictness checks to make sure that a deployment is correctly setup + // for production purposes. + Production Mode = "production" ) diff --git a/bundle/config/mutator/expand_workspace_root_test.go b/bundle/config/mutator/expand_workspace_root_test.go index e872dc8355..0ec11a07db 100644 --- a/bundle/config/mutator/expand_workspace_root_test.go +++ b/bundle/config/mutator/expand_workspace_root_test.go @@ -16,8 +16,10 @@ func TestExpandWorkspaceRoot(t *testing.T) { bundle := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - CurrentUser: &iam.User{ - UserName: "jane@doe.com", + CurrentUser: &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, }, RootPath: "~/foo", }, @@ -32,8 +34,10 @@ func TestExpandWorkspaceRootDoesNothing(t *testing.T) { bundle := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - CurrentUser: &iam.User{ - UserName: "jane@doe.com", + CurrentUser: &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, }, RootPath: "/Users/charly@doe.com/foo", }, @@ -48,8 +52,10 @@ func TestExpandWorkspaceRootWithoutRoot(t *testing.T) { bundle := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - CurrentUser: &iam.User{ - UserName: "jane@doe.com", + CurrentUser: &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, }, }, }, diff --git a/bundle/config/mutator/populate_current_user.go b/bundle/config/mutator/populate_current_user.go index 34c6ff6e3e..cbaa2d30ba 100644 --- a/bundle/config/mutator/populate_current_user.go +++ b/bundle/config/mutator/populate_current_user.go @@ -2,8 +2,11 @@ package mutator import ( "context" + "strings" + "unicode" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" ) type populateCurrentUser struct{} @@ -24,6 +27,22 @@ func (m *populateCurrentUser) Apply(ctx context.Context, b *bundle.Bundle) error return err } - b.Config.Workspace.CurrentUser = me + b.Config.Workspace.CurrentUser = &config.User{ + ShortName: getShortUserName(me.UserName), + User: me, + } return nil } + +// Get a short-form username, based on the user's primary email address. +// We leave the full range of unicode letters in tact, but remove all "special" characters, +// including dots, which are not supported in e.g. experiment names. +func getShortUserName(emailAddress string) string { + r := []rune(strings.Split(emailAddress, "@")[0]) + for i := 0; i < len(r); i++ { + if !unicode.IsLetter(r[i]) { + r[i] = '_' + } + } + return string(r) +} diff --git a/bundle/config/mutator/populate_current_user_test.go b/bundle/config/mutator/populate_current_user_test.go index 4c28d1cd37..79ec52b8f7 100644 --- a/bundle/config/mutator/populate_current_user_test.go +++ b/bundle/config/mutator/populate_current_user_test.go @@ -1,3 +1,40 @@ package mutator -// We need to implement workspace client mocking to implement this test. +import "testing" + +func TestPopulateCurrentUser(t *testing.T) { + // We need to implement workspace client mocking to implement this test. +} + +func TestGetShortUserName(t *testing.T) { + tests := []struct { + name string + email string + expected string + }{ + { + name: "test alphanumeric characters", + email: "test.user@example.com", + expected: "test_user", + }, + { + name: "test unicode characters", + email: "tést.üser@example.com", + expected: "tést_üser", + }, + { + name: "test special characters", + email: "test$.user@example.com", + expected: "test__user", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getShortUserName(tt.email) + if result != tt.expected { + t.Errorf("getShortUserName(%q) = %q; expected %q", tt.email, result, tt.expected) + } + }) + } +} diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 3e1b7e8196..94a2b1853a 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -4,9 +4,11 @@ import ( "context" "fmt" "path" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/ml" ) @@ -26,15 +28,17 @@ func (m *processEnvironmentMode) Name() string { // Mark all resources as being for 'development' purposes, i.e. // changing their their name, adding tags, and (in the future) // marking them as 'hidden' in the UI. -func processDevelopmentMode(b *bundle.Bundle) error { +func transformDevelopmentMode(b *bundle.Bundle) error { r := b.Config.Resources + prefix := "[dev " + b.Config.Workspace.CurrentUser.ShortName + "] " + for i := range r.Jobs { - r.Jobs[i].Name = "[dev] " + r.Jobs[i].Name + r.Jobs[i].Name = prefix + r.Jobs[i].Name if r.Jobs[i].Tags == nil { r.Jobs[i].Tags = make(map[string]string) } - r.Jobs[i].Tags["dev"] = "" + r.Jobs[i].Tags["dev"] = b.Config.Workspace.CurrentUser.DisplayName if r.Jobs[i].MaxConcurrentRuns == 0 { r.Jobs[i].MaxConcurrentRuns = developmentConcurrentRuns } @@ -50,13 +54,13 @@ func processDevelopmentMode(b *bundle.Bundle) error { } for i := range r.Pipelines { - r.Pipelines[i].Name = "[dev] " + r.Pipelines[i].Name + r.Pipelines[i].Name = prefix + r.Pipelines[i].Name r.Pipelines[i].Development = true // (pipelines don't yet support tags) } for i := range r.Models { - r.Models[i].Name = "[dev] " + r.Models[i].Name + r.Models[i].Name = prefix + r.Models[i].Name r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: "dev", Value: ""}) } @@ -65,20 +69,122 @@ func processDevelopmentMode(b *bundle.Bundle) error { dir := path.Dir(filepath) base := path.Base(filepath) if dir == "." { - r.Experiments[i].Name = "[dev] " + base + r.Experiments[i].Name = prefix + base } else { - r.Experiments[i].Name = dir + "/[dev] " + base + r.Experiments[i].Name = dir + "/" + prefix + base + } + r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: b.Config.Workspace.CurrentUser.DisplayName}) + } + + return nil +} + +func validateDevelopmentMode(b *bundle.Bundle) error { + if path := findIncorrectPath(b, config.Development); path != "" { + return fmt.Errorf("%s must start with '~/' or contain the current username when using 'mode: development'", path) + } + return nil +} + +func findIncorrectPath(b *bundle.Bundle, mode config.Mode) string { + username := b.Config.Workspace.CurrentUser.UserName + containsExpected := true + if mode == config.Production { + containsExpected = false + } + + if strings.Contains(b.Config.Workspace.RootPath, username) != containsExpected && b.Config.Workspace.RootPath != "" { + return "root_path" + } + if strings.Contains(b.Config.Workspace.StatePath, username) != containsExpected { + return "state_path" + } + if strings.Contains(b.Config.Workspace.FilesPath, username) != containsExpected { + return "files_path" + } + if strings.Contains(b.Config.Workspace.ArtifactsPath, username) != containsExpected { + return "artifacts_path" + } + return "" +} + +func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUsed bool) error { + r := b.Config.Resources + for i := range r.Pipelines { + if r.Pipelines[i].Development { + return fmt.Errorf("environment with 'mode: production' cannot specify a pipeline with 'development: true'") } - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: ""}) } + if !isPrincipalUsed { + if path := findIncorrectPath(b, config.Production); path != "" { + message := "%s must not contain the current username when using 'mode: production' without a service principal" + if path == "root_path" { + return fmt.Errorf(message+"\n tip: set workspace.root_path to a shared path such as /Shared/.bundle/${bundle.name}/${bundle.environment}", path) + } else { + return fmt.Errorf(message, path) + } + } + + if !arePermissionsSetExplicitly(r) { + return fmt.Errorf("permissions and run_as must be set when using 'mode_production' without a service principals") + } + } return nil } +// Determines whether a service principal identity is used to run the CLI. +func isServicePrincipalUsed(ctx context.Context, b *bundle.Bundle) (bool, error) { + ws := b.WorkspaceClient() + + _, err := ws.ServicePrincipals.GetById(ctx, b.Config.Workspace.CurrentUser.Id) + if err != nil { + apiError, ok := err.(*apierr.APIError) + if ok && apiError.StatusCode == 404 { + return false, nil + } + return false, err + } + return false, nil +} + +// Determines whether permissions and run_as are explicitly set for all resources. +// We do this in a best-effort fashion; we may not actually test all resources, +// as we expect customers to use the top-level 'permissions' and 'run_as' fields. +// We'd rather not check for those specific fields though, as customers might +// set specific permissions instead! +func arePermissionsSetExplicitly(r config.Resources) bool { + for i := range r.Pipelines { + if r.Pipelines[i].Permissions == nil { + return false + } + } + + for i := range r.Jobs { + if r.Jobs[i].Permissions == nil { + return false + } + if r.Jobs[i].RunAs == nil { + return false + } + } + return true +} + func (m *processEnvironmentMode) Apply(ctx context.Context, b *bundle.Bundle) error { switch b.Config.Bundle.Mode { case config.Development: - return processDevelopmentMode(b) + err := validateDevelopmentMode(b) + if err != nil { + return err + } + return transformDevelopmentMode(b) + case config.Production: + isPrincipal, err := isServicePrincipalUsed(ctx, b) + if err != nil { + return err + } + return validateProductionMode(ctx, b, isPrincipal) case "": // No action default: diff --git a/bundle/config/mutator/process_environment_mode_test.go b/bundle/config/mutator/process_environment_mode_test.go index 5342de2125..f083f2d7eb 100644 --- a/bundle/config/mutator/process_environment_mode_test.go +++ b/bundle/config/mutator/process_environment_mode_test.go @@ -1,13 +1,15 @@ -package mutator_test +package mutator import ( "context" + "reflect" + "strings" "testing" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/ml" "github.com/databricks/databricks-sdk-go/service/pipelines" @@ -15,11 +17,23 @@ import ( "github.com/stretchr/testify/require" ) -func TestProcessEnvironmentModeApplyDebug(t *testing.T) { - bundle := &bundle.Bundle{ +func mockBundle(mode config.Mode) *bundle.Bundle { + return &bundle.Bundle{ Config: config.Root{ Bundle: config.Bundle{ - Mode: config.Development, + Mode: mode, + }, + Workspace: config.Workspace{ + CurrentUser: &config.User{ + ShortName: "lennart", + User: &iam.User{ + UserName: "lennart@company.com", + Id: "1", + }, + }, + StatePath: "/Users/lennart@company.com/.bundle/x/y/state", + ArtifactsPath: "/Users/lennart@company.com/.bundle/x/y/artifacts", + FilesPath: "/Users/lennart@company.com/.bundle/x/y/files", }, Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -38,40 +52,124 @@ func TestProcessEnvironmentModeApplyDebug(t *testing.T) { }, }, } +} - m := mutator.ProcessEnvironmentMode() +func TestProcessEnvironmentModeDevelopment(t *testing.T) { + bundle := mockBundle(config.Development) + + m := ProcessEnvironmentMode() err := m.Apply(context.Background(), bundle) require.NoError(t, err) - assert.Equal(t, "[dev] job1", bundle.Config.Resources.Jobs["job1"].Name) - assert.Equal(t, "[dev] pipeline1", bundle.Config.Resources.Pipelines["pipeline1"].Name) - assert.Equal(t, "/Users/lennart.kats@databricks.com/[dev] experiment1", bundle.Config.Resources.Experiments["experiment1"].Name) - assert.Equal(t, "[dev] experiment2", bundle.Config.Resources.Experiments["experiment2"].Name) - assert.Equal(t, "[dev] model1", bundle.Config.Resources.Models["model1"].Name) + assert.Equal(t, "[dev lennart] job1", bundle.Config.Resources.Jobs["job1"].Name) + assert.Equal(t, "[dev lennart] pipeline1", bundle.Config.Resources.Pipelines["pipeline1"].Name) + assert.Equal(t, "/Users/lennart.kats@databricks.com/[dev lennart] experiment1", bundle.Config.Resources.Experiments["experiment1"].Name) + assert.Equal(t, "[dev lennart] experiment2", bundle.Config.Resources.Experiments["experiment2"].Name) + assert.Equal(t, "[dev lennart] model1", bundle.Config.Resources.Models["model1"].Name) assert.Equal(t, "dev", bundle.Config.Resources.Experiments["experiment1"].Experiment.Tags[0].Key) assert.True(t, bundle.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) } -func TestProcessEnvironmentModeApplyDefault(t *testing.T) { - bundle := &bundle.Bundle{ - Config: config.Root{ - Bundle: config.Bundle{ - Mode: "", - }, - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job1": {JobSettings: &jobs.JobSettings{Name: "job1"}}, - }, - Pipelines: map[string]*resources.Pipeline{ - "pipeline1": {PipelineSpec: &pipelines.PipelineSpec{Name: "pipeline1"}}, - }, - }, +func TestProcessEnvironmentModeDefault(t *testing.T) { + bundle := mockBundle("") + + m := ProcessEnvironmentMode() + err := m.Apply(context.Background(), bundle) + require.NoError(t, err) + assert.Equal(t, "job1", bundle.Config.Resources.Jobs["job1"].Name) + assert.Equal(t, "pipeline1", bundle.Config.Resources.Pipelines["pipeline1"].Name) + assert.False(t, bundle.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) +} + +func TestProcessEnvironmentModeProduction(t *testing.T) { + bundle := mockBundle(config.Production) + + err := validateProductionMode(context.Background(), bundle, false) + require.ErrorContains(t, err, "state_path") + + bundle.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state" + bundle.Config.Workspace.ArtifactsPath = "/Shared/.bundle/x/y/artifacts" + bundle.Config.Workspace.FilesPath = "/Shared/.bundle/x/y/files" + + err = validateProductionMode(context.Background(), bundle, false) + require.ErrorContains(t, err, "permissions") + + permissions := []resources.Permission{ + { + Level: "CAN_MANAGE", + UserName: "user@company.com", }, } + bundle.Config.Resources.Jobs["job1"].Permissions = permissions + bundle.Config.Resources.Jobs["job1"].RunAs = &jobs.JobRunAs{UserName: "user@company.com"} + bundle.Config.Resources.Pipelines["pipeline1"].Permissions = permissions + bundle.Config.Resources.Experiments["experiment1"].Permissions = permissions + bundle.Config.Resources.Experiments["experiment2"].Permissions = permissions + bundle.Config.Resources.Models["model1"].Permissions = permissions - m := mutator.ProcessEnvironmentMode() - err := m.Apply(context.Background(), bundle) + err = validateProductionMode(context.Background(), bundle, false) require.NoError(t, err) + assert.Equal(t, "job1", bundle.Config.Resources.Jobs["job1"].Name) assert.Equal(t, "pipeline1", bundle.Config.Resources.Pipelines["pipeline1"].Name) assert.False(t, bundle.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) } + +func TestProcessEnvironmentModeProductionOkForPrincipal(t *testing.T) { + bundle := mockBundle(config.Production) + + // Our environment has all kinds of problems when not using service principals ... + err := validateProductionMode(context.Background(), bundle, false) + require.Error(t, err) + + // ... but we're much less strict when a principal is used + err = validateProductionMode(context.Background(), bundle, true) + require.NoError(t, err) +} + +// Make sure that we have test coverage for all resource types +func TestAllResourcesMocked(t *testing.T) { + bundle := mockBundle(config.Development) + resources := reflect.ValueOf(bundle.Config.Resources) + + for i := 0; i < resources.NumField(); i++ { + field := resources.Field(i) + if field.Kind() == reflect.Map { + assert.True( + t, + !field.IsNil() && field.Len() > 0, + "process_environment_mode should support '%s' (please add it to process_environment_mode.go and extend the test suite)", + resources.Type().Field(i).Name, + ) + } + } +} + +// Make sure that we at least rename all resources +func TestAllResourcesRenamed(t *testing.T) { + bundle := mockBundle(config.Development) + resources := reflect.ValueOf(bundle.Config.Resources) + + m := ProcessEnvironmentMode() + err := m.Apply(context.Background(), bundle) + require.NoError(t, err) + + for i := 0; i < resources.NumField(); i++ { + field := resources.Field(i) + + if field.Kind() == reflect.Map { + for _, key := range field.MapKeys() { + resource := field.MapIndex(key) + nameField := resource.Elem().FieldByName("Name") + if nameField.IsValid() && nameField.Kind() == reflect.String { + assert.True( + t, + strings.Contains(nameField.String(), "dev"), + "process_environment_mode should rename '%s' in '%s'", + key, + resources.Type().Field(i).Name, + ) + } + } + } + } +} diff --git a/bundle/config/workspace.go b/bundle/config/workspace.go index ee09bb8b49..f278ea1797 100644 --- a/bundle/config/workspace.go +++ b/bundle/config/workspace.go @@ -42,7 +42,7 @@ type Workspace struct { // CurrentUser holds the current user. // This is set after configuration initialization. - CurrentUser *iam.User `json:"current_user,omitempty" bundle:"readonly"` + CurrentUser *User `json:"current_user,omitempty" bundle:"readonly"` // Remote workspace base path for deployment state, for artifacts, as synchronization target. // This defaults to "~/.bundle/${bundle.name}/${bundle.environment}" where "~" expands to @@ -62,6 +62,13 @@ type Workspace struct { StatePath string `json:"state_path,omitempty"` } +type User struct { + // A short name for the user, based on the user's UserName. + ShortName string `json:"short_name,omitempty" bundle:"readonly"` + + *iam.User +} + func (w *Workspace) Client() (*databricks.WorkspaceClient, error) { cfg := databricks.Config{ // Generic diff --git a/bundle/deploy/files/sync.go b/bundle/deploy/files/sync.go index 77c64e529d..84d79dc81b 100644 --- a/bundle/deploy/files/sync.go +++ b/bundle/deploy/files/sync.go @@ -15,9 +15,10 @@ func getSync(ctx context.Context, b *bundle.Bundle) (*sync.Sync, error) { } opts := sync.SyncOptions{ - LocalPath: b.Config.Path, - RemotePath: b.Config.Workspace.FilesPath, - Full: false, + LocalPath: b.Config.Path, + RemotePath: b.Config.Workspace.FilesPath, + Full: false, + CurrentUser: b.Config.Workspace.CurrentUser.User, SnapshotBasePath: cacheDir, WorkspaceClient: b.WorkspaceClient(), diff --git a/bundle/tests/job_and_pipeline/databricks.yml b/bundle/tests/job_and_pipeline/databricks.yml index d6942e8a79..e29fa03499 100644 --- a/bundle/tests/job_and_pipeline/databricks.yml +++ b/bundle/tests/job_and_pipeline/databricks.yml @@ -23,6 +23,7 @@ environments: development: false production: + mode: production resources: pipelines: nyc_taxi_pipeline: diff --git a/internal/sync_test.go b/internal/sync_test.go index 09418a8556..66b5fd3cad 100644 --- a/internal/sync_test.go +++ b/internal/sync_test.go @@ -509,12 +509,12 @@ func TestAccSyncEnsureRemotePathIsUsableIfRepoDoesntExist(t *testing.T) { // Hypothetical repo path doesn't exist. nonExistingRepoPath := fmt.Sprintf("/Repos/%s/%s", me.UserName, RandomName("doesnt-exist-")) - err = sync.EnsureRemotePathIsUsable(ctx, wsc, nonExistingRepoPath) + err = sync.EnsureRemotePathIsUsable(ctx, wsc, nonExistingRepoPath, nil) assert.ErrorContains(t, err, " does not exist; please create it first") // Paths nested under a hypothetical repo path should yield the same error. nestedPath := path.Join(nonExistingRepoPath, "nested/directory") - err = sync.EnsureRemotePathIsUsable(ctx, wsc, nestedPath) + err = sync.EnsureRemotePathIsUsable(ctx, wsc, nestedPath, nil) assert.ErrorContains(t, err, " does not exist; please create it first") } @@ -526,12 +526,12 @@ func TestAccSyncEnsureRemotePathIsUsableIfRepoExists(t *testing.T) { _, remoteRepoPath := setupRepo(t, wsc, ctx) // Repo itself is usable. - err := sync.EnsureRemotePathIsUsable(ctx, wsc, remoteRepoPath) + err := sync.EnsureRemotePathIsUsable(ctx, wsc, remoteRepoPath, nil) assert.NoError(t, err) // Path nested under repo path is usable. nestedPath := path.Join(remoteRepoPath, "nested/directory") - err = sync.EnsureRemotePathIsUsable(ctx, wsc, nestedPath) + err = sync.EnsureRemotePathIsUsable(ctx, wsc, nestedPath, nil) assert.NoError(t, err) // Verify that the directory has been created. @@ -549,7 +549,7 @@ func TestAccSyncEnsureRemotePathIsUsableInWorkspace(t *testing.T) { require.NoError(t, err) remotePath := fmt.Sprintf("/Users/%s/%s", me.UserName, RandomName("ensure-path-exists-test-")) - err = sync.EnsureRemotePathIsUsable(ctx, wsc, remotePath) + err = sync.EnsureRemotePathIsUsable(ctx, wsc, remotePath, me) assert.NoError(t, err) // Clean up directory after test. diff --git a/libs/sync/path.go b/libs/sync/path.go index a04c28d30b..d44336c16e 100644 --- a/libs/sync/path.go +++ b/libs/sync/path.go @@ -24,10 +24,13 @@ func repoPathForPath(me *iam.User, remotePath string) string { // EnsureRemotePathIsUsable checks if the specified path is nested under // expected base paths and if it is a directory or repository. -func EnsureRemotePathIsUsable(ctx context.Context, wsc *databricks.WorkspaceClient, remotePath string) error { - me, err := wsc.CurrentUser.Me(ctx) - if err != nil { - return err +func EnsureRemotePathIsUsable(ctx context.Context, wsc *databricks.WorkspaceClient, remotePath string, me *iam.User) error { + var err error + if me == nil { + me, err = wsc.CurrentUser.Me(ctx) + if err != nil { + return err + } } // Ensure that the remote path exists. diff --git a/libs/sync/sync.go b/libs/sync/sync.go index 5c4c9d8f68..a299214d02 100644 --- a/libs/sync/sync.go +++ b/libs/sync/sync.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/service/iam" ) type SyncOptions struct { @@ -23,6 +24,8 @@ type SyncOptions struct { WorkspaceClient *databricks.WorkspaceClient + CurrentUser *iam.User + Host string } @@ -50,7 +53,7 @@ func New(ctx context.Context, opts SyncOptions) (*Sync, error) { } // Verify that the remote path we're about to synchronize to is valid and allowed. - err = EnsureRemotePathIsUsable(ctx, opts.WorkspaceClient, opts.RemotePath) + err = EnsureRemotePathIsUsable(ctx, opts.WorkspaceClient, opts.RemotePath, opts.CurrentUser) if err != nil { return nil, err } From 498aaa6b0fc180354b4f884ef82534bcd53f03ae Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 15 Jul 2023 17:13:34 +0200 Subject: [PATCH 02/17] Simplify error messages --- bundle/config/mutator/process_environment_mode.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 94a2b1853a..41e5624f04 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -118,7 +118,7 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs if !isPrincipalUsed { if path := findIncorrectPath(b, config.Production); path != "" { - message := "%s must not contain the current username when using 'mode: production' without a service principal" + message := "%s must not contain the current username when using 'mode: production'" if path == "root_path" { return fmt.Errorf(message+"\n tip: set workspace.root_path to a shared path such as /Shared/.bundle/${bundle.name}/${bundle.environment}", path) } else { @@ -127,7 +127,7 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs } if !arePermissionsSetExplicitly(r) { - return fmt.Errorf("permissions and run_as must be set when using 'mode_production' without a service principals") + return fmt.Errorf("'permissions' and 'run_as' must be set when using 'mode_production'") } } return nil From 3767c1ca28e6f82009d59f036b0e884482ea0617 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 22 Jul 2023 22:12:09 +0200 Subject: [PATCH 03/17] Fix service principal test for non-admins --- bundle/config/mutator/process_environment_mode.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 41e5624f04..19effe7f8b 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -8,7 +8,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/ml" ) @@ -137,15 +137,13 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs func isServicePrincipalUsed(ctx context.Context, b *bundle.Bundle) (bool, error) { ws := b.WorkspaceClient() - _, err := ws.ServicePrincipals.GetById(ctx, b.Config.Workspace.CurrentUser.Id) + matches, err := ws.ServicePrincipals.ListAll(ctx, iam.ListServicePrincipalsRequest{ + Filter: "id eq " + b.Config.Workspace.CurrentUser.Id, + }) if err != nil { - apiError, ok := err.(*apierr.APIError) - if ok && apiError.StatusCode == 404 { - return false, nil - } return false, err } - return false, nil + return len(matches) > 0, nil } // Determines whether permissions and run_as are explicitly set for all resources. From 1ab5598a8208d7b36c95edf93f987654c81b9f6e Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 22 Jul 2023 22:12:23 +0200 Subject: [PATCH 04/17] Relax run_as/permissions check We need top-level run_as for this logic to be fully effective --- .../mutator/process_environment_mode.go | 23 +++++-------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 19effe7f8b..a0414cf49c 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -126,8 +126,8 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs } } - if !arePermissionsSetExplicitly(r) { - return fmt.Errorf("'permissions' and 'run_as' must be set when using 'mode_production'") + if !isRunAsSet(r) { + return fmt.Errorf("'run_as' must be set for all jobs when using 'mode: production'") } } return nil @@ -146,22 +146,11 @@ func isServicePrincipalUsed(ctx context.Context, b *bundle.Bundle) (bool, error) return len(matches) > 0, nil } -// Determines whether permissions and run_as are explicitly set for all resources. -// We do this in a best-effort fashion; we may not actually test all resources, -// as we expect customers to use the top-level 'permissions' and 'run_as' fields. -// We'd rather not check for those specific fields though, as customers might -// set specific permissions instead! -func arePermissionsSetExplicitly(r config.Resources) bool { - for i := range r.Pipelines { - if r.Pipelines[i].Permissions == nil { - return false - } - } - +// Determines whether run_as is explicitly set for all resources. +// We do this in a best-effort fashion rather than check the top-level +// 'run_as' field because the latter is not required to be set. +func isRunAsSet(r config.Resources) bool { for i := range r.Jobs { - if r.Jobs[i].Permissions == nil { - return false - } if r.Jobs[i].RunAs == nil { return false } From e795273a77b82008defb191521d0621e17035541 Mon Sep 17 00:00:00 2001 From: "Lennart Kats (databricks)" Date: Thu, 20 Jul 2023 14:06:14 +0200 Subject: [PATCH 05/17] Update bundle/config/environment.go Co-authored-by: Fabian Jakobs --- bundle/config/environment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/environment.go b/bundle/config/environment.go index 3e66977e52..c1f4f4ad78 100644 --- a/bundle/config/environment.go +++ b/bundle/config/environment.go @@ -33,7 +33,7 @@ type Environment struct { const ( // Development mode: deployments done purely for running things in development. - // Any deployed resources will be marked as "dev" and might hidden or cleaned up. + // Any deployed resources will be marked as "dev" and might be hidden or cleaned up. Development Mode = "development" // Production mode: deployments done for production purposes. From 2ab8dfb2b21f6cb879cb1518441039c39ea0a8bc Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 24 Jul 2023 09:55:58 +0200 Subject: [PATCH 06/17] Fix test --- bundle/config/mutator/process_environment_mode_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_environment_mode_test.go b/bundle/config/mutator/process_environment_mode_test.go index f083f2d7eb..6f53abd898 100644 --- a/bundle/config/mutator/process_environment_mode_test.go +++ b/bundle/config/mutator/process_environment_mode_test.go @@ -91,7 +91,7 @@ func TestProcessEnvironmentModeProduction(t *testing.T) { bundle.Config.Workspace.FilesPath = "/Shared/.bundle/x/y/files" err = validateProductionMode(context.Background(), bundle, false) - require.ErrorContains(t, err, "permissions") + require.ErrorContains(t, err, "production") permissions := []resources.Permission{ { From 707c86f73f3666b3707a65bc28be3e0409fee4e9 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 24 Jul 2023 10:17:15 +0200 Subject: [PATCH 07/17] Add TODO --- libs/sync/path.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/sync/path.go b/libs/sync/path.go index d44336c16e..97a9089652 100644 --- a/libs/sync/path.go +++ b/libs/sync/path.go @@ -26,6 +26,9 @@ func repoPathForPath(me *iam.User, remotePath string) string { // expected base paths and if it is a directory or repository. func EnsureRemotePathIsUsable(ctx context.Context, wsc *databricks.WorkspaceClient, remotePath string, me *iam.User) error { var err error + + // TODO: we should cache CurrentUser.Me at the SDK level + // for now we let clients pass in any existing user they might already have if me == nil { me, err = wsc.CurrentUser.Me(ctx) if err != nil { From 1cb777468b2c9fa5a65e9a4964f49f7d994d8d18 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 28 Jul 2023 08:00:05 +0200 Subject: [PATCH 08/17] Add clarifying comment --- bundle/config/mutator/process_environment_mode.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index a0414cf49c..65d8a68934 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -137,6 +137,8 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs func isServicePrincipalUsed(ctx context.Context, b *bundle.Bundle) (bool, error) { ws := b.WorkspaceClient() + // Check if a principal with the current user's ID exists. + // We need to use the ListAll method since Get is only usable by admins. matches, err := ws.ServicePrincipals.ListAll(ctx, iam.ListServicePrincipalsRequest{ Filter: "id eq " + b.Config.Workspace.CurrentUser.Id, }) From 16daac289684c54459ecca0c51d839427d54baa1 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 15 Jul 2023 17:16:35 +0200 Subject: [PATCH 09/17] Add Git checks --- bundle/config/environment.go | 2 ++ bundle/config/git.go | 3 +++ bundle/config/mutator/load_git_details.go | 1 + bundle/config/mutator/process_environment_mode.go | 4 ++++ .../mutator/process_environment_mode_test.go | 15 +++++++++++++++ bundle/config/root.go | 14 ++++++++++++++ bundle/tests/autoload_git/databricks.yml | 10 ++++++++-- bundle/tests/autoload_git_test.go | 13 ++++++++----- bundle/tests/loader.go | 7 +++++++ 9 files changed, 62 insertions(+), 7 deletions(-) diff --git a/bundle/config/environment.go b/bundle/config/environment.go index c1f4f4ad78..7152f791f6 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 7ada8dfbc4..e9f5fc0cc6 100644 --- a/bundle/config/git.go +++ b/bundle/config/git.go @@ -4,4 +4,7 @@ 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"` } diff --git a/bundle/config/mutator/load_git_details.go b/bundle/config/mutator/load_git_details.go index 121924c62c..85e9eae7b7 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -32,6 +32,7 @@ func (m *loadGitDetails) Apply(ctx context.Context, b *bundle.Bundle) error { } else { b.Config.Bundle.Git.Branch = branch } + b.Config.Bundle.Git.Inferred = true } // load commit hash if undefined if b.Config.Bundle.Git.Commit == "" { diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 65d8a68934..c5b6be79f1 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -109,6 +109,10 @@ 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 { + return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.git' configuration") + } + 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 6f53abd898..36e0396e2d 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/root.go b/bundle/config/root.go index f5a4f00d36..68aa757f01 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -225,5 +225,19 @@ func (r *Root) MergeEnvironment(env *Environment) error { r.Bundle.ComputeID = env.ComputeID } + if env.Git.Branch != "" { + if r.Bundle.Git.Inferred && r.Bundle.Git.Branch != env.Git.Branch { + return fmt.Errorf("not on the right Git branch:\n expected according to configuration: %s\n actual: %s", env.Git.Branch, r.Bundle.Git.Branch) + } + r.Bundle.Git.Branch = env.Git.Branch + r.Bundle.Git.Inferred = false + } + if env.Git.Commit != "" { + r.Bundle.Git.Commit = env.Git.Commit + } + if env.Git.OriginURL != "" { + r.Bundle.Git.OriginURL = env.Git.OriginURL + } + return nil } diff --git a/bundle/tests/autoload_git/databricks.yml b/bundle/tests/autoload_git/databricks.yml index d0e1de60f4..bb34adb63f 100644 --- a/bundle/tests/autoload_git/databricks.yml +++ b/bundle/tests/autoload_git/databricks.yml @@ -1,4 +1,10 @@ bundle: name: autoload git config test - git: - branch: foo + +environments: + development: + default: true + + production: + git: + branch: production-branch diff --git a/bundle/tests/autoload_git_test.go b/bundle/tests/autoload_git_test.go index 87c7180e7a..34297a7f50 100644 --- a/bundle/tests/autoload_git_test.go +++ b/bundle/tests/autoload_git_test.go @@ -6,10 +6,13 @@ 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.NotEqual(t, "", b.Config.Bundle.Git.Branch) + assert.Contains(t, b.Config.Bundle.Git.OriginURL, "/cli") +} + +func TestWrongBranch(t *testing.T) { + err := loadEnvironmentWithError(t, "./autoload_git", "production") + assert.ErrorContains(t, err, "not on the right Git branch") } diff --git a/bundle/tests/loader.go b/bundle/tests/loader.go index 42f1fc5be2..271e044494 100644 --- a/bundle/tests/loader.go +++ b/bundle/tests/loader.go @@ -23,3 +23,10 @@ func loadEnvironment(t *testing.T, path, env string) *bundle.Bundle { require.NoError(t, err) return b } + +func loadEnvironmentWithError(t *testing.T, path, env string) error { + b := load(t, path) + err := bundle.Apply(context.Background(), b, mutator.SelectEnvironment(env)) + require.Error(t, err) + return err +} From dc34fc77e896ab006c71a0ab7664da82c34f0b58 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 15 Jul 2023 17:20:04 +0200 Subject: [PATCH 10/17] Only validate Git details when deploying --- bundle/config/git.go | 3 + bundle/config/mutator/load_git_details.go | 3 +- bundle/config/mutator/validate_git_details.go | 29 +++++++++ .../mutator/validate_git_details_test.go | 65 +++++++++++++++++++ bundle/config/root.go | 3 - bundle/phases/deploy.go | 2 + bundle/tests/autoload_git/databricks.yml | 3 +- bundle/tests/autoload_git_test.go | 9 ++- bundle/tests/loader.go | 7 -- 9 files changed, 109 insertions(+), 15 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/git.go b/bundle/config/git.go index e9f5fc0cc6..760134a861 100644 --- a/bundle/config/git.go +++ b/bundle/config/git.go @@ -7,4 +7,7 @@ type Git struct { // 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 85e9eae7b7..f22aafe01d 100644 --- a/bundle/config/mutator/load_git_details.go +++ b/bundle/config/mutator/load_git_details.go @@ -31,8 +31,9 @@ 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 } - b.Config.Bundle.Git.Inferred = true } // load commit hash if undefined if b.Config.Bundle.Git.Commit == "" { diff --git a/bundle/config/mutator/validate_git_details.go b/bundle/config/mutator/validate_git_details.go new file mode 100644 index 0000000000..b0422a1e14 --- /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 { + return fmt.Errorf("not on the right Git branch:\n expected according to configuration: %s\n actual: %s", 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 0000000000..9d6fb28e18 --- /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" + 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 68aa757f01..eb1d3d049b 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -226,9 +226,6 @@ func (r *Root) MergeEnvironment(env *Environment) error { } if env.Git.Branch != "" { - if r.Bundle.Git.Inferred && r.Bundle.Git.Branch != env.Git.Branch { - return fmt.Errorf("not on the right Git branch:\n expected according to configuration: %s\n actual: %s", env.Git.Branch, r.Bundle.Git.Branch) - } r.Bundle.Git.Branch = env.Git.Branch r.Bundle.Git.Inferred = false } diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 8b53273c7e..011bb4b2ba 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 bb34adb63f..ba4785aed0 100644 --- a/bundle/tests/autoload_git/databricks.yml +++ b/bundle/tests/autoload_git/databricks.yml @@ -6,5 +6,6 @@ environments: default: true production: + # production can only be deployed from the 'main' branch git: - branch: production-branch + branch: main diff --git a/bundle/tests/autoload_git_test.go b/bundle/tests/autoload_git_test.go index 34297a7f50..93e874c308 100644 --- a/bundle/tests/autoload_git_test.go +++ b/bundle/tests/autoload_git_test.go @@ -9,10 +9,13 @@ import ( func TestAutoLoad(t *testing.T) { b := load(t, "./autoload_git") assert.NotEqual(t, "", b.Config.Bundle.Git.Branch) + assert.True(t, b.Config.Bundle.Git.Inferred) assert.Contains(t, b.Config.Bundle.Git.OriginURL, "/cli") } -func TestWrongBranch(t *testing.T) { - err := loadEnvironmentWithError(t, "./autoload_git", "production") - assert.ErrorContains(t, err, "not on the right Git branch") +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/bundle/tests/loader.go b/bundle/tests/loader.go index 271e044494..42f1fc5be2 100644 --- a/bundle/tests/loader.go +++ b/bundle/tests/loader.go @@ -23,10 +23,3 @@ func loadEnvironment(t *testing.T, path, env string) *bundle.Bundle { require.NoError(t, err) return b } - -func loadEnvironmentWithError(t *testing.T, path, env string) error { - b := load(t, path) - err := bundle.Apply(context.Background(), b, mutator.SelectEnvironment(env)) - require.Error(t, err) - return err -} From 8732b532b35e1ecb2e09ebf2b7563bf52831d392 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 17 Jul 2023 10:37:25 +0200 Subject: [PATCH 11/17] Fix test --- bundle/config/mutator/process_environment_mode.go | 2 +- bundle/tests/autoload_git_test.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index c5b6be79f1..373013ec4c 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -110,7 +110,7 @@ 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 { - return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.git' configuration") + return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.environment.git' configuration") } r := b.Config.Resources diff --git a/bundle/tests/autoload_git_test.go b/bundle/tests/autoload_git_test.go index 93e874c308..a1075198fa 100644 --- a/bundle/tests/autoload_git_test.go +++ b/bundle/tests/autoload_git_test.go @@ -8,7 +8,6 @@ import ( func TestAutoLoad(t *testing.T) { b := load(t, "./autoload_git") - assert.NotEqual(t, "", b.Config.Bundle.Git.Branch) assert.True(t, b.Config.Bundle.Git.Inferred) assert.Contains(t, b.Config.Bundle.Git.OriginURL, "/cli") } From fc9b52527e69077c1a0b1239014929f6f63e7a39 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Tue, 18 Jul 2023 09:00:59 +0200 Subject: [PATCH 12/17] Improve error --- bundle/config/mutator/process_environment_mode.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 373013ec4c..9bcb3bd8bf 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -110,7 +110,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 { - return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.environment.git' configuration") + env := b.Config.Bundle.Environment + if env == "" { + env = "" + } + return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.%s.git' configuration", env) } r := b.Config.Resources From b69f784d631f7add14193275bf7ae4b578e0edb0 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 22 Jul 2023 22:05:15 +0200 Subject: [PATCH 13/17] Allow --force to override Git validation --- bundle/config/bundle.go | 4 ++++ bundle/config/lock.go | 4 ---- bundle/config/mutator/validate_git_details.go | 4 ++-- bundle/config/mutator/validate_git_details_test.go | 2 +- bundle/deploy/lock/acquire.go | 2 +- cmd/bundle/deploy.go | 10 ++++++++-- cmd/bundle/destroy.go | 2 +- 7 files changed, 17 insertions(+), 11 deletions(-) diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index cf38647754..43ec99a5e4 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -25,6 +25,10 @@ type Bundle struct { // Lock configures locking behavior on deployment. Lock Lock `json:"lock" bundle:"readonly"` + // Force-override deployment lock and Git branch validation. + // This may be necessary if a prior deployment failed to release the lock. + Force bool `json:"-" 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/lock.go b/bundle/config/lock.go index 28d5a5acf5..83c02094a7 100644 --- a/bundle/config/lock.go +++ b/bundle/config/lock.go @@ -5,10 +5,6 @@ type Lock struct { // Use a pointer value so that only explicitly configured values are set // and we don't merge configuration with zero-initialized values. Enabled *bool `json:"enabled"` - - // Force acquisition of deployment lock even if it is currently held. - // This may be necessary if a prior deployment failed to release the lock. - Force bool `json:"force"` } func (lock Lock) IsEnabled() bool { diff --git a/bundle/config/mutator/validate_git_details.go b/bundle/config/mutator/validate_git_details.go index b0422a1e14..116498bfc1 100644 --- a/bundle/config/mutator/validate_git_details.go +++ b/bundle/config/mutator/validate_git_details.go @@ -22,8 +22,8 @@ func (m *validateGitDetails) Apply(ctx context.Context, b *bundle.Bundle) error return nil } - if b.Config.Bundle.Git.Branch != b.Config.Bundle.Git.ActualBranch { - return fmt.Errorf("not on the right Git branch:\n expected according to configuration: %s\n actual: %s", b.Config.Bundle.Git.Branch, b.Config.Bundle.Git.ActualBranch) + 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 index 9d6fb28e18..252964eeb7 100644 --- a/bundle/config/mutator/validate_git_details_test.go +++ b/bundle/config/mutator/validate_git_details_test.go @@ -42,7 +42,7 @@ func TestValidateGitDetailsNonMatchingBranches(t *testing.T) { m := ValidateGitDetails() err := m.Apply(context.Background(), bundle) - expectedError := "not on the right Git branch:\n expected according to configuration: main\n actual: feature" + 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) } diff --git a/bundle/deploy/lock/acquire.go b/bundle/deploy/lock/acquire.go index 18778aa5dd..1a5b980132 100644 --- a/bundle/deploy/lock/acquire.go +++ b/bundle/deploy/lock/acquire.go @@ -42,7 +42,7 @@ func (m *acquire) Apply(ctx context.Context, b *bundle.Bundle) error { return err } - force := b.Config.Bundle.Lock.Force + force := b.Config.Bundle.Force log.Infof(ctx, "Acquiring deployment lock (force: %v)", force) err = b.Locker.Lock(ctx, force) if err != nil { diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index a39f199690..5887027cae 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -22,7 +22,7 @@ func newDeployCommand() *cobra.Command { 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 = forceDeploy b.Config.Bundle.ComputeID = computeID return bundle.Apply(cmd.Context(), b, bundle.Seq( @@ -32,5 +32,11 @@ func newDeployCommand() *cobra.Command { )) } - return cmd +var forceDeploy bool +var computeID string + +func init() { + AddCommand(deployCmd) + deployCmd.Flags().BoolVar(&forceDeploy, "force", false, "Force-override deployment lock and Git branch validation.") + deployCmd.Flags().StringVarP(&computeID, "compute-id", "c", "", "Override compute in the deployment with the given compute ID.") } diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index 82d821441b..ea4d826a08 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -30,7 +30,7 @@ func newDestroyCommand() *cobra.Command { b := bundle.Get(ctx) // If `--force` is specified, force acquisition of the deployment lock. - b.Config.Bundle.Lock.Force = forceDestroy + b.Config.Bundle.Force = forceDestroy // If `--auto-approve`` is specified, we skip confirmation checks b.AutoApprove = autoApprove From ffaacec0cfef78a4d21373d84c6b8cdb6c56e56e Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sun, 30 Jul 2023 14:11:33 +0200 Subject: [PATCH 14/17] Cleanup --- bundle/config/mutator/process_environment_mode.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/bundle/config/mutator/process_environment_mode.go b/bundle/config/mutator/process_environment_mode.go index 9bcb3bd8bf..d20302347c 100644 --- a/bundle/config/mutator/process_environment_mode.go +++ b/bundle/config/mutator/process_environment_mode.go @@ -111,9 +111,6 @@ 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 - if env == "" { - env = "" - } return fmt.Errorf("environment with 'mode: production' must specify an explicit 'environments.%s.git' configuration", env) } From ea412daa01a6f186f3e484590b61cbe137876a86 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sun, 30 Jul 2023 14:22:09 +0200 Subject: [PATCH 15/17] Restore separate --force-lock option --- bundle/config/bundle.go | 5 ++--- bundle/config/lock.go | 4 ++++ bundle/deploy/lock/acquire.go | 2 +- cmd/bundle/deploy.go | 18 ++++++------------ cmd/bundle/destroy.go | 6 +++--- 5 files changed, 16 insertions(+), 19 deletions(-) diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index 43ec99a5e4..f3401477f0 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -25,9 +25,8 @@ type Bundle struct { // Lock configures locking behavior on deployment. Lock Lock `json:"lock" bundle:"readonly"` - // Force-override deployment lock and Git branch validation. - // This may be necessary if a prior deployment failed to release the lock. - Force bool `json:"-" 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 diff --git a/bundle/config/lock.go b/bundle/config/lock.go index 83c02094a7..28d5a5acf5 100644 --- a/bundle/config/lock.go +++ b/bundle/config/lock.go @@ -5,6 +5,10 @@ type Lock struct { // Use a pointer value so that only explicitly configured values are set // and we don't merge configuration with zero-initialized values. Enabled *bool `json:"enabled"` + + // Force acquisition of deployment lock even if it is currently held. + // This may be necessary if a prior deployment failed to release the lock. + Force bool `json:"force"` } func (lock Lock) IsEnabled() bool { diff --git a/bundle/deploy/lock/acquire.go b/bundle/deploy/lock/acquire.go index 1a5b980132..18778aa5dd 100644 --- a/bundle/deploy/lock/acquire.go +++ b/bundle/deploy/lock/acquire.go @@ -42,7 +42,7 @@ func (m *acquire) Apply(ctx context.Context, b *bundle.Bundle) error { return err } - force := b.Config.Bundle.Force + force := b.Config.Bundle.Lock.Force log.Infof(ctx, "Acquiring deployment lock (force: %v)", force) err = b.Locker.Lock(ctx, force) if err != nil { diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index 5887027cae..567425f573 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.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( @@ -31,12 +33,4 @@ func newDeployCommand() *cobra.Command { phases.Deploy(), )) } - -var forceDeploy bool -var computeID string - -func init() { - AddCommand(deployCmd) - deployCmd.Flags().BoolVar(&forceDeploy, "force", false, "Force-override deployment lock and Git branch validation.") - deployCmd.Flags().StringVarP(&computeID, "compute-id", "c", "", "Override compute in the deployment with the given compute ID.") } diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index ea4d826a08..22d998abe2 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -23,14 +23,14 @@ 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. - b.Config.Bundle.Force = forceDestroy + // 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 b.AutoApprove = autoApprove From c704196c5f098e54022d574cf4ecaea8549bce1b Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sun, 30 Jul 2023 14:29:39 +0200 Subject: [PATCH 16/17] Cleanup --- bundle/config/root.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bundle/config/root.go b/bundle/config/root.go index eb1d3d049b..52f8873788 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -225,15 +225,16 @@ func (r *Root) MergeEnvironment(env *Environment) error { r.Bundle.ComputeID = env.ComputeID } + git := &r.Bundle.Git if env.Git.Branch != "" { - r.Bundle.Git.Branch = env.Git.Branch - r.Bundle.Git.Inferred = false + git.Branch = env.Git.Branch + git.Inferred = false } if env.Git.Commit != "" { - r.Bundle.Git.Commit = env.Git.Commit + git.Commit = env.Git.Commit } if env.Git.OriginURL != "" { - r.Bundle.Git.OriginURL = env.Git.OriginURL + git.OriginURL = env.Git.OriginURL } return nil From 8d27d7775244cd14eead930c2b09f669d22f0a04 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sun, 30 Jul 2023 14:39:15 +0200 Subject: [PATCH 17/17] Fix missing return statement --- cmd/bundle/deploy.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index 567425f573..807bb982da 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -33,4 +33,6 @@ func newDeployCommand() *cobra.Command { phases.Deploy(), )) } + + return cmd }