diff --git a/.nextchanges/bundles/invalid-identifiers.md b/.nextchanges/bundles/invalid-identifiers.md new file mode 100644 index 00000000000..796251a3d74 --- /dev/null +++ b/.nextchanges/bundles/invalid-identifiers.md @@ -0,0 +1,3 @@ +Reject empty, blank, or control-character resource identifiers and incomplete pipeline library paths during bundle validation. + +Configs that previously only warned on missing names (for example models and apps) now fail validate. Explicit empty strings for UC parent fields such as catalog_name are rejected; omitted parents still warn. diff --git a/acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl b/acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl deleted file mode 100644 index 290f0ef2d0d..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/databricks.yml.tmpl +++ /dev/null @@ -1,7 +0,0 @@ -bundle: - name: catalog-empty-name-$UNIQUE_NAME - -resources: - catalogs: - mycatalog: - name: "" diff --git a/acceptance/bundle/resources/catalogs/empty-name/out.test.toml b/acceptance/bundle/resources/catalogs/empty-name/out.test.toml deleted file mode 100644 index 8c52d40aa2d..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/out.test.toml +++ /dev/null @@ -1,3 +0,0 @@ -Cloud = true -RequiresUnityCatalog = true -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/catalogs/empty-name/output.txt b/acceptance/bundle/resources/catalogs/empty-name/output.txt deleted file mode 100644 index 51b04630435..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/output.txt +++ /dev/null @@ -1,11 +0,0 @@ - ->>> musterr [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/catalog-empty-name-[UNIQUE_NAME]/default/files... -Error: cannot create resources.catalogs.mycatalog: Invalid input: RPC CreateCatalog Field managedcatalog.CatalogInfo.name: name "" is not a valid name. Valid names cannot contain spaces, periods, forward slashes, or control characters. (400 INVALID_PARAMETER_VALUE) - -Endpoint: POST [DATABRICKS_URL]/api/2.1/unity-catalog/catalogs -HTTP Status: 400 Bad Request -API error_code: INVALID_PARAMETER_VALUE -API message: Invalid input: RPC CreateCatalog Field managedcatalog.CatalogInfo.name: name "" is not a valid name. Valid names cannot contain spaces, periods, forward slashes, or control characters. - -Files: 5 uploaded, 0 deleted diff --git a/acceptance/bundle/resources/catalogs/empty-name/script b/acceptance/bundle/resources/catalogs/empty-name/script deleted file mode 100644 index dc9e56639a9..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/script +++ /dev/null @@ -1,5 +0,0 @@ -# The deploy fails only after the files are uploaded, so $UNIQUE_NAME in the bundle -# name keeps concurrent cloud legs apart and lets the sweeper find what is left. -envsubst < databricks.yml.tmpl > databricks.yml - -trace musterr $CLI bundle deploy diff --git a/acceptance/bundle/resources/catalogs/empty-name/test.toml b/acceptance/bundle/resources/catalogs/empty-name/test.toml deleted file mode 100644 index 829c6239443..00000000000 --- a/acceptance/bundle/resources/catalogs/empty-name/test.toml +++ /dev/null @@ -1,9 +0,0 @@ -# The golden asserts UC's message verbatim, so run on cloud to catch it drifting. -Cloud = true -RequiresUnityCatalog = true -RecordRequests = false -Ignore = [".databricks"] - -# Terraform rejects catalog resources before any API call, so there is nothing to -# assert on that engine. -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl b/acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl deleted file mode 100644 index a5fd377721f..00000000000 --- a/acceptance/bundle/resources/models/empty-name/databricks.yml.tmpl +++ /dev/null @@ -1,7 +0,0 @@ -bundle: - name: model-empty-name-$UNIQUE_NAME - -resources: - models: - mymodel: - name: "" diff --git a/acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt b/acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt deleted file mode 100644 index 50232dbed92..00000000000 --- a/acceptance/bundle/resources/models/empty-name/out.deploy.direct.txt +++ /dev/null @@ -1,11 +0,0 @@ - ->>> musterr [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/model-empty-name-[UNIQUE_NAME]/default/files... -Error: cannot create resources.models.mymodel: Got an invalid name ''. Registered Model names cannot be empty strings. (400 INVALID_PARAMETER_VALUE) - -Endpoint: POST [DATABRICKS_URL]/api/2.0/mlflow/registered-models/create -HTTP Status: 400 Bad Request -API error_code: INVALID_PARAMETER_VALUE -API message: Got an invalid name ''. Registered Model names cannot be empty strings. - -Files: 6 uploaded, 0 deleted diff --git a/acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt b/acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt deleted file mode 100644 index adb61687bf8..00000000000 --- a/acceptance/bundle/resources/models/empty-name/out.deploy.terraform.txt +++ /dev/null @@ -1,14 +0,0 @@ - ->>> musterr [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/model-empty-name-[UNIQUE_NAME]/default/files... -Error: terraform apply: exit status 1 - -Error: cannot create mlflow model: Got an invalid name ''. Registered Model names cannot be empty strings. - - with databricks_mlflow_model.mymodel, - on bundle.tf.json line 17, in resource.databricks_mlflow_model.mymodel: - 17: } - - - -Files: 6 uploaded, 0 deleted diff --git a/acceptance/bundle/resources/models/empty-name/output.txt b/acceptance/bundle/resources/models/empty-name/output.txt deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/acceptance/bundle/resources/models/empty-name/script b/acceptance/bundle/resources/models/empty-name/script deleted file mode 100644 index 0336c9c6ca0..00000000000 --- a/acceptance/bundle/resources/models/empty-name/script +++ /dev/null @@ -1,7 +0,0 @@ -# The deploy fails only after the files are uploaded, so $UNIQUE_NAME in the bundle -# name keeps concurrent cloud legs apart and lets the sweeper find what is left. -envsubst < databricks.yml.tmpl > databricks.yml - -# Both engines reach the create call, but terraform wraps the message in its own -# output, so the goldens are per-engine. -trace musterr $CLI bundle deploy &> out.deploy.$DATABRICKS_BUNDLE_ENGINE.txt diff --git a/acceptance/bundle/resources/models/empty-name/test.toml b/acceptance/bundle/resources/models/empty-name/test.toml deleted file mode 100644 index 64c48c920e6..00000000000 --- a/acceptance/bundle/resources/models/empty-name/test.toml +++ /dev/null @@ -1,4 +0,0 @@ -# The golden asserts MLflow's message verbatim, so run on cloud to catch it drifting. -Cloud = true -RecordRequests = false -Ignore = [".databricks"] diff --git a/acceptance/bundle/validate/dashboard_required_name/output.txt b/acceptance/bundle/validate/dashboard_required_name/output.txt index 3c185500b6e..353c6ce93af 100644 --- a/acceptance/bundle/validate/dashboard_required_name/output.txt +++ b/acceptance/bundle/validate/dashboard_required_name/output.txt @@ -1,7 +1,7 @@ >>> [CLI] bundle validate Error: dashboard display_name is required - at resources.dashboards.my_dashboard + at resources.dashboards.my_dashboard.display_name in databricks.yml:8:7 Name: test-bundle diff --git a/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt b/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt index 9e0b38d9271..2160d3288ef 100644 --- a/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt +++ b/acceptance/bundle/validate/dashboard_required_warehouse_id/output.txt @@ -1,7 +1,7 @@ >>> [CLI] bundle validate Error: dashboard warehouse_id is required - at resources.dashboards.my_dashboard + at resources.dashboards.my_dashboard.warehouse_id in databricks.yml:8:7 Name: test-bundle diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index 0ff8111602e..2895a29ecb3 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -33,8 +33,8 @@ } === resources.models.rname === -Warning: required field "name" is not set - at resources.models.rname +Error: model name is required + at resources.models.rname.name in databricks.yml:6:12 { @@ -53,6 +53,10 @@ Warning: required field "name" is not set } === resources.registered_models.rname === +Error: registered_model name is required + at resources.registered_models.rname.name + in databricks.yml:6:12 + { "registered_models": { "rname": {} @@ -79,11 +83,11 @@ Warning: required field "table_name" is not set } === resources.schemas.rname === -Warning: required field "catalog_name" is not set - at resources.schemas.rname +Error: schema name is required + at resources.schemas.rname.name in databricks.yml:6:12 -Warning: required field "name" is not set +Warning: required field "catalog_name" is not set at resources.schemas.rname in databricks.yml:6:12 @@ -94,11 +98,11 @@ Warning: required field "name" is not set } === resources.volumes.rname === -Warning: required field "catalog_name" is not set - at resources.volumes.rname +Error: volume name is required + at resources.volumes.rname.name in databricks.yml:6:12 -Warning: required field "name" is not set +Warning: required field "catalog_name" is not set at resources.volumes.rname in databricks.yml:6:12 @@ -126,11 +130,11 @@ Warning: required field "schema_name" is not set === resources.dashboards.rname === Error: dashboard display_name is required - at resources.dashboards.rname + at resources.dashboards.rname.display_name in databricks.yml:6:12 Error: dashboard warehouse_id is required - at resources.dashboards.rname + at resources.dashboards.rname.warehouse_id in databricks.yml:6:12 { @@ -143,15 +147,10 @@ Error: dashboard warehouse_id is required } === resources.apps.rname === -Warning: required field "name" is not set - at resources.apps.rname - in databricks.yml:6:12 - -Error: Missing app source code path or git source +Error: app name is required + at resources.apps.rname.name in databricks.yml:6:12 -app resource 'rname' should have either source_code_path or git_source field - { "apps": { "rname": { @@ -162,7 +161,7 @@ app resource 'rname' should have either source_code_path or git_source field === resources.sql_warehouses.rname === Error: sql_warehouse name is required - at resources.sql_warehouses.rname + at resources.sql_warehouses.rname.name in databricks.yml:6:12 { @@ -177,8 +176,8 @@ Error: sql_warehouse name is required } === resources.secret_scopes.rname === -Warning: required field "name" is not set - at resources.secret_scopes.rname +Error: secret_scope name is required + at resources.secret_scopes.rname.name in databricks.yml:6:12 { @@ -188,8 +187,8 @@ Warning: required field "name" is not set } === resources.alerts.rname === -Warning: required field "display_name" is not set - at resources.alerts.rname +Error: alert display_name is required + at resources.alerts.rname.display_name in databricks.yml:6:12 Warning: required field "evaluation" is not set diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index 38c4dc55d19..132b3387910 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -45,8 +45,8 @@ Warning: unknown field: grants at resources.models.rname in databricks.yml:7:7 -Warning: required field "name" is not set - at resources.models.rname +Error: model name is required + at resources.models.rname.name in databricks.yml:7:7 { @@ -69,6 +69,10 @@ Warning: unknown field: grants } === resources.registered_models.rname === +Error: registered_model name is required + at resources.registered_models.rname.name + in databricks.yml:7:7 + { "registered_models": { "rname": { @@ -101,11 +105,11 @@ Warning: required field "table_name" is not set } === resources.schemas.rname === -Warning: required field "catalog_name" is not set - at resources.schemas.rname +Error: schema name is required + at resources.schemas.rname.name in databricks.yml:7:7 -Warning: required field "name" is not set +Warning: required field "catalog_name" is not set at resources.schemas.rname in databricks.yml:7:7 @@ -118,11 +122,11 @@ Warning: required field "name" is not set } === resources.volumes.rname === -Warning: required field "catalog_name" is not set - at resources.volumes.rname +Error: volume name is required + at resources.volumes.rname.name in databricks.yml:7:7 -Warning: required field "name" is not set +Warning: required field "catalog_name" is not set at resources.volumes.rname in databricks.yml:7:7 @@ -159,11 +163,11 @@ Warning: unknown field: grants in databricks.yml:7:7 Error: dashboard display_name is required - at resources.dashboards.rname + at resources.dashboards.rname.display_name in databricks.yml:7:7 Error: dashboard warehouse_id is required - at resources.dashboards.rname + at resources.dashboards.rname.warehouse_id in databricks.yml:7:7 { @@ -180,15 +184,10 @@ Warning: unknown field: grants at resources.apps.rname in databricks.yml:7:7 -Warning: required field "name" is not set - at resources.apps.rname - in databricks.yml:7:7 - -Error: Missing app source code path or git source +Error: app name is required + at resources.apps.rname.name in databricks.yml:7:7 -app resource 'rname' should have either source_code_path or git_source field - { "apps": { "rname": { @@ -203,7 +202,7 @@ Warning: unknown field: grants in databricks.yml:7:7 Error: sql_warehouse name is required - at resources.sql_warehouses.rname + at resources.sql_warehouses.rname.name in databricks.yml:7:7 { @@ -222,8 +221,8 @@ Warning: unknown field: grants at resources.secret_scopes.rname in databricks.yml:7:7 -Warning: required field "name" is not set - at resources.secret_scopes.rname +Error: secret_scope name is required + at resources.secret_scopes.rname.name in databricks.yml:7:7 { @@ -237,8 +236,8 @@ Warning: unknown field: grants at resources.alerts.rname in databricks.yml:7:7 -Warning: required field "display_name" is not set - at resources.alerts.rname +Error: alert display_name is required + at resources.alerts.rname.display_name in databricks.yml:7:7 Warning: required field "evaluation" is not set diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index cef0b18baa3..fbe4bb946ca 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -33,8 +33,8 @@ } === resources.models.rname === -Warning: required field "name" is not set - at resources.models.rname +Error: model name is required + at resources.models.rname.name in databricks.yml:7:7 { @@ -57,6 +57,10 @@ Warning: unknown field: permissions at resources.registered_models.rname in databricks.yml:7:7 +Error: registered_model name is required + at resources.registered_models.rname.name + in databricks.yml:7:7 + { "registered_models": { "rname": {} @@ -91,11 +95,11 @@ Warning: unknown field: permissions at resources.schemas.rname in databricks.yml:7:7 -Warning: required field "catalog_name" is not set - at resources.schemas.rname +Error: schema name is required + at resources.schemas.rname.name in databricks.yml:7:7 -Warning: required field "name" is not set +Warning: required field "catalog_name" is not set at resources.schemas.rname in databricks.yml:7:7 @@ -110,11 +114,11 @@ Warning: unknown field: permissions at resources.volumes.rname in databricks.yml:7:7 -Warning: required field "catalog_name" is not set - at resources.volumes.rname +Error: volume name is required + at resources.volumes.rname.name in databricks.yml:7:7 -Warning: required field "name" is not set +Warning: required field "catalog_name" is not set at resources.volumes.rname in databricks.yml:7:7 @@ -142,11 +146,11 @@ Warning: required field "schema_name" is not set === resources.dashboards.rname === Error: dashboard display_name is required - at resources.dashboards.rname + at resources.dashboards.rname.display_name in databricks.yml:7:7 Error: dashboard warehouse_id is required - at resources.dashboards.rname + at resources.dashboards.rname.warehouse_id in databricks.yml:7:7 { @@ -159,15 +163,10 @@ Error: dashboard warehouse_id is required } === resources.apps.rname === -Warning: required field "name" is not set - at resources.apps.rname - in databricks.yml:7:7 - -Error: Missing app source code path or git source +Error: app name is required + at resources.apps.rname.name in databricks.yml:7:7 -app resource 'rname' should have either source_code_path or git_source field - { "apps": { "rname": { @@ -178,7 +177,7 @@ app resource 'rname' should have either source_code_path or git_source field === resources.sql_warehouses.rname === Error: sql_warehouse name is required - at resources.sql_warehouses.rname + at resources.sql_warehouses.rname.name in databricks.yml:7:7 { @@ -193,8 +192,8 @@ Error: sql_warehouse name is required } === resources.secret_scopes.rname === -Warning: required field "name" is not set - at resources.secret_scopes.rname +Error: secret_scope name is required + at resources.secret_scopes.rname.name in databricks.yml:7:7 { @@ -204,8 +203,8 @@ Warning: required field "name" is not set } === resources.alerts.rname === -Warning: required field "display_name" is not set - at resources.alerts.rname +Error: alert display_name is required + at resources.alerts.rname.display_name in databricks.yml:7:7 Warning: required field "evaluation" is not set diff --git a/acceptance/bundle/validate/invalid_identifiers/databricks.yml b/acceptance/bundle/validate/invalid_identifiers/databricks.yml new file mode 100644 index 00000000000..73a6f8a65be --- /dev/null +++ b/acceptance/bundle/validate/invalid_identifiers/databricks.yml @@ -0,0 +1,49 @@ +bundle: + name: invalid-identifiers + +resources: + models: + empty_model: + name: "" + + catalogs: + empty_catalog: + name: "" + + volumes: + tab_volume: + name: "tab\there" + catalog_name: main + schema_name: default + volume_type: MANAGED + + tab_catalog: + name: valid + catalog_name: "main\tcatalog" + schema_name: default + volume_type: MANAGED + + empty_catalog_name: + name: valid + catalog_name: "" + schema_name: default + volume_type: MANAGED + + model_serving_endpoints: + newline_endpoint: + name: "line1\nline2" + + vector_search_endpoints: + tab_endpoint: + name: "bad\tname" + endpoint_type: STANDARD + + experiments: + blank_experiment: + name: " " + + pipelines: + incomplete_file: + name: incomplete-file + libraries: + - file: {} diff --git a/acceptance/bundle/resources/models/empty-name/out.test.toml b/acceptance/bundle/validate/invalid_identifiers/out.test.toml similarity index 81% rename from acceptance/bundle/resources/models/empty-name/out.test.toml rename to acceptance/bundle/validate/invalid_identifiers/out.test.toml index 2a13818c13f..98ea5040486 100644 --- a/acceptance/bundle/resources/models/empty-name/out.test.toml +++ b/acceptance/bundle/validate/invalid_identifiers/out.test.toml @@ -1,2 +1,2 @@ -Cloud = true +Cloud = false EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/validate/invalid_identifiers/output.txt b/acceptance/bundle/validate/invalid_identifiers/output.txt new file mode 100644 index 00000000000..e10f4c117dd --- /dev/null +++ b/acceptance/bundle/validate/invalid_identifiers/output.txt @@ -0,0 +1,53 @@ + +>>> musterr [CLI] bundle validate +Error: catalog name is required + at resources.catalogs.empty_catalog.name + in databricks.yml:11:13 + +Error: experiment name must not be blank + at resources.experiments.blank_experiment.name + in databricks.yml:43:13 + +Error: model name is required + at resources.models.empty_model.name + in databricks.yml:7:13 + +Error: model_serving_endpoint name must not contain control characters + at resources.model_serving_endpoints.newline_endpoint.name + in databricks.yml:34:13 + +The value contains U+000A at byte offset 5 + +Error: pipeline library file path is required + at resources.pipelines.incomplete_file.libraries[0].file + in databricks.yml:49:17 + +Error: vector_search_endpoint name must not contain control characters + at resources.vector_search_endpoints.tab_endpoint.name + in databricks.yml:38:13 + +The value contains U+0009 at byte offset 3 + +Error: volume catalog_name is required + at resources.volumes.empty_catalog_name.catalog_name + in databricks.yml:28:21 + +Error: volume catalog_name must not contain control characters + at resources.volumes.tab_catalog.catalog_name + in databricks.yml:22:21 + +The value contains U+0009 at byte offset 4 + +Error: volume name must not contain control characters + at resources.volumes.tab_volume.name + in databricks.yml:15:13 + +The value contains U+0009 at byte offset 3 + +Name: invalid-identifiers +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/invalid-identifiers/default + +Found 9 errors diff --git a/acceptance/bundle/validate/invalid_identifiers/script b/acceptance/bundle/validate/invalid_identifiers/script new file mode 100644 index 00000000000..3d3b920843f --- /dev/null +++ b/acceptance/bundle/validate/invalid_identifiers/script @@ -0,0 +1,2 @@ +# Catch invalid values before deployment reaches the API. +trace musterr $CLI bundle validate diff --git a/acceptance/bundle/validate/models/missing_name/output.txt b/acceptance/bundle/validate/models/missing_name/output.txt index 24e6c1d2ca5..dcbbaa92615 100644 --- a/acceptance/bundle/validate/models/missing_name/output.txt +++ b/acceptance/bundle/validate/models/missing_name/output.txt @@ -1,5 +1,5 @@ -Warning: required field "name" is not set - at resources.models.mymodel +Error: model name is required + at resources.models.mymodel.name in databricks.yml:6:14 Name: test-bundle @@ -8,4 +8,6 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 1 warning +Found 1 error + +Exit code: 1 diff --git a/acceptance/bundle/validate/models/user_id/databricks.yml b/acceptance/bundle/validate/models/user_id/databricks.yml index 61fc1746331..d34c78144bd 100644 --- a/acceptance/bundle/validate/models/user_id/databricks.yml +++ b/acceptance/bundle/validate/models/user_id/databricks.yml @@ -4,4 +4,5 @@ bundle: resources: models: mymodel: + name: mymodel user_id: 123 diff --git a/acceptance/bundle/validate/models/user_id/output.txt b/acceptance/bundle/validate/models/user_id/output.txt index 95c7ddde0e5..ec9c10f52e3 100644 --- a/acceptance/bundle/validate/models/user_id/output.txt +++ b/acceptance/bundle/validate/models/user_id/output.txt @@ -1,10 +1,6 @@ Warning: unknown field: user_id at resources.models.mymodel - in databricks.yml:7:7 - -Warning: required field "name" is not set - at resources.models.mymodel - in databricks.yml:7:7 + in databricks.yml:8:7 Name: test-bundle Target: default @@ -12,4 +8,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 2 warnings +Found 1 warning diff --git a/acceptance/bundle/validate/required/databricks.yml b/acceptance/bundle/validate/required/databricks.yml index 82175fb553c..41d22c9bdda 100644 --- a/acceptance/bundle/validate/required/databricks.yml +++ b/acceptance/bundle/validate/required/databricks.yml @@ -7,15 +7,6 @@ artifacts: - {} resources: - # Required field name is missing. - models: - my_model_1: - description: "hello" - - # Empty string should not trigger a warning. - my_model_2: - name: "" - jobs: my_job_1: tasks: @@ -29,7 +20,6 @@ resources: run_job_task: # Catalog name and schema name are required. - # but are not set. volumes: my_volume: name: "baz" diff --git a/acceptance/bundle/validate/required/output.txt b/acceptance/bundle/validate/required/output.txt index ae3e6dad2f8..13fe5147375 100644 --- a/acceptance/bundle/validate/required/output.txt +++ b/acceptance/bundle/validate/required/output.txt @@ -2,19 +2,15 @@ >>> [CLI] bundle validate Warning: required field "catalog_name" is not set at resources.volumes.my_volume - in databricks.yml:35:7 + in databricks.yml:25:7 Warning: required field "job_id" is not set at resources.jobs.my_job_1.tasks[1].run_job_task - in databricks.yml:29:24 - -Warning: required field "name" is not set - at resources.models.my_model_1 - in databricks.yml:13:7 + in databricks.yml:20:24 Warning: required field "schema_name" is not set at resources.volumes.my_volume - in databricks.yml:35:7 + in databricks.yml:25:7 Warning: required field "source" is not set at artifacts.my_artifact.files[0] @@ -26,4 +22,4 @@ Workspace: User: [USERNAME] Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default -Found 5 warnings +Found 4 warnings diff --git a/acceptance/bundle/validate/sql_warehouse_required_name/output.txt b/acceptance/bundle/validate/sql_warehouse_required_name/output.txt index fc66fed24e1..4bba9b58c00 100644 --- a/acceptance/bundle/validate/sql_warehouse_required_name/output.txt +++ b/acceptance/bundle/validate/sql_warehouse_required_name/output.txt @@ -1,13 +1,13 @@ >>> [CLI] bundle validate Error: sql_warehouse name is required - at resources.sql_warehouses.blank_warehouse - in databricks.yml:11:7 - -Error: sql_warehouse name is required - at resources.sql_warehouses.my_warehouse + at resources.sql_warehouses.my_warehouse.name in databricks.yml:8:7 +Error: sql_warehouse name must not be blank + at resources.sql_warehouses.blank_warehouse.name + in databricks.yml:11:13 + Name: test-bundle Target: default Workspace: diff --git a/acceptance/bundle/validate/volume_defaults/databricks.yml b/acceptance/bundle/validate/volume_defaults/databricks.yml index 159db5c7511..9fb56a3a966 100644 --- a/acceptance/bundle/validate/volume_defaults/databricks.yml +++ b/acceptance/bundle/validate/volume_defaults/databricks.yml @@ -4,10 +4,19 @@ bundle: resources: volumes: v1: + name: v1 + catalog_name: main + schema_name: default volume_type: "" v2: + name: v2 + catalog_name: main + schema_name: default volume_type: "already-set" v3: + name: v3 + catalog_name: main + schema_name: default comment: hello diff --git a/acceptance/bundle/validate/volume_defaults/output.txt b/acceptance/bundle/validate/volume_defaults/output.txt index ecdbd7ae7b7..d92703ffb69 100644 --- a/acceptance/bundle/validate/volume_defaults/output.txt +++ b/acceptance/bundle/validate/volume_defaults/output.txt @@ -1,59 +1,32 @@ -Warning: required field "catalog_name" is not set - at resources.volumes.v2 - in databricks.yml:10:7 - -Warning: required field "catalog_name" is not set - at resources.volumes.v3 - in databricks.yml:13:7 - -Warning: required field "catalog_name" is not set - at resources.volumes.v1 - in databricks.yml:7:7 - -Warning: required field "name" is not set - at resources.volumes.v2 - in databricks.yml:10:7 - -Warning: required field "name" is not set - at resources.volumes.v3 - in databricks.yml:13:7 - -Warning: required field "name" is not set - at resources.volumes.v1 - in databricks.yml:7:7 - -Warning: required field "schema_name" is not set - at resources.volumes.v2 - in databricks.yml:10:7 - -Warning: required field "schema_name" is not set - at resources.volumes.v3 - in databricks.yml:13:7 - -Warning: required field "schema_name" is not set - at resources.volumes.v1 - in databricks.yml:7:7 - Warning: invalid value "" for enum field. Valid values are [EXTERNAL MANAGED] at resources.volumes.v1.volume_type - in databricks.yml:7:20 + in databricks.yml:10:20 Warning: invalid value "already-set" for enum field. Valid values are [EXTERNAL MANAGED] at resources.volumes.v2.volume_type - in databricks.yml:10:20 + in databricks.yml:16:20 { "v1": { - "volume_path": "/Volumes///", + "catalog_name": "main", + "name": "v1", + "schema_name": "default", + "volume_path": "/Volumes/main/default/v1", "volume_type": "" }, "v2": { - "volume_path": "/Volumes///", + "catalog_name": "main", + "name": "v2", + "schema_name": "default", + "volume_path": "/Volumes/main/default/v2", "volume_type": "already-set" }, "v3": { + "catalog_name": "main", "comment": "hello", - "volume_path": "/Volumes///", + "name": "v3", + "schema_name": "default", + "volume_path": "/Volumes/main/default/v3", "volume_type": "MANAGED" } } diff --git a/bundle/config/validate/invalid_identifiers.go b/bundle/config/validate/invalid_identifiers.go new file mode 100644 index 00000000000..2182f30f23f --- /dev/null +++ b/bundle/config/validate/invalid_identifiers.go @@ -0,0 +1,171 @@ +package validate + +import ( + "context" + "fmt" + "strings" + "sync" + "unicode" + "unicode/utf8" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/internal/validation/generated" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +type compiledIdentifierRules struct { + trie *dyn.TrieNode + fields map[string][]string +} + +// identifierFields explicitly defines identifier semantics instead of assuming every +// field ending in "_name" is an identifier. True means omission is also an error. +var identifierFields = map[string]bool{ + "catalog_name": false, + "credential_name": false, + "database_instance_name": false, + "database_name": false, + "display_name": true, + "endpoint_name": false, + "instance_pool_name": true, + "name": true, + "output_schema_name": false, + "schema_name": false, + "table_name": false, +} + +// supplementalIdentifierFields covers backend requirements absent from OpenAPI. +var supplementalIdentifierFields = map[string][]string{ + "resources.dashboards.*": {"display_name"}, + "resources.registered_models.*": {"catalog_name", "name", "schema_name"}, + "resources.sql_warehouses.*": {"name"}, +} + +var identifierRules = sync.OnceValues(compileIdentifierRules) + +func compileIdentifierRules() (compiledIdentifierRules, error) { + fields := make(map[string][]string) + for pattern, required := range generated.RequiredFields { + if !isIdentifierObjectPattern(pattern) { + continue + } + for _, field := range required { + if _, ok := identifierFields[field]; ok { + fields[pattern] = append(fields[pattern], field) + } + } + } + for pattern, supplemental := range supplementalIdentifierFields { + fields[pattern] = append(fields[pattern], supplemental...) + } + + trie := &dyn.TrieNode{} + for value := range fields { + pattern, err := dyn.NewPatternFromString(value) + if err != nil { + return compiledIdentifierRules{}, fmt.Errorf("invalid pattern %q for identifier validation: %w", value, err) + } + if err := trie.Insert(pattern); err != nil { + return compiledIdentifierRules{}, fmt.Errorf("failed to insert pattern %q into trie: %w", value, err) + } + } + return compiledIdentifierRules{trie: trie, fields: fields}, nil +} + +// validateIdentifiers rejects values that the backend cannot use as identifiers. +func validateIdentifiers(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + rules, err := identifierRules() + if err != nil { + return diag.FromErr(err) + } + + var diags diag.Diagnostics + err = dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { + pattern, ok := rules.trie.SearchPath(path) + if !ok { + return nil + } + for _, name := range rules.fields[pattern.String()] { + field := value.Get(name) + if field.Kind() == dyn.KindInvalid || field.Kind() == dyn.KindNil { + if identifierFields[name] { + diags = diags.Append(identifierDiag(b, path, name, "is required", "")) + } + continue + } + if field.Kind() != dyn.KindString { + continue + } + reason, detail := invalidIdentifierReason(field.MustString()) + if reason != "" { + diags = diags.Append(identifierDiag(b, path, name, reason, detail)) + } + } + return nil + }) + if err != nil { + return diag.FromErr(err) + } + return diags +} + +func missingIdentifierIsError(pattern, field string) bool { + return isIdentifierObjectPattern(pattern) && identifierFields[field] +} + +func isIdentifierObjectPattern(pattern string) bool { + if pattern == "bundle" { + return true + } + parts := strings.Split(pattern, ".") + return len(parts) == 3 && parts[0] == "resources" && parts[2] == "*" +} + +func identifierDiag(b *bundle.Bundle, resourcePath dyn.Path, field, reason, detail string) diag.Diagnostic { + fieldPath := resourcePath.Append(dyn.Key(field)) + return diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("%s %s %s", requiredObjectName(resourcePath), field, reason), + Detail: detail, + Locations: locationsFor(b, fieldPath, resourcePath), + Paths: []dyn.Path{fieldPath}, + } +} + +func locationsFor(b *bundle.Bundle, path, fallback dyn.Path) []dyn.Location { + if v, err := dyn.GetByPath(b.Config.Value(), path); err == nil && len(v.Locations()) > 0 { + return v.Locations() + } + v, err := dyn.GetByPath(b.Config.Value(), fallback) + if err != nil { + return nil + } + return v.Locations() +} + +func invalidIdentifierReason(value string) (reason, detail string) { + if value == "" { + return "is required", "" + } + if i := strings.IndexFunc(value, unicode.IsControl); i >= 0 { + r, _ := utf8.DecodeRuneInString(value[i:]) + return "must not contain control characters", fmt.Sprintf("The value contains %U at byte offset %d", r, i) + } + if strings.TrimSpace(value) == "" { + return "must not be blank", "" + } + return "", "" +} + +func requiredObjectName(path dyn.Path) string { + if len(path) == 1 { + return path[0].Key() + } + plural := path[1].Key() + if desc, ok := config.SupportedResources()[plural]; ok && desc.SingularName != "" { + return desc.SingularName + } + return plural +} diff --git a/bundle/config/validate/invalid_identifiers_test.go b/bundle/config/validate/invalid_identifiers_test.go new file mode 100644 index 00000000000..86d0a1fb8d5 --- /dev/null +++ b/bundle/config/validate/invalid_identifiers_test.go @@ -0,0 +1,228 @@ +package validate_test + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/databricks/databricks-sdk-go/service/ml" + "github.com/databricks/databricks-sdk-go/service/serving" + "github.com/databricks/databricks-sdk-go/service/vectorsearch" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequiredRejectsEmptyAndControlCharIdentifiers(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Models: map[string]*resources.MlflowModel{ + "empty": {CreateModelRequest: ml.CreateModelRequest{Name: ""}}, + }, + Catalogs: map[string]*resources.Catalog{ + "blank": {CreateCatalog: catalog.CreateCatalog{Name: " "}}, + }, + Volumes: map[string]*resources.Volume{ + "ctrl": { + CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ + Name: "tab\there", + CatalogName: "main", + SchemaName: "default", + VolumeType: catalog.VolumeTypeManaged, + }, + }, + "empty_parent": { + CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ + Name: "v", + CatalogName: "", + SchemaName: "default", + VolumeType: catalog.VolumeTypeManaged, + }, + }, + }, + ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ + "nl": { + CreateServingEndpoint: serving.CreateServingEndpoint{ + Name: "line1\nline2", + }, + }, + }, + VectorSearchEndpoints: map[string]*resources.VectorSearchEndpoint{ + "vse": { + CreateEndpoint: vectorsearch.CreateEndpoint{ + Name: "bad\tname", + EndpointType: vectorsearch.EndpointTypeStandard, + }, + }, + }, + Experiments: map[string]*resources.MlflowExperiment{ + "e": {CreateExperiment: ml.CreateExperiment{Name: ""}}, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + + assert.ElementsMatch(t, []string{ + "catalog name must not be blank", + "model name is required", + "volume name must not contain control characters", + "model_serving_endpoint name must not contain control characters", + "vector_search_endpoint name must not contain control characters", + "experiment name is required", + // empty catalog_name is omitted by FromTyped(omitempty) in tests; warning only. + "required field \"catalog_name\" is not set", + }, diagSummaries(diags)) +} + +func TestRequiredRejectsExplicitEmptyUCParentInDyn(t *testing.T) { + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "volumes": dyn.V(map[string]dyn.Value{ + "v": dyn.V(map[string]dyn.Value{ + "name": dyn.V("v"), + "catalog_name": dyn.V(""), + "schema_name": dyn.V("default"), + "volume_type": dyn.V("MANAGED"), + }), + }), + }), + }), nil + })) + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.Contains(t, diagSummaries(diags), "volume catalog_name is required") +} + +func TestRequiredIdentifierValidationScope(t *testing.T) { + b := &bundle.Bundle{} + require.NoError(t, b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + return dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V(" \t"), + }), + "resources": dyn.V(map[string]dyn.Value{ + "dashboards": dyn.V(map[string]dyn.Value{ + "dashboard": dyn.V(map[string]dyn.Value{ + "display_name": dyn.V("bad\nname"), + "warehouse_id": dyn.V("warehouse"), + }), + }), + "jobs": dyn.V(map[string]dyn.Value{ + "job": dyn.V(map[string]dyn.Value{ + "parameters": dyn.V([]dyn.Value{ + dyn.V(map[string]dyn.Value{"default": dyn.V("value")}), + }), + }), + }), + "sql_warehouses": dyn.V(map[string]dyn.Value{ + "warehouse": dyn.V(map[string]dyn.Value{ + "name": dyn.V(" "), + }), + }), + }), + }), nil + })) + + diags := bundle.Apply(t.Context(), b, validate.Required()) + assert.ElementsMatch(t, []string{ + "bundle name must not contain control characters", + "dashboard display_name must not contain control characters", + "required field \"name\" is not set", + "sql_warehouse name must not be blank", + }, diagSummaries(diags)) +} + +func TestRequiredDoesNotPanicOnMetacharacterResourceKey(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Volumes: map[string]*resources.Volume{ + "weird[0]key": { + CreateVolumeRequestContent: catalog.CreateVolumeRequestContent{ + Name: "", + CatalogName: "main", + SchemaName: "default", + VolumeType: catalog.VolumeTypeManaged, + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.Contains(t, diagSummaries(diags), "volume name is required") +} + +func TestRequiredAcceptsValidIdentifiers(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Models: map[string]*resources.MlflowModel{ + "model": {CreateModelRequest: ml.CreateModelRequest{Name: "model"}}, + }, + }, + }, + } + + assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) +} + +func TestRequiredAcceptsMissingOptionalUCParents(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + RegisteredModels: map[string]*resources.RegisteredModel{ + "model": { + CreateRegisteredModelRequest: catalog.CreateRegisteredModelRequest{ + Name: "model", + }, + }, + }, + }, + }, + } + + assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) +} + +func TestRequiredRejectsBlankOptionalUCParent(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + RegisteredModels: map[string]*resources.RegisteredModel{ + "model": { + CreateRegisteredModelRequest: catalog.CreateRegisteredModelRequest{ + Name: "model", + CatalogName: " ", + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.Contains(t, diagSummaries(diags), "registered_model catalog_name must not be blank") +} + +func diagSummaries(diags diag.Diagnostics) []string { + out := make([]string, 0, len(diags)) + for _, d := range diags { + out = append(out, d.Summary) + } + return out +} diff --git a/bundle/config/validate/pipeline_libraries.go b/bundle/config/validate/pipeline_libraries.go new file mode 100644 index 00000000000..ff4ac06879b --- /dev/null +++ b/bundle/config/validate/pipeline_libraries.go @@ -0,0 +1,48 @@ +package validate + +import ( + "context" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" +) + +// errorForIncompletePipelineLibraries rejects file, notebook, and glob entries +// without paths because the pipelines API rejects them. +func errorForIncompletePipelineLibraries(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + for key, pipeline := range b.Config.Resources.Pipelines { + for i, library := range pipeline.Libraries { + base := dyn.NewPath( + dyn.Key("resources"), + dyn.Key("pipelines"), + dyn.Key(key), + dyn.Key("libraries"), + dyn.Index(i), + ) + if library.File != nil && strings.TrimSpace(library.File.Path) == "" { + diags = diags.Append(libraryPathDiag(b, base, "file", "pipeline library file path is required")) + } + if library.Notebook != nil && strings.TrimSpace(library.Notebook.Path) == "" { + diags = diags.Append(libraryPathDiag(b, base, "notebook", "pipeline library notebook path is required")) + } + if library.Glob != nil && strings.TrimSpace(library.Glob.Include) == "" { + diags = diags.Append(libraryPathDiag(b, base, "glob", "pipeline library glob include is required")) + } + } + } + return diags +} + +// libraryPathDiag reports a missing path on one pipeline library variant. +func libraryPathDiag(b *bundle.Bundle, base dyn.Path, field, summary string) diag.Diagnostic { + fieldPath := base.Append(dyn.Key(field)) + return diag.Diagnostic{ + Severity: diag.Error, + Summary: summary, + Locations: locationsFor(b, fieldPath, base), + Paths: []dyn.Path{fieldPath}, + } +} diff --git a/bundle/config/validate/pipeline_libraries_test.go b/bundle/config/validate/pipeline_libraries_test.go new file mode 100644 index 00000000000..17f3e9e4734 --- /dev/null +++ b/bundle/config/validate/pipeline_libraries_test.go @@ -0,0 +1,66 @@ +package validate_test + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/validate" + "github.com/databricks/databricks-sdk-go/service/pipelines" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequiredRejectsIncompletePipelineLibraries(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "weird[0]key": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "p", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{}}, + {Notebook: &pipelines.NotebookLibrary{}}, + {Glob: &pipelines.PathPattern{}}, + {File: &pipelines.FileLibrary{Path: "ok.py"}}, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(t.Context(), b, validate.Required()) + require.True(t, diags.HasError()) + assert.ElementsMatch(t, []string{ + "pipeline library file path is required", + "pipeline library notebook path is required", + "pipeline library glob include is required", + }, diagSummaries(diags)) +} + +func TestRequiredAcceptsCompletePipelineLibraries(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline": { + CreatePipeline: pipelines.CreatePipeline{ + Name: "pipeline", + Libraries: []pipelines.PipelineLibrary{ + {File: &pipelines.FileLibrary{Path: "file.py"}}, + {Notebook: &pipelines.NotebookLibrary{Path: "notebook.py"}}, + {Glob: &pipelines.PathPattern{Include: "src/**"}}, + }, + }, + }, + }, + }, + }, + } + + assert.Empty(t, bundle.Apply(t.Context(), b, validate.Required())) +} diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index b886c2c1d73..2c72cdd49b8 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "slices" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/internal/validation/generated" @@ -23,54 +22,68 @@ func (f *required) Name() string { return "validate:required" } -// Warn for missing fields, based on annotations in the Go SDK / OpenAPI spec. +// warnForMissingFields reports fields marked as required by the OpenAPI spec. func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := diag.Diagnostics{} - - // Generate prefix tree for all required fields. trie := &dyn.TrieNode{} - for k := range generated.RequiredFields { - pattern, err := dyn.NewPatternFromString(k) + for value := range generated.RequiredFields { + pattern, err := dyn.NewPatternFromString(value) if err != nil { - return diag.FromErr(fmt.Errorf("invalid pattern %q for required field validation: %w", k, err)) + return diag.FromErr(fmt.Errorf("invalid pattern %q for required field validation: %w", value, err)) } - - err = trie.Insert(pattern) - if err != nil { - return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", k, err)) + if err := trie.Insert(pattern); err != nil { + return diag.FromErr(fmt.Errorf("failed to insert pattern %q into trie: %w", value, err)) } } - err := dyn.WalkReadOnly(b.Config.Value(), func(p dyn.Path, v dyn.Value) error { - // If the path is not found in the prefix tree, we do not need to validate any required - // fields in it. - pattern, ok := trie.SearchPath(p) + var diags diag.Diagnostics + err := dyn.WalkReadOnly(b.Config.Value(), func(path dyn.Path, value dyn.Value) error { + pattern, ok := trie.SearchPath(path) if !ok { return nil } - - cloneP := slices.Clone(p) - - fields := generated.RequiredFields[pattern.String()] - for _, field := range fields { - vv := v.Get(field) - if vv.Kind() == dyn.KindInvalid || vv.Kind() == dyn.KindNil { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("required field %q is not set", field), - Locations: v.Locations(), - Paths: []dyn.Path{cloneP}, - }) + for _, field := range generated.RequiredFields[pattern.String()] { + if missingIdentifierIsError(pattern.String(), field) { + continue + } + v := value.Get(field) + if v.Kind() != dyn.KindInvalid && v.Kind() != dyn.KindNil { + continue } + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("required field %q is not set", field), + Locations: value.Locations(), + Paths: []dyn.Path{slices.Clone(path)}, + }) } return nil }) if err != nil { return diag.FromErr(err) } + return diags +} - sortDiagnostics(diags) - +// errorForMissingDashboardWarehouseID covers a backend requirement absent from OpenAPI. +func errorForMissingDashboardWarehouseID(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + for key, dashboard := range b.Config.Resources.Dashboards { + if dashboard.WarehouseId != "" { + continue + } + resourcePath := dyn.NewPath( + dyn.Key("resources"), + dyn.Key("dashboards"), + dyn.Key(key), + ) + fieldPath := resourcePath.Append(dyn.Key("warehouse_id")) + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "dashboard warehouse_id is required", + Locations: locationsFor(b, fieldPath, resourcePath), + Paths: []dyn.Path{fieldPath}, + }) + } return diags } @@ -78,7 +91,10 @@ func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostic // by walking maps with random iteration order. func sortDiagnostics(diags diag.Diagnostics) { slices.SortFunc(diags, func(a, b diag.Diagnostic) int { - // First sort by summary + // Keep errors ahead of warnings, then sort each group by summary. + if n := cmp.Compare(a.Severity, b.Severity); n != 0 { + return n + } if n := cmp.Compare(a.Summary, b.Summary); n != 0 { return n } @@ -93,62 +109,6 @@ func sortDiagnostics(diags diag.Diagnostics) { }) } -// Bespoke code to error for fields that are not marked as required in the Go SDK / OpenAPI spec. -func errorForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Dashboards should always have a name and warehouse_id. - var nameLocations []dyn.Location - var namePaths []dyn.Path - var warehouseIdLocations []dyn.Location - var warehouseIdPaths []dyn.Path - - diags := diag.Diagnostics{} - for key, dashboard := range b.Config.Resources.Dashboards { - if dashboard.DisplayName == "" { - nameLocations = append(nameLocations, b.Config.GetLocations("resources.dashboards."+key)...) - namePaths = append(namePaths, dyn.MustPathFromString("resources.dashboards."+key)) - } - if dashboard.WarehouseId == "" { - warehouseIdLocations = append(warehouseIdLocations, b.Config.GetLocations("resources.dashboards."+key)...) - warehouseIdPaths = append(warehouseIdPaths, dyn.MustPathFromString("resources.dashboards."+key)) - } - } - - if len(nameLocations) > 0 { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "dashboard display_name is required", - Locations: nameLocations, - Paths: namePaths, - }) - } - if len(warehouseIdLocations) > 0 { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "dashboard warehouse_id is required", - Locations: warehouseIdLocations, - Paths: warehouseIdPaths, - }) - } - - // sql_warehouses.name is optional in the SDK (json:"name,omitempty") but required - // by the backend, which rejects whitespace-only names (name.trim.nonEmpty). - for key, warehouse := range b.Config.Resources.SqlWarehouses { - if strings.TrimSpace(warehouse.Name) == "" { - path := "resources.sql_warehouses." + key - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Error, - Summary: "sql_warehouse name is required", - Locations: b.Config.GetLocations(path), - Paths: []dyn.Path{dyn.MustPathFromString(path)}, - }) - } - } - - sortDiagnostics(diags) - - return diags -} - // errorForInvalidGrants errors for grants the backend rejects or that never converge: // a missing principal is rejected, and an empty privileges list re-plans forever because // the backend drops principals with no privileges. Erroring here (rather than warning) @@ -242,12 +202,12 @@ func isMissingOrEmptySequence(v dyn.Value) bool { } func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := errorForMissingFields(ctx, b) + diags := validateIdentifiers(ctx, b) + diags = diags.Extend(errorForMissingDashboardWarehouseID(ctx, b)) diags = diags.Extend(errorForInvalidGrants(ctx, b)) diags = diags.Extend(errorForInvalidSecretScopePermissions(ctx, b)) - if diags.HasError() { - return diags - } + diags = diags.Extend(errorForIncompletePipelineLibraries(ctx, b)) diags = diags.Extend(warnForMissingFields(ctx, b)) + sortDiagnostics(diags) return diags }