Fix include diagnostics pointing at the wrong include entry#5514
Open
simonfaltum wants to merge 5 commits into
Open
Fix include diagnostics pointing at the wrong include entry#5514simonfaltum wants to merge 5 commits into
simonfaltum wants to merge 5 commits into
Conversation
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: f248788
25 interesting tests: 15 SKIP, 7 RECOVERED, 3 FAIL
Top 25 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
janniklasrose and
pietern
and removed request for
andrewnester and
pietern
| assert.Equal(t, []string{"a.yml"}, b.Config.Include) | ||
| } | ||
|
|
||
| func TestProcessRootIncludesNonYamlGlobLocations(t *testing.T) { |
Contributor
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
Member
Author
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
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.