Fix include diagnostics pointing at the wrong include entry#5514
Fix include diagnostics pointing at the wrong include entry#5514simonfaltum wants to merge 3 commits into
Conversation
Co-authored-by: Isaac
Approval status: pending
|
|
Commit: 89afacc
22 interesting tests: 15 SKIP, 7 KNOWN
Top 28 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
| assert.Equal(t, []string{"a.yml"}, b.Config.Include) | ||
| } | ||
|
|
||
| func TestProcessRootIncludesNonYamlGlobLocations(t *testing.T) { |
There was a problem hiding this comment.
hard to reason about the output and the fix from this test, can we do an acceptance test using bundle validate? Ran this locally to compare
Latest release
% databricks bundle validate
Error: Files in the 'include' configuration section must be YAML or JSON files.
in databricks.yml:5:5
The file a.txt in the 'include' configuration section is not a YAML or JSON file, and only such files are supported. To include files to sync, specify them in the 'sync.include' configuration section instead.
Error: Files in the 'include' configuration section must be YAML or JSON files.
The file b.txt in the 'include' configuration section is not a YAML or JSON file, and only such files are supported. To include files to sync, specify them in the 'sync.include' configuration section instead.
Name: test
Found 2 errors
This PR
% ../cli bundle validate
Error: Files in the 'include' configuration section must be YAML or JSON files.
in databricks.yml:5:5
The file a.txt in the 'include' configuration section is not a YAML or JSON file, and only such files are supported. To include files to sync, specify them in the 'sync.include' configuration section instead.
Error: Files in the 'include' configuration section must be YAML or JSON files.
in databricks.yml:5:5
The file b.txt in the 'include' configuration section is not a YAML or JSON file, and only such files are supported. To include files to sync, specify them in the 'sync.include' configuration section instead.
Name: test
Found 2 errors
There was a problem hiding this comment.
Good idea, done in 89afacc. Replaced the unit test with acceptance coverage: the existing non_yaml_in_include test now uses a glob that matches two non-YAML files, and bundle validate shows both errors pointing at the glob's own include entry (databricks.yml:6:5). With the old code the first error pointed at the resources/*.yml entry instead. I verified the test fails if the fix is reverted.
Per review feedback, demonstrate the fix through 'bundle validate' output instead of a unit test. The non_yaml_in_include acceptance test now uses a glob matching two non-YAML files; both errors point at the glob's own include entry (databricks.yml:6:5). With the previous code the first error pointed at include[0] (the resources/*.yml entry) instead. Co-authored-by: Isaac
Why
Found during a full-repo review of the CLI. When an entry in the
includesection matches a non-YAML file, the error diagnostic should point at the include entry indatabricks.ymlthat matched it. The code used the index of the match within the glob's result list instead of the index of the include entry, so when a glob matched more than one file (or the offending entry was not the first one), the error pointed at the wrong include entry or carried no location at all.Changes
Before, the
include[%d]location lookup used the glob-match index; now it uses the index of the include entry being processed. Inbundle/config/loader/process_root_includes.go, the entry index from the outer loop overb.Config.Includeis carried into the match loop and used for the diagnostic location.Test plan
TestProcessRootIncludesNonYamlGlobLocationswith one glob matching two non-YAML files, asserting both diagnostics point at the glob's include entry. It fails before the fix (the second diagnostic had no location) and passes after.go test ./bundle/config/loaderpasses.go test ./acceptance -run 'TestAccept/bundle/includes'passes unchanged, including the existingnon_yaml_in_includecase../task fmt-q,./task lint-q, and./task checkspass.This pull request and its description were written by Isaac.