From e5ac74d31a4bab6bca86a2831c370b1c431fa2d0 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 8 Jun 2024 18:14:02 +0200 Subject: [PATCH 01/23] Initial draft of customizable transformation --- bundle/config/bundle.go | 4 + bundle/config/mutator/process_target_mode.go | 102 +++++--------- .../mutator/process_target_mode_test.go | 6 +- bundle/config/mutator/transformers.go | 131 ++++++++++++++++++ bundle/config/mutator/transformers_test.go | 1 + bundle/config/target.go | 4 + bundle/config/transformers.go | 37 +++++ cmd/bundle/deploy.go | 5 + 8 files changed, 219 insertions(+), 71 deletions(-) create mode 100644 bundle/config/mutator/transformers.go create mode 100644 bundle/config/mutator/transformers_test.go create mode 100644 bundle/config/transformers.go diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index 78648dfd7f..9d49b406e3 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -38,6 +38,10 @@ type Bundle struct { // Annotated readonly as this should be set at the target level. Mode Mode `json:"mode,omitempty" bundle:"readonly"` + // Transformers apply some transformation throughout the bundle, e.g. + // adding a name prefix to deployed resources. + Transformers Transformers `json:"mutators,omitempty"` + // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 8e70fab73b..f4067f289b 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -2,7 +2,6 @@ package mutator import ( "context" - "path" "strings" "github.com/databricks/cli/bundle" @@ -10,9 +9,8 @@ import ( "github.com/databricks/cli/libs/auth" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/merge" "github.com/databricks/cli/libs/log" - "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/databricks/databricks-sdk-go/service/ml" ) type processTargetMode struct{} @@ -33,87 +31,55 @@ func (m *processTargetMode) Name() string { func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { if !b.Config.Bundle.Deployment.Lock.IsExplicitlyEnabled() { log.Infof(ctx, "Development mode: disabling deployment lock since bundle.deployment.lock.enabled is not set to true") - err := disableDeploymentLock(b) + err := setConfig(b, "bundle.deployment.lock.enabled", false) if err != nil { - return diag.FromErr(err) + return err } } - r := b.Config.Resources + m := b.Config.Bundle.Transformers shortName := b.Config.Workspace.CurrentUser.ShortName - prefix := "[dev " + shortName + "] " - - // Generate a normalized version of the short name that can be used as a tag value. - tagValue := b.Tagging.NormalizeValue(shortName) - - for i := range r.Jobs { - 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"] = tagValue - if r.Jobs[i].MaxConcurrentRuns == 0 { - r.Jobs[i].MaxConcurrentRuns = developmentConcurrentRuns - } - - // Pause each job. As an exception, we don't pause jobs that are explicitly - // marked as "unpaused". This allows users to override the default behavior - // of the development mode. - if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus != jobs.PauseStatusUnpaused { - r.Jobs[i].Schedule.PauseStatus = jobs.PauseStatusPaused - } - if r.Jobs[i].Continuous != nil && r.Jobs[i].Continuous.PauseStatus != jobs.PauseStatusUnpaused { - r.Jobs[i].Continuous.PauseStatus = jobs.PauseStatusPaused - } - if r.Jobs[i].Trigger != nil && r.Jobs[i].Trigger.PauseStatus != jobs.PauseStatusUnpaused { - r.Jobs[i].Trigger.PauseStatus = jobs.PauseStatusPaused - } - } - - for i := range r.Pipelines { - 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 = prefix + r.Models[i].Name - r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: "dev", Value: tagValue}) + // TODO: only set these configs if they're not already set + err := setConfig(b, "bundle.mutators.prefix.value", "[dev "+shortName+"] ") + if err != nil { + return err } - for i := range r.Experiments { - filepath := r.Experiments[i].Name - dir := path.Dir(filepath) - base := path.Base(filepath) - if dir == "." { - r.Experiments[i].Name = prefix + base - } else { - r.Experiments[i].Name = dir + "/" + prefix + base - } - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: tagValue}) - } + // TODO: only set tag if it doesn't already exist + setConfigMapping(b, "bundle.mutators.tags.tags", "dev", b.Tagging.NormalizeValue(shortName)) - for i := range r.ModelServingEndpoints { - prefix = "dev_" + b.Config.Workspace.CurrentUser.ShortName + "_" - r.ModelServingEndpoints[i].Name = prefix + r.ModelServingEndpoints[i].Name - // (model serving doesn't yet support tags) - } + setConfig(b, "bundle.mutators.jobs_max_concurrent_runs.value", developmentConcurrentRuns) // } - for i := range r.RegisteredModels { - prefix = "dev_" + b.Config.Workspace.CurrentUser.ShortName + "_" - r.RegisteredModels[i].Name = prefix + r.RegisteredModels[i].Name - // (registered models in Unity Catalog don't yet support tags) + if m.JobsSchedulePauseStatus.Enabled == nil { + enabled := true + setConfig(b, "bundle.mutators.job_schedule_pause_status.enabled", enabled) } return nil } -func disableDeploymentLock(b *bundle.Bundle) error { - return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - return dyn.Map(v, "bundle.deployment.lock", func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { - return dyn.Set(v, "enabled", dyn.V(false)) - }) +func setConfig(b *bundle.Bundle, path string, value any) diag.Diagnostics { + err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, path, dyn.V(value)) + }) + return diag.FromErr(err) +} + +func setConfigMapping(b *bundle.Bundle, path string, key string, value string) diag.Diagnostics { + err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + existingMapping, err := dyn.Get(v, path) + if err != nil { + return dyn.InvalidValue, err + } + newMapping := dyn.V(map[string]dyn.Value{key: dyn.V(value)}) + merged, err := merge.Merge(newMapping, existingMapping) + if err != nil { + return dyn.InvalidValue, err + } + return dyn.Set(v, path, merged) }) + return diag.FromErr(err) } func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index cf8229bfeb..6f3df0c62d 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -112,7 +112,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { func TestProcessTargetModeDevelopment(t *testing.T) { b := mockBundle(config.Development) - m := ProcessTargetMode() + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -198,7 +198,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { func TestProcessTargetModeDefault(t *testing.T) { b := mockBundle("") - m := ProcessTargetMode() + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) @@ -284,7 +284,7 @@ func TestAllResourcesMocked(t *testing.T) { func TestAllResourcesRenamed(t *testing.T) { b := mockBundle(config.Development) - m := ProcessTargetMode() + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go new file mode 100644 index 0000000000..88b2ca9578 --- /dev/null +++ b/bundle/config/mutator/transformers.go @@ -0,0 +1,131 @@ +package mutator + +import ( + "context" + "path" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/textutil" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/databricks/databricks-sdk-go/service/ml" +) + +type transformers struct{} + +func TransformersMutator() *transformers { + return &transformers{} +} + +func (m *transformers) Name() string { + return "TransformersMutator" +} + +func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + r := b.Config.Resources + mutators := b.Config.Bundle.Transformers + + prefix := mutators.Prefix.Value + if !isEnabled(mutators.Prefix.Enabled) { + prefix = "" + } + + for i := range r.Jobs { + r.Jobs[i].Name = prefix + r.Jobs[i].Name + if isEnabled(mutators.JobsMaxConcurrentRuns.Enabled) && r.Jobs[i].MaxConcurrentRuns == 0 { + r.Jobs[i].MaxConcurrentRuns = mutators.JobsMaxConcurrentRuns.Value + } + + if isEnabled(mutators.JobsSchedulePauseStatus.Enabled) { + if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus != jobs.PauseStatusUnpaused { + r.Jobs[i].Schedule.PauseStatus = jobs.PauseStatusPaused + } + if r.Jobs[i].Continuous != nil && r.Jobs[i].Continuous.PauseStatus != jobs.PauseStatusUnpaused { + r.Jobs[i].Continuous.PauseStatus = jobs.PauseStatusPaused + } + if r.Jobs[i].Trigger != nil && r.Jobs[i].Trigger.PauseStatus != jobs.PauseStatusUnpaused { + r.Jobs[i].Trigger.PauseStatus = jobs.PauseStatusPaused + } + } + } + + for i := range r.Pipelines { + r.Pipelines[i].Name = prefix + r.Pipelines[i].Name + if isEnabled(mutators.PipelinesDevelopment.Enabled) { + r.Pipelines[i].Development = true + } + } + + for i := range r.Models { + r.Models[i].Name = prefix + r.Models[i].Name + } + + for i := range r.Experiments { + filepath := r.Experiments[i].Name + dir := path.Dir(filepath) + base := path.Base(filepath) + if dir == "." { + r.Experiments[i].Name = prefix + base + } else { + r.Experiments[i].Name = dir + "/" + prefix + base + } + } + + for i := range r.ModelServingEndpoints { + r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name + } + + for i := range r.RegisteredModels { + r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name + } + + addTags(b) + + return nil +} + +func isEnabled(enabled *bool) bool { + return enabled != nil && *enabled +} + +// Normalize prefix strings like '[dev lennart] ' to 'dev_lennart_'. +// We leave unicode letters and numbers but remove all "special characters." +func normalizePrefix(prefix string) string { + prefix = strings.ReplaceAll(prefix, "[", "") + prefix = strings.ReplaceAll(prefix, "] ", "_") + return textutil.NormalizeString(prefix) +} + +// Add tags for supported resources. +// As of 2024-04, pipelines, model serving, and registered models in Unity Catalog +// don't yet support tags. +func addTags(b *bundle.Bundle) { + mutators := b.Config.Bundle.Transformers + if !isEnabled(mutators.Tags.Enabled) { + return + } + + r := b.Config.Resources + tags := mutators.Tags.Tags + for tagKey, tagValue := range tags { + + for i := range r.Jobs { + if r.Jobs[i].Tags == nil { + r.Jobs[i].Tags = make(map[string]string) + } + r.Jobs[i].Tags[tagKey] = tagValue + if r.Jobs[i].MaxConcurrentRuns == 0 { + r.Jobs[i].MaxConcurrentRuns = developmentConcurrentRuns + } + } + + for i := range r.Models { + r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: tagKey, Value: tagValue}) + } + + for i := range r.Experiments { + r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: tagKey, Value: tagValue}) + } + } +} diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/transformers_test.go new file mode 100644 index 0000000000..b8c71c8d43 --- /dev/null +++ b/bundle/config/mutator/transformers_test.go @@ -0,0 +1 @@ +package mutator diff --git a/bundle/config/target.go b/bundle/config/target.go index acc493574b..d7ed723cfa 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -20,6 +20,10 @@ type Target struct { // development purposes. Mode Mode `json:"mode,omitempty"` + // Mutator configurations that e.g. change the + // name prefix of deployed resources. + Mutators Transformers `json:"mutators,omitempty"` + // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/config/transformers.go b/bundle/config/transformers.go new file mode 100644 index 0000000000..393ef13a5f --- /dev/null +++ b/bundle/config/transformers.go @@ -0,0 +1,37 @@ +package config + +type Prefix struct { + Value string `json:"value,omitempty"` + // Whether this feature is enabled. Treated as 'true' when not set. + Enabled *bool `json:"enabled,omitempty"` +} + +type PipelinesDevelopment struct { + // Whether this feature is enabled. Treated as 'true' when not set. + Enabled *bool `json:"enabled,omitempty"` +} + +type JobsSchedulePauseStatus struct { + // Whether to set PauseStatus to PauseStatusPaused + Enabled *bool `json:"enabled,omitempty"` +} + +type JobsMaxConcurrentRuns struct { + Value int `json:"value,omitempty"` + // Whether this feature is enabled. Treated as 'true' when not set. + Enabled *bool `json:"enabled,omitempty"` +} + +type Tags struct { + Tags map[string]string `json:"tags,omitempty"` + // Whether this feature is enabled. Treated as 'true' when not set. + Enabled *bool `json:"enabled,omitempty"` +} + +type Transformers struct { + Prefix Prefix `json:"prefix,omitempty"` + PipelinesDevelopment PipelinesDevelopment `json:"pipelines_set_development,omitempty"` + JobsSchedulePauseStatus JobsSchedulePauseStatus `json:"jobs_schedule_pause_status,omitempty"` + JobsMaxConcurrentRuns JobsMaxConcurrentRuns `json:"jobs_max_concurrent_runs,omitempty"` + Tags Tags `json:"tags,omitempty"` +} diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index 919b15a723..cb9e15a9f9 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -2,11 +2,13 @@ package bundle import ( "context" + "fmt" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" "github.com/databricks/cli/cmd/root" + "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" "github.com/spf13/cobra" ) @@ -56,6 +58,9 @@ func newDeployCommand() *cobra.Command { if err := diags.Error(); err != nil { return err } + for _, diag := range diags { + cmdio.LogString(ctx, fmt.Sprintf("Warning: %s", diag.Summary)) + } return nil } From 6eaee84446d6325dce68aa859c00bd713e4d5f1a Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Tue, 11 Jun 2024 21:28:02 +0200 Subject: [PATCH 02/23] Refactor to one function per transformer --- bundle/config/mutator/process_target_mode.go | 2 +- bundle/config/mutator/transformers.go | 51 ++++++++++++-------- bundle/config/transformers.go | 12 ++--- 3 files changed, 39 insertions(+), 26 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index f4067f289b..a323cec956 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -51,7 +51,7 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno setConfig(b, "bundle.mutators.jobs_max_concurrent_runs.value", developmentConcurrentRuns) // } - if m.JobsSchedulePauseStatus.Enabled == nil { + if m.TriggerPauseStatus.Enabled == nil { enabled := true setConfig(b, "bundle.mutators.job_schedule_pause_status.enabled", enabled) } diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go index 88b2ca9578..025db3e29e 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/transformers.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/textutil" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -23,21 +24,36 @@ func (m *transformers) Name() string { } func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - r := b.Config.Resources mutators := b.Config.Bundle.Transformers + r := b.Config.Resources - prefix := mutators.Prefix.Value - if !isEnabled(mutators.Prefix.Enabled) { - prefix = "" + if isEnabled(mutators.Prefix.Enabled) { + transformPrefix(r, mutators.Prefix.Value) + } + if isEnabled(mutators.Tags.Enabled) { + transformTags(r, mutators.Tags.Tags) + } + if isEnabled(mutators.JobsMaxConcurrentRuns.Enabled) { + transformJobsMaxConcurrentRuns(r, mutators.JobsMaxConcurrentRuns.Value) + } + if isEnabled(mutators.TriggerPauseStatus.Enabled) { + transformTriggerPauseStatus(r, mutators.TriggerPauseStatus.Enabled) } + if isEnabled(mutators.PipelinesDevelopment.Enabled) { + transformPipelinesDevelopment(r) + } + + return nil +} +func transformPrefix(r config.Resources, prefix string) { for i := range r.Jobs { r.Jobs[i].Name = prefix + r.Jobs[i].Name if isEnabled(mutators.JobsMaxConcurrentRuns.Enabled) && r.Jobs[i].MaxConcurrentRuns == 0 { r.Jobs[i].MaxConcurrentRuns = mutators.JobsMaxConcurrentRuns.Value } - if isEnabled(mutators.JobsSchedulePauseStatus.Enabled) { + if isEnabled(mutators.TriggerPauseStatus.Enabled) { if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus != jobs.PauseStatusUnpaused { r.Jobs[i].Schedule.PauseStatus = jobs.PauseStatusPaused } @@ -50,6 +66,7 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } } + // Pipelines transformers: Prefix, PipelinesDevelopment for i := range r.Pipelines { r.Pipelines[i].Name = prefix + r.Pipelines[i].Name if isEnabled(mutators.PipelinesDevelopment.Enabled) { @@ -57,10 +74,12 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } } + // Models transformers: Prefix for i := range r.Models { r.Models[i].Name = prefix + r.Models[i].Name } + // Experiments transformers: Prefix for i := range r.Experiments { filepath := r.Experiments[i].Name dir := path.Dir(filepath) @@ -72,19 +91,19 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } } + // Model serving endpoint transformers: Prefix for i := range r.ModelServingEndpoints { r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name } + // Registered models transformers: Prefix for i := range r.RegisteredModels { r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name } - - addTags(b) - - return nil } +// Test whether a transformer is enabled. +// Enablement has three states: explicitly enabled, explicitly disabled, and not set. func isEnabled(enabled *bool) bool { return enabled != nil && *enabled } @@ -98,16 +117,7 @@ func normalizePrefix(prefix string) string { } // Add tags for supported resources. -// As of 2024-04, pipelines, model serving, and registered models in Unity Catalog -// don't yet support tags. -func addTags(b *bundle.Bundle) { - mutators := b.Config.Bundle.Transformers - if !isEnabled(mutators.Tags.Enabled) { - return - } - - r := b.Config.Resources - tags := mutators.Tags.Tags +func transformTags(r config.Resources, tags map[string]string) { for tagKey, tagValue := range tags { for i := range r.Jobs { @@ -127,5 +137,8 @@ func addTags(b *bundle.Bundle) { for i := range r.Experiments { r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: tagKey, Value: tagValue}) } + + // As of 2024-04, pipelines, model serving, and registered models in Unity Catalog + // don't yet support tags. } } diff --git a/bundle/config/transformers.go b/bundle/config/transformers.go index 393ef13a5f..228aef21c4 100644 --- a/bundle/config/transformers.go +++ b/bundle/config/transformers.go @@ -11,7 +11,7 @@ type PipelinesDevelopment struct { Enabled *bool `json:"enabled,omitempty"` } -type JobsSchedulePauseStatus struct { +type TriggerPauseStatus struct { // Whether to set PauseStatus to PauseStatusPaused Enabled *bool `json:"enabled,omitempty"` } @@ -29,9 +29,9 @@ type Tags struct { } type Transformers struct { - Prefix Prefix `json:"prefix,omitempty"` - PipelinesDevelopment PipelinesDevelopment `json:"pipelines_set_development,omitempty"` - JobsSchedulePauseStatus JobsSchedulePauseStatus `json:"jobs_schedule_pause_status,omitempty"` - JobsMaxConcurrentRuns JobsMaxConcurrentRuns `json:"jobs_max_concurrent_runs,omitempty"` - Tags Tags `json:"tags,omitempty"` + Prefix Prefix `json:"prefix,omitempty"` + PipelinesDevelopment PipelinesDevelopment `json:"pipelines_set_development,omitempty"` + TriggerPauseStatus TriggerPauseStatus `json:"trigger_pause_status,omitempty"` + JobsMaxConcurrentRuns JobsMaxConcurrentRuns `json:"jobs_max_concurrent_runs,omitempty"` + Tags Tags `json:"tags,omitempty"` } From 601c32b24f56c22f43afcfd66ee8da07a6f1908b Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Tue, 11 Jun 2024 23:37:20 +0200 Subject: [PATCH 03/23] WIP --- bundle/config/bundle.go | 2 +- bundle/config/mutator/process_target_mode.go | 45 +++- .../mutator/process_target_mode_test.go | 109 ++++++++- bundle/config/mutator/transformers.go | 132 +++++------ bundle/config/mutator/transformers_test.go | 215 +++++++++++++++++- bundle/config/target.go | 2 +- bundle/config/transformers.go | 48 +++- 7 files changed, 451 insertions(+), 102 deletions(-) diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index 9d49b406e3..263fb25d6f 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -40,7 +40,7 @@ type Bundle struct { // Transformers apply some transformation throughout the bundle, e.g. // adding a name prefix to deployed resources. - Transformers Transformers `json:"mutators,omitempty"` + Transformers Transformers `json:"transformers,omitempty"` // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index a323cec956..40b6e53574 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -40,20 +40,39 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno m := b.Config.Bundle.Transformers shortName := b.Config.Workspace.CurrentUser.ShortName - // TODO: only set these configs if they're not already set - err := setConfig(b, "bundle.mutators.prefix.value", "[dev "+shortName+"] ") - if err != nil { - return err + if m.Prefix.IsEnabled() && m.Prefix.Value == "" { + err := setConfig(b, "bundle.transformers.prefix.value", "[dev "+shortName+"] ") + if err != nil { + return err + } + } + + if m.Tags.IsEnabled() && m.Tags.Tags == nil { + err := setConfigMapping(b, "bundle.transformers.tags.tags", "dev", b.Tagging.NormalizeValue(shortName)) + if err != nil { + return err + } } - // TODO: only set tag if it doesn't already exist - setConfigMapping(b, "bundle.mutators.tags.tags", "dev", b.Tagging.NormalizeValue(shortName)) + if m.JobsMaxConcurrentRuns.IsEnabled() && m.JobsMaxConcurrentRuns.Value == 0 { + err := setConfig(b, "bundle.transformers.jobs_max_concurrent_runs.value", developmentConcurrentRuns) + if err != nil { + return err + } + } - setConfig(b, "bundle.mutators.jobs_max_concurrent_runs.value", developmentConcurrentRuns) // } + if !m.TriggerPauseStatus.IsExplicitlyDisabled() { + err := setConfig(b, "bundle.transformers.trigger_pause_status.enabled", true) + if err != nil { + return err + } + } - if m.TriggerPauseStatus.Enabled == nil { - enabled := true - setConfig(b, "bundle.mutators.job_schedule_pause_status.enabled", enabled) + if !m.PipelinesDevelopment.IsExplicitlyDisabled() { + err := setConfig(b, "bundle.transformers.pipelines_development.enabled", true) + if err != nil { + return err + } } return nil @@ -68,11 +87,13 @@ func setConfig(b *bundle.Bundle, path string, value any) diag.Diagnostics { func setConfigMapping(b *bundle.Bundle, path string, key string, value string) diag.Diagnostics { err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + newMapping := dyn.V(map[string]dyn.Value{key: dyn.V(value)}) + existingMapping, err := dyn.Get(v, path) if err != nil { - return dyn.InvalidValue, err + return dyn.Set(v, path, newMapping) } - newMapping := dyn.V(map[string]dyn.Value{key: dyn.V(value)}) + merged, err := merge.Merge(newMapping, existingMapping) if err != nil { return dyn.InvalidValue, err diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 6f3df0c62d..1ae61695b5 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -51,6 +51,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Schedule: &jobs.CronSchedule{ QuartzCronExpression: "* * * * *", }, + Tags: map[string]string{"existing": "tag"}, }, }, "job2": { @@ -118,6 +119,7 @@ func TestProcessTargetModeDevelopment(t *testing.T) { // Job 1 assert.Equal(t, "[dev lennart] job1", b.Config.Resources.Jobs["job1"].Name) + assert.Equal(t, b.Config.Resources.Jobs["job1"].Tags["existing"], "tag") assert.Equal(t, b.Config.Resources.Jobs["job1"].Tags["dev"], "lennart") assert.Equal(t, b.Config.Resources.Jobs["job1"].Schedule.PauseStatus, jobs.PauseStatusPaused) @@ -160,7 +162,8 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAws(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - diags := bundle.Apply(context.Background(), b, ProcessTargetMode()) + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) // Assert that tag normalization took place. @@ -174,7 +177,8 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAzure(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - diags := bundle.Apply(context.Background(), b, ProcessTargetMode()) + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) // Assert that tag normalization took place (Azure allows more characters than AWS). @@ -188,7 +192,8 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - diags := bundle.Apply(context.Background(), b, ProcessTargetMode()) + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) // Assert that tag normalization took place. @@ -329,3 +334,101 @@ func TestDisableLockingDisabled(t *testing.T) { require.Nil(t, err) assert.True(t, b.Config.Bundle.Deployment.Lock.IsEnabled(), "Deployment lock should remain enabled in development mode when explicitly enabled") } + +func TestPrefixAlreadySet(t *testing.T) { + b := mockBundle(config.Development) + b.Config.Bundle.Transformers.Prefix.Value = "custom_prefix_" + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.Equal(t, "custom_prefix_job1", b.Config.Resources.Jobs["job1"].Name) +} + +func TestPrefixDisabled(t *testing.T) { + b := mockBundle(config.Development) + notEnabled := false + b.Config.Bundle.Transformers.Prefix.Value = "custom_prefix_" + b.Config.Bundle.Transformers.Prefix.Enabled = ¬Enabled + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) +} + +func TestTagsAlreadySet(t *testing.T) { + b := mockBundle(config.Development) + b.Config.Bundle.Transformers.Tags.Tags = map[string]string{"custom": "tag"} + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["custom"]) +} + +func TestTagsDisabled(t *testing.T) { + b := mockBundle(config.Development) + notEnabled := false + b.Config.Bundle.Transformers.Tags.Tags = map[string]string{"custom": "tag"} + b.Config.Bundle.Transformers.Tags.Enabled = ¬Enabled + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["existing"]) + assert.Empty(t, b.Config.Resources.Jobs["job2"].Tags) +} + +func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { + b := mockBundle(config.Development) + b.Config.Bundle.Transformers.JobsMaxConcurrentRuns.Value = 10 + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.Equal(t, 10, b.Config.Resources.Jobs["job1"].MaxConcurrentRuns) +} + +func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { + b := mockBundle(config.Development) + notEnabled := false + b.Config.Bundle.Transformers.JobsMaxConcurrentRuns.Value = 10 + b.Config.Bundle.Transformers.JobsMaxConcurrentRuns.Enabled = ¬Enabled + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.Zero(t, b.Config.Resources.Jobs["job1"].MaxConcurrentRuns) +} + +func TestTriggerPauseStatusDisabled(t *testing.T) { + b := mockBundle(config.Development) + notEnabled := false + b.Config.Bundle.Transformers.TriggerPauseStatus.Enabled = ¬Enabled + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + // Pause status should remain unchanged (unset) + assert.Equal(t, jobs.PauseStatus(""), b.Config.Resources.Jobs["job3"].Trigger.PauseStatus) +} + +func TestPipelinesDevelopmentDisabled(t *testing.T) { + b := mockBundle(config.Development) + notEnabled := false + b.Config.Bundle.Transformers.PipelinesDevelopment.Enabled = ¬Enabled + + m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + assert.False(t, b.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) +} diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go index 025db3e29e..c637bc69e2 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/transformers.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/textutil" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -24,36 +23,28 @@ func (m *transformers) Name() string { } func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - mutators := b.Config.Bundle.Transformers r := b.Config.Resources + t := b.Config.Bundle.Transformers + prefix := t.Prefix.Value + tags := t.Tags.Tags - if isEnabled(mutators.Prefix.Enabled) { - transformPrefix(r, mutators.Prefix.Value) - } - if isEnabled(mutators.Tags.Enabled) { - transformTags(r, mutators.Tags.Tags) - } - if isEnabled(mutators.JobsMaxConcurrentRuns.Enabled) { - transformJobsMaxConcurrentRuns(r, mutators.JobsMaxConcurrentRuns.Value) - } - if isEnabled(mutators.TriggerPauseStatus.Enabled) { - transformTriggerPauseStatus(r, mutators.TriggerPauseStatus.Enabled) - } - if isEnabled(mutators.PipelinesDevelopment.Enabled) { - transformPipelinesDevelopment(r) - } - - return nil -} - -func transformPrefix(r config.Resources, prefix string) { + // Jobs transformers: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus for i := range r.Jobs { - r.Jobs[i].Name = prefix + r.Jobs[i].Name - if isEnabled(mutators.JobsMaxConcurrentRuns.Enabled) && r.Jobs[i].MaxConcurrentRuns == 0 { - r.Jobs[i].MaxConcurrentRuns = mutators.JobsMaxConcurrentRuns.Value + if t.Prefix.IsEnabled() { + r.Jobs[i].Name = prefix + r.Jobs[i].Name } - - if isEnabled(mutators.TriggerPauseStatus.Enabled) { + if t.Tags.IsEnabled() { + for tagKey, tagValue := range tags { + if r.Jobs[i].Tags == nil { + r.Jobs[i].Tags = make(map[string]string) + } + r.Jobs[i].Tags[tagKey] = tagValue + } + } + if t.JobsMaxConcurrentRuns.IsEnabled() && r.Jobs[i].MaxConcurrentRuns == 0 { + r.Jobs[i].MaxConcurrentRuns = t.JobsMaxConcurrentRuns.Value + } + if t.TriggerPauseStatus.IsEnabled() { if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus != jobs.PauseStatusUnpaused { r.Jobs[i].Schedule.PauseStatus = jobs.PauseStatusPaused } @@ -68,44 +59,66 @@ func transformPrefix(r config.Resources, prefix string) { // Pipelines transformers: Prefix, PipelinesDevelopment for i := range r.Pipelines { - r.Pipelines[i].Name = prefix + r.Pipelines[i].Name - if isEnabled(mutators.PipelinesDevelopment.Enabled) { + if t.Prefix.IsEnabled() { + r.Pipelines[i].Name = prefix + r.Pipelines[i].Name + } + if t.PipelinesDevelopment.IsEnabled() { r.Pipelines[i].Development = true } + + // As of 2024-06, pipelines don't yet support tags } - // Models transformers: Prefix + // Models transformers: Prefix, Tags for i := range r.Models { - r.Models[i].Name = prefix + r.Models[i].Name + if t.Prefix.IsEnabled() { + r.Models[i].Name = prefix + r.Models[i].Name + } + if t.Tags.IsEnabled() { + for tagKey, tagValue := range tags { + r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: tagKey, Value: tagValue}) + } + } } - // Experiments transformers: Prefix + // Experiments transformers: Prefix, Tags for i := range r.Experiments { - filepath := r.Experiments[i].Name - dir := path.Dir(filepath) - base := path.Base(filepath) - if dir == "." { - r.Experiments[i].Name = prefix + base - } else { - r.Experiments[i].Name = dir + "/" + prefix + base + if t.Prefix.IsEnabled() { + filepath := r.Experiments[i].Name + dir := path.Dir(filepath) + base := path.Base(filepath) + if dir == "." { + r.Experiments[i].Name = prefix + base + } else { + r.Experiments[i].Name = dir + "/" + prefix + base + } + } + if t.Tags.IsEnabled() { + for tagKey, tagValue := range tags { + r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: tagKey, Value: tagValue}) + } } } // Model serving endpoint transformers: Prefix for i := range r.ModelServingEndpoints { - r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name + if t.Prefix.IsEnabled() { + r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name + } + + // As of 2024-06, model serving endpoints don't yet support tags } // Registered models transformers: Prefix for i := range r.RegisteredModels { - r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name + if t.Prefix.IsEnabled() { + r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name + } + + // As of 2024-06, registered models don't yet support tags } -} -// Test whether a transformer is enabled. -// Enablement has three states: explicitly enabled, explicitly disabled, and not set. -func isEnabled(enabled *bool) bool { - return enabled != nil && *enabled + return nil } // Normalize prefix strings like '[dev lennart] ' to 'dev_lennart_'. @@ -115,30 +128,3 @@ func normalizePrefix(prefix string) string { prefix = strings.ReplaceAll(prefix, "] ", "_") return textutil.NormalizeString(prefix) } - -// Add tags for supported resources. -func transformTags(r config.Resources, tags map[string]string) { - for tagKey, tagValue := range tags { - - for i := range r.Jobs { - if r.Jobs[i].Tags == nil { - r.Jobs[i].Tags = make(map[string]string) - } - r.Jobs[i].Tags[tagKey] = tagValue - if r.Jobs[i].MaxConcurrentRuns == 0 { - r.Jobs[i].MaxConcurrentRuns = developmentConcurrentRuns - } - } - - for i := range r.Models { - r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: tagKey, Value: tagValue}) - } - - for i := range r.Experiments { - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: tagKey, Value: tagValue}) - } - - // As of 2024-04, pipelines, model serving, and registered models in Unity Catalog - // don't yet support tags. - } -} diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/transformers_test.go index b8c71c8d43..a6d46ef040 100644 --- a/bundle/config/mutator/transformers_test.go +++ b/bundle/config/mutator/transformers_test.go @@ -1 +1,214 @@ -package mutator +package mutator_test + +import ( + "context" + "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/jobs" +) + +func TestTransformersMutator_Prefix(t *testing.T) { + enabled := true + + tests := []struct { + name string + prefix string + job *resources.Job + want string + }{ + { + name: "add prefix to job", + prefix: "prefix-", + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + }, + }, + want: "prefix-job1", + }, + { + name: "add empty prefix to job", + prefix: "", + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + }, + }, + want: "job1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": tt.job, + }, + }, + Bundle: config.Bundle{ + Transformers: config.Transformers{ + Prefix: config.Prefix{ + EnableableDefaultOn: config.EnableableDefaultOn{Enabled: &enabled}, + Value: tt.prefix, + }, + }, + }, + }, + } + + mutator := mutator.TransformersMutator() + diag := mutator.Apply(context.Background(), b) + + if diag.HasError() { + t.Fatalf("unexpected error: %v", diag) + } + + if got := b.Config.Resources.Jobs["job1"].Name; got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} + +func TestTransformersMutator_Tags(t *testing.T) { + enabled := true + + tests := []struct { + name string + tags map[string]string + job *resources.Job + want map[string]string + }{ + { + name: "add tags to job", + tags: map[string]string{"env": "dev"}, + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + Tags: nil, + }, + }, + want: map[string]string{"env": "dev"}, + }, + { + name: "merge tags with existing job tags", + tags: map[string]string{"env": "dev"}, + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + Tags: map[string]string{"team": "data"}, + }, + }, + want: map[string]string{"env": "dev", "team": "data"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": tt.job, + }, + }, + Bundle: config.Bundle{ + Transformers: config.Transformers{ + Tags: config.Tags{ + EnableableDefaultOn: config.EnableableDefaultOn{Enabled: &enabled}, + Tags: tt.tags, + }, + }, + }, + }, + } + + mutator := mutator.TransformersMutator() + diag := mutator.Apply(context.Background(), b) + + if diag.HasError() { + t.Fatalf("unexpected error: %v", diag) + } + + got := b.Config.Resources.Jobs["job1"].Tags + for k, v := range tt.want { + if got[k] != v { + t.Errorf("tag %v: got %v, want %v", k, got[k], v) + } + } + }) + } +} + +func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { + enabled := true + + tests := []struct { + name string + job *resources.Job + max int + want int + }{ + { + name: "set max concurrent runs", + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + MaxConcurrentRuns: 0, + }, + }, + max: 5, + want: 5, + }, + { + name: "do not override existing max concurrent runs", + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + MaxConcurrentRuns: 3, + }, + }, + max: 5, + want: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": tt.job, + }, + }, + Bundle: config.Bundle{ + Transformers: config.Transformers{ + JobsMaxConcurrentRuns: config.JobsMaxConcurrentRuns{ + EnableableDefaultOn: config.EnableableDefaultOn{Enabled: &enabled}, + Value: tt.max, + }, + }, + }, + }, + } + + mutator := mutator.TransformersMutator() + diag := mutator.Apply(context.Background(), b) + + if diag.HasError() { + t.Fatalf("unexpected error: %v", diag) + } + + if got := b.Config.Resources.Jobs["job1"].MaxConcurrentRuns; got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} diff --git a/bundle/config/target.go b/bundle/config/target.go index d7ed723cfa..09016258ae 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -22,7 +22,7 @@ type Target struct { // Mutator configurations that e.g. change the // name prefix of deployed resources. - Mutators Transformers `json:"mutators,omitempty"` + Transformers Transformers `json:"transformers,omitempty"` // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/config/transformers.go b/bundle/config/transformers.go index 228aef21c4..da6b11e558 100644 --- a/bundle/config/transformers.go +++ b/bundle/config/transformers.go @@ -1,36 +1,62 @@ package config +// A feature with an enablement flag that is on by default. +type EnableableDefaultOn struct { + // Enabled is a user-configurable value controlling enablement + // Use IsEnabled to read this value. + Enabled *bool `json:"enabled,omitempty"` +} + +// IsEnabled tests whether this feature is enabled. +// EnableableDefaultOn features are considered enabled by default. +func (e EnableableDefaultOn) IsEnabled() bool { + return e.Enabled == nil || *e.Enabled +} + +// IsExplicitlyDisabled tests whether this feature is explicitly disabled. +func (e EnableableDefaultOff) IsExplicitlyDisabled() bool { + return e.Enabled != nil && !*e.Enabled +} + +// A feature with an enablement flag that is off by default. +type EnableableDefaultOff struct { + // Enabled is a user-configurable value controlling enablement + // Use IsEnabled to read this value. + Enabled *bool `json:"enabled,omitempty"` +} + +// IsEnabled tests whether this feature is enabled. +// EnableableDefaultOn features are considered disabled by default. +func (e EnableableDefaultOff) IsEnabled() bool { + return e.Enabled != nil && *e.Enabled +} + type Prefix struct { + EnableableDefaultOn Value string `json:"value,omitempty"` - // Whether this feature is enabled. Treated as 'true' when not set. - Enabled *bool `json:"enabled,omitempty"` } type PipelinesDevelopment struct { - // Whether this feature is enabled. Treated as 'true' when not set. - Enabled *bool `json:"enabled,omitempty"` + EnableableDefaultOff } type TriggerPauseStatus struct { - // Whether to set PauseStatus to PauseStatusPaused - Enabled *bool `json:"enabled,omitempty"` + EnableableDefaultOff } type JobsMaxConcurrentRuns struct { + EnableableDefaultOn Value int `json:"value,omitempty"` - // Whether this feature is enabled. Treated as 'true' when not set. - Enabled *bool `json:"enabled,omitempty"` } type Tags struct { + EnableableDefaultOn Tags map[string]string `json:"tags,omitempty"` - // Whether this feature is enabled. Treated as 'true' when not set. - Enabled *bool `json:"enabled,omitempty"` } type Transformers struct { Prefix Prefix `json:"prefix,omitempty"` - PipelinesDevelopment PipelinesDevelopment `json:"pipelines_set_development,omitempty"` + PipelinesDevelopment PipelinesDevelopment `json:"pipelines_development,omitempty"` TriggerPauseStatus TriggerPauseStatus `json:"trigger_pause_status,omitempty"` JobsMaxConcurrentRuns JobsMaxConcurrentRuns `json:"jobs_max_concurrent_runs,omitempty"` Tags Tags `json:"tags,omitempty"` From b16c18c131afd49c1382c34b6cd1ea4f65c4f55b Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 12 Jun 2024 09:27:53 +0200 Subject: [PATCH 04/23] Remove enabled fields --- bundle/config/mutator/process_target_mode.go | 22 ++-- .../mutator/process_target_mode_test.go | 35 +++--- bundle/config/mutator/transformers.go | 107 +++++++++++------- bundle/config/mutator/transformers_test.go | 48 ++++---- bundle/config/transformers.go | 69 +++-------- 5 files changed, 130 insertions(+), 151 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 40b6e53574..43d1715d43 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -37,39 +37,39 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno } } - m := b.Config.Bundle.Transformers + t := b.Config.Bundle.Transformers shortName := b.Config.Workspace.CurrentUser.ShortName - if m.Prefix.IsEnabled() && m.Prefix.Value == "" { - err := setConfig(b, "bundle.transformers.prefix.value", "[dev "+shortName+"] ") + if t.Prefix == "" { + err := setConfig(b, "bundle.transformers.prefix", "[dev "+shortName+"] ") if err != nil { return err } } - if m.Tags.IsEnabled() && m.Tags.Tags == nil { - err := setConfigMapping(b, "bundle.transformers.tags.tags", "dev", b.Tagging.NormalizeValue(shortName)) + if t.Tags == nil { + err := setConfigMapping(b, "bundle.transformers.tags", "dev", b.Tagging.NormalizeValue(shortName)) if err != nil { return err } } - if m.JobsMaxConcurrentRuns.IsEnabled() && m.JobsMaxConcurrentRuns.Value == 0 { - err := setConfig(b, "bundle.transformers.jobs_max_concurrent_runs.value", developmentConcurrentRuns) + if t.DefaultJobsMaxConcurrentRuns == 0 { + err := setConfig(b, "bundle.transformers.default_jobs_max_concurrent_runs", developmentConcurrentRuns) if err != nil { return err } } - if !m.TriggerPauseStatus.IsExplicitlyDisabled() { - err := setConfig(b, "bundle.transformers.trigger_pause_status.enabled", true) + if !config.IsExplicitlyDisabled(t.DefaultTriggerPauseStatus) { + err := setConfig(b, "bundle.transformers.default_trigger_pause_status", true) if err != nil { return err } } - if !m.PipelinesDevelopment.IsExplicitlyDisabled() { - err := setConfig(b, "bundle.transformers.pipelines_development.enabled", true) + if !config.IsExplicitlyDisabled(t.DefaultPipelinesDevelopment) { + err := setConfig(b, "bundle.transformers.default_pipelines_development", true) if err != nil { return err } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 1ae61695b5..b95b948652 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -337,7 +337,7 @@ func TestDisableLockingDisabled(t *testing.T) { func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.Prefix.Value = "custom_prefix_" + b.Config.Bundle.Transformers.Prefix = "custom_prefix_" m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -346,47 +346,42 @@ func TestPrefixAlreadySet(t *testing.T) { assert.Equal(t, "custom_prefix_job1", b.Config.Resources.Jobs["job1"].Name) } -func TestPrefixDisabled(t *testing.T) { +func TestTagsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - notEnabled := false - b.Config.Bundle.Transformers.Prefix.Value = "custom_prefix_" - b.Config.Bundle.Transformers.Prefix.Enabled = ¬Enabled + b.Config.Bundle.Transformers.Tags = &map[string]string{"custom": "tag"} m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) + assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["custom"]) } -func TestTagsAlreadySet(t *testing.T) { +func TestTagsNil(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.Tags.Tags = map[string]string{"custom": "tag"} + b.Config.Bundle.Transformers.Tags = nil m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["custom"]) + assert.Equal(t, "lennart", b.Config.Resources.Jobs["job2"].Tags["dev"]) } -func TestTagsDisabled(t *testing.T) { +func TestTagsEmptySet(t *testing.T) { b := mockBundle(config.Development) - notEnabled := false - b.Config.Bundle.Transformers.Tags.Tags = map[string]string{"custom": "tag"} - b.Config.Bundle.Transformers.Tags.Enabled = ¬Enabled + b.Config.Bundle.Transformers.Tags = &map[string]string{} m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["existing"]) assert.Empty(t, b.Config.Resources.Jobs["job2"].Tags) } func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.JobsMaxConcurrentRuns.Value = 10 + b.Config.Bundle.Transformers.DefaultJobsMaxConcurrentRuns = 10 m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -397,21 +392,19 @@ func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { b := mockBundle(config.Development) - notEnabled := false - b.Config.Bundle.Transformers.JobsMaxConcurrentRuns.Value = 10 - b.Config.Bundle.Transformers.JobsMaxConcurrentRuns.Enabled = ¬Enabled + b.Config.Bundle.Transformers.DefaultJobsMaxConcurrentRuns = 1 m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - assert.Zero(t, b.Config.Resources.Jobs["job1"].MaxConcurrentRuns) + assert.Equal(t, 1, b.Config.Resources.Jobs["job1"].MaxConcurrentRuns) } func TestTriggerPauseStatusDisabled(t *testing.T) { b := mockBundle(config.Development) notEnabled := false - b.Config.Bundle.Transformers.TriggerPauseStatus.Enabled = ¬Enabled + b.Config.Bundle.Transformers.DefaultTriggerPauseStatus = ¬Enabled m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -424,7 +417,7 @@ func TestTriggerPauseStatusDisabled(t *testing.T) { func TestPipelinesDevelopmentDisabled(t *testing.T) { b := mockBundle(config.Development) notEnabled := false - b.Config.Bundle.Transformers.PipelinesDevelopment.Enabled = ¬Enabled + b.Config.Bundle.Transformers.DefaultPipelinesDevelopment = ¬Enabled m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go index c637bc69e2..aca4766619 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/transformers.go @@ -3,9 +3,11 @@ package mutator import ( "context" "path" + "sort" "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/textutil" "github.com/databricks/databricks-sdk-go/service/jobs" @@ -18,6 +20,11 @@ func TransformersMutator() *transformers { return &transformers{} } +type Tag struct { + Key string + Value string +} + func (m *transformers) Name() string { return "TransformersMutator" } @@ -25,26 +32,24 @@ func (m *transformers) Name() string { func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { r := b.Config.Resources t := b.Config.Bundle.Transformers - prefix := t.Prefix.Value - tags := t.Tags.Tags + prefix := t.Prefix + tags := toTagArray(t.Tags) // Jobs transformers: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus for i := range r.Jobs { - if t.Prefix.IsEnabled() { - r.Jobs[i].Name = prefix + 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) } - if t.Tags.IsEnabled() { - for tagKey, tagValue := range tags { - if r.Jobs[i].Tags == nil { - r.Jobs[i].Tags = make(map[string]string) - } - r.Jobs[i].Tags[tagKey] = tagValue + for _, tag := range tags { + if r.Jobs[i].Tags[tag.Key] == "" { + r.Jobs[i].Tags[tag.Key] = tag.Value } } - if t.JobsMaxConcurrentRuns.IsEnabled() && r.Jobs[i].MaxConcurrentRuns == 0 { - r.Jobs[i].MaxConcurrentRuns = t.JobsMaxConcurrentRuns.Value + if r.Jobs[i].MaxConcurrentRuns == 0 { + r.Jobs[i].MaxConcurrentRuns = t.DefaultJobsMaxConcurrentRuns } - if t.TriggerPauseStatus.IsEnabled() { + if config.IsExplicitlyEnabled(t.DefaultTriggerPauseStatus) { if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus != jobs.PauseStatusUnpaused { r.Jobs[i].Schedule.PauseStatus = jobs.PauseStatusPaused } @@ -59,10 +64,8 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // Pipelines transformers: Prefix, PipelinesDevelopment for i := range r.Pipelines { - if t.Prefix.IsEnabled() { - r.Pipelines[i].Name = prefix + r.Pipelines[i].Name - } - if t.PipelinesDevelopment.IsEnabled() { + r.Pipelines[i].Name = prefix + r.Pipelines[i].Name + if config.IsExplicitlyEnabled(t.DefaultPipelinesDevelopment) { r.Pipelines[i].Development = true } @@ -71,49 +74,55 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // Models transformers: Prefix, Tags for i := range r.Models { - if t.Prefix.IsEnabled() { - r.Models[i].Name = prefix + r.Models[i].Name - } - if t.Tags.IsEnabled() { - for tagKey, tagValue := range tags { - r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: tagKey, Value: tagValue}) + r.Models[i].Name = prefix + r.Models[i].Name + for _, t := range tags { + exists := false + for _, modelTag := range r.Models[i].Tags { + if modelTag.Key == t.Key { + exists = true + break + } + } + if !exists { + r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: t.Key, Value: t.Value}) } } } // Experiments transformers: Prefix, Tags for i := range r.Experiments { - if t.Prefix.IsEnabled() { - filepath := r.Experiments[i].Name - dir := path.Dir(filepath) - base := path.Base(filepath) - if dir == "." { - r.Experiments[i].Name = prefix + base - } else { - r.Experiments[i].Name = dir + "/" + prefix + base - } + filepath := r.Experiments[i].Name + dir := path.Dir(filepath) + base := path.Base(filepath) + if dir == "." { + r.Experiments[i].Name = prefix + base + } else { + r.Experiments[i].Name = dir + "/" + prefix + base } - if t.Tags.IsEnabled() { - for tagKey, tagValue := range tags { - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: tagKey, Value: tagValue}) + for _, t := range tags { + exists := false + for _, experimentTag := range r.Experiments[i].Tags { + if experimentTag.Key == t.Key { + exists = true + break + } + } + if !exists { + r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: t.Key, Value: t.Value}) } } } // Model serving endpoint transformers: Prefix for i := range r.ModelServingEndpoints { - if t.Prefix.IsEnabled() { - r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name - } + r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name // As of 2024-06, model serving endpoints don't yet support tags } // Registered models transformers: Prefix for i := range r.RegisteredModels { - if t.Prefix.IsEnabled() { - r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name - } + r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name // As of 2024-06, registered models don't yet support tags } @@ -121,6 +130,22 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos return nil } +// Convert a map of tags to an array of tags. +// We sort tags so we always produce a consistent list of tags. +func toTagArray(tags *map[string]string) []Tag { + var tagArray []Tag + if tags == nil { + return tagArray + } + for key, value := range *tags { + tagArray = append(tagArray, Tag{Key: key, Value: value}) + } + sort.Slice(tagArray, func(i, j int) bool { + return tagArray[i].Key < tagArray[j].Key + }) + return tagArray +} + // Normalize prefix strings like '[dev lennart] ' to 'dev_lennart_'. // We leave unicode letters and numbers but remove all "special characters." func normalizePrefix(prefix string) string { diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/transformers_test.go index a6d46ef040..2b2a6e9d9d 100644 --- a/bundle/config/mutator/transformers_test.go +++ b/bundle/config/mutator/transformers_test.go @@ -12,8 +12,6 @@ import ( ) func TestTransformersMutator_Prefix(t *testing.T) { - enabled := true - tests := []struct { name string prefix string @@ -53,10 +51,7 @@ func TestTransformersMutator_Prefix(t *testing.T) { }, Bundle: config.Bundle{ Transformers: config.Transformers{ - Prefix: config.Prefix{ - EnableableDefaultOn: config.EnableableDefaultOn{Enabled: &enabled}, - Value: tt.prefix, - }, + Prefix: tt.prefix, }, }, }, @@ -77,8 +72,6 @@ func TestTransformersMutator_Prefix(t *testing.T) { } func TestTransformersMutator_Tags(t *testing.T) { - enabled := true - tests := []struct { name string tags map[string]string @@ -107,6 +100,17 @@ func TestTransformersMutator_Tags(t *testing.T) { }, want: map[string]string{"env": "dev", "team": "data"}, }, + { + name: "don't override existing job tags", + tags: map[string]string{"env": "dev"}, + job: &resources.Job{ + JobSettings: &jobs.JobSettings{ + Name: "job1", + Tags: map[string]string{"env": "prod"}, + }, + }, + want: map[string]string{"env": "prod"}, + }, } for _, tt := range tests { @@ -120,10 +124,7 @@ func TestTransformersMutator_Tags(t *testing.T) { }, Bundle: config.Bundle{ Transformers: config.Transformers{ - Tags: config.Tags{ - EnableableDefaultOn: config.EnableableDefaultOn{Enabled: &enabled}, - Tags: tt.tags, - }, + Tags: &tt.tags, }, }, }, @@ -147,13 +148,11 @@ func TestTransformersMutator_Tags(t *testing.T) { } func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { - enabled := true - tests := []struct { - name string - job *resources.Job - max int - want int + name string + job *resources.Job + setting int + want int }{ { name: "set max concurrent runs", @@ -163,8 +162,8 @@ func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { MaxConcurrentRuns: 0, }, }, - max: 5, - want: 5, + setting: 5, + want: 5, }, { name: "do not override existing max concurrent runs", @@ -174,8 +173,8 @@ func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { MaxConcurrentRuns: 3, }, }, - max: 5, - want: 3, + setting: 5, + want: 3, }, } @@ -190,10 +189,7 @@ func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { }, Bundle: config.Bundle{ Transformers: config.Transformers{ - JobsMaxConcurrentRuns: config.JobsMaxConcurrentRuns{ - EnableableDefaultOn: config.EnableableDefaultOn{Enabled: &enabled}, - Value: tt.max, - }, + DefaultJobsMaxConcurrentRuns: tt.setting, }, }, }, diff --git a/bundle/config/transformers.go b/bundle/config/transformers.go index da6b11e558..08eaf06abd 100644 --- a/bundle/config/transformers.go +++ b/bundle/config/transformers.go @@ -1,63 +1,28 @@ package config -// A feature with an enablement flag that is on by default. -type EnableableDefaultOn struct { - // Enabled is a user-configurable value controlling enablement - // Use IsEnabled to read this value. - Enabled *bool `json:"enabled,omitempty"` -} - -// IsEnabled tests whether this feature is enabled. -// EnableableDefaultOn features are considered enabled by default. -func (e EnableableDefaultOn) IsEnabled() bool { - return e.Enabled == nil || *e.Enabled -} - -// IsExplicitlyDisabled tests whether this feature is explicitly disabled. -func (e EnableableDefaultOff) IsExplicitlyDisabled() bool { - return e.Enabled != nil && !*e.Enabled -} - -// A feature with an enablement flag that is off by default. -type EnableableDefaultOff struct { - // Enabled is a user-configurable value controlling enablement - // Use IsEnabled to read this value. - Enabled *bool `json:"enabled,omitempty"` -} - -// IsEnabled tests whether this feature is enabled. -// EnableableDefaultOn features are considered disabled by default. -func (e EnableableDefaultOff) IsEnabled() bool { - return e.Enabled != nil && *e.Enabled -} +type Transformers struct { + // Prefix to prepend to all resource names. + Prefix string `json:"prefix,omitempty"` -type Prefix struct { - EnableableDefaultOn - Value string `json:"value,omitempty"` -} + // DefaultPipelinesDevelopment is the default value for the development field of pipelines. + DefaultPipelinesDevelopment *bool `json:"default_pipelines_development,omitempty"` -type PipelinesDevelopment struct { - EnableableDefaultOff -} + // DefaultTriggerPauseStatus is the default value for the pause status of all triggers and schedules. + DefaultTriggerPauseStatus *bool `json:"default_trigger_pause_status,omitempty"` -type TriggerPauseStatus struct { - EnableableDefaultOff -} + // DefaultJobsMaxConcurrentRuns is the default value for the max concurrent runs of jobs. + DefaultJobsMaxConcurrentRuns int `json:"default_jobs_max_concurrent_runs,omitempty"` -type JobsMaxConcurrentRuns struct { - EnableableDefaultOn - Value int `json:"value,omitempty"` + // Tags to add to all resources. + Tags *map[string]string `json:"tags,omitempty"` } -type Tags struct { - EnableableDefaultOn - Tags map[string]string `json:"tags,omitempty"` +// IsExplicitlyEnabled tests whether this feature is explicitly enabled. +func IsExplicitlyEnabled(feature *bool) bool { + return feature != nil && *feature } -type Transformers struct { - Prefix Prefix `json:"prefix,omitempty"` - PipelinesDevelopment PipelinesDevelopment `json:"pipelines_development,omitempty"` - TriggerPauseStatus TriggerPauseStatus `json:"trigger_pause_status,omitempty"` - JobsMaxConcurrentRuns JobsMaxConcurrentRuns `json:"jobs_max_concurrent_runs,omitempty"` - Tags Tags `json:"tags,omitempty"` +// IsExplicitlyDisabled tests whether this feature is explicitly disabled. +func IsExplicitlyDisabled(feature *bool) bool { + return feature != nil && !*feature } From 901097ac4abf983beb81935cef0538ddddd8783f Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 12 Jun 2024 10:24:18 +0200 Subject: [PATCH 05/23] Add config merging & test --- bundle/config/bundle.go | 4 --- bundle/config/mutator/process_target_mode.go | 12 ++++----- .../mutator/process_target_mode_test.go | 16 +++++------ bundle/config/mutator/transformers.go | 2 +- bundle/config/mutator/transformers_test.go | 18 +++++-------- bundle/config/root.go | 13 +++++++++ bundle/tests/transformers/databricks.yml | 20 ++++++++++++++ bundle/tests/transformers_test.go | 27 +++++++++++++++++++ 8 files changed, 81 insertions(+), 31 deletions(-) create mode 100644 bundle/tests/transformers/databricks.yml create mode 100644 bundle/tests/transformers_test.go diff --git a/bundle/config/bundle.go b/bundle/config/bundle.go index 263fb25d6f..78648dfd7f 100644 --- a/bundle/config/bundle.go +++ b/bundle/config/bundle.go @@ -38,10 +38,6 @@ type Bundle struct { // Annotated readonly as this should be set at the target level. Mode Mode `json:"mode,omitempty" bundle:"readonly"` - // Transformers apply some transformation throughout the bundle, e.g. - // adding a name prefix to deployed resources. - Transformers Transformers `json:"transformers,omitempty"` - // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 43d1715d43..46cdad399f 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -37,39 +37,39 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno } } - t := b.Config.Bundle.Transformers + t := b.Config.Transformers shortName := b.Config.Workspace.CurrentUser.ShortName if t.Prefix == "" { - err := setConfig(b, "bundle.transformers.prefix", "[dev "+shortName+"] ") + err := setConfig(b, "transformers.prefix", "[dev "+shortName+"] ") if err != nil { return err } } if t.Tags == nil { - err := setConfigMapping(b, "bundle.transformers.tags", "dev", b.Tagging.NormalizeValue(shortName)) + err := setConfigMapping(b, "transformers.tags", "dev", b.Tagging.NormalizeValue(shortName)) if err != nil { return err } } if t.DefaultJobsMaxConcurrentRuns == 0 { - err := setConfig(b, "bundle.transformers.default_jobs_max_concurrent_runs", developmentConcurrentRuns) + err := setConfig(b, "transformers.default_jobs_max_concurrent_runs", developmentConcurrentRuns) if err != nil { return err } } if !config.IsExplicitlyDisabled(t.DefaultTriggerPauseStatus) { - err := setConfig(b, "bundle.transformers.default_trigger_pause_status", true) + err := setConfig(b, "transformers.default_trigger_pause_status", true) if err != nil { return err } } if !config.IsExplicitlyDisabled(t.DefaultPipelinesDevelopment) { - err := setConfig(b, "bundle.transformers.default_pipelines_development", true) + err := setConfig(b, "transformers.default_pipelines_development", true) if err != nil { return err } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index b95b948652..bafd8584ab 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -337,7 +337,7 @@ func TestDisableLockingDisabled(t *testing.T) { func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.Prefix = "custom_prefix_" + b.Config.Transformers.Prefix = "custom_prefix_" m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -348,7 +348,7 @@ func TestPrefixAlreadySet(t *testing.T) { func TestTagsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.Tags = &map[string]string{"custom": "tag"} + b.Config.Transformers.Tags = &map[string]string{"custom": "tag"} m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -359,7 +359,7 @@ func TestTagsAlreadySet(t *testing.T) { func TestTagsNil(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.Tags = nil + b.Config.Transformers.Tags = nil m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -370,7 +370,7 @@ func TestTagsNil(t *testing.T) { func TestTagsEmptySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.Tags = &map[string]string{} + b.Config.Transformers.Tags = &map[string]string{} m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -381,7 +381,7 @@ func TestTagsEmptySet(t *testing.T) { func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.DefaultJobsMaxConcurrentRuns = 10 + b.Config.Transformers.DefaultJobsMaxConcurrentRuns = 10 m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -392,7 +392,7 @@ func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { b := mockBundle(config.Development) - b.Config.Bundle.Transformers.DefaultJobsMaxConcurrentRuns = 1 + b.Config.Transformers.DefaultJobsMaxConcurrentRuns = 1 m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -404,7 +404,7 @@ func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { func TestTriggerPauseStatusDisabled(t *testing.T) { b := mockBundle(config.Development) notEnabled := false - b.Config.Bundle.Transformers.DefaultTriggerPauseStatus = ¬Enabled + b.Config.Transformers.DefaultTriggerPauseStatus = ¬Enabled m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) @@ -417,7 +417,7 @@ func TestTriggerPauseStatusDisabled(t *testing.T) { func TestPipelinesDevelopmentDisabled(t *testing.T) { b := mockBundle(config.Development) notEnabled := false - b.Config.Bundle.Transformers.DefaultPipelinesDevelopment = ¬Enabled + b.Config.Transformers.DefaultPipelinesDevelopment = ¬Enabled m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) diags := bundle.Apply(context.Background(), b, m) diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go index aca4766619..afc5f81e7d 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/transformers.go @@ -31,7 +31,7 @@ func (m *transformers) Name() string { func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { r := b.Config.Resources - t := b.Config.Bundle.Transformers + t := b.Config.Transformers prefix := t.Prefix tags := toTagArray(t.Tags) diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/transformers_test.go index 2b2a6e9d9d..996ad64273 100644 --- a/bundle/config/mutator/transformers_test.go +++ b/bundle/config/mutator/transformers_test.go @@ -49,10 +49,8 @@ func TestTransformersMutator_Prefix(t *testing.T) { "job1": tt.job, }, }, - Bundle: config.Bundle{ - Transformers: config.Transformers{ - Prefix: tt.prefix, - }, + Transformers: config.Transformers{ + Prefix: tt.prefix, }, }, } @@ -122,10 +120,8 @@ func TestTransformersMutator_Tags(t *testing.T) { "job1": tt.job, }, }, - Bundle: config.Bundle{ - Transformers: config.Transformers{ - Tags: &tt.tags, - }, + Transformers: config.Transformers{ + Tags: &tt.tags, }, }, } @@ -187,10 +183,8 @@ func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { "job1": tt.job, }, }, - Bundle: config.Bundle{ - Transformers: config.Transformers{ - DefaultJobsMaxConcurrentRuns: tt.setting, - }, + Transformers: config.Transformers{ + DefaultJobsMaxConcurrentRuns: tt.setting, }, }, } diff --git a/bundle/config/root.go b/bundle/config/root.go index 88197c2b87..1de158940b 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -60,6 +60,10 @@ type Root struct { // RunAs section allows to define an execution identity for jobs and pipelines runs RunAs *jobs.JobRunAs `json:"run_as,omitempty"` + // Transformers apply some transformation throughout the bundle, e.g. + // adding a name prefix to deployed resources. + Transformers Transformers `json:"transformers,omitempty"` + Experimental *Experimental `json:"experimental,omitempty"` // Permissions section allows to define permissions which will be @@ -330,6 +334,7 @@ func (r *Root) MergeTargetOverrides(name string) error { "sync", "permissions", "variables", + "transformers", } { if root, err = mergeField(root, target, f); err != nil { return err @@ -344,6 +349,14 @@ func (r *Root) MergeTargetOverrides(name string) error { } } + // Merge `transformers.tags`. This field must be overwritten if set, not merged. + if v, _ := dyn.Get(target, "transformers.tags"); v != dyn.NilValue { + root, err = dyn.Set(root, "transformers.tags", v) + if err != nil { + return err + } + } + // Below, we're setting fields on the bundle key, so make sure it exists. if root.Get("bundle") == dyn.NilValue { root, err = dyn.Set(root, "bundle", dyn.NewValue(map[string]dyn.Value{}, dyn.Location{})) diff --git a/bundle/tests/transformers/databricks.yml b/bundle/tests/transformers/databricks.yml new file mode 100644 index 0000000000..17decc2d8b --- /dev/null +++ b/bundle/tests/transformers/databricks.yml @@ -0,0 +1,20 @@ +bundle: + name: transformers + +transformers: + tags: + prod: true + default_pipelines_development: true + +targets: + dev: + transformers: + prefix: "myprefix" + default_pipelines_development: true + default_trigger_pause_status: true + default_jobs_max_concurrent_runs: 10 + tags: + dev: true + prod: + transformers: + default_pipelines_development: false diff --git a/bundle/tests/transformers_test.go b/bundle/tests/transformers_test.go new file mode 100644 index 0000000000..c275a3349a --- /dev/null +++ b/bundle/tests/transformers_test.go @@ -0,0 +1,27 @@ +package config_tests + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestTransformersDev(t *testing.T) { + b := loadTarget(t, "./transformers", "dev") + + assert.Equal(t, "myprefix", b.Config.Transformers.Prefix) + assert.Equal(t, true, *b.Config.Transformers.DefaultTriggerPauseStatus) + assert.Equal(t, 10, b.Config.Transformers.DefaultJobsMaxConcurrentRuns) + assert.Equal(t, true, *b.Config.Transformers.DefaultPipelinesDevelopment) + assert.Equal(t, "true", (*b.Config.Transformers.Tags)["dev"]) + + // Tags transformer is overridden by the dev target, so prod is not set + assert.Equal(t, "", (*b.Config.Transformers.Tags)["prod"]) +} + +func TestTransformersProd(t *testing.T) { + b := loadTarget(t, "./transformers", "prod") + + assert.Equal(t, false, *b.Config.Transformers.DefaultPipelinesDevelopment) + assert.Equal(t, "true", (*b.Config.Transformers.Tags)["prod"]) +} From a815f301052d9d7f391c7ca139096f1cff2d023b Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 12 Jun 2024 10:40:29 +0200 Subject: [PATCH 06/23] e2e fixes --- .../mutator/process_target_mode_test.go | 28 +++++++++---------- bundle/config/mutator/transformers.go | 6 ++-- bundle/config/mutator/transformers_test.go | 6 ++-- bundle/config/root.go | 2 +- bundle/phases/initialize.go | 1 + 5 files changed, 23 insertions(+), 20 deletions(-) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index bafd8584ab..7841bb4bf2 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -113,7 +113,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { func TestProcessTargetModeDevelopment(t *testing.T) { b := mockBundle(config.Development) - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -162,7 +162,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAws(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -177,7 +177,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAzure(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -192,7 +192,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -203,7 +203,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { func TestProcessTargetModeDefault(t *testing.T) { b := mockBundle("") - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) @@ -289,7 +289,7 @@ func TestAllResourcesMocked(t *testing.T) { func TestAllResourcesRenamed(t *testing.T) { b := mockBundle(config.Development) - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -339,7 +339,7 @@ func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) b.Config.Transformers.Prefix = "custom_prefix_" - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -350,7 +350,7 @@ func TestTagsAlreadySet(t *testing.T) { b := mockBundle(config.Development) b.Config.Transformers.Tags = &map[string]string{"custom": "tag"} - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -361,7 +361,7 @@ func TestTagsNil(t *testing.T) { b := mockBundle(config.Development) b.Config.Transformers.Tags = nil - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -372,7 +372,7 @@ func TestTagsEmptySet(t *testing.T) { b := mockBundle(config.Development) b.Config.Transformers.Tags = &map[string]string{} - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -383,7 +383,7 @@ func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { b := mockBundle(config.Development) b.Config.Transformers.DefaultJobsMaxConcurrentRuns = 10 - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -394,7 +394,7 @@ func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { b := mockBundle(config.Development) b.Config.Transformers.DefaultJobsMaxConcurrentRuns = 1 - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -406,7 +406,7 @@ func TestTriggerPauseStatusDisabled(t *testing.T) { notEnabled := false b.Config.Transformers.DefaultTriggerPauseStatus = ¬Enabled - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -419,7 +419,7 @@ func TestPipelinesDevelopmentDisabled(t *testing.T) { notEnabled := false b.Config.Transformers.DefaultPipelinesDevelopment = ¬Enabled - m := bundle.Seq(ProcessTargetMode(), TransformersMutator()) + m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go index afc5f81e7d..5b30253ce2 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/transformers.go @@ -16,7 +16,9 @@ import ( type transformers struct{} -func TransformersMutator() *transformers { +// Apply all transformers, e.g. the prefix transformer that +// adds a prefix to all names of all resources. +func Transformers() *transformers { return &transformers{} } @@ -26,7 +28,7 @@ type Tag struct { } func (m *transformers) Name() string { - return "TransformersMutator" + return "Transformers" } func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/transformers_test.go index 996ad64273..d2954b850f 100644 --- a/bundle/config/mutator/transformers_test.go +++ b/bundle/config/mutator/transformers_test.go @@ -55,7 +55,7 @@ func TestTransformersMutator_Prefix(t *testing.T) { }, } - mutator := mutator.TransformersMutator() + mutator := mutator.Transformers() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -126,7 +126,7 @@ func TestTransformersMutator_Tags(t *testing.T) { }, } - mutator := mutator.TransformersMutator() + mutator := mutator.Transformers() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -189,7 +189,7 @@ func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { }, } - mutator := mutator.TransformersMutator() + mutator := mutator.Transformers() diag := mutator.Apply(context.Background(), b) if diag.HasError() { diff --git a/bundle/config/root.go b/bundle/config/root.go index 1de158940b..6cfcd88018 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -350,7 +350,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Merge `transformers.tags`. This field must be overwritten if set, not merged. - if v, _ := dyn.Get(target, "transformers.tags"); v != dyn.NilValue { + if v, _ := dyn.Get(target, "transformers.tags"); v != dyn.InvalidValue { root, err = dyn.Set(root, "transformers.tags", v) if err != nil { return err diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index ded2e19808..f9715ddd2b 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -38,6 +38,7 @@ func Initialize() bundle.Mutator { mutator.SetRunAs(), mutator.OverrideCompute(), mutator.ProcessTargetMode(), + mutator.Transformers(), mutator.DefaultQueueing(), mutator.ExpandPipelineGlobPaths(), mutator.TranslatePaths(), From 13630dd79c1a58857ed1200e36d61f58dd11f15f Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 12 Jun 2024 11:00:58 +0200 Subject: [PATCH 07/23] Cleanup --- cmd/bundle/deploy.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index cb9e15a9f9..919b15a723 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -2,13 +2,11 @@ package bundle import ( "context" - "fmt" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" "github.com/databricks/cli/cmd/root" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" "github.com/spf13/cobra" ) @@ -58,9 +56,6 @@ func newDeployCommand() *cobra.Command { if err := diags.Error(); err != nil { return err } - for _, diag := range diags { - cmdio.LogString(ctx, fmt.Sprintf("Warning: %s", diag.Summary)) - } return nil } From e3b0435a14f93c34de94546f1ce155970d55c912 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 12 Jun 2024 16:49:42 +0200 Subject: [PATCH 08/23] Use PAUSED/UNPAUSED instead of a boolean --- bundle/config/mutator/process_target_mode.go | 4 +-- .../mutator/process_target_mode_test.go | 9 +++-- bundle/config/mutator/transformers.go | 36 +++++++++++++++---- bundle/config/mutator/transformers_test.go | 21 +++++++++-- bundle/config/transformers.go | 6 +++- bundle/tests/transformers/databricks.yml | 2 +- bundle/tests/transformers_test.go | 3 +- 7 files changed, 61 insertions(+), 20 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 46cdad399f..89440d4390 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -61,8 +61,8 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno } } - if !config.IsExplicitlyDisabled(t.DefaultTriggerPauseStatus) { - err := setConfig(b, "transformers.default_trigger_pause_status", true) + if t.DefaultTriggerPauseStatus == "" { + err := setConfig(b, "transformers.default_trigger_pause_status", config.Paused) if err != nil { return err } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 7841bb4bf2..73ae07feae 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -401,17 +401,16 @@ func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { assert.Equal(t, 1, b.Config.Resources.Jobs["job1"].MaxConcurrentRuns) } -func TestTriggerPauseStatusDisabled(t *testing.T) { +func TestTriggerPauseStatusWhenUnpaused(t *testing.T) { b := mockBundle(config.Development) - notEnabled := false - b.Config.Transformers.DefaultTriggerPauseStatus = ¬Enabled + b.Config.Transformers.DefaultTriggerPauseStatus = config.Unpaused m := bundle.Seq(ProcessTargetMode(), Transformers()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - // Pause status should remain unchanged (unset) - assert.Equal(t, jobs.PauseStatus(""), b.Config.Resources.Jobs["job3"].Trigger.PauseStatus) + // Pause status should take the value from the override above + assert.Equal(t, jobs.PauseStatusUnpaused, b.Config.Resources.Jobs["job3"].Trigger.PauseStatus) } func TestPipelinesDevelopmentDisabled(t *testing.T) { diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/transformers.go index 5b30253ce2..0eb7dcf139 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/transformers.go @@ -31,7 +31,24 @@ func (m *transformers) Name() string { return "Transformers" } +func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { + p := b.Config.Transformers.DefaultTriggerPauseStatus + if p == "" || p == config.Paused || p == config.Unpaused { + return nil + } + return diag.Diagnostics{{ + Summary: "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", + Severity: diag.Error, + Location: b.Config.GetLocation("transformers.default_trigger_pause_status"), + }} +} + func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + diag := validatePauseStatus(b) + if diag != nil { + return diag + } + r := b.Config.Resources t := b.Config.Transformers prefix := t.Prefix @@ -51,15 +68,20 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos if r.Jobs[i].MaxConcurrentRuns == 0 { r.Jobs[i].MaxConcurrentRuns = t.DefaultJobsMaxConcurrentRuns } - if config.IsExplicitlyEnabled(t.DefaultTriggerPauseStatus) { - if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus != jobs.PauseStatusUnpaused { - r.Jobs[i].Schedule.PauseStatus = jobs.PauseStatusPaused + if t.DefaultTriggerPauseStatus != "" { + paused := jobs.PauseStatusPaused + if t.DefaultTriggerPauseStatus == config.Unpaused { + paused = jobs.PauseStatusUnpaused + } + + if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus == "" { + r.Jobs[i].Schedule.PauseStatus = paused } - if r.Jobs[i].Continuous != nil && r.Jobs[i].Continuous.PauseStatus != jobs.PauseStatusUnpaused { - r.Jobs[i].Continuous.PauseStatus = jobs.PauseStatusPaused + if r.Jobs[i].Continuous != nil && r.Jobs[i].Continuous.PauseStatus == "" { + r.Jobs[i].Continuous.PauseStatus = paused } - if r.Jobs[i].Trigger != nil && r.Jobs[i].Trigger.PauseStatus != jobs.PauseStatusUnpaused { - r.Jobs[i].Trigger.PauseStatus = jobs.PauseStatusPaused + if r.Jobs[i].Trigger != nil && r.Jobs[i].Trigger.PauseStatus == "" { + r.Jobs[i].Trigger.PauseStatus = paused } } } diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/transformers_test.go index d2954b850f..3b6cfbe382 100644 --- a/bundle/config/mutator/transformers_test.go +++ b/bundle/config/mutator/transformers_test.go @@ -9,9 +9,10 @@ import ( "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/assert" ) -func TestTransformersMutator_Prefix(t *testing.T) { +func TestTransformersPrefix(t *testing.T) { tests := []struct { name string prefix string @@ -69,7 +70,7 @@ func TestTransformersMutator_Prefix(t *testing.T) { } } -func TestTransformersMutator_Tags(t *testing.T) { +func TestTransformersTags(t *testing.T) { tests := []struct { name string tags map[string]string @@ -143,7 +144,7 @@ func TestTransformersMutator_Tags(t *testing.T) { } } -func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { +func TestTransformersJobsMaxConcurrentRuns(t *testing.T) { tests := []struct { name string job *resources.Job @@ -202,3 +203,17 @@ func TestTransformersMutator_JobsMaxConcurrentRuns(t *testing.T) { }) } } + +func TestTransformersValidation(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Transformers: config.Transformers{ + DefaultTriggerPauseStatus: "invalid", + }, + }, + } + + mutator := mutator.Transformers() + diag := mutator.Apply(context.Background(), b) + assert.Equal(t, "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) +} diff --git a/bundle/config/transformers.go b/bundle/config/transformers.go index 08eaf06abd..f277319141 100644 --- a/bundle/config/transformers.go +++ b/bundle/config/transformers.go @@ -1,5 +1,8 @@ package config +const Paused = "PAUSED" +const Unpaused = "UNPAUSED" + type Transformers struct { // Prefix to prepend to all resource names. Prefix string `json:"prefix,omitempty"` @@ -8,7 +11,8 @@ type Transformers struct { DefaultPipelinesDevelopment *bool `json:"default_pipelines_development,omitempty"` // DefaultTriggerPauseStatus is the default value for the pause status of all triggers and schedules. - DefaultTriggerPauseStatus *bool `json:"default_trigger_pause_status,omitempty"` + // Either config.Paused, config.Unpaused, or empty. + DefaultTriggerPauseStatus string `json:"default_trigger_pause_status,omitempty"` // DefaultJobsMaxConcurrentRuns is the default value for the max concurrent runs of jobs. DefaultJobsMaxConcurrentRuns int `json:"default_jobs_max_concurrent_runs,omitempty"` diff --git a/bundle/tests/transformers/databricks.yml b/bundle/tests/transformers/databricks.yml index 17decc2d8b..20da05b81a 100644 --- a/bundle/tests/transformers/databricks.yml +++ b/bundle/tests/transformers/databricks.yml @@ -11,7 +11,7 @@ targets: transformers: prefix: "myprefix" default_pipelines_development: true - default_trigger_pause_status: true + default_trigger_pause_status: PAUSED default_jobs_max_concurrent_runs: 10 tags: dev: true diff --git a/bundle/tests/transformers_test.go b/bundle/tests/transformers_test.go index c275a3349a..7a19002b31 100644 --- a/bundle/tests/transformers_test.go +++ b/bundle/tests/transformers_test.go @@ -3,6 +3,7 @@ package config_tests import ( "testing" + "github.com/databricks/cli/bundle/config" "github.com/stretchr/testify/assert" ) @@ -10,7 +11,7 @@ func TestTransformersDev(t *testing.T) { b := loadTarget(t, "./transformers", "dev") assert.Equal(t, "myprefix", b.Config.Transformers.Prefix) - assert.Equal(t, true, *b.Config.Transformers.DefaultTriggerPauseStatus) + assert.Equal(t, config.Paused, b.Config.Transformers.DefaultTriggerPauseStatus) assert.Equal(t, 10, b.Config.Transformers.DefaultJobsMaxConcurrentRuns) assert.Equal(t, true, *b.Config.Transformers.DefaultPipelinesDevelopment) assert.Equal(t, "true", (*b.Config.Transformers.Tags)["dev"]) From 10a1ffc0b1f6de7c79241c02b79722865cf081ba Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 12 Jun 2024 17:18:04 +0200 Subject: [PATCH 09/23] Rename to transform for now --- .../{transformers.go => apply_transforms.go} | 32 +++++++------- ...rmers_test.go => apply_transforms_test.go} | 24 +++++----- bundle/config/mutator/process_target_mode.go | 12 ++--- .../mutator/process_target_mode_test.go | 44 +++++++++---------- bundle/config/root.go | 12 ++--- bundle/config/target.go | 2 +- .../config/{transformers.go => transforms.go} | 2 +- bundle/phases/initialize.go | 2 +- .../databricks.yml | 8 ++-- bundle/tests/transform_test.go | 28 ++++++++++++ bundle/tests/transformers_test.go | 28 ------------ 11 files changed, 97 insertions(+), 97 deletions(-) rename bundle/config/mutator/{transformers.go => apply_transforms.go} (84%) rename bundle/config/mutator/{transformers_test.go => apply_transforms_test.go} (89%) rename bundle/config/{transformers.go => transforms.go} (97%) rename bundle/tests/{transformers => transform}/databricks.yml (81%) create mode 100644 bundle/tests/transform_test.go delete mode 100644 bundle/tests/transformers_test.go diff --git a/bundle/config/mutator/transformers.go b/bundle/config/mutator/apply_transforms.go similarity index 84% rename from bundle/config/mutator/transformers.go rename to bundle/config/mutator/apply_transforms.go index 0eb7dcf139..e5f120014d 100644 --- a/bundle/config/mutator/transformers.go +++ b/bundle/config/mutator/apply_transforms.go @@ -14,12 +14,12 @@ import ( "github.com/databricks/databricks-sdk-go/service/ml" ) -type transformers struct{} +type applyTransforms struct{} -// Apply all transformers, e.g. the prefix transformer that +// Apply all transforms, e.g. the prefix transform that // adds a prefix to all names of all resources. -func Transformers() *transformers { - return &transformers{} +func ApplyTransforms() *applyTransforms { + return &applyTransforms{} } type Tag struct { @@ -27,34 +27,34 @@ type Tag struct { Value string } -func (m *transformers) Name() string { - return "Transformers" +func (m *applyTransforms) Name() string { + return "ApplyTransforms" } func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { - p := b.Config.Transformers.DefaultTriggerPauseStatus + p := b.Config.Transform.DefaultTriggerPauseStatus if p == "" || p == config.Paused || p == config.Unpaused { return nil } return diag.Diagnostics{{ Summary: "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", Severity: diag.Error, - Location: b.Config.GetLocation("transformers.default_trigger_pause_status"), + Location: b.Config.GetLocation("transform.default_trigger_pause_status"), }} } -func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diag := validatePauseStatus(b) if diag != nil { return diag } r := b.Config.Resources - t := b.Config.Transformers + t := b.Config.Transform prefix := t.Prefix tags := toTagArray(t.Tags) - // Jobs transformers: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus + // Jobs transforms: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus for i := range r.Jobs { r.Jobs[i].Name = prefix + r.Jobs[i].Name if r.Jobs[i].Tags == nil { @@ -86,7 +86,7 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } } - // Pipelines transformers: Prefix, PipelinesDevelopment + // Pipelines transforms: Prefix, PipelinesDevelopment for i := range r.Pipelines { r.Pipelines[i].Name = prefix + r.Pipelines[i].Name if config.IsExplicitlyEnabled(t.DefaultPipelinesDevelopment) { @@ -96,7 +96,7 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos // As of 2024-06, pipelines don't yet support tags } - // Models transformers: Prefix, Tags + // Models transforms: Prefix, Tags for i := range r.Models { r.Models[i].Name = prefix + r.Models[i].Name for _, t := range tags { @@ -113,7 +113,7 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } } - // Experiments transformers: Prefix, Tags + // Experiments transforms: Prefix, Tags for i := range r.Experiments { filepath := r.Experiments[i].Name dir := path.Dir(filepath) @@ -137,14 +137,14 @@ func (m *transformers) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos } } - // Model serving endpoint transformers: Prefix + // Model serving endpoint transforms: Prefix for i := range r.ModelServingEndpoints { r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name // As of 2024-06, model serving endpoints don't yet support tags } - // Registered models transformers: Prefix + // Registered models transforms: Prefix for i := range r.RegisteredModels { r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name diff --git a/bundle/config/mutator/transformers_test.go b/bundle/config/mutator/apply_transforms_test.go similarity index 89% rename from bundle/config/mutator/transformers_test.go rename to bundle/config/mutator/apply_transforms_test.go index 3b6cfbe382..fa27744dbe 100644 --- a/bundle/config/mutator/transformers_test.go +++ b/bundle/config/mutator/apply_transforms_test.go @@ -12,7 +12,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestTransformersPrefix(t *testing.T) { +func TestTransformPrefix(t *testing.T) { tests := []struct { name string prefix string @@ -50,13 +50,13 @@ func TestTransformersPrefix(t *testing.T) { "job1": tt.job, }, }, - Transformers: config.Transformers{ + Transform: config.Transforms{ Prefix: tt.prefix, }, }, } - mutator := mutator.Transformers() + mutator := mutator.ApplyTransforms() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -70,7 +70,7 @@ func TestTransformersPrefix(t *testing.T) { } } -func TestTransformersTags(t *testing.T) { +func TestTransformTags(t *testing.T) { tests := []struct { name string tags map[string]string @@ -121,13 +121,13 @@ func TestTransformersTags(t *testing.T) { "job1": tt.job, }, }, - Transformers: config.Transformers{ + Transform: config.Transforms{ Tags: &tt.tags, }, }, } - mutator := mutator.Transformers() + mutator := mutator.ApplyTransforms() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -144,7 +144,7 @@ func TestTransformersTags(t *testing.T) { } } -func TestTransformersJobsMaxConcurrentRuns(t *testing.T) { +func TestTransformJobsMaxConcurrentRuns(t *testing.T) { tests := []struct { name string job *resources.Job @@ -184,13 +184,13 @@ func TestTransformersJobsMaxConcurrentRuns(t *testing.T) { "job1": tt.job, }, }, - Transformers: config.Transformers{ + Transform: config.Transforms{ DefaultJobsMaxConcurrentRuns: tt.setting, }, }, } - mutator := mutator.Transformers() + mutator := mutator.ApplyTransforms() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -204,16 +204,16 @@ func TestTransformersJobsMaxConcurrentRuns(t *testing.T) { } } -func TestTransformersValidation(t *testing.T) { +func TestTransformValidation(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ - Transformers: config.Transformers{ + Transform: config.Transforms{ DefaultTriggerPauseStatus: "invalid", }, }, } - mutator := mutator.Transformers() + mutator := mutator.ApplyTransforms() diag := mutator.Apply(context.Background(), b) assert.Equal(t, "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) } diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 89440d4390..a2385e1e05 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -37,39 +37,39 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno } } - t := b.Config.Transformers + t := b.Config.Transform shortName := b.Config.Workspace.CurrentUser.ShortName if t.Prefix == "" { - err := setConfig(b, "transformers.prefix", "[dev "+shortName+"] ") + err := setConfig(b, "transform.prefix", "[dev "+shortName+"] ") if err != nil { return err } } if t.Tags == nil { - err := setConfigMapping(b, "transformers.tags", "dev", b.Tagging.NormalizeValue(shortName)) + err := setConfigMapping(b, "transform.tags", "dev", b.Tagging.NormalizeValue(shortName)) if err != nil { return err } } if t.DefaultJobsMaxConcurrentRuns == 0 { - err := setConfig(b, "transformers.default_jobs_max_concurrent_runs", developmentConcurrentRuns) + err := setConfig(b, "transform.default_jobs_max_concurrent_runs", developmentConcurrentRuns) if err != nil { return err } } if t.DefaultTriggerPauseStatus == "" { - err := setConfig(b, "transformers.default_trigger_pause_status", config.Paused) + err := setConfig(b, "transform.default_trigger_pause_status", config.Paused) if err != nil { return err } } if !config.IsExplicitlyDisabled(t.DefaultPipelinesDevelopment) { - err := setConfig(b, "transformers.default_pipelines_development", true) + err := setConfig(b, "transform.default_pipelines_development", true) if err != nil { return err } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 73ae07feae..52c34e3495 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -113,7 +113,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { func TestProcessTargetModeDevelopment(t *testing.T) { b := mockBundle(config.Development) - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -162,7 +162,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAws(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -177,7 +177,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAzure(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -192,7 +192,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -203,7 +203,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { func TestProcessTargetModeDefault(t *testing.T) { b := mockBundle("") - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) @@ -289,7 +289,7 @@ func TestAllResourcesMocked(t *testing.T) { func TestAllResourcesRenamed(t *testing.T) { b := mockBundle(config.Development) - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -337,9 +337,9 @@ func TestDisableLockingDisabled(t *testing.T) { func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.Prefix = "custom_prefix_" + b.Config.Transform.Prefix = "custom_prefix_" - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -348,9 +348,9 @@ func TestPrefixAlreadySet(t *testing.T) { func TestTagsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.Tags = &map[string]string{"custom": "tag"} + b.Config.Transform.Tags = &map[string]string{"custom": "tag"} - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -359,9 +359,9 @@ func TestTagsAlreadySet(t *testing.T) { func TestTagsNil(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.Tags = nil + b.Config.Transform.Tags = nil - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -370,9 +370,9 @@ func TestTagsNil(t *testing.T) { func TestTagsEmptySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.Tags = &map[string]string{} + b.Config.Transform.Tags = &map[string]string{} - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -381,9 +381,9 @@ func TestTagsEmptySet(t *testing.T) { func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.DefaultJobsMaxConcurrentRuns = 10 + b.Config.Transform.DefaultJobsMaxConcurrentRuns = 10 - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -392,9 +392,9 @@ func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.DefaultJobsMaxConcurrentRuns = 1 + b.Config.Transform.DefaultJobsMaxConcurrentRuns = 1 - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -403,9 +403,9 @@ func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { func TestTriggerPauseStatusWhenUnpaused(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transformers.DefaultTriggerPauseStatus = config.Unpaused + b.Config.Transform.DefaultTriggerPauseStatus = config.Unpaused - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -416,9 +416,9 @@ func TestTriggerPauseStatusWhenUnpaused(t *testing.T) { func TestPipelinesDevelopmentDisabled(t *testing.T) { b := mockBundle(config.Development) notEnabled := false - b.Config.Transformers.DefaultPipelinesDevelopment = ¬Enabled + b.Config.Transform.DefaultPipelinesDevelopment = ¬Enabled - m := bundle.Seq(ProcessTargetMode(), Transformers()) + m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) diff --git a/bundle/config/root.go b/bundle/config/root.go index 6cfcd88018..bf3b3f213e 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -60,9 +60,9 @@ type Root struct { // RunAs section allows to define an execution identity for jobs and pipelines runs RunAs *jobs.JobRunAs `json:"run_as,omitempty"` - // Transformers apply some transformation throughout the bundle, e.g. + // Transform applies transformations throughout the bundle, e.g. // adding a name prefix to deployed resources. - Transformers Transformers `json:"transformers,omitempty"` + Transform Transforms `json:"transform,omitempty"` Experimental *Experimental `json:"experimental,omitempty"` @@ -334,7 +334,7 @@ func (r *Root) MergeTargetOverrides(name string) error { "sync", "permissions", "variables", - "transformers", + "transform", } { if root, err = mergeField(root, target, f); err != nil { return err @@ -349,9 +349,9 @@ func (r *Root) MergeTargetOverrides(name string) error { } } - // Merge `transformers.tags`. This field must be overwritten if set, not merged. - if v, _ := dyn.Get(target, "transformers.tags"); v != dyn.InvalidValue { - root, err = dyn.Set(root, "transformers.tags", v) + // Merge `transform.tags`. This field must be overwritten if set, not merged. + if v, _ := dyn.Get(target, "transform.tags"); v != dyn.InvalidValue { + root, err = dyn.Set(root, "transform.tags", v) if err != nil { return err } diff --git a/bundle/config/target.go b/bundle/config/target.go index 09016258ae..ad643937fd 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -22,7 +22,7 @@ type Target struct { // Mutator configurations that e.g. change the // name prefix of deployed resources. - Transformers Transformers `json:"transformers,omitempty"` + Transform Transforms `json:"transform,omitempty"` // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/config/transformers.go b/bundle/config/transforms.go similarity index 97% rename from bundle/config/transformers.go rename to bundle/config/transforms.go index f277319141..ed7a77b856 100644 --- a/bundle/config/transformers.go +++ b/bundle/config/transforms.go @@ -3,7 +3,7 @@ package config const Paused = "PAUSED" const Unpaused = "UNPAUSED" -type Transformers struct { +type Transforms struct { // Prefix to prepend to all resource names. Prefix string `json:"prefix,omitempty"` diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index f9715ddd2b..03e1d0d3b5 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -38,7 +38,7 @@ func Initialize() bundle.Mutator { mutator.SetRunAs(), mutator.OverrideCompute(), mutator.ProcessTargetMode(), - mutator.Transformers(), + mutator.ApplyTransforms(), mutator.DefaultQueueing(), mutator.ExpandPipelineGlobPaths(), mutator.TranslatePaths(), diff --git a/bundle/tests/transformers/databricks.yml b/bundle/tests/transform/databricks.yml similarity index 81% rename from bundle/tests/transformers/databricks.yml rename to bundle/tests/transform/databricks.yml index 20da05b81a..4463ddf511 100644 --- a/bundle/tests/transformers/databricks.yml +++ b/bundle/tests/transform/databricks.yml @@ -1,14 +1,14 @@ bundle: - name: transformers + name: transform -transformers: +transform: tags: prod: true default_pipelines_development: true targets: dev: - transformers: + transform: prefix: "myprefix" default_pipelines_development: true default_trigger_pause_status: PAUSED @@ -16,5 +16,5 @@ targets: tags: dev: true prod: - transformers: + transform: default_pipelines_development: false diff --git a/bundle/tests/transform_test.go b/bundle/tests/transform_test.go new file mode 100644 index 0000000000..f3f9fc20ff --- /dev/null +++ b/bundle/tests/transform_test.go @@ -0,0 +1,28 @@ +package config_tests + +import ( + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/stretchr/testify/assert" +) + +func TestTransformDev(t *testing.T) { + b := loadTarget(t, "./transform", "dev") + + assert.Equal(t, "myprefix", b.Config.Transform.Prefix) + assert.Equal(t, config.Paused, b.Config.Transform.DefaultTriggerPauseStatus) + assert.Equal(t, 10, b.Config.Transform.DefaultJobsMaxConcurrentRuns) + assert.Equal(t, true, *b.Config.Transform.DefaultPipelinesDevelopment) + assert.Equal(t, "true", (*b.Config.Transform.Tags)["dev"]) + + // Tags transform is overridden by the dev target, so prod is not set + assert.Equal(t, "", (*b.Config.Transform.Tags)["prod"]) +} + +func TestTransformProd(t *testing.T) { + b := loadTarget(t, "./transform", "prod") + + assert.Equal(t, false, *b.Config.Transform.DefaultPipelinesDevelopment) + assert.Equal(t, "true", (*b.Config.Transform.Tags)["prod"]) +} diff --git a/bundle/tests/transformers_test.go b/bundle/tests/transformers_test.go deleted file mode 100644 index 7a19002b31..0000000000 --- a/bundle/tests/transformers_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package config_tests - -import ( - "testing" - - "github.com/databricks/cli/bundle/config" - "github.com/stretchr/testify/assert" -) - -func TestTransformersDev(t *testing.T) { - b := loadTarget(t, "./transformers", "dev") - - assert.Equal(t, "myprefix", b.Config.Transformers.Prefix) - assert.Equal(t, config.Paused, b.Config.Transformers.DefaultTriggerPauseStatus) - assert.Equal(t, 10, b.Config.Transformers.DefaultJobsMaxConcurrentRuns) - assert.Equal(t, true, *b.Config.Transformers.DefaultPipelinesDevelopment) - assert.Equal(t, "true", (*b.Config.Transformers.Tags)["dev"]) - - // Tags transformer is overridden by the dev target, so prod is not set - assert.Equal(t, "", (*b.Config.Transformers.Tags)["prod"]) -} - -func TestTransformersProd(t *testing.T) { - b := loadTarget(t, "./transformers", "prod") - - assert.Equal(t, false, *b.Config.Transformers.DefaultPipelinesDevelopment) - assert.Equal(t, "true", (*b.Config.Transformers.Tags)["prod"]) -} From 7323d02d26a8e1f7b2ea6b277bb864b5e39e81cb Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 14 Jun 2024 14:34:24 +0200 Subject: [PATCH 10/23] Cleanup --- bundle/config/mutator/apply_transforms.go | 24 +++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/bundle/config/mutator/apply_transforms.go b/bundle/config/mutator/apply_transforms.go index e5f120014d..dacfa6ca56 100644 --- a/bundle/config/mutator/apply_transforms.go +++ b/bundle/config/mutator/apply_transforms.go @@ -31,18 +31,6 @@ func (m *applyTransforms) Name() string { return "ApplyTransforms" } -func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { - p := b.Config.Transform.DefaultTriggerPauseStatus - if p == "" || p == config.Paused || p == config.Unpaused { - return nil - } - return diag.Diagnostics{{ - Summary: "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", - Severity: diag.Error, - Location: b.Config.GetLocation("transform.default_trigger_pause_status"), - }} -} - func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diag := validatePauseStatus(b) if diag != nil { @@ -154,6 +142,18 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag return nil } +func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { + p := b.Config.Transform.DefaultTriggerPauseStatus + if p == "" || p == config.Paused || p == config.Unpaused { + return nil + } + return diag.Diagnostics{{ + Summary: "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", + Severity: diag.Error, + Location: b.Config.GetLocation("transform.default_trigger_pause_status"), + }} +} + // Convert a map of tags to an array of tags. // We sort tags so we always produce a consistent list of tags. func toTagArray(tags *map[string]string) []Tag { From 4dc5f412c48c261a31f89ce0497dd96ade4c77a9 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 14 Jun 2024 17:33:07 +0200 Subject: [PATCH 11/23] Add stricter validations --- bundle/config/mutator/process_target_mode.go | 38 +++++++++++++--- .../mutator/process_target_mode_test.go | 45 ++++++++++++++++--- 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index a2385e1e05..76397ccba4 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -28,7 +28,7 @@ func (m *processTargetMode) 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 transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) error { if !b.Config.Bundle.Deployment.Lock.IsExplicitlyEnabled() { log.Infof(ctx, "Development mode: disabling deployment lock since bundle.deployment.lock.enabled is not set to true") err := setConfig(b, "bundle.deployment.lock.enabled", false) @@ -78,14 +78,14 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) diag.Diagno return nil } -func setConfig(b *bundle.Bundle, path string, value any) diag.Diagnostics { +func setConfig(b *bundle.Bundle, path string, value any) error { err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { return dyn.Set(v, path, dyn.V(value)) }) - return diag.FromErr(err) + return err } -func setConfigMapping(b *bundle.Bundle, path string, key string, value string) diag.Diagnostics { +func setConfigMapping(b *bundle.Bundle, path string, key string, value string) error { err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { newMapping := dyn.V(map[string]dyn.Value{key: dyn.V(value)}) @@ -100,13 +100,33 @@ func setConfigMapping(b *bundle.Bundle, path string, key string, value string) d } return dyn.Set(v, path, merged) }) - return diag.FromErr(err) + return err } func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { + t := b.Config.Transform + u := b.Config.Workspace.CurrentUser + + // Make sure everything is paused by default to avoid surprises + if t.DefaultTriggerPauseStatus == config.Unpaused { + return diag.Diagnostics{{ + Severity: diag.Error, + Summary: "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default", + Location: b.Config.GetLocation("transform.default_trigger_pause_status"), + }} + } + + // Make sure this development copy has unique names and paths to avoid conflicts if path := findNonUserPath(b); path != "" { return diag.Errorf("%s must start with '~/' or contain the current username when using 'mode: development'", path) } + if t.Prefix != "" && !strings.Contains(t.Prefix, u.ShortName) && !strings.Contains(t.Prefix, u.UserName) { + return diag.Diagnostics{{ + Severity: diag.Error, + Summary: "prefix should contain the current username or ${workspace.current_user.short_name} to ensure uniqueness when using 'mode: development'", + Location: b.Config.GetLocation("transform.prefix"), + }} + } return nil } @@ -163,10 +183,14 @@ func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Di switch b.Config.Bundle.Mode { case config.Development: diags := validateDevelopmentMode(b) - if diags != nil { + if diags.HasError() { return diags } - return transformDevelopmentMode(ctx, b) + err := transformDevelopmentMode(ctx, b) + if err != nil { + return diag.FromErr(err) + } + return diags case config.Production: isPrincipal := auth.IsServicePrincipal(b.Config.Workspace.CurrentUser.UserName) return validateProductionMode(ctx, b, isPrincipal) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 52c34e3495..6a49c56cae 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/tags" sdkconfig "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/service/catalog" @@ -200,6 +201,41 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { assert.Equal(t, "Hello_world", b.Config.Resources.Jobs["job1"].Tags["dev"]) } +func TestValidateDevelopmentMode(t *testing.T) { + // Test with a valid development mode bundle + b := mockBundle(config.Development) + diags := validateDevelopmentMode(b) + require.NoError(t, diags.Error()) + + // Test with a bundle that has a non-user path + b.Config.Workspace.RootPath = "/Shared/.bundle/x/y/state" + diags = validateDevelopmentMode(b) + require.ErrorContains(t, diags.Error(), "root_path") + + // Test with a bundle that has an unpaused trigger pause status + b = mockBundle(config.Development) + b.Config.Transform.DefaultTriggerPauseStatus = config.Unpaused + diags = validateDevelopmentMode(b) + require.ErrorContains(t, diags.Error(), "UNPAUSED") + + // Test with a bundle that has a prefix not containing the username or short name + b = mockBundle(config.Development) + b.Config.Transform.Prefix = "[prod]" + diags = validateDevelopmentMode(b) + require.Len(t, diags, 1) + assert.Equal(t, diag.Error, diags[0].Severity) + assert.Contains(t, diags[0].Summary, "") + + // Test with a bundle that has valid user paths + b = mockBundle(config.Development) + b.Config.Workspace.RootPath = "/Users/lennart@company.com/.bundle/x/y/state" + b.Config.Workspace.StatePath = "/Users/lennart@company.com/.bundle/x/y/state" + b.Config.Workspace.FilePath = "/Users/lennart@company.com/.bundle/x/y/files" + b.Config.Workspace.ArtifactPath = "/Users/lennart@company.com/.bundle/x/y/artifacts" + diags = validateDevelopmentMode(b) + require.NoError(t, diags.Error()) +} + func TestProcessTargetModeDefault(t *testing.T) { b := mockBundle("") @@ -337,13 +373,13 @@ func TestDisableLockingDisabled(t *testing.T) { func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.Prefix = "custom_prefix_" + b.Config.Transform.Prefix = "custom_lennart_deploy_" m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - assert.Equal(t, "custom_prefix_job1", b.Config.Resources.Jobs["job1"].Name) + assert.Equal(t, "custom_lennart_deploy_job1", b.Config.Resources.Jobs["job1"].Name) } func TestTagsAlreadySet(t *testing.T) { @@ -407,10 +443,7 @@ func TestTriggerPauseStatusWhenUnpaused(t *testing.T) { m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) diags := bundle.Apply(context.Background(), b, m) - require.NoError(t, diags.Error()) - - // Pause status should take the value from the override above - assert.Equal(t, jobs.PauseStatusUnpaused, b.Config.Resources.Jobs["job3"].Trigger.PauseStatus) + require.ErrorContains(t, diags.Error(), "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default") } func TestPipelinesDevelopmentDisabled(t *testing.T) { From 82e1d49c0907339104df356e521d64f7dad38733 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 19 Jun 2024 10:30:48 +0200 Subject: [PATCH 12/23] Cleanup --- bundle/config/mutator/apply_transforms.go | 12 +++++------- bundle/config/mutator/apply_transforms_test.go | 17 +++++------------ 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/bundle/config/mutator/apply_transforms.go b/bundle/config/mutator/apply_transforms.go index dacfa6ca56..d55a070ec6 100644 --- a/bundle/config/mutator/apply_transforms.go +++ b/bundle/config/mutator/apply_transforms.go @@ -3,6 +3,7 @@ package mutator import ( "context" "path" + "slices" "sort" "strings" @@ -88,14 +89,11 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag for i := range r.Models { r.Models[i].Name = prefix + r.Models[i].Name for _, t := range tags { - exists := false - for _, modelTag := range r.Models[i].Tags { - if modelTag.Key == t.Key { - exists = true - break - } - } + exists := slices.ContainsFunc(r.Models[i].Tags, func(modelTag ml.ModelTag) bool { + return modelTag.Key == t.Key + }) if !exists { + // Only add this tag if the resource didn't include any tag that overrides its value. r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: t.Key, Value: t.Value}) } } diff --git a/bundle/config/mutator/apply_transforms_test.go b/bundle/config/mutator/apply_transforms_test.go index fa27744dbe..05655bf642 100644 --- a/bundle/config/mutator/apply_transforms_test.go +++ b/bundle/config/mutator/apply_transforms_test.go @@ -10,6 +10,7 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestTransformPrefix(t *testing.T) { @@ -63,9 +64,7 @@ func TestTransformPrefix(t *testing.T) { t.Fatalf("unexpected error: %v", diag) } - if got := b.Config.Resources.Jobs["job1"].Name; got != tt.want { - t.Errorf("got %v, want %v", got, tt.want) - } + require.Equal(t, tt.want, b.Config.Resources.Jobs["job1"].Name) }) } } @@ -134,12 +133,8 @@ func TestTransformTags(t *testing.T) { t.Fatalf("unexpected error: %v", diag) } - got := b.Config.Resources.Jobs["job1"].Tags - for k, v := range tt.want { - if got[k] != v { - t.Errorf("tag %v: got %v, want %v", k, got[k], v) - } - } + tags := b.Config.Resources.Jobs["job1"].Tags + require.Equal(t, tt.want, tags) }) } } @@ -197,9 +192,7 @@ func TestTransformJobsMaxConcurrentRuns(t *testing.T) { t.Fatalf("unexpected error: %v", diag) } - if got := b.Config.Resources.Jobs["job1"].MaxConcurrentRuns; got != tt.want { - t.Errorf("got %v, want %v", got, tt.want) - } + require.Equal(t, tt.want, b.Config.Resources.Jobs["job1"].MaxConcurrentRuns) }) } } From 6d75e844a86ccf9e03844b44174a2325955f542f Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 19 Jun 2024 15:42:26 +0200 Subject: [PATCH 13/23] Simply mutations, no need for dyn here --- bundle/config/mutator/process_target_mode.go | 61 ++++---------------- 1 file changed, 10 insertions(+), 51 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 76397ccba4..a560f5fc71 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -8,8 +8,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/auth" "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" - "github.com/databricks/cli/libs/dyn/merge" "github.com/databricks/cli/libs/log" ) @@ -31,78 +29,39 @@ func (m *processTargetMode) Name() string { func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) error { if !b.Config.Bundle.Deployment.Lock.IsExplicitlyEnabled() { log.Infof(ctx, "Development mode: disabling deployment lock since bundle.deployment.lock.enabled is not set to true") - err := setConfig(b, "bundle.deployment.lock.enabled", false) - if err != nil { - return err - } + disabled := false + b.Config.Bundle.Deployment.Lock.Enabled = &disabled } - t := b.Config.Transform + t := &b.Config.Transform shortName := b.Config.Workspace.CurrentUser.ShortName if t.Prefix == "" { - err := setConfig(b, "transform.prefix", "[dev "+shortName+"] ") - if err != nil { - return err - } + b.Config.Transform.Prefix = "[dev " + shortName + "] " } if t.Tags == nil { - err := setConfigMapping(b, "transform.tags", "dev", b.Tagging.NormalizeValue(shortName)) - if err != nil { - return err + b.Config.Transform.Tags = &map[string]string{ + "dev": b.Tagging.NormalizeValue(shortName), } } if t.DefaultJobsMaxConcurrentRuns == 0 { - err := setConfig(b, "transform.default_jobs_max_concurrent_runs", developmentConcurrentRuns) - if err != nil { - return err - } + t.DefaultJobsMaxConcurrentRuns = developmentConcurrentRuns } if t.DefaultTriggerPauseStatus == "" { - err := setConfig(b, "transform.default_trigger_pause_status", config.Paused) - if err != nil { - return err - } + t.DefaultTriggerPauseStatus = config.Paused } if !config.IsExplicitlyDisabled(t.DefaultPipelinesDevelopment) { - err := setConfig(b, "transform.default_pipelines_development", true) - if err != nil { - return err - } + enabled := true + t.DefaultPipelinesDevelopment = &enabled } return nil } -func setConfig(b *bundle.Bundle, path string, value any) error { - err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - return dyn.Set(v, path, dyn.V(value)) - }) - return err -} - -func setConfigMapping(b *bundle.Bundle, path string, key string, value string) error { - err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - newMapping := dyn.V(map[string]dyn.Value{key: dyn.V(value)}) - - existingMapping, err := dyn.Get(v, path) - if err != nil { - return dyn.Set(v, path, newMapping) - } - - merged, err := merge.Merge(newMapping, existingMapping) - if err != nil { - return dyn.InvalidValue, err - } - return dyn.Set(v, path, merged) - }) - return err -} - func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { t := b.Config.Transform u := b.Config.Workspace.CurrentUser From 40b004ed04a78c3be742bb556abc779c287016fd Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 5 Jul 2024 14:46:08 +0200 Subject: [PATCH 14/23] Cleanup --- bundle/config/mutator/apply_transforms.go | 54 +++++++++---------- bundle/config/mutator/process_target_mode.go | 13 ++--- .../mutator/process_target_mode_test.go | 6 +-- 3 files changed, 33 insertions(+), 40 deletions(-) diff --git a/bundle/config/mutator/apply_transforms.go b/bundle/config/mutator/apply_transforms.go index d55a070ec6..83718965a1 100644 --- a/bundle/config/mutator/apply_transforms.go +++ b/bundle/config/mutator/apply_transforms.go @@ -44,18 +44,18 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag tags := toTagArray(t.Tags) // Jobs transforms: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus - for i := range r.Jobs { - r.Jobs[i].Name = prefix + r.Jobs[i].Name - if r.Jobs[i].Tags == nil { - r.Jobs[i].Tags = make(map[string]string) + for _, j := range r.Jobs { + j.Name = prefix + j.Name + if j.Tags == nil { + j.Tags = make(map[string]string) } for _, tag := range tags { - if r.Jobs[i].Tags[tag.Key] == "" { - r.Jobs[i].Tags[tag.Key] = tag.Value + if j.Tags[tag.Key] == "" { + j.Tags[tag.Key] = tag.Value } } - if r.Jobs[i].MaxConcurrentRuns == 0 { - r.Jobs[i].MaxConcurrentRuns = t.DefaultJobsMaxConcurrentRuns + if j.MaxConcurrentRuns == 0 { + j.MaxConcurrentRuns = t.DefaultJobsMaxConcurrentRuns } if t.DefaultTriggerPauseStatus != "" { paused := jobs.PauseStatusPaused @@ -63,14 +63,14 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag paused = jobs.PauseStatusUnpaused } - if r.Jobs[i].Schedule != nil && r.Jobs[i].Schedule.PauseStatus == "" { - r.Jobs[i].Schedule.PauseStatus = paused + if j.Schedule != nil && j.Schedule.PauseStatus == "" { + j.Schedule.PauseStatus = paused } - if r.Jobs[i].Continuous != nil && r.Jobs[i].Continuous.PauseStatus == "" { - r.Jobs[i].Continuous.PauseStatus = paused + if j.Continuous != nil && j.Continuous.PauseStatus == "" { + j.Continuous.PauseStatus = paused } - if r.Jobs[i].Trigger != nil && r.Jobs[i].Trigger.PauseStatus == "" { - r.Jobs[i].Trigger.PauseStatus = paused + if j.Trigger != nil && j.Trigger.PauseStatus == "" { + j.Trigger.PauseStatus = paused } } } @@ -86,39 +86,39 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag } // Models transforms: Prefix, Tags - for i := range r.Models { - r.Models[i].Name = prefix + r.Models[i].Name + for _, m := range r.Models { + m.Name = prefix + m.Name for _, t := range tags { - exists := slices.ContainsFunc(r.Models[i].Tags, func(modelTag ml.ModelTag) bool { + exists := slices.ContainsFunc(m.Tags, func(modelTag ml.ModelTag) bool { return modelTag.Key == t.Key }) if !exists { // Only add this tag if the resource didn't include any tag that overrides its value. - r.Models[i].Tags = append(r.Models[i].Tags, ml.ModelTag{Key: t.Key, Value: t.Value}) + m.Tags = append(m.Tags, ml.ModelTag{Key: t.Key, Value: t.Value}) } } } // Experiments transforms: Prefix, Tags - for i := range r.Experiments { - filepath := r.Experiments[i].Name + for _, e := range r.Experiments { + filepath := e.Name dir := path.Dir(filepath) base := path.Base(filepath) if dir == "." { - r.Experiments[i].Name = prefix + base + e.Name = prefix + base } else { - r.Experiments[i].Name = dir + "/" + prefix + base + e.Name = dir + "/" + prefix + base } for _, t := range tags { exists := false - for _, experimentTag := range r.Experiments[i].Tags { + for _, experimentTag := range e.Tags { if experimentTag.Key == t.Key { exists = true break } } if !exists { - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: t.Key, Value: t.Value}) + e.Tags = append(e.Tags, ml.ExperimentTag{Key: t.Key, Value: t.Value}) } } } @@ -152,8 +152,8 @@ func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { }} } -// Convert a map of tags to an array of tags. -// We sort tags so we always produce a consistent list of tags. +// toTagArray converts a map of tags to an array of tags. +// We sort tags so ensure stable ordering. func toTagArray(tags *map[string]string) []Tag { var tagArray []Tag if tags == nil { @@ -168,7 +168,7 @@ func toTagArray(tags *map[string]string) []Tag { return tagArray } -// Normalize prefix strings like '[dev lennart] ' to 'dev_lennart_'. +// normalizePrefix prefixes strings like '[dev lennart] ' to 'dev_lennart_'. // We leave unicode letters and numbers but remove all "special characters." func normalizePrefix(prefix string) string { prefix = strings.ReplaceAll(prefix, "[", "") diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index a560f5fc71..be4e960a69 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -26,7 +26,7 @@ func (m *processTargetMode) 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 transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) error { +func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { if !b.Config.Bundle.Deployment.Lock.IsExplicitlyEnabled() { log.Infof(ctx, "Development mode: disabling deployment lock since bundle.deployment.lock.enabled is not set to true") disabled := false @@ -37,11 +37,11 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) error { shortName := b.Config.Workspace.CurrentUser.ShortName if t.Prefix == "" { - b.Config.Transform.Prefix = "[dev " + shortName + "] " + t.Prefix = "[dev " + shortName + "] " } if t.Tags == nil { - b.Config.Transform.Tags = &map[string]string{ + t.Tags = &map[string]string{ "dev": b.Tagging.NormalizeValue(shortName), } } @@ -58,8 +58,6 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) error { enabled := true t.DefaultPipelinesDevelopment = &enabled } - - return nil } func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { @@ -145,10 +143,7 @@ func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Di if diags.HasError() { return diags } - err := transformDevelopmentMode(ctx, b) - if err != nil { - return diag.FromErr(err) - } + transformDevelopmentMode(ctx, b) return diags case config.Production: isPrincipal := auth.IsServicePrincipal(b.Config.Workspace.CurrentUser.UserName) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 6a49c56cae..c2b191db19 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -355,8 +355,7 @@ func TestDisableLocking(t *testing.T) { ctx := context.Background() b := mockBundle(config.Development) - err := transformDevelopmentMode(ctx, b) - require.Nil(t, err) + transformDevelopmentMode(ctx, b) assert.False(t, b.Config.Bundle.Deployment.Lock.IsEnabled()) } @@ -366,8 +365,7 @@ func TestDisableLockingDisabled(t *testing.T) { explicitlyEnabled := true b.Config.Bundle.Deployment.Lock.Enabled = &explicitlyEnabled - err := transformDevelopmentMode(ctx, b) - require.Nil(t, err) + transformDevelopmentMode(ctx, b) assert.True(t, b.Config.Bundle.Deployment.Lock.IsEnabled(), "Deployment lock should remain enabled in development mode when explicitly enabled") } From b353a2fe1ca5c21b7b137dfcea380e3139c4a5e0 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 6 Jul 2024 17:17:48 +0200 Subject: [PATCH 15/23] Rename to presets --- .../{apply_transforms.go => apply_presets.go} | 46 +++++++++--------- ...ansforms_test.go => apply_presets_test.go} | 30 ++++++------ bundle/config/mutator/process_target_mode.go | 18 +++---- .../mutator/process_target_mode_test.go | 48 +++++++++---------- bundle/config/{transforms.go => presets.go} | 14 +++--- bundle/config/root.go | 12 ++--- bundle/config/target.go | 2 +- bundle/phases/initialize.go | 2 +- bundle/tests/presets/databricks.yml | 20 ++++++++ bundle/tests/presets_test.go | 30 ++++++++++++ bundle/tests/transform/databricks.yml | 20 -------- bundle/tests/transform_test.go | 28 ----------- 12 files changed, 136 insertions(+), 134 deletions(-) rename bundle/config/mutator/{apply_transforms.go => apply_presets.go} (78%) rename bundle/config/mutator/{apply_transforms_test.go => apply_presets_test.go} (84%) rename bundle/config/{transforms.go => presets.go} (52%) create mode 100644 bundle/tests/presets/databricks.yml create mode 100644 bundle/tests/presets_test.go delete mode 100644 bundle/tests/transform/databricks.yml delete mode 100644 bundle/tests/transform_test.go diff --git a/bundle/config/mutator/apply_transforms.go b/bundle/config/mutator/apply_presets.go similarity index 78% rename from bundle/config/mutator/apply_transforms.go rename to bundle/config/mutator/apply_presets.go index 4a6441dc4d..be44612c11 100644 --- a/bundle/config/mutator/apply_transforms.go +++ b/bundle/config/mutator/apply_presets.go @@ -16,12 +16,12 @@ import ( "github.com/databricks/databricks-sdk-go/service/ml" ) -type applyTransforms struct{} +type applyPresets struct{} -// Apply all transforms, e.g. the prefix transform that +// Apply all presets, e.g. the prefix presets that // adds a prefix to all names of all resources. -func ApplyTransforms() *applyTransforms { - return &applyTransforms{} +func ApplyPresets() *applyPresets { + return &applyPresets{} } type Tag struct { @@ -29,22 +29,22 @@ type Tag struct { Value string } -func (m *applyTransforms) Name() string { - return "ApplyTransforms" +func (m *applyPresets) Name() string { + return "ApplyPresets" } -func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diag := validatePauseStatus(b) if diag != nil { return diag } r := b.Config.Resources - t := b.Config.Transform + t := b.Config.Presets prefix := t.Prefix tags := toTagArray(t.Tags) - // Jobs transforms: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus + // Jobs presets: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus for _, j := range r.Jobs { j.Name = prefix + j.Name if j.Tags == nil { @@ -56,11 +56,11 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag } } if j.MaxConcurrentRuns == 0 { - j.MaxConcurrentRuns = t.DefaultJobsMaxConcurrentRuns + j.MaxConcurrentRuns = t.JobsMaxConcurrentRuns } - if t.DefaultTriggerPauseStatus != "" { + if t.TriggerPauseStatus != "" { paused := jobs.PauseStatusPaused - if t.DefaultTriggerPauseStatus == config.Unpaused { + if t.TriggerPauseStatus == config.Unpaused { paused = jobs.PauseStatusUnpaused } @@ -76,17 +76,17 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag } } - // Pipelines transforms: Prefix, PipelinesDevelopment + // Pipelines presets: Prefix, PipelinesDevelopment for i := range r.Pipelines { r.Pipelines[i].Name = prefix + r.Pipelines[i].Name - if config.IsExplicitlyEnabled(t.DefaultPipelinesDevelopment) { + if config.IsExplicitlyEnabled(t.PipelinesDevelopment) { r.Pipelines[i].Development = true } // As of 2024-06, pipelines don't yet support tags } - // Models transforms: Prefix, Tags + // Models presets: Prefix, Tags for _, m := range r.Models { m.Name = prefix + m.Name for _, t := range tags { @@ -100,7 +100,7 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag } } - // Experiments transforms: Prefix, Tags + // Experiments presets: Prefix, Tags for _, e := range r.Experiments { filepath := e.Name dir := path.Dir(filepath) @@ -124,22 +124,22 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag } } - // Model serving endpoint transforms: Prefix + // Model serving endpoint presets: Prefix for i := range r.ModelServingEndpoints { r.ModelServingEndpoints[i].Name = normalizePrefix(prefix) + r.ModelServingEndpoints[i].Name // As of 2024-06, model serving endpoints don't yet support tags } - // Registered models transforms: Prefix + // Registered models presets: Prefix for i := range r.RegisteredModels { r.RegisteredModels[i].Name = normalizePrefix(prefix) + r.RegisteredModels[i].Name // As of 2024-06, registered models don't yet support tags } - // Quality monitors transforms: Prefix - if t.DefaultTriggerPauseStatus == config.Paused { + // Quality monitors presets: Prefix + if t.TriggerPauseStatus == config.Paused { for i := range r.QualityMonitors { // Remove all schedules from monitors, since they don't support pausing/unpausing. // Quality monitors might support the "pause" property in the future, so at the @@ -154,14 +154,14 @@ func (m *applyTransforms) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag } func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { - p := b.Config.Transform.DefaultTriggerPauseStatus + p := b.Config.Presets.TriggerPauseStatus if p == "" || p == config.Paused || p == config.Unpaused { return nil } return diag.Diagnostics{{ - Summary: "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", + Summary: "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED", Severity: diag.Error, - Location: b.Config.GetLocation("transform.default_trigger_pause_status"), + Location: b.Config.GetLocation("presets.trigger_pause_status"), }} } diff --git a/bundle/config/mutator/apply_transforms_test.go b/bundle/config/mutator/apply_presets_test.go similarity index 84% rename from bundle/config/mutator/apply_transforms_test.go rename to bundle/config/mutator/apply_presets_test.go index 05655bf642..f19652189d 100644 --- a/bundle/config/mutator/apply_transforms_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestTransformPrefix(t *testing.T) { +func TestApplyPresetsPrefix(t *testing.T) { tests := []struct { name string prefix string @@ -51,13 +51,13 @@ func TestTransformPrefix(t *testing.T) { "job1": tt.job, }, }, - Transform: config.Transforms{ + Presets: config.Presets{ Prefix: tt.prefix, }, }, } - mutator := mutator.ApplyTransforms() + mutator := mutator.ApplyPresets() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -69,7 +69,7 @@ func TestTransformPrefix(t *testing.T) { } } -func TestTransformTags(t *testing.T) { +func TestApplyPresetsTags(t *testing.T) { tests := []struct { name string tags map[string]string @@ -120,13 +120,13 @@ func TestTransformTags(t *testing.T) { "job1": tt.job, }, }, - Transform: config.Transforms{ + Presets: config.Presets{ Tags: &tt.tags, }, }, } - mutator := mutator.ApplyTransforms() + mutator := mutator.ApplyPresets() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -139,7 +139,7 @@ func TestTransformTags(t *testing.T) { } } -func TestTransformJobsMaxConcurrentRuns(t *testing.T) { +func TestApplyPresetsJobsMaxConcurrentRuns(t *testing.T) { tests := []struct { name string job *resources.Job @@ -179,13 +179,13 @@ func TestTransformJobsMaxConcurrentRuns(t *testing.T) { "job1": tt.job, }, }, - Transform: config.Transforms{ - DefaultJobsMaxConcurrentRuns: tt.setting, + Presets: config.Presets{ + JobsMaxConcurrentRuns: tt.setting, }, }, } - mutator := mutator.ApplyTransforms() + mutator := mutator.ApplyPresets() diag := mutator.Apply(context.Background(), b) if diag.HasError() { @@ -197,16 +197,16 @@ func TestTransformJobsMaxConcurrentRuns(t *testing.T) { } } -func TestTransformValidation(t *testing.T) { +func TestApplyPresetsValidation(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ - Transform: config.Transforms{ - DefaultTriggerPauseStatus: "invalid", + Presets: config.Presets{ + TriggerPauseStatus: "invalid", }, }, } - mutator := mutator.ApplyTransforms() + mutator := mutator.ApplyPresets() diag := mutator.Apply(context.Background(), b) - assert.Equal(t, "Invalid value for default_trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) + assert.Equal(t, "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) } diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index be4e960a69..a085c01b55 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -33,7 +33,7 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { b.Config.Bundle.Deployment.Lock.Enabled = &disabled } - t := &b.Config.Transform + t := &b.Config.Presets shortName := b.Config.Workspace.CurrentUser.ShortName if t.Prefix == "" { @@ -46,26 +46,26 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { } } - if t.DefaultJobsMaxConcurrentRuns == 0 { - t.DefaultJobsMaxConcurrentRuns = developmentConcurrentRuns + if t.JobsMaxConcurrentRuns == 0 { + t.JobsMaxConcurrentRuns = developmentConcurrentRuns } - if t.DefaultTriggerPauseStatus == "" { - t.DefaultTriggerPauseStatus = config.Paused + if t.TriggerPauseStatus == "" { + t.TriggerPauseStatus = config.Paused } - if !config.IsExplicitlyDisabled(t.DefaultPipelinesDevelopment) { + if !config.IsExplicitlyDisabled(t.PipelinesDevelopment) { enabled := true - t.DefaultPipelinesDevelopment = &enabled + t.PipelinesDevelopment = &enabled } } func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { - t := b.Config.Transform + t := b.Config.Presets u := b.Config.Workspace.CurrentUser // Make sure everything is paused by default to avoid surprises - if t.DefaultTriggerPauseStatus == config.Unpaused { + if t.TriggerPauseStatus == config.Unpaused { return diag.Diagnostics{{ Severity: diag.Error, Summary: "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default", diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 10994bb755..52b13bfb9e 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -128,7 +128,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { func TestProcessTargetModeDevelopment(t *testing.T) { b := mockBundle(config.Development) - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -179,7 +179,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAws(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -194,7 +194,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForAzure(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -209,7 +209,7 @@ func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { }) b.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -230,13 +230,13 @@ func TestValidateDevelopmentMode(t *testing.T) { // Test with a bundle that has an unpaused trigger pause status b = mockBundle(config.Development) - b.Config.Transform.DefaultTriggerPauseStatus = config.Unpaused + b.Config.Presets.TriggerPauseStatus = config.Unpaused diags = validateDevelopmentMode(b) require.ErrorContains(t, diags.Error(), "UNPAUSED") // Test with a bundle that has a prefix not containing the username or short name b = mockBundle(config.Development) - b.Config.Transform.Prefix = "[prod]" + b.Config.Presets.Prefix = "[prod]" diags = validateDevelopmentMode(b) require.Len(t, diags, 1) assert.Equal(t, diag.Error, diags[0].Severity) @@ -255,7 +255,7 @@ func TestValidateDevelopmentMode(t *testing.T) { func TestProcessTargetModeDefault(t *testing.T) { b := mockBundle("") - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name) @@ -341,7 +341,7 @@ func TestAllResourcesMocked(t *testing.T) { func TestAllResourcesRenamed(t *testing.T) { b := mockBundle(config.Development) - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -387,9 +387,9 @@ func TestDisableLockingDisabled(t *testing.T) { func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.Prefix = "custom_lennart_deploy_" + b.Config.Presets.Prefix = "custom_lennart_deploy_" - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -398,9 +398,9 @@ func TestPrefixAlreadySet(t *testing.T) { func TestTagsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.Tags = &map[string]string{"custom": "tag"} + b.Config.Presets.Tags = &map[string]string{"custom": "tag"} - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -409,9 +409,9 @@ func TestTagsAlreadySet(t *testing.T) { func TestTagsNil(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.Tags = nil + b.Config.Presets.Tags = nil - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -420,9 +420,9 @@ func TestTagsNil(t *testing.T) { func TestTagsEmptySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.Tags = &map[string]string{} + b.Config.Presets.Tags = &map[string]string{} - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -431,9 +431,9 @@ func TestTagsEmptySet(t *testing.T) { func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.DefaultJobsMaxConcurrentRuns = 10 + b.Config.Presets.JobsMaxConcurrentRuns = 10 - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -442,9 +442,9 @@ func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.DefaultJobsMaxConcurrentRuns = 1 + b.Config.Presets.JobsMaxConcurrentRuns = 1 - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) @@ -453,9 +453,9 @@ func TestJobsMaxConcurrentRunsDisabled(t *testing.T) { func TestTriggerPauseStatusWhenUnpaused(t *testing.T) { b := mockBundle(config.Development) - b.Config.Transform.DefaultTriggerPauseStatus = config.Unpaused + b.Config.Presets.TriggerPauseStatus = config.Unpaused - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.ErrorContains(t, diags.Error(), "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default") } @@ -463,9 +463,9 @@ func TestTriggerPauseStatusWhenUnpaused(t *testing.T) { func TestPipelinesDevelopmentDisabled(t *testing.T) { b := mockBundle(config.Development) notEnabled := false - b.Config.Transform.DefaultPipelinesDevelopment = ¬Enabled + b.Config.Presets.PipelinesDevelopment = ¬Enabled - m := bundle.Seq(ProcessTargetMode(), ApplyTransforms()) + m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) diff --git a/bundle/config/transforms.go b/bundle/config/presets.go similarity index 52% rename from bundle/config/transforms.go rename to bundle/config/presets.go index ed7a77b856..7184f1debf 100644 --- a/bundle/config/transforms.go +++ b/bundle/config/presets.go @@ -3,19 +3,19 @@ package config const Paused = "PAUSED" const Unpaused = "UNPAUSED" -type Transforms struct { +type Presets struct { // Prefix to prepend to all resource names. Prefix string `json:"prefix,omitempty"` - // DefaultPipelinesDevelopment is the default value for the development field of pipelines. - DefaultPipelinesDevelopment *bool `json:"default_pipelines_development,omitempty"` + // PipelinesDevelopment is the default value for the development field of pipelines. + PipelinesDevelopment *bool `json:"pipelines_development,omitempty"` - // DefaultTriggerPauseStatus is the default value for the pause status of all triggers and schedules. + // TriggerPauseStatus is the default value for the pause status of all triggers and schedules. // Either config.Paused, config.Unpaused, or empty. - DefaultTriggerPauseStatus string `json:"default_trigger_pause_status,omitempty"` + TriggerPauseStatus string `json:"trigger_pause_status,omitempty"` - // DefaultJobsMaxConcurrentRuns is the default value for the max concurrent runs of jobs. - DefaultJobsMaxConcurrentRuns int `json:"default_jobs_max_concurrent_runs,omitempty"` + // JobsMaxConcurrentRuns is the default value for the max concurrent runs of jobs. + JobsMaxConcurrentRuns int `json:"jobs_max_concurrent_runs,omitempty"` // Tags to add to all resources. Tags *map[string]string `json:"tags,omitempty"` diff --git a/bundle/config/root.go b/bundle/config/root.go index 386d98c2f9..272b03478a 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -60,9 +60,9 @@ type Root struct { // RunAs section allows to define an execution identity for jobs and pipelines runs RunAs *jobs.JobRunAs `json:"run_as,omitempty"` - // Transform applies transformations throughout the bundle, e.g. + // Presets applies preset transformations throughout the bundle, e.g. // adding a name prefix to deployed resources. - Transform Transforms `json:"transform,omitempty"` + Presets Presets `json:"presets,omitempty"` Experimental *Experimental `json:"experimental,omitempty"` @@ -342,7 +342,7 @@ func (r *Root) MergeTargetOverrides(name string) error { "resources", "sync", "permissions", - "transform", + "presets", } { if root, err = mergeField(root, target, f); err != nil { return err @@ -378,9 +378,9 @@ func (r *Root) MergeTargetOverrides(name string) error { root, err = dyn.Set(root, "run_as", v) } - // Merge `transform.tags`. This field must be overwritten if set, not merged. - if v, _ := dyn.Get(target, "transform.tags"); v != dyn.InvalidValue { - root, err = dyn.Set(root, "transform.tags", v) + // Merge `presets.tags`. This field must be overwritten if set, not merged. + if v, _ := dyn.Get(target, "presets.tags"); v != dyn.InvalidValue { + root, err = dyn.Set(root, "presets.tags", v) if err != nil { return err } diff --git a/bundle/config/target.go b/bundle/config/target.go index ad643937fd..a2ef4d7356 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -22,7 +22,7 @@ type Target struct { // Mutator configurations that e.g. change the // name prefix of deployed resources. - Transform Transforms `json:"transform,omitempty"` + Presets Presets `json:"presets,omitempty"` // Overrides the compute used for jobs and other supported assets. ComputeID string `json:"compute_id,omitempty"` diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index f0666fa5cf..1f4af2a260 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -44,7 +44,7 @@ func Initialize() bundle.Mutator { mutator.SetRunAs(), mutator.OverrideCompute(), mutator.ProcessTargetMode(), - mutator.ApplyTransforms(), + mutator.ApplyPresets(), mutator.DefaultQueueing(), mutator.ExpandPipelineGlobPaths(), diff --git a/bundle/tests/presets/databricks.yml b/bundle/tests/presets/databricks.yml new file mode 100644 index 0000000000..b8263840cf --- /dev/null +++ b/bundle/tests/presets/databricks.yml @@ -0,0 +1,20 @@ +bundle: + name: presets + +presets: + tags: + prod: true + pipelines_development: true + +targets: + dev: + presets: + prefix: "myprefix" + pipelines_development: true + trigger_pause_status: PAUSED + jobs_max_concurrent_runs: 10 + tags: + dev: true + prod: + presets: + pipelines_development: false diff --git a/bundle/tests/presets_test.go b/bundle/tests/presets_test.go new file mode 100644 index 0000000000..9a0926f3db --- /dev/null +++ b/bundle/tests/presets_test.go @@ -0,0 +1,30 @@ +package config_tests + +import ( + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/stretchr/testify/assert" +) + +func TestPresetsDev(t *testing.T) { + b := loadTarget(t, "./presets", "dev") + + assert.Equal(t, "myprefix", b.Config.Presets.Prefix) + assert.Equal(t, config.Paused, b.Config.Presets.TriggerPauseStatus) + assert.Equal(t, 10, b.Config.Presets.JobsMaxConcurrentRuns) + assert.Equal(t, true, *b.Config.Presets.PipelinesDevelopment) + assert.Equal(t, "true", (*b.Config.Presets.Tags)["dev"]) + assert.NotContains(t, b.Config.Presets.Tags, "prod") + assert.NotContains(t, b.Config.Presets.Tags, "~prod") + + // Tags transform is overridden by the dev target, so prod is not set + assert.Equal(t, "", (*b.Config.Presets.Tags)["prod"]) +} + +func TestPresetsProd(t *testing.T) { + b := loadTarget(t, "./presets", "prod") + + assert.Equal(t, false, *b.Config.Presets.PipelinesDevelopment) + assert.Equal(t, "true", (*b.Config.Presets.Tags)["prod"]) +} diff --git a/bundle/tests/transform/databricks.yml b/bundle/tests/transform/databricks.yml deleted file mode 100644 index 4463ddf511..0000000000 --- a/bundle/tests/transform/databricks.yml +++ /dev/null @@ -1,20 +0,0 @@ -bundle: - name: transform - -transform: - tags: - prod: true - default_pipelines_development: true - -targets: - dev: - transform: - prefix: "myprefix" - default_pipelines_development: true - default_trigger_pause_status: PAUSED - default_jobs_max_concurrent_runs: 10 - tags: - dev: true - prod: - transform: - default_pipelines_development: false diff --git a/bundle/tests/transform_test.go b/bundle/tests/transform_test.go deleted file mode 100644 index f3f9fc20ff..0000000000 --- a/bundle/tests/transform_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package config_tests - -import ( - "testing" - - "github.com/databricks/cli/bundle/config" - "github.com/stretchr/testify/assert" -) - -func TestTransformDev(t *testing.T) { - b := loadTarget(t, "./transform", "dev") - - assert.Equal(t, "myprefix", b.Config.Transform.Prefix) - assert.Equal(t, config.Paused, b.Config.Transform.DefaultTriggerPauseStatus) - assert.Equal(t, 10, b.Config.Transform.DefaultJobsMaxConcurrentRuns) - assert.Equal(t, true, *b.Config.Transform.DefaultPipelinesDevelopment) - assert.Equal(t, "true", (*b.Config.Transform.Tags)["dev"]) - - // Tags transform is overridden by the dev target, so prod is not set - assert.Equal(t, "", (*b.Config.Transform.Tags)["prod"]) -} - -func TestTransformProd(t *testing.T) { - b := loadTarget(t, "./transform", "prod") - - assert.Equal(t, false, *b.Config.Transform.DefaultPipelinesDevelopment) - assert.Equal(t, "true", (*b.Config.Transform.Tags)["prod"]) -} From f636e09d39e598ee3adf2b5fa08e47b4304bcc58 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 6 Jul 2024 20:56:16 +0200 Subject: [PATCH 16/23] Allow tags to merge instead of override --- bundle/config/mutator/apply_presets.go | 28 +++++++++++++++---- bundle/config/mutator/apply_presets_test.go | 16 ++++++++--- bundle/config/mutator/process_target_mode.go | 8 ++++-- .../mutator/process_target_mode_test.go | 8 ++++-- bundle/config/presets.go | 2 +- bundle/config/root.go | 8 ------ bundle/tests/presets/databricks.yml | 2 ++ bundle/tests/presets_test.go | 11 +++----- 8 files changed, 53 insertions(+), 30 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index be44612c11..f6a07a8983 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -34,9 +34,11 @@ func (m *applyPresets) Name() string { } func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diag := validatePauseStatus(b) - if diag != nil { - return diag + if d := validatePauseStatus(b); d != nil { + return d + } + if d := validateTags(b); d != nil { + return d } r := b.Config.Resources @@ -165,14 +167,30 @@ func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { }} } +func validateTags(b *bundle.Bundle) diag.Diagnostics { + tags := b.Config.Presets.Tags + for key := range tags { + // Tags keys starting with a "~" are reserved. + // They may someday be used to indicate that a tag should be removed. + // For example {"~dev:": nil} could indicate that the "dev" tag should be removed. + if strings.HasPrefix(key, "~") { + return diag.Diagnostics{{ + Summary: "Invalid tag key: " + key, + Severity: diag.Error, + }} + } + } + return nil +} + // toTagArray converts a map of tags to an array of tags. // We sort tags so ensure stable ordering. -func toTagArray(tags *map[string]string) []Tag { +func toTagArray(tags map[string]string) []Tag { var tagArray []Tag if tags == nil { return tagArray } - for key, value := range *tags { + for key, value := range tags { tagArray = append(tagArray, Tag{Key: key, Value: value}) } sort.Slice(tagArray, func(i, j int) bool { diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index f19652189d..9832b2efe9 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/jobs" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -121,7 +120,7 @@ func TestApplyPresetsTags(t *testing.T) { }, }, Presets: config.Presets{ - Tags: &tt.tags, + Tags: tt.tags, }, }, } @@ -205,8 +204,17 @@ func TestApplyPresetsValidation(t *testing.T) { }, }, } - mutator := mutator.ApplyPresets() diag := mutator.Apply(context.Background(), b) - assert.Equal(t, "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) + require.Equal(t, "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) + + b = &bundle.Bundle{ + Config: config.Root{ + Presets: config.Presets{ + Tags: map[string]string{"~tilde": "tag"}, + }, + }, + } + diag = mutator.Apply(context.Background(), b) + require.Contains(t, diag[0].Summary, "Invalid tag key") } diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index a085c01b55..735953dc88 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -41,9 +41,11 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { } if t.Tags == nil { - t.Tags = &map[string]string{ - "dev": b.Tagging.NormalizeValue(shortName), - } + t.Tags = map[string]string{} + } + _, exists := t.Tags["dev"] + if !exists { + t.Tags["dev"] = b.Tagging.NormalizeValue(shortName) } if t.JobsMaxConcurrentRuns == 0 { diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 52b13bfb9e..915dee726d 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -398,13 +398,17 @@ func TestPrefixAlreadySet(t *testing.T) { func TestTagsAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Presets.Tags = &map[string]string{"custom": "tag"} + b.Config.Presets.Tags = map[string]string{ + "custom": "tag", + "dev": "foo", + } m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["custom"]) + assert.Equal(t, "foo", b.Config.Resources.Jobs["job1"].Tags["dev"]) } func TestTagsNil(t *testing.T) { @@ -420,7 +424,7 @@ func TestTagsNil(t *testing.T) { func TestTagsEmptySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Presets.Tags = &map[string]string{} + b.Config.Presets.Tags = map[string]string{} m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) diff --git a/bundle/config/presets.go b/bundle/config/presets.go index 7184f1debf..07712edc9d 100644 --- a/bundle/config/presets.go +++ b/bundle/config/presets.go @@ -18,7 +18,7 @@ type Presets struct { JobsMaxConcurrentRuns int `json:"jobs_max_concurrent_runs,omitempty"` // Tags to add to all resources. - Tags *map[string]string `json:"tags,omitempty"` + Tags map[string]string `json:"tags,omitempty"` } // IsExplicitlyEnabled tests whether this feature is explicitly enabled. diff --git a/bundle/config/root.go b/bundle/config/root.go index 272b03478a..4ca46461ec 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -378,14 +378,6 @@ func (r *Root) MergeTargetOverrides(name string) error { root, err = dyn.Set(root, "run_as", v) } - // Merge `presets.tags`. This field must be overwritten if set, not merged. - if v, _ := dyn.Get(target, "presets.tags"); v != dyn.InvalidValue { - root, err = dyn.Set(root, "presets.tags", v) - if err != nil { - return err - } - } - // Below, we're setting fields on the bundle key, so make sure it exists. if root.Get("bundle").Kind() == dyn.KindInvalid { root, err = dyn.Set(root, "bundle", dyn.NewValue(map[string]dyn.Value{}, dyn.Location{})) diff --git a/bundle/tests/presets/databricks.yml b/bundle/tests/presets/databricks.yml index b8263840cf..a4ec379ee7 100644 --- a/bundle/tests/presets/databricks.yml +++ b/bundle/tests/presets/databricks.yml @@ -4,6 +4,7 @@ bundle: presets: tags: prod: true + team: finance pipelines_development: true targets: @@ -15,6 +16,7 @@ targets: jobs_max_concurrent_runs: 10 tags: dev: true + prod: false prod: presets: pipelines_development: false diff --git a/bundle/tests/presets_test.go b/bundle/tests/presets_test.go index 9a0926f3db..f3e9a4b04c 100644 --- a/bundle/tests/presets_test.go +++ b/bundle/tests/presets_test.go @@ -14,17 +14,14 @@ func TestPresetsDev(t *testing.T) { assert.Equal(t, config.Paused, b.Config.Presets.TriggerPauseStatus) assert.Equal(t, 10, b.Config.Presets.JobsMaxConcurrentRuns) assert.Equal(t, true, *b.Config.Presets.PipelinesDevelopment) - assert.Equal(t, "true", (*b.Config.Presets.Tags)["dev"]) - assert.NotContains(t, b.Config.Presets.Tags, "prod") - assert.NotContains(t, b.Config.Presets.Tags, "~prod") - - // Tags transform is overridden by the dev target, so prod is not set - assert.Equal(t, "", (*b.Config.Presets.Tags)["prod"]) + assert.Equal(t, "true", b.Config.Presets.Tags["dev"]) + assert.Equal(t, "finance", b.Config.Presets.Tags["team"]) + assert.Equal(t, "false", b.Config.Presets.Tags["prod"]) } func TestPresetsProd(t *testing.T) { b := loadTarget(t, "./presets", "prod") assert.Equal(t, false, *b.Config.Presets.PipelinesDevelopment) - assert.Equal(t, "true", (*b.Config.Presets.Tags)["prod"]) + assert.Equal(t, "true", b.Config.Presets.Tags["prod"]) } From 347e24e93411a193cca41bfe1d9a67a4e5b77fa7 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 6 Jul 2024 22:07:43 +0200 Subject: [PATCH 17/23] Fix test --- bundle/config/mutator/process_target_mode_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 915dee726d..7f0e6d11aa 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -430,7 +430,7 @@ func TestTagsEmptySet(t *testing.T) { diags := bundle.Apply(context.Background(), b, m) require.NoError(t, diags.Error()) - assert.Empty(t, b.Config.Resources.Jobs["job2"].Tags) + assert.Equal(t, "lennart", b.Config.Resources.Jobs["job2"].Tags["dev"]) } func TestJobsMaxConcurrentRunsAlreadySet(t *testing.T) { From 3e003c0fb2a8825639e073b0d4d41791523db97f Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Thu, 11 Jul 2024 23:21:06 +0200 Subject: [PATCH 18/23] Pause continuous pipelines when 'mode: development' is used --- bundle/config/mutator/apply_presets.go | 3 +++ bundle/config/mutator/process_target_mode_test.go | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index f6a07a8983..5189960806 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -84,6 +84,9 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos if config.IsExplicitlyEnabled(t.PipelinesDevelopment) { r.Pipelines[i].Development = true } + if t.TriggerPauseStatus == config.Paused { + r.Pipelines[i].Continuous = false + } // As of 2024-06, pipelines don't yet support tags } diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 7f0e6d11aa..2c2f58bdcc 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -84,7 +84,7 @@ func mockBundle(mode config.Mode) *bundle.Bundle { }, }, Pipelines: map[string]*resources.Pipeline{ - "pipeline1": {PipelineSpec: &pipelines.PipelineSpec{Name: "pipeline1"}}, + "pipeline1": {PipelineSpec: &pipelines.PipelineSpec{Name: "pipeline1", Continuous: true}}, }, Experiments: map[string]*resources.MlflowExperiment{ "experiment1": {Experiment: &ml.Experiment{Name: "/Users/lennart.kats@databricks.com/experiment1"}}, @@ -145,6 +145,7 @@ func TestProcessTargetModeDevelopment(t *testing.T) { // Pipeline 1 assert.Equal(t, "[dev lennart] pipeline1", b.Config.Resources.Pipelines["pipeline1"].Name) + assert.Equal(t, false, b.Config.Resources.Pipelines["pipeline1"].Continuous) assert.True(t, b.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) // Experiment 1 From 40f3bb44ce5be76a74bf9b854e8adbc2da4e0619 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 19 Jul 2024 20:56:01 +0200 Subject: [PATCH 19/23] Use extension configuration --- .vscode/settings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 869465286f..9697e221d9 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -6,7 +6,7 @@ "files.trimTrailingWhitespace": true, "files.insertFinalNewline": true, "files.trimFinalNewlines": true, - "python.envFile": "${workspaceFolder}/.databricks/.databricks.env", + "python.envFile": "${workspaceRoot}/.env", "databricks.python.envFile": "${workspaceFolder}/.env", "python.analysis.stubPath": ".vscode", "jupyter.interactiveWindow.cellMarker.codeRegex": "^# COMMAND ----------|^# Databricks notebook source|^(#\\s*%%|#\\s*\\|#\\s*In\\[\\d*?\\]|#\\s*In\\[ \\])", From b1427b3391fada069072ea0da6c6681d012b7f6f Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 19 Jul 2024 21:40:35 +0200 Subject: [PATCH 20/23] Address reviewer comments, fix names --- bundle/config/mutator/apply_presets.go | 21 +--------------- bundle/config/mutator/apply_presets_test.go | 25 +------------------ bundle/config/mutator/process_target_mode.go | 25 +++++++++++++------ .../mutator/process_target_mode_test.go | 4 +-- bundle/config/presets.go | 4 +-- bundle/config/root.go | 3 +++ bundle/tests/presets/databricks.yml | 2 +- bundle/tests/presets_test.go | 2 +- 8 files changed, 28 insertions(+), 58 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 5189960806..189dce4e7f 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -37,13 +37,10 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos if d := validatePauseStatus(b); d != nil { return d } - if d := validateTags(b); d != nil { - return d - } r := b.Config.Resources t := b.Config.Presets - prefix := t.Prefix + prefix := t.NamePrefix tags := toTagArray(t.Tags) // Jobs presets: Prefix, Tags, JobsMaxConcurrentRuns, TriggerPauseStatus @@ -170,22 +167,6 @@ func validatePauseStatus(b *bundle.Bundle) diag.Diagnostics { }} } -func validateTags(b *bundle.Bundle) diag.Diagnostics { - tags := b.Config.Presets.Tags - for key := range tags { - // Tags keys starting with a "~" are reserved. - // They may someday be used to indicate that a tag should be removed. - // For example {"~dev:": nil} could indicate that the "dev" tag should be removed. - if strings.HasPrefix(key, "~") { - return diag.Diagnostics{{ - Summary: "Invalid tag key: " + key, - Severity: diag.Error, - }} - } - } - return nil -} - // toTagArray converts a map of tags to an array of tags. // We sort tags so ensure stable ordering. func toTagArray(tags map[string]string) []Tag { diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 9832b2efe9..55df351e74 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -51,7 +51,7 @@ func TestApplyPresetsPrefix(t *testing.T) { }, }, Presets: config.Presets{ - Prefix: tt.prefix, + NamePrefix: tt.prefix, }, }, } @@ -195,26 +195,3 @@ func TestApplyPresetsJobsMaxConcurrentRuns(t *testing.T) { }) } } - -func TestApplyPresetsValidation(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Presets: config.Presets{ - TriggerPauseStatus: "invalid", - }, - }, - } - mutator := mutator.ApplyPresets() - diag := mutator.Apply(context.Background(), b) - require.Equal(t, "Invalid value for trigger_pause_status, should be PAUSED or UNPAUSED", diag[0].Summary) - - b = &bundle.Bundle{ - Config: config.Root{ - Presets: config.Presets{ - Tags: map[string]string{"~tilde": "tag"}, - }, - }, - } - diag = mutator.Apply(context.Background(), b) - require.Contains(t, diag[0].Summary, "Invalid tag key") -} diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 735953dc88..525371ecfd 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -36,8 +36,8 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { t := &b.Config.Presets shortName := b.Config.Workspace.CurrentUser.ShortName - if t.Prefix == "" { - t.Prefix = "[dev " + shortName + "] " + if t.NamePrefix == "" { + t.NamePrefix = "[dev " + shortName + "] " } if t.Tags == nil { @@ -63,15 +63,20 @@ func transformDevelopmentMode(ctx context.Context, b *bundle.Bundle) { } func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { - t := b.Config.Presets + p := b.Config.Presets u := b.Config.Workspace.CurrentUser - // Make sure everything is paused by default to avoid surprises - if t.TriggerPauseStatus == config.Unpaused { + // Make sure presets don't set the trigger status to UNPAUSED; + // this could be surprising since most users (and tools) expect triggers + // to be paused in development. + // (Note that there still is an exceptional case where users set the trigger + // status to UNPAUSED at the level of an individual object, whic hwas + // historically allowed.) + if p.TriggerPauseStatus == config.Unpaused { return diag.Diagnostics{{ Severity: diag.Error, Summary: "target with 'mode: development' cannot set trigger pause status to UNPAUSED by default", - Location: b.Config.GetLocation("transform.default_trigger_pause_status"), + Location: b.Config.GetLocation("presets.trigger_pause_status"), }} } @@ -79,11 +84,15 @@ func validateDevelopmentMode(b *bundle.Bundle) diag.Diagnostics { if path := findNonUserPath(b); path != "" { return diag.Errorf("%s must start with '~/' or contain the current username when using 'mode: development'", path) } - if t.Prefix != "" && !strings.Contains(t.Prefix, u.ShortName) && !strings.Contains(t.Prefix, u.UserName) { + if p.NamePrefix != "" && !strings.Contains(p.NamePrefix, u.ShortName) && !strings.Contains(p.NamePrefix, u.UserName) { + // Resources such as pipelines require a unique name, e.g. '[dev steve] my_pipeline'. + // For this reason we require the name prefix to contain the current username; + // it's a pitfall for users if they don't include it and later find out that + // only a single user can do development deployments. return diag.Diagnostics{{ Severity: diag.Error, Summary: "prefix should contain the current username or ${workspace.current_user.short_name} to ensure uniqueness when using 'mode: development'", - Location: b.Config.GetLocation("transform.prefix"), + Location: b.Config.GetLocation("presets.name_prefix"), }} } return nil diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 2c2f58bdcc..351d3c3603 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -237,7 +237,7 @@ func TestValidateDevelopmentMode(t *testing.T) { // Test with a bundle that has a prefix not containing the username or short name b = mockBundle(config.Development) - b.Config.Presets.Prefix = "[prod]" + b.Config.Presets.NamePrefix = "[prod]" diags = validateDevelopmentMode(b) require.Len(t, diags, 1) assert.Equal(t, diag.Error, diags[0].Severity) @@ -388,7 +388,7 @@ func TestDisableLockingDisabled(t *testing.T) { func TestPrefixAlreadySet(t *testing.T) { b := mockBundle(config.Development) - b.Config.Presets.Prefix = "custom_lennart_deploy_" + b.Config.Presets.NamePrefix = "custom_lennart_deploy_" m := bundle.Seq(ProcessTargetMode(), ApplyPresets()) diags := bundle.Apply(context.Background(), b, m) diff --git a/bundle/config/presets.go b/bundle/config/presets.go index 07712edc9d..61009a2521 100644 --- a/bundle/config/presets.go +++ b/bundle/config/presets.go @@ -4,8 +4,8 @@ const Paused = "PAUSED" const Unpaused = "UNPAUSED" type Presets struct { - // Prefix to prepend to all resource names. - Prefix string `json:"prefix,omitempty"` + // NamePrefix to prepend to all resource names. + NamePrefix string `json:"name_prefix,omitempty"` // PipelinesDevelopment is the default value for the development field of pipelines. PipelinesDevelopment *bool `json:"pipelines_development,omitempty"` diff --git a/bundle/config/root.go b/bundle/config/root.go index 4ca46461ec..cdb58dad4b 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -376,6 +376,9 @@ func (r *Root) MergeTargetOverrides(name string) error { // Merge `run_as`. This field must be overwritten if set, not merged. if v := target.Get("run_as"); v.Kind() != dyn.KindInvalid { root, err = dyn.Set(root, "run_as", v) + if err != nil { + return err + } } // Below, we're setting fields on the bundle key, so make sure it exists. diff --git a/bundle/tests/presets/databricks.yml b/bundle/tests/presets/databricks.yml index a4ec379ee7..d83d318015 100644 --- a/bundle/tests/presets/databricks.yml +++ b/bundle/tests/presets/databricks.yml @@ -10,7 +10,7 @@ presets: targets: dev: presets: - prefix: "myprefix" + name_prefix: "myprefix" pipelines_development: true trigger_pause_status: PAUSED jobs_max_concurrent_runs: 10 diff --git a/bundle/tests/presets_test.go b/bundle/tests/presets_test.go index f3e9a4b04c..e249fa60f7 100644 --- a/bundle/tests/presets_test.go +++ b/bundle/tests/presets_test.go @@ -10,7 +10,7 @@ import ( func TestPresetsDev(t *testing.T) { b := loadTarget(t, "./presets", "dev") - assert.Equal(t, "myprefix", b.Config.Presets.Prefix) + assert.Equal(t, "myprefix", b.Config.Presets.NamePrefix) assert.Equal(t, config.Paused, b.Config.Presets.TriggerPauseStatus) assert.Equal(t, 10, b.Config.Presets.JobsMaxConcurrentRuns) assert.Equal(t, true, *b.Config.Presets.PipelinesDevelopment) From fb902c9bc8adc54ab0edcaa3dc04aebafdf4b248 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 19 Jul 2024 21:47:40 +0200 Subject: [PATCH 21/23] Fix regression in main --- bundle/config/mutator/apply_presets.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/apply_presets.go b/bundle/config/mutator/apply_presets.go index 189dce4e7f..b63d7e8ef2 100644 --- a/bundle/config/mutator/apply_presets.go +++ b/bundle/config/mutator/apply_presets.go @@ -187,6 +187,14 @@ func toTagArray(tags map[string]string) []Tag { // We leave unicode letters and numbers but remove all "special characters." func normalizePrefix(prefix string) string { prefix = strings.ReplaceAll(prefix, "[", "") - prefix = strings.ReplaceAll(prefix, "] ", "_") - return textutil.NormalizeString(prefix) + prefix = strings.Trim(prefix, " ") + + // If the prefix ends with a ']', we add an underscore to the end. + // This makes sure that we get names like "dev_user_endpoint" instead of "dev_userendpoint" + suffix := "" + if strings.HasSuffix(prefix, "]") { + suffix = "_" + } + + return textutil.NormalizeString(prefix) + suffix } From b4564f2e5850ef710c04db3455709008207e9384 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 14 Aug 2024 16:59:51 +0200 Subject: [PATCH 22/23] Use bundle.Apply() for tests --- bundle/config/mutator/apply_presets_test.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/bundle/config/mutator/apply_presets_test.go b/bundle/config/mutator/apply_presets_test.go index 55df351e74..35dac1f7dc 100644 --- a/bundle/config/mutator/apply_presets_test.go +++ b/bundle/config/mutator/apply_presets_test.go @@ -56,8 +56,8 @@ func TestApplyPresetsPrefix(t *testing.T) { }, } - mutator := mutator.ApplyPresets() - diag := mutator.Apply(context.Background(), b) + ctx := context.Background() + diag := bundle.Apply(ctx, b, mutator.ApplyPresets()) if diag.HasError() { t.Fatalf("unexpected error: %v", diag) @@ -125,8 +125,8 @@ func TestApplyPresetsTags(t *testing.T) { }, } - mutator := mutator.ApplyPresets() - diag := mutator.Apply(context.Background(), b) + ctx := context.Background() + diag := bundle.Apply(ctx, b, mutator.ApplyPresets()) if diag.HasError() { t.Fatalf("unexpected error: %v", diag) @@ -183,9 +183,8 @@ func TestApplyPresetsJobsMaxConcurrentRuns(t *testing.T) { }, }, } - - mutator := mutator.ApplyPresets() - diag := mutator.Apply(context.Background(), b) + ctx := context.Background() + diag := bundle.Apply(ctx, b, mutator.ApplyPresets()) if diag.HasError() { t.Fatalf("unexpected error: %v", diag) From 70d8988391b4fd88f7670ddd16ac332ea0543a73 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 16 Aug 2024 09:27:58 +0200 Subject: [PATCH 23/23] Add assertion --- bundle/tests/presets_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/bundle/tests/presets_test.go b/bundle/tests/presets_test.go index e249fa60f7..5fcb5d95b5 100644 --- a/bundle/tests/presets_test.go +++ b/bundle/tests/presets_test.go @@ -23,5 +23,6 @@ func TestPresetsProd(t *testing.T) { b := loadTarget(t, "./presets", "prod") assert.Equal(t, false, *b.Config.Presets.PipelinesDevelopment) + assert.Equal(t, "finance", b.Config.Presets.Tags["team"]) assert.Equal(t, "true", b.Config.Presets.Tags["prod"]) }