Skip to content

Commit

Permalink
Make bundle deploy work if no resources are defined (#767)
Browse files Browse the repository at this point in the history
## Changes

This PR sets "resource" to nil in the terraform representation if no
resources are defined in the bundle configuration. This solves two
problems:

1. Makes bundle deploy work without any resources specified. 
2. Previously if a `resources` block was removed after a deployment,
that would fail with an error. Now the resources would get destroyed as
expected.

Also removes `TerraformHasNoResources` which is no longer needed.

## Tests
New e2e tests.
  • Loading branch information
shreyas-goenka committed Sep 13, 2023
1 parent be55310 commit fe32c46
Show file tree
Hide file tree
Showing 12 changed files with 138 additions and 23 deletions.
4 changes: 0 additions & 4 deletions bundle/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 0 additions & 4 deletions bundle/deploy/terraform/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
10 changes: 8 additions & 2 deletions bundle/deploy/terraform/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 requires >= 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 {
Expand Down
22 changes: 11 additions & 11 deletions bundle/deploy/terraform/convert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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)
}
Expand All @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down
3 changes: 1 addition & 2 deletions bundle/deploy/terraform/write.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"properties": {
"unique_id": {
"type": "string",
"description": "Unique ID for job name"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
bundle:
name: deploy-then-remove

workspace:
root_path: "~/.bundle/{{.unique_id}}"

include:
- "./*.yml"
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
print("hello")
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
resources:
pipelines:
bar:
name: test-bundle-pipeline-{{.unique_id}}
libraries:
- notebook:
path: "./foo.py"
2 changes: 2 additions & 0 deletions internal/bundle/bundles/empty_bundle/databricks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bundle:
name: abc
55 changes: 55 additions & 0 deletions internal/bundle/deploy_then_remove_resources_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
37 changes: 37 additions & 0 deletions internal/bundle/empty_bundle_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package bundle

import (
"fmt"
"os"
"path/filepath"
"testing"

"github.com/databricks/cli/internal"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
)

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)

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)

t.Cleanup(func() {
err = destroyBundle(t, tmpDir)
require.NoError(t, err)
})
}

0 comments on commit fe32c46

Please sign in to comment.