diff --git a/.nextchanges/bundles/drop-empty-strings.md b/.nextchanges/bundles/drop-empty-strings.md new file mode 100644 index 00000000000..b9201997bac --- /dev/null +++ b/.nextchanges/bundles/drop-empty-strings.md @@ -0,0 +1 @@ +Empty-string values on optional (omitempty) resource fields are now dropped before deployment instead of being sent to the backend. This fixes deploys failing with errors like `'' is not a valid cluster policy ID` when a field such as `policy_id` was set to `""` (often via a variable that resolved to an empty string). The behavior now matches between the terraform and direct engines and is reflected in `bundle validate -o json`. diff --git a/acceptance/bundle/empty_string_dropped/base.yml b/acceptance/bundle/empty_string_dropped/base.yml index aa7dc2b8894..16c893adb0b 100644 --- a/acceptance/bundle/empty_string_dropped/base.yml +++ b/acceptance/bundle/empty_string_dropped/base.yml @@ -21,6 +21,9 @@ resources: clusters: my_cluster: num_workers: 1 + # spark_version is tagged omitempty in the SDK but required by terraform's + # schema and the backend, so it must have a real value or the deploy fails. + spark_version: 13.3.x-scala2.12 aws_attributes: availability: SPOT cluster_log_conf: @@ -38,6 +41,7 @@ resources: - job_cluster_key: main new_cluster: num_workers: 1 + spark_version: 13.3.x-scala2.12 aws_attributes: availability: SPOT cluster_log_conf: diff --git a/acceptance/bundle/empty_string_dropped/databricks.yml b/acceptance/bundle/empty_string_dropped/databricks.yml index 44ae3b88b3f..80ff85d9762 100644 --- a/acceptance/bundle/empty_string_dropped/databricks.yml +++ b/acceptance/bundle/empty_string_dropped/databricks.yml @@ -29,7 +29,7 @@ resources: num_workers: 1 policy_id: '' single_user_name: '' - spark_version: '' + spark_version: 13.3.x-scala2.12 database_instances: my_db_instance: capacity: CU_1 @@ -67,7 +67,7 @@ resources: num_workers: 1 policy_id: '' single_user_name: '' - spark_version: '' + spark_version: 13.3.x-scala2.12 max_concurrent_runs: 1 name: '' parent_path: '' diff --git a/acceptance/bundle/empty_string_dropped/out.empty_dropped_by_both.txt b/acceptance/bundle/empty_string_dropped/out.empty_dropped_by_both.txt index 742e279b1cc..1e5d5eb2a6b 100644 --- a/acceptance/bundle/empty_string_dropped/out.empty_dropped_by_both.txt +++ b/acceptance/bundle/empty_string_dropped/out.empty_dropped_by_both.txt @@ -1 +1,58 @@ +/apps budget_policy_id +/apps space +/apps usage_policy_id +/database/instances usage_policy_id +/experiments/create artifact_location +/mlflow/registered-models description +/pipelines budget_policy_id +/pipelines clusters.aws_attributes.instance_profile_arn +/pipelines clusters.aws_attributes.zone_id +/pipelines clusters.driver_instance_pool_id +/pipelines clusters.driver_node_type_id +/pipelines clusters.instance_pool_id +/pipelines clusters.node_type_id +/pipelines clusters.policy_id +/pipelines libraries.jar +/pipelines libraries.whl +/pipelines name +/pipelines serverless_compute_id +/pipelines usage_policy_id +/unity-catalog/models comment +/unity-catalog/models created_by +/unity-catalog/models full_name +/unity-catalog/models metastore_id +/unity-catalog/models owner +/unity-catalog/models storage_location +/unity-catalog/models updated_by +/unity-catalog/schemas comment +/unity-catalog/schemas storage_root +/unity-catalog/volumes comment +/unity-catalog/volumes storage_location /unity-catalog/volumes volume_path +/warehouses instance_profile_arn +clusters/create aws_attributes.instance_profile_arn +clusters/create aws_attributes.zone_id +clusters/create cluster_log_conf.s3.canned_acl +clusters/create cluster_log_conf.s3.encryption_type +clusters/create cluster_log_conf.s3.endpoint +clusters/create cluster_log_conf.s3.kms_key +clusters/create cluster_log_conf.s3.region +clusters/create cluster_name +clusters/create node_type_id +clusters/create policy_id +clusters/create single_user_name +jobs/create budget_policy_id +jobs/create description +jobs/create job_clusters.new_cluster.aws_attributes.instance_profile_arn +jobs/create job_clusters.new_cluster.aws_attributes.zone_id +jobs/create job_clusters.new_cluster.cluster_log_conf.s3.canned_acl +jobs/create job_clusters.new_cluster.cluster_log_conf.s3.encryption_type +jobs/create job_clusters.new_cluster.cluster_log_conf.s3.endpoint +jobs/create job_clusters.new_cluster.cluster_log_conf.s3.kms_key +jobs/create job_clusters.new_cluster.cluster_log_conf.s3.region +jobs/create job_clusters.new_cluster.cluster_name +jobs/create job_clusters.new_cluster.node_type_id +jobs/create job_clusters.new_cluster.policy_id +jobs/create job_clusters.new_cluster.single_user_name +jobs/create parent_path +jobs/create usage_policy_id diff --git a/acceptance/bundle/empty_string_dropped/out.empty_sent_by_both.txt b/acceptance/bundle/empty_string_dropped/out.empty_sent_by_both.txt index 9bb4de3c47b..ea9330c71cd 100644 --- a/acceptance/bundle/empty_string_dropped/out.empty_sent_by_both.txt +++ b/acceptance/bundle/empty_string_dropped/out.empty_sent_by_both.txt @@ -1,6 +1 @@ -/api/2.0/apps budget_policy_id /api/2.0/apps description -/api/2.0/apps space -/api/2.0/apps usage_policy_id -/api/2.0/database/instances usage_policy_id -/api/2.1/clusters/create spark_version diff --git a/acceptance/bundle/empty_string_dropped/out.empty_sent_by_direct_only.txt b/acceptance/bundle/empty_string_dropped/out.empty_sent_by_direct_only.txt index 77ae65f8efa..e69de29bb2d 100644 --- a/acceptance/bundle/empty_string_dropped/out.empty_sent_by_direct_only.txt +++ b/acceptance/bundle/empty_string_dropped/out.empty_sent_by_direct_only.txt @@ -1,55 +0,0 @@ -/api/2.0/mlflow/experiments/create artifact_location -/api/2.0/mlflow/registered-models/create description -/api/2.0/pipelines budget_policy_id -/api/2.0/pipelines clusters.aws_attributes.instance_profile_arn -/api/2.0/pipelines clusters.aws_attributes.zone_id -/api/2.0/pipelines clusters.driver_instance_pool_id -/api/2.0/pipelines clusters.driver_node_type_id -/api/2.0/pipelines clusters.instance_pool_id -/api/2.0/pipelines clusters.node_type_id -/api/2.0/pipelines clusters.policy_id -/api/2.0/pipelines libraries.jar -/api/2.0/pipelines libraries.whl -/api/2.0/pipelines name -/api/2.0/pipelines serverless_compute_id -/api/2.0/pipelines usage_policy_id -/api/2.0/sql/warehouses instance_profile_arn -/api/2.1/clusters/create aws_attributes.instance_profile_arn -/api/2.1/clusters/create aws_attributes.zone_id -/api/2.1/clusters/create cluster_log_conf.s3.canned_acl -/api/2.1/clusters/create cluster_log_conf.s3.encryption_type -/api/2.1/clusters/create cluster_log_conf.s3.endpoint -/api/2.1/clusters/create cluster_log_conf.s3.kms_key -/api/2.1/clusters/create cluster_log_conf.s3.region -/api/2.1/clusters/create cluster_name -/api/2.1/clusters/create node_type_id -/api/2.1/clusters/create policy_id -/api/2.1/clusters/create single_user_name -/api/2.1/unity-catalog/models comment -/api/2.1/unity-catalog/models created_by -/api/2.1/unity-catalog/models full_name -/api/2.1/unity-catalog/models metastore_id -/api/2.1/unity-catalog/models owner -/api/2.1/unity-catalog/models storage_location -/api/2.1/unity-catalog/models updated_by -/api/2.1/unity-catalog/schemas comment -/api/2.1/unity-catalog/schemas storage_root -/api/2.1/unity-catalog/volumes comment -/api/2.1/unity-catalog/volumes storage_location -/api/2.2/jobs/create budget_policy_id -/api/2.2/jobs/create description -/api/2.2/jobs/create job_clusters.new_cluster.aws_attributes.instance_profile_arn -/api/2.2/jobs/create job_clusters.new_cluster.aws_attributes.zone_id -/api/2.2/jobs/create job_clusters.new_cluster.cluster_log_conf.s3.canned_acl -/api/2.2/jobs/create job_clusters.new_cluster.cluster_log_conf.s3.encryption_type -/api/2.2/jobs/create job_clusters.new_cluster.cluster_log_conf.s3.endpoint -/api/2.2/jobs/create job_clusters.new_cluster.cluster_log_conf.s3.kms_key -/api/2.2/jobs/create job_clusters.new_cluster.cluster_log_conf.s3.region -/api/2.2/jobs/create job_clusters.new_cluster.cluster_name -/api/2.2/jobs/create job_clusters.new_cluster.node_type_id -/api/2.2/jobs/create job_clusters.new_cluster.policy_id -/api/2.2/jobs/create job_clusters.new_cluster.single_user_name -/api/2.2/jobs/create job_clusters.new_cluster.spark_version -/api/2.2/jobs/create name -/api/2.2/jobs/create parent_path -/api/2.2/jobs/create usage_policy_id diff --git a/acceptance/bundle/empty_string_dropped/out.requests.direct.json b/acceptance/bundle/empty_string_dropped/out.requests.direct.json index d48b05f9dc0..c9bba29b9fa 100644 --- a/acceptance/bundle/empty_string_dropped/out.requests.direct.json +++ b/acceptance/bundle/empty_string_dropped/out.requests.direct.json @@ -5,11 +5,8 @@ "no_compute": "true" }, "body": { - "budget_policy_id": "", "description": "", - "name": "test-app-direct", - "space": "", - "usage_policy_id": "" + "name": "test-app-direct" } } { @@ -17,15 +14,13 @@ "path": "/api/2.0/database/instances", "body": { "capacity": "CU_1", - "name": "test-db-instance", - "usage_policy_id": "" + "name": "test-db-instance" } } { "method": "POST", "path": "/api/2.0/mlflow/experiments/create", "body": { - "artifact_location": "", "name": "/Users/[USERNAME]/test-experiment" } } @@ -33,7 +28,6 @@ "method": "POST", "path": "/api/2.0/mlflow/registered-models/create", "body": { - "description": "", "name": "test-model" } } @@ -41,21 +35,13 @@ "method": "POST", "path": "/api/2.0/pipelines", "body": { - "budget_policy_id": "", "channel": "CURRENT", "clusters": [ { "aws_attributes": { - "availability": "SPOT", - "instance_profile_arn": "", - "zone_id": "" + "availability": "SPOT" }, - "driver_instance_pool_id": "", - "driver_node_type_id": "", - "instance_pool_id": "", - "label": "default", - "node_type_id": "", - "policy_id": "" + "label": "default" } ], "deployment": { @@ -67,15 +53,10 @@ { "file": { "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files/pipeline.py" - }, - "jar": "", - "whl": "" + } } ], - "name": "", - "root_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files", - "serverless_compute_id": "", - "usage_policy_id": "" + "root_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files" } } { @@ -85,7 +66,6 @@ "auto_stop_mins": 10, "cluster_size": "2X-Small", "enable_photon": true, - "instance_profile_arn": "", "max_num_clusters": 1, "min_num_clusters": 1, "name": "test-warehouse", @@ -98,26 +78,15 @@ "body": { "autotermination_minutes": 60, "aws_attributes": { - "availability": "SPOT", - "instance_profile_arn": "", - "zone_id": "" + "availability": "SPOT" }, "cluster_log_conf": { "s3": { - "canned_acl": "", - "destination": "s3://test-bucket/logs", - "encryption_type": "", - "endpoint": "", - "kms_key": "", - "region": "" + "destination": "s3://test-bucket/logs" } }, - "cluster_name": "", - "node_type_id": "", "num_workers": 1, - "policy_id": "", - "single_user_name": "", - "spark_version": "" + "spark_version": "13.3.x-scala2.12" } } { @@ -125,15 +94,8 @@ "path": "/api/2.1/unity-catalog/models", "body": { "catalog_name": "main", - "comment": "", - "created_by": "", - "full_name": "", - "metastore_id": "", "name": "test-registered-model", - "owner": "", - "schema_name": "default", - "storage_location": "", - "updated_by": "" + "schema_name": "default" } } { @@ -141,9 +103,7 @@ "path": "/api/2.1/unity-catalog/schemas", "body": { "catalog_name": "main", - "comment": "", - "name": "test-schema", - "storage_root": "" + "name": "test-schema" } } { @@ -151,10 +111,8 @@ "path": "/api/2.1/unity-catalog/volumes", "body": { "catalog_name": "main", - "comment": "", "name": "test-volume", "schema_name": "default", - "storage_location": "", "volume_type": "MANAGED" } } @@ -162,12 +120,10 @@ "method": "POST", "path": "/api/2.2/jobs/create", "body": { - "budget_policy_id": "", "deployment": { "kind": "BUNDLE", "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/state/metadata.json" }, - "description": "", "edit_mode": "UI_LOCKED", "format": "MULTI_TASK", "git_source": { @@ -180,35 +136,21 @@ "job_cluster_key": "main", "new_cluster": { "aws_attributes": { - "availability": "SPOT", - "instance_profile_arn": "", - "zone_id": "" + "availability": "SPOT" }, "cluster_log_conf": { "s3": { - "canned_acl": "", - "destination": "s3://test-bucket/logs", - "encryption_type": "", - "endpoint": "", - "kms_key": "", - "region": "" + "destination": "s3://test-bucket/logs" } }, - "cluster_name": "", - "node_type_id": "", "num_workers": 1, - "policy_id": "", - "single_user_name": "", - "spark_version": "" + "spark_version": "13.3.x-scala2.12" } } ], "max_concurrent_runs": 1, - "name": "", - "parent_path": "", "queue": { "enabled": true - }, - "usage_policy_id": "" + } } } diff --git a/acceptance/bundle/empty_string_dropped/out.requests.terraform.json b/acceptance/bundle/empty_string_dropped/out.requests.terraform.json index 9ace00bae97..b2e9c328cd2 100644 --- a/acceptance/bundle/empty_string_dropped/out.requests.terraform.json +++ b/acceptance/bundle/empty_string_dropped/out.requests.terraform.json @@ -5,11 +5,8 @@ "no_compute": "true" }, "body": { - "budget_policy_id": "", "description": "", - "name": "test-app-tf", - "space": "", - "usage_policy_id": "" + "name": "test-app-tf" } } { @@ -17,8 +14,7 @@ "path": "/api/2.0/database/instances", "body": { "capacity": "CU_1", - "name": "test-db-instance", - "usage_policy_id": "" + "name": "test-db-instance" } } { @@ -90,7 +86,7 @@ } }, "num_workers": 1, - "spark_version": "" + "spark_version": "13.3.x-scala2.12" } } { @@ -147,11 +143,13 @@ "destination": "s3://test-bucket/logs" } }, - "num_workers": 1 + "num_workers": 1, + "spark_version": "13.3.x-scala2.12" } } ], "max_concurrent_runs": 1, + "name": "Untitled", "queue": { "enabled": true } diff --git a/acceptance/bundle/empty_string_dropped/out.validate.json b/acceptance/bundle/empty_string_dropped/out.validate.json index dee47c7f4b9..e627b2a428f 100644 --- a/acceptance/bundle/empty_string_dropped/out.validate.json +++ b/acceptance/bundle/empty_string_dropped/out.validate.json @@ -1,61 +1,43 @@ { "apps": { "my_app": { - "budget_policy_id": "", "description": "", "name": "test-app-direct", - "source_code_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files/app", - "space": "", - "usage_policy_id": "" + "source_code_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files/app" } }, "clusters": { "my_cluster": { "autotermination_minutes": 60, "aws_attributes": { - "availability": "SPOT", - "instance_profile_arn": "", - "zone_id": "" + "availability": "SPOT" }, "cluster_log_conf": { "s3": { - "canned_acl": "", - "destination": "s3://test-bucket/logs", - "encryption_type": "", - "endpoint": "", - "kms_key": "", - "region": "" + "destination": "s3://test-bucket/logs" } }, - "cluster_name": "", - "node_type_id": "", "num_workers": 1, - "policy_id": "", - "single_user_name": "", - "spark_version": "" + "spark_version": "13.3.x-scala2.12" } }, "database_instances": { "my_db_instance": { "capacity": "CU_1", - "name": "test-db-instance", - "usage_policy_id": "" + "name": "test-db-instance" } }, "experiments": { "my_experiment": { - "artifact_location": "", "name": "/Users/[USERNAME]/test-experiment" } }, "jobs": { "my_job": { - "budget_policy_id": "", "deployment": { "kind": "BUNDLE", "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/state/metadata.json" }, - "description": "", "edit_mode": "UI_LOCKED", "format": "MULTI_TASK", "git_source": { @@ -68,61 +50,38 @@ "job_cluster_key": "main", "new_cluster": { "aws_attributes": { - "availability": "SPOT", - "instance_profile_arn": "", - "zone_id": "" + "availability": "SPOT" }, "cluster_log_conf": { "s3": { - "canned_acl": "", - "destination": "s3://test-bucket/logs", - "encryption_type": "", - "endpoint": "", - "kms_key": "", - "region": "" + "destination": "s3://test-bucket/logs" } }, - "cluster_name": "", - "node_type_id": "", "num_workers": 1, - "policy_id": "", - "single_user_name": "", - "spark_version": "" + "spark_version": "13.3.x-scala2.12" } } ], "max_concurrent_runs": 1, - "name": "", - "parent_path": "", "queue": { "enabled": true - }, - "usage_policy_id": "" + } } }, "models": { "my_model": { - "description": "", "name": "test-model" } }, "pipelines": { "my_pipeline": { - "budget_policy_id": "", "channel": "CURRENT", "clusters": [ { "aws_attributes": { - "availability": "SPOT", - "instance_profile_arn": "", - "zone_id": "" + "availability": "SPOT" }, - "driver_instance_pool_id": "", - "driver_node_type_id": "", - "instance_pool_id": "", - "label": "default", - "node_type_id": "", - "policy_id": "" + "label": "default" } ], "deployment": { @@ -134,37 +93,23 @@ { "file": { "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files/pipeline.py" - }, - "jar": "", - "whl": "" + } } ], - "name": "", - "root_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files", - "serverless_compute_id": "", - "usage_policy_id": "" + "root_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/direct/files" } }, "registered_models": { "my_registered_model": { "catalog_name": "main", - "comment": "", - "created_by": "", - "full_name": "", - "metastore_id": "", "name": "test-registered-model", - "owner": "", - "schema_name": "default", - "storage_location": "", - "updated_by": "" + "schema_name": "default" } }, "schemas": { "my_schema": { "catalog_name": "main", - "comment": "", - "name": "test-schema", - "storage_root": "" + "name": "test-schema" } }, "sql_warehouses": { @@ -172,7 +117,6 @@ "auto_stop_mins": 10, "cluster_size": "2X-Small", "enable_photon": true, - "instance_profile_arn": "", "max_num_clusters": 1, "min_num_clusters": 1, "name": "test-warehouse", @@ -182,10 +126,8 @@ "volumes": { "my_volume": { "catalog_name": "main", - "comment": "", "name": "test-volume", "schema_name": "default", - "storage_location": "", "volume_path": "/Volumes/main/default/test-volume", "volume_type": "MANAGED" } diff --git a/acceptance/bundle/empty_string_variable/databricks.yml b/acceptance/bundle/empty_string_variable/databricks.yml new file mode 100644 index 00000000000..88e38c86cf3 --- /dev/null +++ b/acceptance/bundle/empty_string_variable/databricks.yml @@ -0,0 +1,16 @@ +bundle: + name: test-bundle + +variables: + policy: + default: "" + +resources: + clusters: + my_cluster: + num_workers: 1 + spark_version: 13.3.x-scala2.12 + # The original bug: a variable resolves to "" and policy_id (omitempty) was + # force-sent, so the backend rejected it with "'' is not a valid cluster + # policy ID". It must now be dropped from the resolved config. + policy_id: ${var.policy} diff --git a/acceptance/bundle/empty_string_variable/out.test.toml b/acceptance/bundle/empty_string_variable/out.test.toml new file mode 100644 index 00000000000..e90b6d5d1ba --- /dev/null +++ b/acceptance/bundle/empty_string_variable/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/empty_string_variable/output.txt b/acceptance/bundle/empty_string_variable/output.txt new file mode 100644 index 00000000000..8b89eda8902 --- /dev/null +++ b/acceptance/bundle/empty_string_variable/output.txt @@ -0,0 +1,7 @@ + +>>> [CLI] bundle validate -o json +{ + "spark_version": "13.3.x-scala2.12", + "policy_id": null, + "has_policy_id": false +} diff --git a/acceptance/bundle/empty_string_variable/script b/acceptance/bundle/empty_string_variable/script new file mode 100644 index 00000000000..73a83e9bcce --- /dev/null +++ b/acceptance/bundle/empty_string_variable/script @@ -0,0 +1,3 @@ +# policy_id resolves to "" via the variable. It must be absent from the resolved +# config (dropped as an empty omitempty string), not present as "". +trace $CLI bundle validate -o json | jq '.resources.clusters.my_cluster | {spark_version, policy_id, has_policy_id: has("policy_id")}' diff --git a/acceptance/bundle/empty_string_variable/test.toml b/acceptance/bundle/empty_string_variable/test.toml new file mode 100644 index 00000000000..6aab6ad6906 --- /dev/null +++ b/acceptance/bundle/empty_string_variable/test.toml @@ -0,0 +1,2 @@ +# validate does not deploy, so engine choice is irrelevant: run once on direct. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/bundle/config/mutator/drop_empty_strings.go b/bundle/config/mutator/drop_empty_strings.go new file mode 100644 index 00000000000..0d4eba75f0b --- /dev/null +++ b/bundle/config/mutator/drop_empty_strings.go @@ -0,0 +1,62 @@ +package mutator + +import ( + "context" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" +) + +type dropEmptyStrings struct{} + +// DropEmptyStrings removes empty-string values on omitempty resource fields, so +// they are not force-sent to the backend. An empty string reaches this point +// either literally (policy_id: "") or via a variable that resolved to "", so +// this must run after variable resolution. +// +// Both engines convert the resolved config through convert.ToTyped, which +// force-sends explicitly-set zero values, defeating the omitempty tag. Dropping +// here fixes it uniformly for terraform and direct and makes the result visible +// in `bundle validate -o json`, which serializes the dynamic value. +func DropEmptyStrings() bundle.Mutator { + return &dropEmptyStrings{} +} + +func (m *dropEmptyStrings) Name() string { + return "DropEmptyStrings" +} + +func (m *dropEmptyStrings) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { + root, err := dyn.Map(root, "resources", func(_ dyn.Path, resources dyn.Value) (dyn.Value, error) { + // Normalize against the resources type so omitempty is known per field. + // Only DropEmptyStrings is set: existing values are kept as-is otherwise. + out, normDiags := convert.Normalize(config.Resources{}, resources, convert.DropEmptyStrings) + diags = diags.Extend(normDiags) + return out, nil + }) + if err != nil { + return root, err + } + + // It seems safe to send an empty description "" to the backend, but for apps + // it causes drift in terraform: the Apps API always returns "" for an unset + // description, so bundle/deploy/terraform/tfdyn/convert_app.go injects it + // anyway. To avoid an unnecessary difference between the direct and terraform + // engines, keep the empty apps description here instead of dropping it. + return dyn.MapByPattern(root, dyn.NewPattern(dyn.Key("resources"), dyn.Key("apps"), dyn.AnyKey()), func(_ dyn.Path, app dyn.Value) (dyn.Value, error) { + if _, err := dyn.Get(app, "description"); err != nil { + return dyn.Set(app, "description", dyn.V("")) + } + return app, nil + }) + }) + if err != nil { + diags = diags.Extend(diag.FromErr(err)) + } + return diags +} diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index bfa2af4124b..31c37064029 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -154,6 +154,12 @@ func Initialize(ctx context.Context, b *bundle.Bundle) { mutator.InitializeVolumePaths(), mutator.ResolveVolumePathReferencesOnlyResources(), + // Drop empty-string values on omitempty resource fields so they are not + // force-sent to the backend. Runs after variable resolution (a variable may + // resolve to "") and after all resource mutations, before validation so that + // required fields (no omitempty) still error if empty. + mutator.DropEmptyStrings(), + // Resolve --select selectors against the materialized resources: normalize // each to its "type.name" form and validate it exists. Runs after all resource // mutations so that dynamically added resources are visible. This does not diff --git a/libs/dyn/convert/normalize.go b/libs/dyn/convert/normalize.go index 0d14b78ea2f..2e13cfa3fa4 100644 --- a/libs/dyn/convert/normalize.go +++ b/libs/dyn/convert/normalize.go @@ -18,10 +18,19 @@ const ( // IncludeMissingFields causes the normalization to include fields that defined on the given // type but are missing in the source value. They are included with their zero values. IncludeMissingFields NormalizeOption = iota + + // DropEmptyStrings drops struct fields whose value is an empty string, unless + // the field is tagged without omitempty (i.e. its zero value must be sent). + // This mirrors JSON serialization, where an omitempty string field set to "" + // is omitted from the request. Without this, an explicitly-set "" reaches + // ToTyped, which force-sends it, and the backend rejects it (e.g. + // "'' is not a valid cluster policy ID"). + DropEmptyStrings ) type normalizeOptions struct { includeMissingFields bool + dropEmptyStrings bool } func Normalize(dst any, src dyn.Value, opts ...NormalizeOption) (dyn.Value, diag.Diagnostics) { @@ -30,6 +39,8 @@ func Normalize(dst any, src dyn.Value, opts ...NormalizeOption) (dyn.Value, diag switch opt { case IncludeMissingFields: n.includeMissingFields = true + case DropEmptyStrings: + n.dropEmptyStrings = true } } @@ -166,6 +177,15 @@ func (n normalizeOptions) normalizeStruct(typ reflect.Type, src dyn.Value, seen } } + // Drop an empty string on an omitempty field so it is not force-sent + // to the backend (see DropEmptyStrings). ForceEmpty marks fields whose + // zero value must be kept, so those are left in place. + if n.dropEmptyStrings && !info.ForceEmpty[fieldName] { + if s, ok := nv.AsString(); ok && s == "" { + continue + } + } + out.SetLoc(pk.MustString(), pk.Locations(), nv) } diff --git a/libs/dyn/convert/normalize_test.go b/libs/dyn/convert/normalize_test.go index 1cde66473b2..69000200293 100644 --- a/libs/dyn/convert/normalize_test.go +++ b/libs/dyn/convert/normalize_test.go @@ -974,3 +974,40 @@ func TestNormalizeAnyFromTime(t *testing.T) { assert.Empty(t, err) assert.Equal(t, dyn.NewValue("2024-08-29", vin.Locations()), vout) } + +func TestNormalizeStructDropEmptyStrings(t *testing.T) { + type Tmp struct { + // Optional: dropped when empty. + Opt string `json:"opt,omitempty"` + // Required (no omitempty): kept even when empty. + Req string `json:"req"` + } + + vin := dyn.V(map[string]dyn.Value{ + "opt": dyn.V(""), + "req": dyn.V(""), + }) + + vout, diags := Normalize(Tmp{}, vin, DropEmptyStrings) + assert.Empty(t, diags) + + _, hasOpt := vout.MustMap().Get(dyn.V("opt")) + assert.False(t, hasOpt, "empty omitempty string should be dropped") + req, hasReq := vout.MustMap().Get(dyn.V("req")) + assert.True(t, hasReq, "empty non-omitempty string should be kept") + assert.Empty(t, req.MustString()) +} + +func TestNormalizeStructDropEmptyStringsKeepsNonEmpty(t *testing.T) { + type Tmp struct { + Opt string `json:"opt,omitempty"` + } + + vin := dyn.V(map[string]dyn.Value{"opt": dyn.V("value")}) + + vout, diags := Normalize(Tmp{}, vin, DropEmptyStrings) + assert.Empty(t, diags) + opt, ok := vout.MustMap().Get(dyn.V("opt")) + assert.True(t, ok) + assert.Equal(t, "value", opt.MustString()) +}