-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for experiments in deployment bind/unbind commands #2434
base: main
Are you sure you want to change the base?
Conversation
@@ -118,6 +118,12 @@ func (r *Resources) FindResourceByConfigKey(key string) (ConfigResource, error) | |||
} | |||
} | |||
|
|||
for k := range r.Experiments { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we just extend it to all supported resources instead of doing 1 by 1? You can rewrite this code to use lib/dyn
to iterate over all existing resources and find the ones matching. So later when new resources are added they will automatically support bind/unbind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd propose to do it after all resources are enabled in bind/unbind
- the PRs still require individual integration tests to be introduced for each resource type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine too unless you have any ideas how maybe we can test bind for all the supported resources instead of writing individual tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think doing it case by case is fine here. It forces thinking about the semantics for bind/unbind for specific resources. For example, I think that for dashboards it might not be as clear cut because of the workspace path / ID duality.
@@ -118,6 +118,12 @@ func (r *Resources) FindResourceByConfigKey(key string) (ConfigResource, error) | |||
} | |||
} | |||
|
|||
for k := range r.Experiments { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine too unless you have any ideas how maybe we can test bind for all the supported resources instead of writing individual tests?
) | ||
|
||
func TestBindExperimentToExistingExperiment(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that existing tests for bind are integration tests but have you explored making new ones acceptance tests instead?
Changes
FindResourceByConfigKey
to return experiment resourcesWhy
This PR adds support for experiment resources in deployment operations, enabling users to:
databricks bundle deployment bind <myexperiment_key> <experiment_id>
databricks bundle deployment unbind <myexperiment_key>
Where:
myexperiment_key
is a resource key defined in the bundle's .yml fileexperiment_id
references an existing experiment in the Databricks workspaceThese capabilities allow for more flexible resource management of experiments within bundles.
Tests
Added a new integration test that tests bind and unbind methods together with bundle deployment and destruction.