Stop tracking file path locations in bundle resources #1673

shreyas-goenka · 2024-08-12T20:52:55Z

Changes

Since locations are already tracked in the dynamic value tree, we no longer need to track it at the resource/artifact level. This PR:

Removes use of paths.Paths. Uses dyn.Location instead.
Refactors the validation of resources not being empty valued to be generic across all resource types.

Tests

Existing unit tests.

bundle/config/resources.go

shreyas-goenka · 2024-08-13T07:16:24Z

bundle/artifacts/prepare.go

@@ -34,11 +34,13 @@ func (m *prepare) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics
 		return diag.Errorf("artifact doesn't exist: %s", m.name)
 	}

+	l := b.Config.GetLocation("artifacts." + m.name)
+	dirPath := filepath.Dir(l.File)


Change covered by TestExpandGlobs_* unit tests

shreyas-goenka · 2024-08-13T07:21:18Z

bundle/config/validate/all_resources_defined.go

+	"github.com/databricks/cli/libs/dyn"
+)
+
+func AllResourcesHaveValues() bundle.Mutator {


Covered by TestUndefinedJobLoadsWithError

shreyas-goenka · 2024-08-13T07:22:07Z

bundle/deploy/metadata/compute.go

@@ -39,7 +39,8 @@ func (m *compute) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics {
 	for name, job := range b.Config.Resources.Jobs {
 		// Compute config file path the job is defined in, relative to the bundle
 		// root
-		relativePath, err := filepath.Rel(b.RootPath, job.ConfigFilePath)
+		l := b.Config.GetLocation("resources.jobs." + name)


Covered by TestComputeMetadataMutator

shreyas-goenka · 2024-08-13T08:50:56Z

Triggered integration tests...

bundle/tests/environments_job_and_pipeline_test.go

andrewnester · 2024-08-13T09:11:41Z

bundle/tests/include_test.go

@@ -31,7 +30,6 @@ func TestIncludeWithGlob(t *testing.T) {

 	job := b.Config.Resources.Jobs["my_job"]
 	assert.Equal(t, "1", job.ID)
-	assert.Equal(t, "include_with_glob/job.yml", filepath.ToSlash(job.ConfigFilePath))


Done. I'm skipping this for the other unit tests, though, since it's really functionality from the dyn packages. Let me know if you disagree and would like to retain coverage.

+1, seems like this is no longer related. Still good to keep coverage for it in a test that isn't specific to a resource.

andrewnester · 2024-08-13T09:14:05Z

bundle/config/validate/all_resources_defined.go

+func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
+	rv := b.Config.Value().Get("resources")
+
+	// Skip if there are no resources block defined, or the resources block is empty.


Isn't it an error or at least warning case? In which case we might not have resources block?

Not having a resources block is allowed. Having a warning for an empty resources block makes sense to me. Should we make it more generic though? Like we emit a warning for any fields in the YAML configuration that only have the key specified but not the value.

bundle/config/validate/all_resources_defined.go

andrewnester · 2024-08-13T09:16:41Z

bundle/config/validate/all_resources_defined.go

+}
+
+func (m *allResourcesHaveValues) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
+	rv := b.Config.Value().Get("resources")


you can call b.Config.Mutate instead, to be consistent with the rest of mutators even though you're not mutating dynamic value

On further thought, I reverted this back to b.Config.Value(). I believe the current approach might be better because we don't need to modify the configuration tree; we just read values from it and emit an error.

bundle/config/validate/all_resources_defined.go

shreyas-goenka · 2024-08-13T12:50:52Z

Integration tests are green (mostly). The failures on GCP are unrelated to this PR.

pietern

Thanks, great to see a bunch of code removed!

Comments are async/non-blocking.

pietern · 2024-08-13T17:42:07Z

bundle/config/validate/all_resources_have_values.go

+	// Skip if there are no resources block defined, or the resources block is empty.
+	if rv.Kind() == dyn.KindInvalid || rv.Kind() == dyn.KindNil {
+		return nil
+	}


No need for this check; map won't traverse nil nor show an error for it.

Using this as part of the pattern also means the path in the callback is correct.

pietern · 2024-08-13T17:43:37Z

bundle/config/validate/all_resources_have_values.go

+		rv,
+		dyn.NewPattern(dyn.AnyKey(), dyn.AnyKey()),
+		func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
+			if v.Kind() == dyn.KindInvalid || v.Kind() == dyn.KindNil {


Won't be invalid by definition (no valid dyn.Value contains an invalid dyn.Value).

pietern · 2024-08-13T17:44:00Z

bundle/config/validate/all_resources_have_values.go

+				rType := strings.TrimSuffix(p[0].Key(), "s")
+
+				rName := p[1].Key()
+				return v, fmt.Errorf("%s %s is not defined", rType, rName)


This would benefit of location information + accumulating all undefined resources in a diags.

pietern · 2024-08-13T18:16:01Z

bundle/tests/include_test.go

@@ -31,7 +30,6 @@ func TestIncludeWithGlob(t *testing.T) {

 	job := b.Config.Resources.Jobs["my_job"]
 	assert.Equal(t, "1", job.ID)
-	assert.Equal(t, "include_with_glob/job.yml", filepath.ToSlash(job.ConfigFilePath))


+1, seems like this is no longer related. Still good to keep coverage for it in a test that isn't specific to a resource.

CLI: * Add command line autocomplete to the fs commands ([#1622](#1622)). * Add trailing slash to directory to produce completions for ([#1666](#1666)). * Fix ability to import the CLI repository as module ([#1671](#1671)). * Fix host resolution order in `auth login` ([#1370](#1370)). * Print text logs in `import-dir` and `export-dir` commands ([#1682](#1682)). Bundles: * Expand and upload local wheel libraries for all task types ([#1649](#1649)). * Clarify file format required for the `config-file` flag in `bundle init` ([#1651](#1651)). * Fixed incorrectly cleaning up python wheel dist folder ([#1656](#1656)). * Merge job parameters based on their name ([#1659](#1659)). * Fix glob expansion after running a generic build command ([#1662](#1662)). * Upload local libraries even if they don't have artifact defined ([#1664](#1664)). Internal: * Fix python wheel task integration tests ([#1648](#1648)). * Skip pushing Terraform state after destroy ([#1667](#1667)). * Enable Spark JAR task test ([#1658](#1658)). * Run Spark JAR task test on multiple DBR versions ([#1665](#1665)). * Stop tracking file path locations in bundle resources ([#1673](#1673)). * Update VS Code settings to match latest value from IDE plugin ([#1677](#1677)). * Use `service.NamedIdMap` to make lookup generation deterministic ([#1678](#1678)). * [Internal] Remove dependency to the `openapi` package of the Go SDK ([#1676](#1676)). * Upgrade TF provider to 1.50.0 ([#1681](#1681)). * Upgrade Go SDK to 0.44.0 ([#1679](#1679)). API Changes: * Changed `databricks account budgets create` command . New request type is . * Changed `databricks account budgets create` command to return . * Changed `databricks account budgets delete` command . New request type is . * Changed `databricks account budgets delete` command to return . * Changed `databricks account budgets get` command . New request type is . * Changed `databricks account budgets get` command to return . * Changed `databricks account budgets list` command to require request of . * Changed `databricks account budgets list` command to return . * Changed `databricks account budgets update` command . New request type is . * Changed `databricks account budgets update` command to return . * Added `databricks account usage-dashboards` command group. * Changed `databricks model-versions get` command to return . * Changed `databricks cluster-policies create` command with new required argument order. * Changed `databricks cluster-policies edit` command with new required argument order. * Added `databricks clusters update` command. * Added `databricks genie` command group. * Changed `databricks permission-migration migrate-permissions` command . New request type is . * Changed `databricks permission-migration migrate-permissions` command to return . * Changed `databricks account workspace-assignment delete` command to return . * Changed `databricks account workspace-assignment update` command with new required argument order. * Changed `databricks account custom-app-integration create` command with new required argument order. * Changed `databricks account custom-app-integration list` command to require request of . * Changed `databricks account published-app-integration list` command to require request of . * Removed `databricks apps` command group. * Added `databricks notification-destinations` command group. * Changed `databricks shares list` command to require request of . * Changed `databricks alerts create` command . New request type is . * Changed `databricks alerts delete` command . New request type is . * Changed `databricks alerts delete` command to return . * Changed `databricks alerts get` command with new required argument order. * Changed `databricks alerts list` command to require request of . * Changed `databricks alerts list` command to return . * Changed `databricks alerts update` command . New request type is . * Changed `databricks alerts update` command to return . * Changed `databricks queries create` command . New request type is . * Changed `databricks queries delete` command . New request type is . * Changed `databricks queries delete` command to return . * Changed `databricks queries get` command with new required argument order. * Changed `databricks queries list` command to return . * Removed `databricks queries restore` command. * Changed `databricks queries update` command . New request type is . * Added `databricks queries list-visualizations` command. * Changed `databricks query-visualizations create` command . New request type is . * Changed `databricks query-visualizations delete` command . New request type is . * Changed `databricks query-visualizations delete` command to return . * Changed `databricks query-visualizations update` command . New request type is . * Changed `databricks statement-execution execute-statement` command to return . * Changed `databricks statement-execution get-statement` command to return . * Added `databricks alerts-legacy` command group. * Added `databricks queries-legacy` command group. * Added `databricks query-visualizations-legacy` command group. OpenAPI commit f98c07f9c71f579de65d2587bb0292f83d10e55d (2024-08-12) Dependency updates: * Bump github.com/hashicorp/hc-install from 0.7.0 to 0.8.0 ([#1652](#1652)). * Bump golang.org/x/sync from 0.7.0 to 0.8.0 ([#1655](#1655)). * Bump golang.org/x/mod from 0.19.0 to 0.20.0 ([#1654](#1654)). * Bump golang.org/x/oauth2 from 0.21.0 to 0.22.0 ([#1653](#1653)). * Bump golang.org/x/text from 0.16.0 to 0.17.0 ([#1670](#1670)). * Bump golang.org/x/term from 0.22.0 to 0.23.0 ([#1669](#1669)).

## Changes This PR addressed post-merge feedback from #1673. ## Tests Unit tests, and manually. ``` Error: experiment undefined-experiment is not defined at resources.experiments.undefined-experiment in databricks.yml:11:26 Error: job undefined-job is not defined at resources.jobs.undefined-job in databricks.yml:6:19 Error: pipeline undefined-pipeline is not defined at resources.pipelines.undefined-pipeline in databricks.yml:14:24 Name: undefined-job Target: default Found 3 errors ```

shreyas-goenka added 20 commits August 12, 2024 01:55

Remove paths.Paths from bundle resources

dabe9a9

Merge remote-tracking branch 'origin' into remove-pathspaths

e66bc0a

remove path.Paths for ml resources

6c43f56

Remove paths.Paths everywhere

e5bf160

cleanup todos

7028cf9

-

f18d0d8

fix test

b7add30

-

39a7d57

Merge remote-tracking branch 'origin' into remove-pathspaths

375c948

Stop tracking file paths in bundle resources

13886fa

todos

2ad2c45

remove paths.Paths from artifacts as well

de37e2f

add validation mutator to assert non empty keys

552f416

remove dynamic value from artifacts

6bd1c74

-

25f7cf7

-

46d10f8

fix artifacts test

4c694a2

cleanup todos

c60407c

add test for pipeline empty validator:

05ef9a0

handle no resources block case

5ffacb9

shreyas-goenka commented Aug 13, 2024

View reviewed changes

shreyas-goenka marked this pull request as ready for review August 13, 2024 08:21

shreyas-goenka requested review from andrewnester and pietern August 13, 2024 08:21

andrewnester reviewed Aug 13, 2024

View reviewed changes

shreyas-goenka added 4 commits August 13, 2024 12:37

address comments

f1f21bb

revert bundle.Config.Mutate

6178456

bundle/tests/environments_job_and_pipeline_test.go dyn

92b3d81

dyn test:

a0fe7b2

shreyas-goenka requested a review from andrewnester August 13, 2024 10:55

andrewnester approved these changes Aug 13, 2024

View reviewed changes

shreyas-goenka added this pull request to the merge queue Aug 13, 2024

Merged via the queue into main with commit 7ae80de Aug 13, 2024
5 checks passed

shreyas-goenka deleted the better-empty-key-validation branch August 13, 2024 12:55

pietern reviewed Aug 13, 2024

View reviewed changes

andrewnester mentioned this pull request Aug 15, 2024

[Release] Release v0.226.0 #1683

Merged

shreyas-goenka mentioned this pull request Aug 15, 2024

Report all empty resources present in error diagnostic #1685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop tracking file path locations in bundle resources #1673

Stop tracking file path locations in bundle resources #1673

shreyas-goenka commented Aug 12, 2024 •

edited

Loading

shreyas-goenka Aug 13, 2024

shreyas-goenka Aug 13, 2024

shreyas-goenka Aug 13, 2024

shreyas-goenka commented Aug 13, 2024

andrewnester Aug 13, 2024

shreyas-goenka Aug 13, 2024

pietern Aug 13, 2024

andrewnester Aug 13, 2024

shreyas-goenka Aug 13, 2024

andrewnester Aug 13, 2024

shreyas-goenka Aug 13, 2024

shreyas-goenka Aug 13, 2024

shreyas-goenka commented Aug 13, 2024

pietern left a comment

pietern Aug 13, 2024

pietern Aug 13, 2024

pietern Aug 13, 2024

pietern Aug 13, 2024

Stop tracking file path locations in bundle resources #1673

Stop tracking file path locations in bundle resources #1673

Conversation

shreyas-goenka commented Aug 12, 2024 • edited Loading

Changes

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka commented Aug 13, 2024

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka commented Aug 12, 2024 •

edited

Loading