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..bc70893a2b --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/output.txt @@ -0,0 +1,59 @@ + +=== 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 +{ + "comment": { + "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 + +=== 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": "" + } +} + +>>> [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 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..fe40688274 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/comment_out_of_band/script @@ -0,0 +1,33 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +CATALOG="test-catalog-${UNIQUE_NAME}" + +cleanup() { + trace $CLI bundle destroy --auto-approve + + # Consume the destroy's requests so none are left recorded. + print_requests.py --method PATCH //unity-catalog/catalogs &> LOG.destroy_requests +} +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' + +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 + +title "The UpdateCatalog payloads the deploys sent" +trace print_requests.py --method PATCH //unity-catalog/catalogs 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..b005fd174f --- /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 + +=== 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" + } +} + +>>> [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 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..12317ae4a6 --- /dev/null +++ b/acceptance/bundle/resources/external_locations/comment_cleared/script @@ -0,0 +1,25 @@ +echo "*" > .gitignore + +cleanup() { + trace $CLI bundle destroy --auto-approve + + # Consume the destroy's requests so none are left recorded. + print_requests.py --method PATCH //external-locations &> LOG.destroy_requests +} +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 + +title "The UpdateExternalLocation payloads the deploys sent" +trace print_requests.py --method PATCH //external-locations 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..44c63e3710 --- /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 + +=== 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]" + } +} + +>>> [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 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..ad43f2474d --- /dev/null +++ b/acceptance/bundle/resources/registered_models/comment_cleared/script @@ -0,0 +1,28 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +MODEL="main.test-schema-${UNIQUE_NAME}.test-model-${UNIQUE_NAME}" + +cleanup() { + trace $CLI bundle destroy --auto-approve + + # Consume the destroy's requests so none are left recorded. + print_requests.py --method PATCH //unity-catalog/models &> LOG.destroy_requests +} +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 + +title "The UpdateRegisteredModel payloads the deploys sent" +trace print_requests.py --method PATCH //unity-catalog/models 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..ea5395330f --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/output.txt @@ -0,0 +1,62 @@ + +=== 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 +{ + "comment": { + "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 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": "" + } +} + +>>> [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 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..70871c43a9 --- /dev/null +++ b/acceptance/bundle/resources/schemas/comment_out_of_band/script @@ -0,0 +1,33 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +SCHEMA="main.test-schema-${UNIQUE_NAME}" + +cleanup() { + trace $CLI bundle destroy --auto-approve + + # Consume the destroy's requests so none are left recorded. + print_requests.py --method PATCH //unity-catalog/schemas &> LOG.destroy_requests +} +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' + +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 + +title "The UpdateSchema payloads the deploys sent" +trace print_requests.py --method PATCH //unity-catalog/schemas 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..a1b0f53281 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/output.txt @@ -0,0 +1,75 @@ + +=== 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 +{ + "comment": { + "action": "update", + "remote": "set outside the bundle" + }, + "storage_location": { + "action": "skip", + "reason": "backend_default", + "remote": "s3://[METASTORE_NAME]/metastore/[UUID]/volumes/[UUID]" + } +} + +=== 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 + +=== 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": "" + } +} + +>>> [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 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..3c01424f36 --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/script @@ -0,0 +1,33 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +VOLUME="main.test-schema-${UNIQUE_NAME}.test-volume-${UNIQUE_NAME}" + +cleanup() { + trace $CLI bundle destroy --auto-approve + + # Consume the destroy's requests so none are left recorded. + print_requests.py --method PATCH //unity-catalog/volumes &> LOG.destroy_requests +} +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' + +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 + +title "The UpdateVolume payloads the deploys sent" +trace print_requests.py --method PATCH //unity-catalog/volumes 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..fa32d22b4e --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_out_of_band/test.toml @@ -0,0 +1,23 @@ +Cloud = true + +# UpdateVolume payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] + +# The plan records the backend-generated storage_location, whose scheme and bucket differ +# per cloud. Normalize to the AWS form before the METASTORE_NAME repl, same as +# grants/volumes/test.toml. +[[Repls]] +Old = 'abfss://decotestprod-unity-iso@decotestprodunityiso.dfs.core.windows.net' +New = 's3://deco-uc-prod-isolated-aws-us-east-1/metastore' +Order = -1 + +[[Repls]] +Old = 'gs://deco-uc-prod-gcp-us-central1-isolated' +New = 's3://deco-uc-prod-isolated-aws-us-east-1' +Order = -1 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..953800978e --- /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 + +=== 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" + } +} + +>>> [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 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..2a9cce62ec --- /dev/null +++ b/acceptance/bundle/resources/volumes/comment_with_rename/script @@ -0,0 +1,27 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +VOLUME="main.test-schema-${UNIQUE_NAME}.vol_one" + +cleanup() { + trace $CLI bundle destroy --auto-approve + + # Consume the destroy's requests so none are left recorded. + print_requests.py --method PATCH //unity-catalog/volumes &> LOG.destroy_requests +} +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" + +title "The rename carried the cleared comment alongside new_name" +trace print_requests.py --method PATCH //unity-catalog/volumes 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..b29f1fa90c 100644 --- a/bundle/direct/dresources/catalog.go +++ b/bundle/direct/dresources/catalog.go @@ -2,6 +2,7 @@ package dresources import ( "context" + "slices" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/utils" @@ -49,6 +50,9 @@ func (r *ResourceCatalog) DoCreate(ctx context.Context, config *catalog.CreateCa return response.Name, response, nil } +// See schemaForceSend. Verified against a real workspace: {"comment": ""} clears it. +var catalogForceSend = []string{"Comment"} + // 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) { updateRequest := catalog.UpdateCatalog{ @@ -62,7 +66,7 @@ func (r *ResourceCatalog) DoUpdate(ctx context.Context, id string, config *catal Options: config.Options, Owner: "", // Not supported by DABs Properties: config.Properties, - ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](append(slices.Clone(catalogForceSend), config.ForceSendFields...), "EnablePredictiveOptimization", "IsolationMode", "Owner"), } response, err := r.client.Catalogs.Update(ctx, updateRequest) @@ -86,7 +90,7 @@ func (r *ResourceCatalog) DoUpdateWithID(ctx context.Context, id string, config Options: config.Options, Owner: "", // Not supported by DABs Properties: config.Properties, - ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](append(slices.Clone(catalogForceSend), config.ForceSendFields...), "EnablePredictiveOptimization", "IsolationMode", "Owner"), } if config.Name != id { diff --git a/bundle/direct/dresources/external_location.go b/bundle/direct/dresources/external_location.go index a1ea24e410..26c957020a 100644 --- a/bundle/direct/dresources/external_location.go +++ b/bundle/direct/dresources/external_location.go @@ -2,6 +2,7 @@ package dresources import ( "context" + "slices" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/utils" @@ -52,6 +53,13 @@ func (r *ResourceExternalLocation) DoCreate(ctx context.Context, config *catalog return response.Name, response, nil } +// See schemaForceSend. Unlike the other four this one is not probed: an external +// location needs a storage credential with cloud IAM setup, which the test workspaces do +// not provision, so its acceptance test is local-only. Comment is carried here because it +// is the same field on the same UC PATCH family, where all four siblings clear on +// {"comment": ""}; re-probe before adding anything else. +var externalLocationForceSend = []string{"Comment"} + // 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) { updateRequest := catalog.UpdateExternalLocation{ @@ -71,7 +79,7 @@ func (r *ResourceExternalLocation) DoUpdate(ctx context.Context, id string, conf ReadOnly: config.ReadOnly, SkipValidation: config.SkipValidation, Url: config.Url, - ForceSendFields: utils.FilterFields[catalog.UpdateExternalLocation](config.ForceSendFields, "IsolationMode", "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateExternalLocation](append(slices.Clone(externalLocationForceSend), config.ForceSendFields...), "IsolationMode", "Owner"), } return r.client.ExternalLocations.Update(ctx, updateRequest) @@ -96,7 +104,7 @@ func (r *ResourceExternalLocation) DoUpdateWithID(ctx context.Context, id string ReadOnly: config.ReadOnly, SkipValidation: config.SkipValidation, Url: config.Url, - ForceSendFields: utils.FilterFields[catalog.UpdateExternalLocation](config.ForceSendFields, "IsolationMode", "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateExternalLocation](append(slices.Clone(externalLocationForceSend), config.ForceSendFields...), "IsolationMode", "Owner"), } if config.Name != id { diff --git a/bundle/direct/dresources/registered_model.go b/bundle/direct/dresources/registered_model.go index b72038a79f..e3c67b3d8c 100644 --- a/bundle/direct/dresources/registered_model.go +++ b/bundle/direct/dresources/registered_model.go @@ -2,6 +2,7 @@ package dresources import ( "context" + "slices" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/utils" @@ -65,13 +66,18 @@ func (r *ResourceRegisteredModel) DoCreate(ctx context.Context, config *catalog. return response.FullName, response, nil } +// See schemaForceSend. Verified against a real workspace: {"comment": ""} clears it. +var registeredModelForceSend = []string{"Comment"} + func (r *ResourceRegisteredModel) DoUpdate(ctx context.Context, id string, config *catalog.CreateRegisteredModelRequest, _ *PlanEntry) (*catalog.RegisteredModelInfo, error) { updateRequest := catalog.UpdateRegisteredModelRequest{ FullName: id, Comment: config.Comment, - ForceSendFields: utils.FilterFields[catalog.UpdateRegisteredModelRequest](config.ForceSendFields, "Owner", "NewName"), + ForceSendFields: utils.FilterFields[catalog.UpdateRegisteredModelRequest](append(slices.Clone(registeredModelForceSend), config.ForceSendFields...), "Owner", "NewName"), - // Owner is not part of the configuration tree + // Owner is settable in the config (it comes from the embedded + // CreateRegisteredModelRequest) and create sends it, but update never has: a + // change to it is silently dropped rather than applied. Owner: "", // Name updates are not supported yet without recreating. Can be added as a follow-up. diff --git a/bundle/direct/dresources/schema.go b/bundle/direct/dresources/schema.go index 24c82ceb6a..b29c89bdce 100644 --- a/bundle/direct/dresources/schema.go +++ b/bundle/direct/dresources/schema.go @@ -2,6 +2,7 @@ package dresources import ( "context" + "slices" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/log" @@ -46,6 +47,22 @@ func (r *ResourceSchema) DoCreate(ctx context.Context, config *catalog.CreateSch return response.FullName, response, nil } +// UC drops a field the payload omits, so a value the config stops declaring never +// reaches the backend and every later plan reports the same pending change. Worse, when +// nothing else is in the payload UC rejects it outright with "Nothing to update" (400), +// which made a schema undeployable once someone set a description on it outside the +// bundle. Sending these explicitly is the only way to express "the bundle owns this". +// +// Only fields verified against a real workspace belong here: {"comment": ""} clears it. Do +// not add a field without probing it -- UC rejects an empty value on some (owner: "" is +// "Could not find principal with name .", new_name: "" is "not a valid name"), and +// ForceSendFields is inert for maps, so listing properties would be a no-op. +// +// custom_max_retention_hours is deliberately absent even though {"...": 0} does clear it: +// terraform never sends it, so force-sending would make the two engines produce different +// payloads for the same config. +var schemaForceSend = []string{"Comment"} + // 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) { updateRequest := catalog.UpdateSchema{ @@ -56,7 +73,7 @@ func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, config *catalo NewName: "", // We recreate schemas on name change intentionally. Owner: "", // Not supported by DABs Properties: config.Properties, - ForceSendFields: utils.FilterFields[catalog.UpdateSchema](config.ForceSendFields, "EnablePredictiveOptimization", "NewName", "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateSchema](append(slices.Clone(schemaForceSend), config.ForceSendFields...), "EnablePredictiveOptimization", "NewName", "Owner"), } response, err := r.client.Schemas.Update(ctx, updateRequest) 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/volume.go b/bundle/direct/dresources/volume.go index 9d33f31ec7..7cd1eaba1c 100644 --- a/bundle/direct/dresources/volume.go +++ b/bundle/direct/dresources/volume.go @@ -3,6 +3,7 @@ package dresources import ( "context" "fmt" + "slices" "strings" "github.com/databricks/cli/bundle/config/resources" @@ -48,6 +49,9 @@ func (r *ResourceVolume) DoCreate(ctx context.Context, config *catalog.CreateVol return response.FullName, response, nil } +// See schemaForceSend. Verified against a real workspace: {"comment": ""} clears it. +var volumeForceSend = []string{"Comment"} + func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalog.CreateVolumeRequestContent, _ *PlanEntry) (*catalog.VolumeInfo, error) { updateRequest := catalog.UpdateVolumeRequestContent{ Comment: config.Comment, @@ -55,7 +59,7 @@ func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalo NewName: "", // Not supported by Update(). Needs DoUpdateWithID() Owner: "", // Not supported by DABs - ForceSendFields: utils.FilterFields[catalog.UpdateVolumeRequestContent](config.ForceSendFields, "NewName", "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateVolumeRequestContent](append(slices.Clone(volumeForceSend), config.ForceSendFields...), "NewName", "Owner"), } nameFromID, err := getNameFromID(id) @@ -87,7 +91,7 @@ func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string, config * NewName: "", // Initialized below if needed Owner: "", // Not supported by DABs - ForceSendFields: utils.FilterFields[catalog.UpdateVolumeRequestContent](config.ForceSendFields, "Owner"), + ForceSendFields: utils.FilterFields[catalog.UpdateVolumeRequestContent](append(slices.Clone(volumeForceSend), config.ForceSendFields...), "Owner"), } items := strings.Split(id, ".") 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..2f077b1b63 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,85 @@ 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() { + goName := src.Type().Field(i).Name + // Repeated updates would otherwise keep appending the same name. + if !slices.Contains(forceSend.Interface().([]string), goName) { + forceSend.Set(reflect.Append(forceSend, reflect.ValueOf(goName))) + } + } + } +} + // 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)