diff --git a/.nextchanges/bundles/schema-comment-nothing-to-update.md b/.nextchanges/bundles/schema-comment-nothing-to-update.md new file mode 100644 index 0000000000..48b969b220 --- /dev/null +++ b/.nextchanges/bundles/schema-comment-nothing-to-update.md @@ -0,0 +1 @@ +Fixed `bundle deploy` on the direct engine failing with `Nothing to update`, or reporting a change it never applied, when a schema, catalog, volume, registered model or external location field was removed from the configuration or set outside the bundle. The update now sends the fields the plan reports as cleared instead of dropping them. diff --git a/acceptance/bundle/resources/catalogs/comment_out_of_band/databricks.yml.tmpl b/acceptance/bundle/resources/catalogs/comment_out_of_band/databricks.yml.tmpl new file mode 100644 index 0000000000..b7fa6d6d95 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/databricks.yml.tmpl @@ -0,0 +1,7 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + catalogs: + catalog1: + name: test-catalog-$UNIQUE_NAME diff --git a/acceptance/bundle/resources/catalogs/comment_out_of_band/out.test.toml b/acceptance/bundle/resources/catalogs/comment_out_of_band/out.test.toml new file mode 100644 index 0000000000..c502b28221 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/out.test.toml @@ -0,0 +1,2 @@ +Cloud = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/catalogs/comment_out_of_band/output.txt b/acceptance/bundle/resources/catalogs/comment_out_of_band/output.txt new file mode 100644 index 0000000000..6ff82f588e --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/output.txt @@ -0,0 +1,57 @@ + +=== Deploy a catalog that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created catalogs.catalog1 +Files: 0 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, the way Catalog Explorer does +=== The remote comment is drift, so the plan updates the catalog +>>> [CLI] bundle plan --output json +{ + "action": "update", + "remote": "set outside the bundle" +} + +=== Redeploy clears the comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated catalogs.catalog1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +=== The comment is gone +>>> [CLI] api get /api/2.1/unity-catalog/catalogs/test-catalog-[UNIQUE_NAME] +"" + +=== Redeploy is a no-op +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 1 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.catalogs.catalog1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 1 deleted + +=== The UpdateCatalog payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/catalogs +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/catalogs/test-catalog-[UNIQUE_NAME]", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/catalogs/test-catalog-[UNIQUE_NAME]", + "body": { + "comment": "" + } +} diff --git a/acceptance/bundle/resources/catalogs/comment_out_of_band/script b/acceptance/bundle/resources/catalogs/comment_out_of_band/script new file mode 100644 index 0000000000..6f1a938177 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/script @@ -0,0 +1,32 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +CATALOG="test-catalog-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it has to run after the last +# request the test makes, which is the destroy below. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateCatalog payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/catalogs +} +trap cleanup EXIT + +title "Deploy a catalog that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, the way Catalog Explorer does" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/catalogs/$CATALOG" --json '{"comment":"set outside the bundle"}' > /dev/null + +title "The remote comment is drift, so the plan updates the catalog" +trace $CLI bundle plan --output json | jq '.plan[].changes.comment' + +title "Redeploy clears the comment" +trace $CLI bundle deploy + +title "The comment is gone" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/catalogs/$CATALOG" | jq '.comment' + +title "Redeploy is a no-op" +trace $CLI bundle deploy diff --git a/acceptance/bundle/resources/catalogs/comment_out_of_band/test.toml b/acceptance/bundle/resources/catalogs/comment_out_of_band/test.toml new file mode 100644 index 0000000000..78d1d7f450 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/test.toml @@ -0,0 +1,10 @@ +Cloud = true + +# UpdateCatalog payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/external_locations/comment_cleared/databricks.yml b/acceptance/bundle/resources/external_locations/comment_cleared/databricks.yml new file mode 100644 index 0000000000..f7cd8e3811 --- /dev/null +++ b/acceptance/bundle/resources/external_locations/comment_cleared/databricks.yml @@ -0,0 +1,10 @@ +bundle: + name: test-bundle + +resources: + external_locations: + location1: + name: myloc + url: s3://test-bucket/path + credential_name: test_storage_credential + comment: declared in the bundle diff --git a/acceptance/bundle/resources/external_locations/comment_cleared/out.test.toml b/acceptance/bundle/resources/external_locations/comment_cleared/out.test.toml new file mode 100644 index 0000000000..0938e67898 --- /dev/null +++ b/acceptance/bundle/resources/external_locations/comment_cleared/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/external_locations/comment_cleared/output.txt b/acceptance/bundle/resources/external_locations/comment_cleared/output.txt new file mode 100644 index 0000000000..0ba9855270 --- /dev/null +++ b/acceptance/bundle/resources/external_locations/comment_cleared/output.txt @@ -0,0 +1,42 @@ + +=== Deploy with a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created external_locations.location1 +Files: 0 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +=== Remove the comment from the configuration and redeploy +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Updated external_locations.location1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +=== The comment the bundle no longer declares +>>> [CLI] api get /api/2.1/unity-catalog/external-locations/myloc +"" + +=== Redeploy +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.external_locations.location1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Destroy: 1 deleted + +=== The UpdateExternalLocation payloads the deploys sent +>>> print_requests.py --method PATCH //external-locations +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/external-locations/myloc", + "body": { + "comment": "", + "credential_name": "test_storage_credential", + "url": "s3://test-bucket/path" + } +} diff --git a/acceptance/bundle/resources/external_locations/comment_cleared/script b/acceptance/bundle/resources/external_locations/comment_cleared/script new file mode 100644 index 0000000000..f5a82dc955 --- /dev/null +++ b/acceptance/bundle/resources/external_locations/comment_cleared/script @@ -0,0 +1,23 @@ +echo "*" > .gitignore + +# print_requests.py consumes out.requests.txt, so it runs after the last request. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateExternalLocation payloads the deploys sent" + trace print_requests.py --method PATCH //external-locations +} +trap cleanup EXIT + +title "Deploy with a comment" +trace $CLI bundle deploy + +title "Remove the comment from the configuration and redeploy" +update_file.py databricks.yml 'comment: declared in the bundle' '' +trace $CLI bundle deploy + +title "The comment the bundle no longer declares" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/external-locations/myloc" | jq '.comment' + +title "Redeploy" +trace $CLI bundle plan diff --git a/acceptance/bundle/resources/external_locations/comment_cleared/test.toml b/acceptance/bundle/resources/external_locations/comment_cleared/test.toml new file mode 100644 index 0000000000..31bed34206 --- /dev/null +++ b/acceptance/bundle/resources/external_locations/comment_cleared/test.toml @@ -0,0 +1,14 @@ +# External locations need real storage credentials with cloud IAM setup, so this +# only runs against the mock server (same as the parent test). +Cloud = false +# The parent test disables request recording; this one asserts on the payload. +RecordRequests = true + +# UpdateExternalLocation payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/external_locations/output.txt b/acceptance/bundle/resources/external_locations/output.txt index 6a6b8253ea..d228250cc5 100644 --- a/acceptance/bundle/resources/external_locations/output.txt +++ b/acceptance/bundle/resources/external_locations/output.txt @@ -14,7 +14,7 @@ Created catalogs.test_catalog Created catalogs.test_catalog.grants Created external_locations.test_location Created external_locations.test_location.grants -Files: 5 uploaded, 0 deleted +Files: 9 uploaded, 0 deleted Resources: 4 created, 0 changed, 0 deleted, 0 unchanged === Assert the catalog is created with grants diff --git a/acceptance/bundle/resources/registered_models/comment_cleared/databricks.yml.tmpl b/acceptance/bundle/resources/registered_models/comment_cleared/databricks.yml.tmpl new file mode 100644 index 0000000000..de63e932c4 --- /dev/null +++ b/acceptance/bundle/resources/registered_models/comment_cleared/databricks.yml.tmpl @@ -0,0 +1,14 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + schema1: + catalog_name: main + name: test-schema-$UNIQUE_NAME + registered_models: + model1: + name: test-model-$UNIQUE_NAME + catalog_name: main + schema_name: ${resources.schemas.schema1.name} + comment: declared in the bundle diff --git a/acceptance/bundle/resources/registered_models/comment_cleared/out.test.toml b/acceptance/bundle/resources/registered_models/comment_cleared/out.test.toml new file mode 100644 index 0000000000..c502b28221 --- /dev/null +++ b/acceptance/bundle/resources/registered_models/comment_cleared/out.test.toml @@ -0,0 +1,2 @@ +Cloud = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/registered_models/comment_cleared/output.txt b/acceptance/bundle/resources/registered_models/comment_cleared/output.txt new file mode 100644 index 0000000000..eb5310e465 --- /dev/null +++ b/acceptance/bundle/resources/registered_models/comment_cleared/output.txt @@ -0,0 +1,48 @@ + +=== Deploy with a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created registered_models.model1 +Created schemas.schema1 +Files: 0 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +=== Remove the comment from the configuration and redeploy +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated registered_models.model1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 1 unchanged + +=== The comment the bundle no longer declares +>>> [CLI] api get /api/2.1/unity-catalog/models/main.test-schema-[UNIQUE_NAME].test-model-[UNIQUE_NAME] +"" + +=== Redeploy +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.registered_models.model1 + delete resources.schemas.schema1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.schema1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 2 deleted + +=== The UpdateRegisteredModel payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/models +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/models/main.test-schema-[UNIQUE_NAME].test-model-[UNIQUE_NAME]", + "body": { + "catalog_name": "main", + "comment": "", + "name": "test-model-[UNIQUE_NAME]", + "schema_name": "test-schema-[UNIQUE_NAME]" + } +} diff --git a/acceptance/bundle/resources/registered_models/comment_cleared/script b/acceptance/bundle/resources/registered_models/comment_cleared/script new file mode 100644 index 0000000000..f466c7f642 --- /dev/null +++ b/acceptance/bundle/resources/registered_models/comment_cleared/script @@ -0,0 +1,26 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +MODEL="main.test-schema-${UNIQUE_NAME}.test-model-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it runs after the last request. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateRegisteredModel payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/models +} +trap cleanup EXIT + +title "Deploy with a comment" +trace $CLI bundle deploy + +title "Remove the comment from the configuration and redeploy" +update_file.py databricks.yml 'comment: declared in the bundle' '' +trace $CLI bundle deploy + +title "The comment the bundle no longer declares" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/models/$MODEL" | jq '.comment' + +title "Redeploy" +trace $CLI bundle plan diff --git a/acceptance/bundle/resources/registered_models/comment_cleared/test.toml b/acceptance/bundle/resources/registered_models/comment_cleared/test.toml new file mode 100644 index 0000000000..3ad98d5764 --- /dev/null +++ b/acceptance/bundle/resources/registered_models/comment_cleared/test.toml @@ -0,0 +1,10 @@ +Cloud = true + +# UpdateRegisteredModel payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/schemas/comment_out_of_band/databricks.yml.tmpl b/acceptance/bundle/resources/schemas/comment_out_of_band/databricks.yml.tmpl new file mode 100644 index 0000000000..50720b6256 --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/databricks.yml.tmpl @@ -0,0 +1,8 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + schema1: + catalog_name: main + name: test-schema-$UNIQUE_NAME diff --git a/acceptance/bundle/resources/schemas/comment_out_of_band/out.test.toml b/acceptance/bundle/resources/schemas/comment_out_of_band/out.test.toml new file mode 100644 index 0000000000..c502b28221 --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/out.test.toml @@ -0,0 +1,2 @@ +Cloud = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/schemas/comment_out_of_band/output.txt b/acceptance/bundle/resources/schemas/comment_out_of_band/output.txt new file mode 100644 index 0000000000..bffdb9c8d3 --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/output.txt @@ -0,0 +1,88 @@ + +=== Deploy a schema that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created schemas.schema1 +Files: 0 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, the way Catalog Explorer does +=== The remote comment is drift, so the plan updates the schema +>>> [CLI] bundle plan --output json +{ + "action": "update", + "remote": "set outside the bundle" +} + +=== Redeploy clears the comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated schemas.schema1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +=== The comment is gone +>>> [CLI] api get /api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME] +"" + +=== Redeploy is a no-op +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 1 unchanged + +=== The same holds for a non-string field set out of band +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated schemas.schema1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +>>> [CLI] api get /api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME] +0 + +=== And it converges +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.schema1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.schema1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 1 deleted + +=== The UpdateSchema payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/schemas +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME]", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME]", + "body": { + "comment": "" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME]", + "body": { + "custom_max_retention_hours": 240 + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME]", + "body": { + "custom_max_retention_hours": 0 + } +} diff --git a/acceptance/bundle/resources/schemas/comment_out_of_band/script b/acceptance/bundle/resources/schemas/comment_out_of_band/script new file mode 100644 index 0000000000..3c2514950e --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/script @@ -0,0 +1,41 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +SCHEMA="main.test-schema-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it has to run after the last +# request the test makes, which is the destroy below. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateSchema payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/schemas +} +trap cleanup EXIT + +title "Deploy a schema that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, the way Catalog Explorer does" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/schemas/$SCHEMA" --json '{"comment":"set outside the bundle"}' > /dev/null + +title "The remote comment is drift, so the plan updates the schema" +trace $CLI bundle plan --output json | jq '.plan[].changes.comment' + +title "Redeploy clears the comment" +trace $CLI bundle deploy + +title "The comment is gone" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/schemas/$SCHEMA" | jq '.comment' + +title "Redeploy is a no-op" +trace $CLI bundle deploy + +# 240h (10 days) is inside UC's valid 7-30 day window; smaller values are rejected. +title "The same holds for a non-string field set out of band" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/schemas/$SCHEMA" --json '{"custom_max_retention_hours":240}' > /dev/null +trace $CLI bundle deploy +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/schemas/$SCHEMA" | jq '.custom_max_retention_hours' + +title "And it converges" +trace $CLI bundle plan | contains.py "Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged" diff --git a/acceptance/bundle/resources/schemas/comment_out_of_band/test.toml b/acceptance/bundle/resources/schemas/comment_out_of_band/test.toml new file mode 100644 index 0000000000..65219bba62 --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/test.toml @@ -0,0 +1,10 @@ +Cloud = true + +# UpdateSchema payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/volumes/comment_out_of_band/databricks.yml.tmpl b/acceptance/bundle/resources/volumes/comment_out_of_band/databricks.yml.tmpl new file mode 100644 index 0000000000..084b752f4a --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/databricks.yml.tmpl @@ -0,0 +1,13 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + schema1: + catalog_name: main + name: test-schema-$UNIQUE_NAME + volumes: + volume1: + catalog_name: main + schema_name: ${resources.schemas.schema1.name} + name: test-volume-$UNIQUE_NAME diff --git a/acceptance/bundle/resources/volumes/comment_out_of_band/out.test.toml b/acceptance/bundle/resources/volumes/comment_out_of_band/out.test.toml new file mode 100644 index 0000000000..c502b28221 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/out.test.toml @@ -0,0 +1,2 @@ +Cloud = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/volumes/comment_out_of_band/output.txt b/acceptance/bundle/resources/volumes/comment_out_of_band/output.txt new file mode 100644 index 0000000000..c1e1f119fa --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/output.txt @@ -0,0 +1,68 @@ + +=== Deploy a volume that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created schemas.schema1 +Created volumes.volume1 +Files: 0 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, the way Catalog Explorer does +=== The remote comment is drift, so the plan updates the volume +>>> [CLI] bundle plan --output json +{ + "action": "update", + "remote": "set outside the bundle" +} + +=== Redeploy clears the comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated volumes.volume1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 1 unchanged + +=== The comment is gone +>>> [CLI] api get /api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].test-volume-[UNIQUE_NAME] +"" + +=== Redeploy is a no-op +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.schema1 + delete resources.volumes.volume1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.schema1 + +This action will result in the deletion of the following volumes. +For managed volumes, the files stored in the volume are also deleted from your +cloud tenant within 30 days. For external volumes, the metadata about the volume +is removed from the catalog, but the underlying files are not deleted: + delete resources.volumes.volume1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 2 deleted + +=== The UpdateVolume payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/volumes +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].test-volume-[UNIQUE_NAME]", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].test-volume-[UNIQUE_NAME]", + "body": { + "comment": "" + } +} diff --git a/acceptance/bundle/resources/volumes/comment_out_of_band/script b/acceptance/bundle/resources/volumes/comment_out_of_band/script new file mode 100644 index 0000000000..c176403ba0 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/script @@ -0,0 +1,32 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +VOLUME="main.test-schema-${UNIQUE_NAME}.test-volume-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it has to run after the last +# request the test makes, which is the destroy below. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateVolume payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/volumes +} +trap cleanup EXIT + +title "Deploy a volume that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, the way Catalog Explorer does" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/volumes/$VOLUME" --json '{"comment":"set outside the bundle"}' > /dev/null + +title "The remote comment is drift, so the plan updates the volume" +trace $CLI bundle plan --output json | jq '.plan["resources.volumes.volume1"].changes.comment' + +title "Redeploy clears the comment" +trace $CLI bundle deploy + +title "The comment is gone" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/volumes/$VOLUME" | jq '.comment' + +title "Redeploy is a no-op" +trace $CLI bundle deploy diff --git a/acceptance/bundle/resources/volumes/comment_out_of_band/test.toml b/acceptance/bundle/resources/volumes/comment_out_of_band/test.toml new file mode 100644 index 0000000000..27f8fb4be3 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/test.toml @@ -0,0 +1,10 @@ +Cloud = true + +# UpdateVolume payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/volumes/comment_with_rename/databricks.yml.tmpl b/acceptance/bundle/resources/volumes/comment_with_rename/databricks.yml.tmpl new file mode 100644 index 0000000000..9c7779e59f --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_with_rename/databricks.yml.tmpl @@ -0,0 +1,13 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + schema1: + catalog_name: main + name: test-schema-$UNIQUE_NAME + volumes: + volume1: + catalog_name: main + schema_name: ${resources.schemas.schema1.name} + name: vol_one diff --git a/acceptance/bundle/resources/volumes/comment_with_rename/out.test.toml b/acceptance/bundle/resources/volumes/comment_with_rename/out.test.toml new file mode 100644 index 0000000000..c502b28221 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_with_rename/out.test.toml @@ -0,0 +1,2 @@ +Cloud = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/volumes/comment_with_rename/output.txt b/acceptance/bundle/resources/volumes/comment_with_rename/output.txt new file mode 100644 index 0000000000..3713e45f08 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_with_rename/output.txt @@ -0,0 +1,58 @@ + +=== Deploy a volume that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created schemas.schema1 +Created volumes.volume1 +Files: 0 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, then rename the volume in the same deploy +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated volumes.volume1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 1 unchanged + +=== The rename and the clear both landed, so it converges in one pass +>>> [CLI] api get /api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].vol_two +"" + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.schema1 + delete resources.volumes.volume1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.schema1 + +This action will result in the deletion of the following volumes. +For managed volumes, the files stored in the volume are also deleted from your +cloud tenant within 30 days. For external volumes, the metadata about the volume +is removed from the catalog, but the underlying files are not deleted: + delete resources.volumes.volume1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 2 deleted + +=== The rename carried the cleared comment alongside new_name +>>> print_requests.py --method PATCH //unity-catalog/volumes +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].vol_one", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].vol_one", + "body": { + "comment": "", + "new_name": "vol_two" + } +} diff --git a/acceptance/bundle/resources/volumes/comment_with_rename/script b/acceptance/bundle/resources/volumes/comment_with_rename/script new file mode 100644 index 0000000000..5229e4ef5c --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_with_rename/script @@ -0,0 +1,25 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +VOLUME="main.test-schema-${UNIQUE_NAME}.vol_one" + +# print_requests.py consumes out.requests.txt, so it runs after the last request. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The rename carried the cleared comment alongside new_name" + trace print_requests.py --method PATCH //unity-catalog/volumes +} +trap cleanup EXIT + +title "Deploy a volume that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, then rename the volume in the same deploy" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/volumes/$VOLUME" --json '{"comment":"set outside the bundle"}' > /dev/null +update_file.py databricks.yml 'name: vol_one' 'name: vol_two' +trace $CLI bundle deploy + +title "The rename and the clear both landed, so it converges in one pass" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/volumes/main.test-schema-${UNIQUE_NAME}.vol_two" | jq '.comment' +trace $CLI bundle plan | contains.py "Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged" diff --git a/acceptance/bundle/resources/volumes/comment_with_rename/test.toml b/acceptance/bundle/resources/volumes/comment_with_rename/test.toml new file mode 100644 index 0000000000..27f8fb4be3 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_with_rename/test.toml @@ -0,0 +1,10 @@ +Cloud = true + +# UpdateVolume payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/bundle/direct/dresources/all_test.go b/bundle/direct/dresources/all_test.go index 0c0d3d05ad..36eaa3e27d 100644 --- a/bundle/direct/dresources/all_test.go +++ b/bundle/direct/dresources/all_test.go @@ -63,6 +63,7 @@ var testConfig map[string]any = map[string]any{ CreateSchema: catalog.CreateSchema{ CatalogName: "main", Name: "myschema", + Comment: "Test schema", }, }, @@ -71,6 +72,7 @@ var testConfig map[string]any = map[string]any{ CatalogName: "main", SchemaName: "myschema", Name: "myvolume", + Comment: "Test volume", }, }, diff --git a/bundle/direct/dresources/catalog.go b/bundle/direct/dresources/catalog.go index 645fb933cd..a18b3dd654 100644 --- a/bundle/direct/dresources/catalog.go +++ b/bundle/direct/dresources/catalog.go @@ -50,7 +50,7 @@ func (r *ResourceCatalog) DoCreate(ctx context.Context, config *catalog.CreateCa } // DoUpdate updates the catalog in place and returns remote state. -func (r *ResourceCatalog) DoUpdate(ctx context.Context, id string, config *catalog.CreateCatalog, _ *PlanEntry) (*catalog.CatalogInfo, error) { +func (r *ResourceCatalog) DoUpdate(ctx context.Context, id string, config *catalog.CreateCatalog, entry *PlanEntry) (*catalog.CatalogInfo, error) { updateRequest := catalog.UpdateCatalog{ Comment: config.Comment, CustomMaxRetentionHours: config.CustomMaxRetentionHours, @@ -65,6 +65,8 @@ func (r *ResourceCatalog) DoUpdate(ctx context.Context, id string, config *catal ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner"), } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.Catalogs.Update(ctx, updateRequest) if err != nil { return nil, err @@ -74,7 +76,7 @@ func (r *ResourceCatalog) DoUpdate(ctx context.Context, id string, config *catal } // DoUpdateWithID updates the catalog and returns the new ID if the name changes. -func (r *ResourceCatalog) DoUpdateWithID(ctx context.Context, id string, config *catalog.CreateCatalog, _ *PlanEntry) (string, *catalog.CatalogInfo, error) { +func (r *ResourceCatalog) DoUpdateWithID(ctx context.Context, id string, config *catalog.CreateCatalog, entry *PlanEntry) (string, *catalog.CatalogInfo, error) { updateRequest := catalog.UpdateCatalog{ Comment: config.Comment, CustomMaxRetentionHours: config.CustomMaxRetentionHours, @@ -93,6 +95,8 @@ func (r *ResourceCatalog) DoUpdateWithID(ctx context.Context, id string, config updateRequest.NewName = config.Name } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.Catalogs.Update(ctx, updateRequest) if err != nil { return "", nil, err diff --git a/bundle/direct/dresources/external_location.go b/bundle/direct/dresources/external_location.go index a1ea24e410..16fea6e8af 100644 --- a/bundle/direct/dresources/external_location.go +++ b/bundle/direct/dresources/external_location.go @@ -53,7 +53,7 @@ func (r *ResourceExternalLocation) DoCreate(ctx context.Context, config *catalog } // DoUpdate updates the external location in place and returns remote state. -func (r *ResourceExternalLocation) DoUpdate(ctx context.Context, id string, config *catalog.CreateExternalLocation, _ *PlanEntry) (*catalog.ExternalLocationInfo, error) { +func (r *ResourceExternalLocation) DoUpdate(ctx context.Context, id string, config *catalog.CreateExternalLocation, entry *PlanEntry) (*catalog.ExternalLocationInfo, error) { updateRequest := catalog.UpdateExternalLocation{ Comment: config.Comment, CredentialName: config.CredentialName, @@ -74,11 +74,13 @@ func (r *ResourceExternalLocation) DoUpdate(ctx context.Context, id string, conf ForceSendFields: utils.FilterFields[catalog.UpdateExternalLocation](config.ForceSendFields, "IsolationMode", "Owner"), } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + return r.client.ExternalLocations.Update(ctx, updateRequest) } // DoUpdateWithID updates the external location and returns the new ID if the name changes. -func (r *ResourceExternalLocation) DoUpdateWithID(ctx context.Context, id string, config *catalog.CreateExternalLocation, _ *PlanEntry) (string, *catalog.ExternalLocationInfo, error) { +func (r *ResourceExternalLocation) DoUpdateWithID(ctx context.Context, id string, config *catalog.CreateExternalLocation, entry *PlanEntry) (string, *catalog.ExternalLocationInfo, error) { updateRequest := catalog.UpdateExternalLocation{ Comment: config.Comment, CredentialName: config.CredentialName, @@ -103,6 +105,8 @@ func (r *ResourceExternalLocation) DoUpdateWithID(ctx context.Context, id string updateRequest.NewName = config.Name } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.ExternalLocations.Update(ctx, updateRequest) if err != nil { return "", nil, err diff --git a/bundle/direct/dresources/registered_model.go b/bundle/direct/dresources/registered_model.go index b72038a79f..26c4b4fb51 100644 --- a/bundle/direct/dresources/registered_model.go +++ b/bundle/direct/dresources/registered_model.go @@ -65,7 +65,7 @@ func (r *ResourceRegisteredModel) DoCreate(ctx context.Context, config *catalog. return response.FullName, response, nil } -func (r *ResourceRegisteredModel) DoUpdate(ctx context.Context, id string, config *catalog.CreateRegisteredModelRequest, _ *PlanEntry) (*catalog.RegisteredModelInfo, error) { +func (r *ResourceRegisteredModel) DoUpdate(ctx context.Context, id string, config *catalog.CreateRegisteredModelRequest, entry *PlanEntry) (*catalog.RegisteredModelInfo, error) { updateRequest := catalog.UpdateRegisteredModelRequest{ FullName: id, Comment: config.Comment, @@ -91,6 +91,8 @@ func (r *ResourceRegisteredModel) DoUpdate(ctx context.Context, id string, confi CatalogName: config.CatalogName, } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.RegisteredModels.Update(ctx, updateRequest) if err != nil { return nil, err diff --git a/bundle/direct/dresources/schema.go b/bundle/direct/dresources/schema.go index 24c82ceb6a..6f77d19e7e 100644 --- a/bundle/direct/dresources/schema.go +++ b/bundle/direct/dresources/schema.go @@ -47,7 +47,7 @@ func (r *ResourceSchema) DoCreate(ctx context.Context, config *catalog.CreateSch } // DoUpdate updates the schema in place and returns remote state. -func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, config *catalog.CreateSchema, _ *PlanEntry) (*catalog.SchemaInfo, error) { +func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, config *catalog.CreateSchema, entry *PlanEntry) (*catalog.SchemaInfo, error) { updateRequest := catalog.UpdateSchema{ Comment: config.Comment, CustomMaxRetentionHours: config.CustomMaxRetentionHours, @@ -59,6 +59,8 @@ func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, config *catalo ForceSendFields: utils.FilterFields[catalog.UpdateSchema](config.ForceSendFields, "EnablePredictiveOptimization", "NewName", "Owner"), } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.Schemas.Update(ctx, updateRequest) if err != nil { return nil, err diff --git a/bundle/direct/dresources/schema_test.go b/bundle/direct/dresources/schema_test.go index d013610e05..584b50816a 100644 --- a/bundle/direct/dresources/schema_test.go +++ b/bundle/direct/dresources/schema_test.go @@ -49,7 +49,10 @@ func TestResourceSchema_DoUpdate_WithUnsupportedForceSendFields(t *testing.T) { resultJSON, err := json.Marshal(result) require.NoError(t, err) + // browse_only is force-sent from create onwards, matching UC, which returns it on + // every read including after an update. expected := `{ + "browse_only": false, "catalog_name": "main", "catalog_type": "MANAGED_CATALOG", "created_at": 0, diff --git a/bundle/direct/dresources/util.go b/bundle/direct/dresources/util.go index fc68f3ea37..9a10ecce69 100644 --- a/bundle/direct/dresources/util.go +++ b/bundle/direct/dresources/util.go @@ -2,10 +2,13 @@ package dresources import ( "errors" + "reflect" "slices" "strings" "github.com/databricks/cli/bundle/deployplan" + "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/cli/libs/structs/structtag" "github.com/databricks/databricks-sdk-go/retries" ) @@ -65,3 +68,43 @@ func collectLeafUpdatePathsWithPrefix(changes Changes, prefix string) []string { slices.Sort(paths) return paths } + +// forceSendClearedFields returns ForceSendFields entries for the request fields the plan +// is clearing: the plan reports a change, but the value is zero, so omitempty would drop +// the field from the payload entirely. +// +// A partial-update API reads an absent field as "leave unchanged", so dropping it +// silently discards the clear and the next plan reports the same drift forever. Unity +// Catalog goes further and rejects a payload carrying no field at all with +// " Nothing to update." (400), which made a schema, catalog or volume +// undeployable as soon as someone set a description on it outside the bundle (#6340). +// +// Every omitempty field assigned from config into a partial-update request shares the +// problem -- roughly 40 of them across the UC resources, not just comment -- so this is +// driven off the plan instead of naming individual fields. Resources whose update +// replaces the whole object (jobs, pipelines, model serving) need none of it: there an +// absent field already means cleared, and they converge on their own. +// +// Matching is by the request type's own JSON names, so a field the request does not +// expose, or names differently (e.g. new_name), is never force-sent. Skipped changes are +// excluded by HasChange, so a field the plan suppressed -- via backend_defaults or +// ignore_remote_changes -- is left alone rather than being cleared behind the plan's back. +func forceSendClearedFields(request any, changes Changes) []string { + value := reflect.ValueOf(request).Elem() + valueType := value.Type() + + var result []string + for i := range valueType.NumField() { + field := valueType.Field(i) + name := structtag.JSONTag(field.Tag.Get("json")).Name() + if name == "" || name == "-" || !value.Field(i).IsZero() { + continue + } + if changes.HasChange(structpath.NewDotString(nil, name)) { + result = append(result, field.Name) + } + } + + slices.Sort(result) + return result +} diff --git a/bundle/direct/dresources/volume.go b/bundle/direct/dresources/volume.go index 9d33f31ec7..0403c2c020 100644 --- a/bundle/direct/dresources/volume.go +++ b/bundle/direct/dresources/volume.go @@ -48,7 +48,7 @@ func (r *ResourceVolume) DoCreate(ctx context.Context, config *catalog.CreateVol return response.FullName, response, nil } -func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalog.CreateVolumeRequestContent, _ *PlanEntry) (*catalog.VolumeInfo, error) { +func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalog.CreateVolumeRequestContent, entry *PlanEntry) (*catalog.VolumeInfo, error) { updateRequest := catalog.UpdateVolumeRequestContent{ Comment: config.Comment, Name: id, @@ -67,6 +67,8 @@ func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalo return nil, fmt.Errorf("internal error: unexpected change of name from %#v to %#v", nameFromID, config.Name) } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.Volumes.Update(ctx, updateRequest) if err != nil { return nil, err @@ -79,7 +81,7 @@ func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalo return response, err } -func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string, config *catalog.CreateVolumeRequestContent, _ *PlanEntry) (string, *catalog.VolumeInfo, error) { +func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string, config *catalog.CreateVolumeRequestContent, entry *PlanEntry) (string, *catalog.VolumeInfo, error) { updateRequest := catalog.UpdateVolumeRequestContent{ Comment: config.Comment, Name: id, @@ -100,6 +102,8 @@ func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string, config * updateRequest.NewName = config.Name } + updateRequest.ForceSendFields = append(updateRequest.ForceSendFields, forceSendClearedFields(&updateRequest, entry.Changes)...) + response, err := r.client.Volumes.Update(ctx, updateRequest) if err != nil || response == nil { return "", nil, err diff --git a/libs/testserver/catalogs.go b/libs/testserver/catalogs.go index 1d0bc06568..347b97082a 100644 --- a/libs/testserver/catalogs.go +++ b/libs/testserver/catalogs.go @@ -88,6 +88,11 @@ func (s *FakeWorkspace) CatalogsUpdate(req Request, name string) Response { } } + fields, errResponse := parseUCUpdate(req.Body, "UpdateCatalog") + if errResponse != nil { + return *errResponse + } + var updateRequest catalog.UpdateCatalog if err := json.Unmarshal(req.Body, &updateRequest); err != nil { return Response{ @@ -96,25 +101,7 @@ func (s *FakeWorkspace) CatalogsUpdate(req Request, name string) Response { } } - // Update only the fields that can be updated - if updateRequest.Comment != "" { - existing.Comment = updateRequest.Comment - } - if updateRequest.CustomMaxRetentionHours != 0 { - existing.CustomMaxRetentionHours = updateRequest.CustomMaxRetentionHours - } - if updateRequest.ManagedEncryptionSettings != nil { - existing.ManagedEncryptionSettings = updateRequest.ManagedEncryptionSettings - } - if updateRequest.Options != nil { - existing.Options = updateRequest.Options - } - if updateRequest.Properties != nil { - existing.Properties = updateRequest.Properties - } - if updateRequest.Owner != "" { - existing.Owner = updateRequest.Owner - } + applyUpdatedFields(&existing, updateRequest, fields) if updateRequest.NewName != "" { existing.Name = updateRequest.NewName existing.FullName = updateRequest.NewName diff --git a/libs/testserver/external_locations.go b/libs/testserver/external_locations.go index b0000deb2a..8fba059804 100644 --- a/libs/testserver/external_locations.go +++ b/libs/testserver/external_locations.go @@ -61,6 +61,11 @@ func (s *FakeWorkspace) ExternalLocationsUpdate(req Request, name string) Respon } } + fields, errResponse := parseUpdateFields(req.Body) + if errResponse != nil { + return *errResponse + } + var updateRequest catalog.UpdateExternalLocation if err := json.Unmarshal(req.Body, &updateRequest); err != nil { return Response{ @@ -69,19 +74,10 @@ func (s *FakeWorkspace) ExternalLocationsUpdate(req Request, name string) Respon } } - // Update only the fields that can be updated - if updateRequest.Comment != "" { - existing.Comment = updateRequest.Comment - } - if updateRequest.Url != "" { - existing.Url = updateRequest.Url - } - if updateRequest.CredentialName != "" { - existing.CredentialName = updateRequest.CredentialName - } - if updateRequest.Owner != "" { - existing.Owner = updateRequest.Owner - } + applyUpdatedFields(&existing, updateRequest, fields) + + // These three are always sent, so they track the request whether or not the payload + // names them (read_only=false and a dropped key mean the same thing here). existing.ReadOnly = updateRequest.ReadOnly existing.EnableFileEvents = updateRequest.EnableFileEvents existing.Fallback = updateRequest.Fallback diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index 25178a9e75..874329ca29 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -5,9 +5,11 @@ import ( "encoding/binary" "encoding/json" "fmt" + "net/http" "os" "path" "path/filepath" + "reflect" "slices" "strconv" "strings" @@ -20,6 +22,7 @@ import ( "github.com/databricks/databricks-sdk-go/service/postgres" "github.com/google/uuid" + "github.com/databricks/cli/libs/structs/structtag" "github.com/databricks/databricks-sdk-go/service/apps" "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/files" @@ -255,6 +258,81 @@ func (s *FakeWorkspace) LockUnlock() func() { return func() { s.mu.Unlock() } } +// parseUpdateFields decodes an update payload into its raw fields, so a handler can tell +// a field explicitly set to a zero value from one the caller omitted. +func parseUpdateFields(body []byte) (map[string]json.RawMessage, *Response) { + var fields map[string]json.RawMessage + + if err := json.Unmarshal(body, &fields); err != nil { + return nil, &Response{ + Body: fmt.Sprintf("internal error: %s", err), + StatusCode: http.StatusInternalServerError, + } + } + return fields, nil +} + +// parseUCUpdate is parseUpdateFields for the UC APIs that reject a payload carrying no +// field to act on, answering " Nothing to update." (400) rather than treating it +// as a no-op. A key set to null does not count. +// +// Verified against a real workspace for schemas, volumes and catalogs: {} and +// {"comment": null} are rejected, while {"comment": ""} and +// {"custom_max_retention_hours": 0} are accepted. Registered models accept {} instead, so +// they use parseUpdateFields. +func parseUCUpdate(body []byte, operation string) (map[string]json.RawMessage, *Response) { + fields, errResponse := parseUpdateFields(body) + if errResponse != nil { + return nil, errResponse + } + + for _, value := range fields { + if string(value) != "null" { + return fields, nil + } + } + + return nil, &Response{ + StatusCode: http.StatusBadRequest, + Body: map[string]string{ + "error_code": "INVALID_PARAMETER_VALUE", + "message": operation + " Nothing to update.", + }, + } +} + +// applyUpdatedFields copies every field the update payload names from update onto +// existing, matched by JSON name, and marks it force-send so a zero value survives the +// response encoding (the stored *Info types are all omitempty). +// +// Fields the payload omits are left untouched: a partial-update API changes only what the +// caller names, and modelling that is the whole point of these fakes. Fields the payload +// names but the stored type lacks (new_name, force) are skipped for the caller to handle. +// existing must be a pointer; update is passed by value. +func applyUpdatedFields(existing, update any, fields map[string]json.RawMessage) { + dst := reflect.ValueOf(existing).Elem() + src := reflect.ValueOf(update) + + for i := range src.Type().NumField() { + name := structtag.JSONTag(src.Type().Field(i).Tag.Get("json")).Name() + if name == "" || name == "-" { + continue + } + if _, ok := fields[name]; !ok { + continue + } + dstField := dst.FieldByName(src.Type().Field(i).Name) + if !dstField.IsValid() || !dstField.CanSet() || dstField.Type() != src.Field(i).Type() { + continue + } + dstField.Set(src.Field(i)) + forceSend := dst.FieldByName("ForceSendFields") + if forceSend.IsValid() && forceSend.CanSet() { + forceSend.Set(reflect.Append(forceSend, reflect.ValueOf(src.Type().Field(i).Name))) + } + } +} + // Generic functions to handle map operations func MapGet[T any](w *FakeWorkspace, collection map[string]T, key string) Response { defer w.LockUnlock()() diff --git a/libs/testserver/registered_models.go b/libs/testserver/registered_models.go index 6b6d3b4b34..b6fb800a62 100644 --- a/libs/testserver/registered_models.go +++ b/libs/testserver/registered_models.go @@ -62,6 +62,11 @@ func (s *FakeWorkspace) RegisteredModelsUpdate(req Request, fullName string) Res } } + fields, errResponse := parseUpdateFields(req.Body) + if errResponse != nil { + return *errResponse + } + var updateRequest catalog.UpdateRegisteredModelRequest if err := json.Unmarshal(req.Body, &updateRequest); err != nil { return Response{ @@ -70,13 +75,8 @@ func (s *FakeWorkspace) RegisteredModelsUpdate(req Request, fullName string) Res } } - // Update only the fields that can be updated - if updateRequest.Comment != "" { - existing.Comment = updateRequest.Comment - } - if updateRequest.Owner != "" { - existing.Owner = updateRequest.Owner - } + applyUpdatedFields(&existing, updateRequest, fields) + if updateRequest.NewName != "" { existing.Name = updateRequest.NewName diff --git a/libs/testserver/schemas.go b/libs/testserver/schemas.go index 66d23a3011..6880a55cb8 100644 --- a/libs/testserver/schemas.go +++ b/libs/testserver/schemas.go @@ -6,7 +6,6 @@ import ( "net/http" "strings" - "dario.cat/mergo" "github.com/databricks/databricks-sdk-go/service/catalog" ) @@ -80,6 +79,11 @@ func (s *FakeWorkspace) SchemasUpdate(req Request, name string) Response { } } + fields, errResponse := parseUCUpdate(req.Body, "UpdateSchema") + if errResponse != nil { + return *errResponse + } + var schemaUpdate catalog.SchemaInfo if err := json.Unmarshal(req.Body, &schemaUpdate); err != nil { @@ -89,13 +93,7 @@ func (s *FakeWorkspace) SchemasUpdate(req Request, name string) Response { } } - err := mergo.Merge(&existing, schemaUpdate, mergo.WithOverride) - if err != nil { - return Response{ - Body: fmt.Sprintf("mergo error: %s", err), - StatusCode: http.StatusInternalServerError, - } - } + applyUpdatedFields(&existing, schemaUpdate, fields) existing.UpdatedAt = nowMilli() existing.UpdatedBy = s.CurrentUser().UserName diff --git a/libs/testserver/volumes.go b/libs/testserver/volumes.go index 88eae7ac02..91f29a94e4 100644 --- a/libs/testserver/volumes.go +++ b/libs/testserver/volumes.go @@ -76,6 +76,11 @@ func (s *FakeWorkspace) VolumesUpdate(req Request, fullname string) Response { } } + fields, errResponse := parseUCUpdate(req.Body, "UpdateVolume") + if errResponse != nil { + return *errResponse + } + var request catalog.UpdateVolumeRequestContent if err := json.Unmarshal(req.Body, &request); err != nil { @@ -85,13 +90,7 @@ func (s *FakeWorkspace) VolumesUpdate(req Request, fullname string) Response { } } - if request.Comment != "" { - existing.Comment = request.Comment - } - - if request.Owner != "" { - existing.Owner = request.Owner - } + applyUpdatedFields(&existing, request, fields) if request.NewName != "" { delete(s.Volumes, fullname)