Fix include diagnostics pointing at the wrong include entry#5514
Open
simonfaltum wants to merge 2 commits into
Open
Fix include diagnostics pointing at the wrong include entry#5514simonfaltum wants to merge 2 commits into
simonfaltum wants to merge 2 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
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
Collaborator
|
Commit: 12b15de
22 interesting tests: 15 SKIP, 7 RECOVERED
Top 30 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
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.