From 01aa127c8ecfb892492aabbe37ff29ff0da9bfb6 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 27 Sep 2023 09:25:05 +0200 Subject: [PATCH 1/4] Use short name for tag value in development mode --- bundle/config/mutator/process_target_mode.go | 6 ++++-- .../config/mutator/process_target_mode_test.go | 17 ++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 93149ad049..5d7d47e7f7 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -39,7 +39,9 @@ func transformDevelopmentMode(b *bundle.Bundle) error { if r.Jobs[i].Tags == nil { r.Jobs[i].Tags = make(map[string]string) } - r.Jobs[i].Tags["dev"] = b.Config.Workspace.CurrentUser.DisplayName + // Note: tag values in jobs must match the following pattern: + // ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$ + r.Jobs[i].Tags["dev"] = b.Config.Workspace.CurrentUser.ShortName if r.Jobs[i].MaxConcurrentRuns == 0 { r.Jobs[i].MaxConcurrentRuns = developmentConcurrentRuns } @@ -74,7 +76,7 @@ func transformDevelopmentMode(b *bundle.Bundle) error { } else { r.Experiments[i].Name = dir + "/" + prefix + base } - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: b.Config.Workspace.CurrentUser.DisplayName}) + r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: b.Config.Workspace.CurrentUser.ShortName}) } for i := range r.ModelServingEndpoints { diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 4ea33c70b1..db0947a71e 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -68,14 +68,29 @@ func TestProcessTargetModeDevelopment(t *testing.T) { m := ProcessTargetMode() err := m.Apply(context.Background(), bundle) require.NoError(t, err) + + // Job 1 assert.Equal(t, "[dev lennart] job1", bundle.Config.Resources.Jobs["job1"].Name) + assert.Equal(t, bundle.Config.Resources.Jobs["job1"].Tags["dev"], "lennart") + + // Pipeline 1 assert.Equal(t, "[dev lennart] pipeline1", bundle.Config.Resources.Pipelines["pipeline1"].Name) + assert.True(t, bundle.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) + + // Experiment 1 assert.Equal(t, "/Users/lennart.kats@databricks.com/[dev lennart] experiment1", bundle.Config.Resources.Experiments["experiment1"].Name) + assert.Contains(t, bundle.Config.Resources.Experiments["experiment1"].Experiment.Tags, ml.ExperimentTag{Key: "dev", Value: "lennart"}) + + // Experiment 2 assert.Equal(t, "[dev lennart] experiment2", bundle.Config.Resources.Experiments["experiment2"].Name) + assert.Contains(t, bundle.Config.Resources.Experiments["experiment2"].Experiment.Tags, ml.ExperimentTag{Key: "dev", Value: "lennart"}) + + // Model 1 assert.Equal(t, "[dev lennart] model1", bundle.Config.Resources.Models["model1"].Name) + + // Model serving endpoint 1 assert.Equal(t, "dev_lennart_servingendpoint1", bundle.Config.Resources.ModelServingEndpoints["servingendpoint1"].Name) assert.Equal(t, "dev", bundle.Config.Resources.Experiments["experiment1"].Experiment.Tags[0].Key) - assert.True(t, bundle.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development) } func TestProcessTargetModeDefault(t *testing.T) { From f6c8f6d45dda2debddc050785807da459965942c Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 23:26:08 +0200 Subject: [PATCH 2/4] Leverage tags library to normalize tag value before using it --- bundle/bundle.go | 5 +++++ bundle/config/mutator/populate_current_user.go | 5 +++++ bundle/config/mutator/process_target_mode.go | 12 +++++++----- bundle/config/mutator/process_target_mode_test.go | 6 ++++++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 61bf1ffe4e..e162517978 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -19,6 +19,7 @@ import ( "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/tags" "github.com/databricks/cli/libs/terraform" "github.com/databricks/databricks-sdk-go" sdkconfig "github.com/databricks/databricks-sdk-go/config" @@ -46,6 +47,10 @@ type Bundle struct { // if true, we skip approval checks for deploy, destroy resources and delete // files AutoApprove bool + + // Tagging is used to normalize tag keys and values. + // The implementation depends on the cloud being targeted. + Tagging tags.Cloud } func Load(ctx context.Context, path string) (*Bundle, error) { diff --git a/bundle/config/mutator/populate_current_user.go b/bundle/config/mutator/populate_current_user.go index b604d67189..5b5d30969d 100644 --- a/bundle/config/mutator/populate_current_user.go +++ b/bundle/config/mutator/populate_current_user.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/tags" ) type populateCurrentUser struct{} @@ -35,6 +36,10 @@ func (m *populateCurrentUser) Apply(ctx context.Context, b *bundle.Bundle) error ShortName: getShortUserName(me.UserName), User: me, } + + // Configure tagging object now that we know we have a valid client. + b.Tagging = tags.ForCloud(w.Config) + return nil } diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 5d7d47e7f7..2f80fe3b30 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -32,16 +32,18 @@ func (m *processTargetMode) Name() string { func transformDevelopmentMode(b *bundle.Bundle) error { r := b.Config.Resources - prefix := "[dev " + b.Config.Workspace.CurrentUser.ShortName + "] " + 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) } - // Note: tag values in jobs must match the following pattern: - // ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$ - r.Jobs[i].Tags["dev"] = b.Config.Workspace.CurrentUser.ShortName + r.Jobs[i].Tags["dev"] = tagValue if r.Jobs[i].MaxConcurrentRuns == 0 { r.Jobs[i].MaxConcurrentRuns = developmentConcurrentRuns } @@ -76,7 +78,7 @@ func transformDevelopmentMode(b *bundle.Bundle) error { } else { r.Experiments[i].Name = dir + "/" + prefix + base } - r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: b.Config.Workspace.CurrentUser.ShortName}) + r.Experiments[i].Tags = append(r.Experiments[i].Tags, ml.ExperimentTag{Key: "dev", Value: tagValue}) } for i := range r.ModelServingEndpoints { diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index db0947a71e..852d49ffca 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -9,6 +9,8 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/tags" + sdkconfig "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/ml" @@ -59,6 +61,10 @@ func mockBundle(mode config.Mode) *bundle.Bundle { }, }, }, + // Use AWS implementation for testing. + Tagging: tags.ForCloud(&sdkconfig.Config{ + Host: "https://company.cloud.databricks.com", + }), } } From 9b9086fbf91ec544be46cf6a289e701b7c8ac1af Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 23:39:23 +0200 Subject: [PATCH 3/4] Fix fixture --- libs/template/renderer_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 070fc5d250..254b06cf8e 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -17,6 +17,7 @@ import ( "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/root" + "github.com/databricks/cli/libs/tags" "github.com/databricks/databricks-sdk-go" workspaceConfig "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/service/iam" @@ -66,6 +67,7 @@ func assertBuiltinTemplateValid(t *testing.T, settings map[string]any, target st // Apply initialize / validation mutators b.Config.Workspace.CurrentUser = &bundleConfig.User{User: cachedUser} + b.Tagging = tags.ForCloud(w.Config) b.WorkspaceClient() b.Config.Bundle.Terraform = &bundleConfig.Terraform{ ExecPath: "sh", From f5f6e6464276ff0e6b5c0fa36149e8ad8674961f Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 29 Sep 2023 13:21:10 +0200 Subject: [PATCH 4/4] Additional tests covering tag normalization for each cloud --- .../mutator/process_target_mode_test.go | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 852d49ffca..a0b2bac853 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -99,6 +99,48 @@ func TestProcessTargetModeDevelopment(t *testing.T) { assert.Equal(t, "dev", bundle.Config.Resources.Experiments["experiment1"].Experiment.Tags[0].Key) } +func TestProcessTargetModeDevelopmentTagNormalizationForAws(t *testing.T) { + bundle := mockBundle(config.Development) + bundle.Tagging = tags.ForCloud(&sdkconfig.Config{ + Host: "https://dbc-XXXXXXXX-YYYY.cloud.databricks.com/", + }) + + bundle.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" + err := ProcessTargetMode().Apply(context.Background(), bundle) + require.NoError(t, err) + + // Assert that tag normalization took place. + assert.Equal(t, "Hello world__", bundle.Config.Resources.Jobs["job1"].Tags["dev"]) +} + +func TestProcessTargetModeDevelopmentTagNormalizationForAzure(t *testing.T) { + bundle := mockBundle(config.Development) + bundle.Tagging = tags.ForCloud(&sdkconfig.Config{ + Host: "https://adb-xxx.y.azuredatabricks.net/", + }) + + bundle.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" + err := ProcessTargetMode().Apply(context.Background(), bundle) + require.NoError(t, err) + + // Assert that tag normalization took place (Azure allows more characters than AWS). + assert.Equal(t, "Héllö wörld?!", bundle.Config.Resources.Jobs["job1"].Tags["dev"]) +} + +func TestProcessTargetModeDevelopmentTagNormalizationForGcp(t *testing.T) { + bundle := mockBundle(config.Development) + bundle.Tagging = tags.ForCloud(&sdkconfig.Config{ + Host: "https://123.4.gcp.databricks.com/", + }) + + bundle.Config.Workspace.CurrentUser.ShortName = "Héllö wörld?!" + err := ProcessTargetMode().Apply(context.Background(), bundle) + require.NoError(t, err) + + // Assert that tag normalization took place. + assert.Equal(t, "Hello_world", bundle.Config.Resources.Jobs["job1"].Tags["dev"]) +} + func TestProcessTargetModeDefault(t *testing.T) { bundle := mockBundle("")