-
Notifications
You must be signed in to change notification settings - Fork 134
direct: Support bind/unbind #4279
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
Conversation
|
Commit: 244d468
19 interesting tests: 10 KNOWN, 7 RECOVERED, 1 SKIP, 1 flaky
Top 43 slowest tests (at least 2 minutes):
|
1033a38 to
7bd6fab
Compare
bundle/phases/bind.go
Outdated
| func jsonDump(v any) string { | ||
| b, err := json.Marshal(v) | ||
| if err != nil { | ||
| return fmt.Sprintf("value=%v marshall error=%s", v, err) |
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.
This should throw an internal error, not be limited to a stdout.
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 changed it to warning. There error here is unlikely, hard to address by users and most likely does not affect operation. The whole rendering of changes can be considered optional.
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.
It should still return an error though here. If there's an internal error, that gives users a call to action to report to us.
The risk is someone deploying directly deploying with the --auto-approve flag after running bind.
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.
We ask the users whether to proceed and there's always a risk associated with --auto-approve.
| ### CLI | ||
|
|
||
| ### Bundles | ||
| * engine/direct: Support bind & unbind. ([#4279](https://github.com/databricks/cli/pull/4279)) |
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.
Changelog entry could be a bit more readbale
| * engine/direct: Support bind & unbind. ([#4279](https://github.com/databricks/cli/pull/4279)) | |
| * Add support for bind and unbind when using the direct deployment engine ([#4279](https://github.com/databricks/cli/pull/4279)) |
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 use "engine/direct: " prefix in other entries, easier to scan for all changes.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "name": "/Workspace/Users/[USERNAME]/test-experiment[UNIQUE_NAME]", | |||
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.
What's the reason for this diff?
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.
no idea #4285
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.
Tried it out. The backend removes the /Workspace prefix. Does this also occur on cloud? Based on my testing both TF and DABs create the experiment with the /Workspace prefix included:
From TF:
2026-01-14T18:21:58.341+0100 [DEBUG] provider.terraform-provider-databricks_v1.97.0: POST /api/2.0/mlflow/experiments/create
> {
> "name": "/Workspace/Users/shreyas.goenka@databricks.com/test-experiment"
> }
< HTTP/2.0 200 OK
< {
< "experiment_id": "1168670625037178"
< }: @module=databricks tf_mux_provider=tf5to6server.v5tov6Server @caller=/home/runner/work/terraform-provider-databricks/terraform-provider-databricks/logger/logger.go:38 tf_provider_addr=registry.terraform.io/databricks/databricks tf_req_id=07161d19-0a80-7fd3-31d5-26418985a129 tf_rpc=ConfigureProvider timestamp="2026-01-14T18:21:58.340+0100"
2026-01-14T18:21:58.555+0100 [DEBUG] provider.terraform-provider-databricks_v1.97.0: GET /api/2.0/mlflow/experiments/get?experiment_id=1168670625037178
< HTTP/2.0 200 OK
< {
< "experiment": {
< "artifact_location": "dbfs:/databricks/mlflow-tracking/1168670625037178",
< "creation_time": 1768411318324,
< "experiment_id": "1168670625037178",
< "last_update_time": 1768411318324,
< "lifecycle_stage": "active",
< "name": "/Users/shreyas.goenka@databricks.com/test-experiment",
< "tags": [
< {
< "key": "mlflow.ownerId",
< "value": "7315180364335929"
< },
< {
< "key": "mlflow.experiment.sourceName",
< "value": "/Users/shreyas.goenka@databricks.com/test-experiment"
< },
< {
< "key": "mlflow.ownerId",
< "value": "7315180364335929"
< },
< {
< "key": "mlflow.ownerEmail",
< "value": "shreyas.goenka@databricks.com"
< },
< {
< "key": "mlflow.experimentType",
< "value": "MLFLOW_EXPERIMENT"
< }
< ]
< }
< }
From DABs:
{
"experiment": {
"artifact_location":"dbfs:/databricks/mlflow-tracking/1168670625040732",
"creation_time":1768411490689,
"experiment_id":"1168670625040732",
"last_update_time":1768411490689,
"lifecycle_stage":"active",
"name":"/Users/shreyas.goenka@databricks.com/test-experiment-bundle",
"tags": [
{
"key":"mlflow.ownerId",
"value":"7315180364335929"
},
{
"key":"mlflow.experiment.sourceName",
"value":"/Users/shreyas.goenka@databricks.com/test-experiment-bundle"
},
{
"key":"mlflow.ownerId",
"value":"7315180364335929"
},
{
"key":"mlflow.ownerEmail",
"value":"shreyas.goenka@databricks.com"
},
{
"key":"mlflow.experimentType",
"value":"MLFLOW_EXPERIMENT"
}
]
}
}
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.
The real backend does remove the prefix. I'll disable this test on Cloud for now.
| "queue": { | ||
| "enabled":true | ||
| }, | ||
| "run_as": { |
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.
suggestion: You can set run_as in the test and then keep the test output same for direct and TF. I presume this is happening because of a default on the TF side?
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'm hesitant to tweak configs to remove these differences, it's good to be aware of them.
| "enabled":true | ||
| }, | ||
| "run_as": { | ||
| "user_name":"[USERNAME]" |
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.
Same as the previous one, up to you though.
| Ignore = [".databricks"] | ||
|
|
||
| # Pipelines is marked for update, not recreation on direct. | ||
| EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform"] |
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.
Why is it not a recreate on direct as well? In TF this is the reason given:
~ catalog = "old_catalog" -> "main" # forces replacement
This should hold true for direct as well.
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.
The test used incorrect configuration (on cloud, it gives Error: Spec contains both storage: old_storage and catalog: old_catalog, only one of storage or catalog can be specified.).
I've enabled this test on cloud, fixed the configuration and now it works on direct as well.
update secret_scopes tests add sort_acls_json update secret_acls.go support groups creation support groups - cont script clean up bind/unbidn direct: Remove unnecessary error wrapping We already have "reading config" prefix at caller side. Noticed this error message: Error: reading config: reading config: unsupported resource type: quality_monitors exclude quality_monitors add auto-approve check add volume_id to testserver wip fix add nolint check if already managed update update WIP WIP
8e80b7a to
6aee674
Compare
6aee674 to
60dade4
Compare
|
Commit: cc6f143
26 interesting tests: 10 KNOWN, 8 RECOVERED, 7 flaky, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
## Release v0.284.0 ### Bundles * Pass additional Azure DevOps `SYSTEM_*` environment variables to Terraform for OIDC authentication: `SYSTEM_COLLECTIONURI`, `SYSTEM_DEFINITIONID`, `SYSTEM_HOSTTYPE`, `SYSTEM_JOBID`, `SYSTEM_TEAMPROJECT` ([#4318](#4318)) * Add support for valueFrom property (similar to app.yaml) inside Apps config field in bundle configuration ([#4297](#4297)) * engine/direct: Support bind & unbind. ([#4279](#4279)) * engine/direct: Ignore changes between nulls and empty slices/maps (([#4313](#4313))) * engine/direct: Ignore changes between nulls and empty structs (([#4338](#4338))) * On terraform `bundle plan -o json` will no longer include plan_version key, it's intended for direct engine only (([#4314](#4314)))
Why
Closes #4061
Tests
Existing tests all pass.
bind/experiments test shows difference in final result, this is unrelated to bind and issue with experiments resource.