From 8b5646ed80f3ffd78e49ca6f0d073432264b99fe Mon Sep 17 00:00:00 2001 From: monalisa Date: Tue, 12 Sep 2023 13:44:01 +0200 Subject: [PATCH 1/5] Make bundle deploy work with no resources --- bundle/deploy/terraform/state_push.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/bundle/deploy/terraform/state_push.go b/bundle/deploy/terraform/state_push.go index ae1d8b8b3e1..0c6da39374b 100644 --- a/bundle/deploy/terraform/state_push.go +++ b/bundle/deploy/terraform/state_push.go @@ -2,6 +2,8 @@ package terraform import ( "context" + "errors" + "io/fs" "os" "path/filepath" @@ -30,6 +32,12 @@ func (l *statePush) Apply(ctx context.Context, b *bundle.Bundle) error { // Expect the state file to live under dir. local, err := os.Open(filepath.Join(dir, TerraformStateFileName)) if err != nil { + if errors.Is(err, fs.ErrNotExist) && b.TerraformHasNoResources { + // A terraform state file is not created for bundle deployments with + // no resources defined. We skip the error for local state file + // not found in that case. + return nil + } return err } defer local.Close() From 382295f29d3bbd42bb9767394b9f65fbde368524 Mon Sep 17 00:00:00 2001 From: monalisa Date: Tue, 12 Sep 2023 13:51:05 +0200 Subject: [PATCH 2/5] - --- bundle/deploy/terraform/state_push.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bundle/deploy/terraform/state_push.go b/bundle/deploy/terraform/state_push.go index 0c6da39374b..d4fbfdc2429 100644 --- a/bundle/deploy/terraform/state_push.go +++ b/bundle/deploy/terraform/state_push.go @@ -33,9 +33,9 @@ func (l *statePush) Apply(ctx context.Context, b *bundle.Bundle) error { local, err := os.Open(filepath.Join(dir, TerraformStateFileName)) if err != nil { if errors.Is(err, fs.ErrNotExist) && b.TerraformHasNoResources { - // A terraform state file is not created for bundle deployments with - // no resources defined. We skip the error for local state file - // not found in that case. + // A terraform state file is not created for new bundle deployments + // with no resources defined. We ignore that a local terraform state + // file is absent if the bundle config has no resources defined. return nil } return err From 66fe1828be0a7370afa07e29cd025c85b3ed73d5 Mon Sep 17 00:00:00 2001 From: monalisa Date: Wed, 13 Sep 2023 18:23:00 +0200 Subject: [PATCH 3/5] - --- bundle/bundle.go | 4 --- bundle/deploy/terraform/apply.go | 4 --- bundle/deploy/terraform/convert.go | 10 ++++-- bundle/deploy/terraform/convert_test.go | 22 ++++++------- bundle/deploy/terraform/state_push.go | 8 ----- bundle/deploy/terraform/write.go | 3 +- .../bundles/empty_bundle/databricks.yml | 2 ++ internal/bundle/empty_bundle_test.go | 32 +++++++++++++++++++ 8 files changed, 54 insertions(+), 31 deletions(-) create mode 100644 internal/bundle/bundles/empty_bundle/databricks.yml create mode 100644 internal/bundle/empty_bundle_test.go diff --git a/bundle/bundle.go b/bundle/bundle.go index 4fc60539804..61bf1ffe4ec 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -38,10 +38,6 @@ type Bundle struct { // Stores an initialized copy of this bundle's Terraform wrapper. Terraform *tfexec.Terraform - // Indicates that the Terraform definition based on this bundle is empty, - // i.e. that it would deploy no resources. - TerraformHasNoResources bool - // Stores the locker responsible for acquiring/releasing a deployment lock. Locker *locker.Locker diff --git a/bundle/deploy/terraform/apply.go b/bundle/deploy/terraform/apply.go index 53cffbbaf68..ab868f765e1 100644 --- a/bundle/deploy/terraform/apply.go +++ b/bundle/deploy/terraform/apply.go @@ -16,10 +16,6 @@ func (w *apply) Name() string { } func (w *apply) Apply(ctx context.Context, b *bundle.Bundle) error { - if b.TerraformHasNoResources { - cmdio.LogString(ctx, "Note: there are no resources to deploy for this bundle") - return nil - } tf := b.Terraform if tf == nil { return fmt.Errorf("terraform not initialized") diff --git a/bundle/deploy/terraform/convert.go b/bundle/deploy/terraform/convert.go index 0956ea7bb5b..3a8a087387c 100644 --- a/bundle/deploy/terraform/convert.go +++ b/bundle/deploy/terraform/convert.go @@ -49,7 +49,7 @@ func convPermission(ac resources.Permission) schema.ResourcePermissionsAccessCon // // NOTE: THIS IS CURRENTLY A HACK. WE NEED A BETTER WAY TO // CONVERT TO/FROM TERRAFORM COMPATIBLE FORMAT. -func BundleToTerraform(config *config.Root) (*schema.Root, bool) { +func BundleToTerraform(config *config.Root) *schema.Root { tfroot := schema.NewRoot() tfroot.Provider = schema.NewProviders() tfroot.Resource = schema.NewResources() @@ -174,7 +174,13 @@ func BundleToTerraform(config *config.Root) (*schema.Root, bool) { } } - return tfroot, noResources + // We explicitly set "resource" to nil to omit it from a JSON encoding. + // This is required because the terraform CLI required >= 1 resources defined + // if the "resource" property is used in a .tf.json file. + if noResources { + tfroot.Resource = nil + } + return tfroot } func TerraformToBundle(state *tfjson.State, config *config.Root) error { diff --git a/bundle/deploy/terraform/convert_test.go b/bundle/deploy/terraform/convert_test.go index ad626606687..b6b29f35ab0 100644 --- a/bundle/deploy/terraform/convert_test.go +++ b/bundle/deploy/terraform/convert_test.go @@ -51,7 +51,7 @@ func TestConvertJob(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.Equal(t, "my job", out.Resource.Job["my_job"].Name) assert.Len(t, out.Resource.Job["my_job"].JobCluster, 1) assert.Equal(t, "https://github.com/foo/bar", out.Resource.Job["my_job"].GitSource.Url) @@ -79,7 +79,7 @@ func TestConvertJobPermissions(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.NotEmpty(t, out.Resource.Permissions["job_my_job"].JobId) assert.Len(t, out.Resource.Permissions["job_my_job"].AccessControl, 1) @@ -115,7 +115,7 @@ func TestConvertJobTaskLibraries(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.Equal(t, "my job", out.Resource.Job["my_job"].Name) require.Len(t, out.Resource.Job["my_job"].Task, 1) require.Len(t, out.Resource.Job["my_job"].Task[0].Library, 1) @@ -149,7 +149,7 @@ func TestConvertPipeline(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.Equal(t, "my pipeline", out.Resource.Pipeline["my_pipeline"].Name) assert.Len(t, out.Resource.Pipeline["my_pipeline"].Library, 2) assert.Nil(t, out.Data) @@ -173,7 +173,7 @@ func TestConvertPipelinePermissions(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.NotEmpty(t, out.Resource.Permissions["pipeline_my_pipeline"].PipelineId) assert.Len(t, out.Resource.Permissions["pipeline_my_pipeline"].AccessControl, 1) @@ -208,7 +208,7 @@ func TestConvertModel(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.Equal(t, "name", out.Resource.MlflowModel["my_model"].Name) assert.Equal(t, "description", out.Resource.MlflowModel["my_model"].Description) assert.Len(t, out.Resource.MlflowModel["my_model"].Tags, 2) @@ -237,7 +237,7 @@ func TestConvertModelPermissions(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.NotEmpty(t, out.Resource.Permissions["mlflow_model_my_model"].RegisteredModelId) assert.Len(t, out.Resource.Permissions["mlflow_model_my_model"].AccessControl, 1) @@ -261,7 +261,7 @@ func TestConvertExperiment(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.Equal(t, "name", out.Resource.MlflowExperiment["my_experiment"].Name) assert.Nil(t, out.Data) } @@ -284,7 +284,7 @@ func TestConvertExperimentPermissions(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.NotEmpty(t, out.Resource.Permissions["mlflow_experiment_my_experiment"].ExperimentId) assert.Len(t, out.Resource.Permissions["mlflow_experiment_my_experiment"].AccessControl, 1) @@ -327,7 +327,7 @@ func TestConvertModelServing(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) resource := out.Resource.ModelServing["my_model_serving_endpoint"] assert.Equal(t, "name", resource.Name) assert.Equal(t, "model_name", resource.Config.ServedModels[0].ModelName) @@ -357,7 +357,7 @@ func TestConvertModelServingPermissions(t *testing.T) { }, } - out, _ := BundleToTerraform(&config) + out := BundleToTerraform(&config) assert.NotEmpty(t, out.Resource.Permissions["model_serving_my_model_serving_endpoint"].ServingEndpointId) assert.Len(t, out.Resource.Permissions["model_serving_my_model_serving_endpoint"].AccessControl, 1) diff --git a/bundle/deploy/terraform/state_push.go b/bundle/deploy/terraform/state_push.go index d4fbfdc2429..ae1d8b8b3e1 100644 --- a/bundle/deploy/terraform/state_push.go +++ b/bundle/deploy/terraform/state_push.go @@ -2,8 +2,6 @@ package terraform import ( "context" - "errors" - "io/fs" "os" "path/filepath" @@ -32,12 +30,6 @@ func (l *statePush) Apply(ctx context.Context, b *bundle.Bundle) error { // Expect the state file to live under dir. local, err := os.Open(filepath.Join(dir, TerraformStateFileName)) if err != nil { - if errors.Is(err, fs.ErrNotExist) && b.TerraformHasNoResources { - // A terraform state file is not created for new bundle deployments - // with no resources defined. We ignore that a local terraform state - // file is absent if the bundle config has no resources defined. - return nil - } return err } defer local.Close() diff --git a/bundle/deploy/terraform/write.go b/bundle/deploy/terraform/write.go index eca79ad21be..b53f9069d2d 100644 --- a/bundle/deploy/terraform/write.go +++ b/bundle/deploy/terraform/write.go @@ -21,8 +21,7 @@ func (w *write) Apply(ctx context.Context, b *bundle.Bundle) error { return err } - root, noResources := BundleToTerraform(&b.Config) - b.TerraformHasNoResources = noResources + root := BundleToTerraform(&b.Config) f, err := os.Create(filepath.Join(dir, "bundle.tf.json")) if err != nil { return err diff --git a/internal/bundle/bundles/empty_bundle/databricks.yml b/internal/bundle/bundles/empty_bundle/databricks.yml new file mode 100644 index 00000000000..efc6278207e --- /dev/null +++ b/internal/bundle/bundles/empty_bundle/databricks.yml @@ -0,0 +1,2 @@ +bundle: + name: abc diff --git a/internal/bundle/empty_bundle_test.go b/internal/bundle/empty_bundle_test.go new file mode 100644 index 00000000000..e8954e951df --- /dev/null +++ b/internal/bundle/empty_bundle_test.go @@ -0,0 +1,32 @@ +package bundle + +import ( + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/internal" + "github.com/stretchr/testify/require" +) + +func TestAccEmptyBundleDeploy(t *testing.T) { + env := internal.GetEnvOrSkipTest(t, "CLOUD_ENV") + t.Log(env) + + tmpDir := t.TempDir() + f, err := os.Create(filepath.Join(tmpDir, "databricks.yml")) + require.NoError(t, err) + + _, err = f.WriteString( + `bundle: + name: abc`) + require.NoError(t, err) + + err = deployBundle(t, tmpDir) + require.NoError(t, err) + + t.Cleanup(func() { + err = destroyBundle(t, tmpDir) + require.NoError(t, err) + }) +} From 2455a562ec37549bd7a94de258833a8210face04 Mon Sep 17 00:00:00 2001 From: monalisa Date: Wed, 13 Sep 2023 19:08:42 +0200 Subject: [PATCH 4/5] added other integratoin test --- .../databricks_template_schema.json | 8 +++ .../template/databricks.yml.tmpl | 8 +++ .../template/foo.py | 1 + .../template/resources.yml.tmpl | 7 +++ .../deploy_then_remove_resources_test.go | 55 +++++++++++++++++++ internal/bundle/empty_bundle_test.go | 11 +++- 6 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 internal/bundle/bundles/deploy_then_remove_resources/databricks_template_schema.json create mode 100644 internal/bundle/bundles/deploy_then_remove_resources/template/databricks.yml.tmpl create mode 100644 internal/bundle/bundles/deploy_then_remove_resources/template/foo.py create mode 100644 internal/bundle/bundles/deploy_then_remove_resources/template/resources.yml.tmpl create mode 100644 internal/bundle/deploy_then_remove_resources_test.go diff --git a/internal/bundle/bundles/deploy_then_remove_resources/databricks_template_schema.json b/internal/bundle/bundles/deploy_then_remove_resources/databricks_template_schema.json new file mode 100644 index 00000000000..cfed842cbdd --- /dev/null +++ b/internal/bundle/bundles/deploy_then_remove_resources/databricks_template_schema.json @@ -0,0 +1,8 @@ +{ + "properties": { + "unique_id": { + "type": "string", + "description": "Unique ID for job name" + } + } +} diff --git a/internal/bundle/bundles/deploy_then_remove_resources/template/databricks.yml.tmpl b/internal/bundle/bundles/deploy_then_remove_resources/template/databricks.yml.tmpl new file mode 100644 index 00000000000..c0e840c857e --- /dev/null +++ b/internal/bundle/bundles/deploy_then_remove_resources/template/databricks.yml.tmpl @@ -0,0 +1,8 @@ +bundle: + name: deploy-then-remove + +workspace: + root_path: "~/.bundle/{{.unique_id}}" + +include: + - "./*.yml" diff --git a/internal/bundle/bundles/deploy_then_remove_resources/template/foo.py b/internal/bundle/bundles/deploy_then_remove_resources/template/foo.py new file mode 100644 index 00000000000..11b15b1a458 --- /dev/null +++ b/internal/bundle/bundles/deploy_then_remove_resources/template/foo.py @@ -0,0 +1 @@ +print("hello") diff --git a/internal/bundle/bundles/deploy_then_remove_resources/template/resources.yml.tmpl b/internal/bundle/bundles/deploy_then_remove_resources/template/resources.yml.tmpl new file mode 100644 index 00000000000..b74344e4ca0 --- /dev/null +++ b/internal/bundle/bundles/deploy_then_remove_resources/template/resources.yml.tmpl @@ -0,0 +1,7 @@ +resources: + pipelines: + bar: + name: test-bundle-pipeline-{{.unique_id}} + libraries: + - notebook: + path: "./foo.py" diff --git a/internal/bundle/deploy_then_remove_resources_test.go b/internal/bundle/deploy_then_remove_resources_test.go new file mode 100644 index 00000000000..73860593c0f --- /dev/null +++ b/internal/bundle/deploy_then_remove_resources_test.go @@ -0,0 +1,55 @@ +package bundle + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/databricks/cli/internal" + "github.com/databricks/databricks-sdk-go" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAccBundleDeployThenRemoveResources(t *testing.T) { + env := internal.GetEnvOrSkipTest(t, "CLOUD_ENV") + t.Log(env) + + uniqueId := uuid.New().String() + bundleRoot, err := initTestTemplate(t, "deploy_then_remove_resources", map[string]any{ + "unique_id": uniqueId, + }) + require.NoError(t, err) + + // deploy pipeline + err = deployBundle(t, bundleRoot) + require.NoError(t, err) + + w, err := databricks.NewWorkspaceClient() + require.NoError(t, err) + + // assert pipeline is created + pipelineName := "test-bundle-pipeline-" + uniqueId + pipeline, err := w.Pipelines.GetByName(context.Background(), pipelineName) + require.NoError(t, err) + assert.Equal(t, pipeline.Name, pipelineName) + + // delete resources.yml + err = os.Remove(filepath.Join(bundleRoot, "resources.yml")) + require.NoError(t, err) + + // deploy again + err = deployBundle(t, bundleRoot) + require.NoError(t, err) + + // assert pipeline is deleted + _, err = w.Pipelines.GetByName(context.Background(), pipelineName) + assert.ErrorContains(t, err, "does not exist") + + t.Cleanup(func() { + err = destroyBundle(t, bundleRoot) + require.NoError(t, err) + }) +} diff --git a/internal/bundle/empty_bundle_test.go b/internal/bundle/empty_bundle_test.go index e8954e951df..9b39368f459 100644 --- a/internal/bundle/empty_bundle_test.go +++ b/internal/bundle/empty_bundle_test.go @@ -1,11 +1,13 @@ package bundle import ( + "fmt" "os" "path/filepath" "testing" "github.com/databricks/cli/internal" + "github.com/google/uuid" "github.com/stretchr/testify/require" ) @@ -13,15 +15,18 @@ func TestAccEmptyBundleDeploy(t *testing.T) { env := internal.GetEnvOrSkipTest(t, "CLOUD_ENV") t.Log(env) + // create empty bundle tmpDir := t.TempDir() f, err := os.Create(filepath.Join(tmpDir, "databricks.yml")) require.NoError(t, err) - _, err = f.WriteString( - `bundle: - name: abc`) + bundleRoot := fmt.Sprintf(`bundle: + name: %s`, uuid.New().String()) + _, err = f.WriteString(bundleRoot) require.NoError(t, err) + f.Close() + // deploy empty bundle err = deployBundle(t, tmpDir) require.NoError(t, err) From e2dd8c6bcda3550382e2afaf2e317c2f0eac4413 Mon Sep 17 00:00:00 2001 From: monalisa Date: Wed, 13 Sep 2023 19:09:46 +0200 Subject: [PATCH 5/5] - --- bundle/deploy/terraform/convert.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/deploy/terraform/convert.go b/bundle/deploy/terraform/convert.go index 3a8a087387c..7d95e719d5d 100644 --- a/bundle/deploy/terraform/convert.go +++ b/bundle/deploy/terraform/convert.go @@ -175,7 +175,7 @@ func BundleToTerraform(config *config.Root) *schema.Root { } // We explicitly set "resource" to nil to omit it from a JSON encoding. - // This is required because the terraform CLI required >= 1 resources defined + // This is required because the terraform CLI requires >= 1 resources defined // if the "resource" property is used in a .tf.json file. if noResources { tfroot.Resource = nil