Rewrite relative paths using `dyn.Location` of the underlying value #1273

pietern · 2024-03-11T12:57:28Z

Changes

This change addresses the path resolution behavior in resource definitions. Previously, all paths were resolved relative to where the resource was first defined, which could lead to confusion and errors when paths were specified in different directories. The new behavior is to resolve paths relative to where they are defined, making it more intuitive.

However, to avoid breaking existing configurations, compatibility with the old behavior is maintained.

Tests

Existing unit tests for path translation pass.
Additional test to cover both the nominal and the fallback behavior.

codecov-commenter · 2024-03-18T11:24:18Z

Codecov Report

Attention: Patch coverage is 92.61364% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 52.32%. Comparing base (e22dd8a) to head (3027df6).

Files	Patch %	Lines
bundle/config/mutator/translate_paths_artifacts.go	70.37%	7 Missing and 1 partial ⚠️
bundle/config/mutator/translate_paths_jobs.go	95.08%	2 Missing and 1 partial ⚠️
bundle/config/mutator/translate_paths_pipelines.go	94.59%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
+ Coverage   52.25%   52.32%   +0.07%     
==========================================
  Files         317      318       +1     
  Lines       18004    18016      +12     
==========================================
+ Hits         9408     9427      +19     
+ Misses       7903     7894       -9     
- Partials      693      695       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pietern · 2024-03-18T12:27:09Z

bundle/tests/bundle/loader.go

+	))
+	require.NoError(t, err)
+	return b
+}


Note: this is a copy from bundle/tests/loader.go to use it from the glob test. Follow up: move the glob test to the parent package so that it can use the reference loader.

pietern · 2024-03-18T12:27:50Z

bundle/tests/bundle/pipeline_glob_paths_test.go

+	ctx := context.Background()
+	err := bundle.Apply(ctx, b, phases.Initialize())
+	require.ErrorContains(t, err, "notebook ./non-existent not found")
+}


These changes were needed because this change no longer has an observable side effect on the configuration if it is partially OK. If there is an error, none of the path translations are persisted in the config.

pietern · 2024-03-18T12:30:34Z

bundle/config/mutator/translate_paths.go

-			}
-			return err
-		}
+		return nv, nil


This function performs the fallback. If a rewrite with first dir doesn't work, it tries a rewrite with the second dir.

pietern · 2024-03-18T12:32:03Z

bundle/config/mutator/translate_paths_artifacts.go

+	base := dyn.NewPattern(
+		dyn.Key("artifacts"),
+		dyn.AnyKey(),
+	)


Note: because artifacts use the translateNoOp translation (which doesn't check if a path exists), the fallback never kicks in. This is why I didn't include fallback for artifact blocks.

We could implement this as long as we check if the directory for the artifact exists, which on its own could cause issues (not sure about the guarantees, though I expect the directory has to exist).

bundle/config/mutator/translate_paths_test.go

bundle/config/mutator/translate_paths.go

bundle/tests/path_translation/nominal/override_job.yml

pietern · 2024-03-18T16:23:35Z

Integration tests pass.

## Changes While working on #1273, I found that calls to `Append` on a `dyn.Pattern` were mutating the original slice. This is expected because appending to a slice will mutate in place if the capacity of the original slice is large enough. This change updates the `Append` call on the `dyn.Path` as well to return a newly allocated slice to avoid inadvertently mutating the originals. We have existing call sites in the `dyn` package that mutate a `dyn.Path` (e.g. walk or visit) and these are modified to continue to do this with a direct call to `append`. Callbacks that use the `dyn.Path` argument outside of the callback need to make a copy to ensure it isn't mutated (this is no different from existing semantics). The `Join` function wasn't used and is removed as part of this change. ## Tests Unit tests.

CLI: * Propagate correct `User-Agent` for CLI during OAuth flow ([#1264](#1264)). * Add usage string when command fails with incorrect arguments ([#1276](#1276)). Bundles: * Include `dyn.Path` as argument to the visit callback function ([#1260](#1260)). * Inline logic to set a value in `dyn.SetByPath` ([#1261](#1261)). * Add assertions for the `dyn.Path` argument to the visit callback ([#1265](#1265)). * Add `dyn.MapByPattern` to map a function to values with matching paths ([#1266](#1266)). * Filter current user from resource permissions ([#1262](#1262)). * Retain location annotation when expanding globs for pipeline libraries ([#1274](#1274)). * Added deployment state for bundles ([#1267](#1267)). * Do CheckRunningResource only after terraform.Write ([#1292](#1292)). * Rewrite relative paths using `dyn.Location` of the underlying value ([#1273](#1273)). * Push deployment state right after files upload ([#1293](#1293)). * Make `Append` function to `dyn.Path` return independent slice ([#1295](#1295)). * Move bundle tests into bundle/tests ([#1299](#1299)). * Upgrade Terraform provider to 1.38.0 ([#1308](#1308)). Internal: * Add integration test for mlops-stacks initialization ([#1155](#1155)). * Update actions/setup-python to v5 ([#1290](#1290)). * Update codecov/codecov-action to v4 ([#1291](#1291)). API Changes: * Changed `databricks catalogs list` command. * Changed `databricks online-tables create` command. * Changed `databricks lakeview publish` command. * Added `databricks lakeview create` command. * Added `databricks lakeview get` command. * Added `databricks lakeview get-published` command. * Added `databricks lakeview trash` command. * Added `databricks lakeview update` command. * Moved settings related commands to `databricks settings` and `databricks account settings`. OpenAPI commit 93763b0d7ae908520c229c786fff28b8fd623261 (2024-03-20) Dependency updates: * Bump golang.org/x/oauth2 from 0.17.0 to 0.18.0 ([#1270](#1270)). * Bump golang.org/x/mod from 0.15.0 to 0.16.0 ([#1271](#1271)). * Update Go SDK to v0.35.0 ([#1300](#1300)). * Update Go SDK to v0.36.0 ([#1304](#1304)).

pietern added 13 commits March 11, 2024 13:56

Rewrite relative paths using dyn.Location of the underlying value

bb6d266

Retain location annotation when expanding globs for pipeline libraries

14384be

Make filenames in test unique

6db5797

Merge branch 'dyn-pipeline-expand-globs' into dyn-translate-paths

56d5c90

Swap expected vs actual arguments

802629a

Merge remote-tracking branch 'origin/main' into dyn-translate-paths

366f6f2

Use fallback path when rewriting paths

1213c22

Add tests

a7ecc0b

Comment

10c81d0

Add nominal tests

2489bb1

Merge remote-tracking branch 'origin/main' into dyn-translate-paths

19e74a1

Update pipeline glob test to work if error means no side effects

457b749

Fix

2b19f9d

pietern added 3 commits March 18, 2024 13:14

filepath.ToSlash

7df6f00

Fix comment

9b21ade

Skip fallback for artifacts

dd3a9c0

pietern changed the title ~~Rewrite relative paths using dyn.Location of the underlying value~~ Rewrite relative paths using dyn.Location of the underlying value Mar 18, 2024

Include test for pattern append

3027df6

pietern commented Mar 18, 2024

View reviewed changes

pietern marked this pull request as ready for review March 18, 2024 12:32

pietern requested a review from andrewnester March 18, 2024 12:32

andrewnester approved these changes Mar 18, 2024

View reviewed changes

bundle/config/mutator/translate_paths_test.go Show resolved Hide resolved

bundle/config/mutator/translate_paths.go Outdated Show resolved Hide resolved

bundle/tests/path_translation/nominal/override_job.yml Show resolved Hide resolved

pietern added 3 commits March 18, 2024 15:20

Make fallback more explicit

89329f0

Perform path translation for for_each_task

2f2bfa4

Use ./ prefix in nominal path translation test

e89273e

pietern added this pull request to the merge queue Mar 18, 2024

Merged via the queue into main with commit 7c4b349 Mar 18, 2024
4 checks passed

pietern deleted the dyn-translate-paths branch March 18, 2024 16:31

pietern mentioned this pull request Mar 18, 2024

Make Append function to dyn.Path return independent slice #1295

Merged

pietern mentioned this pull request Mar 25, 2024

Release v0.216.0 #1309

Merged

pietern mentioned this pull request Apr 2, 2024

Databricks Asset Bundles bundle validate "no file in location" error for jobs #1330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite relative paths using `dyn.Location` of the underlying value #1273

Rewrite relative paths using `dyn.Location` of the underlying value #1273

pietern commented Mar 11, 2024 •

edited

Loading

codecov-commenter commented Mar 18, 2024 •

edited

Loading

pietern Mar 18, 2024

pietern Mar 18, 2024

pietern Mar 18, 2024

pietern Mar 18, 2024

pietern commented Mar 18, 2024

Rewrite relative paths using dyn.Location of the underlying value #1273

Rewrite relative paths using dyn.Location of the underlying value #1273

Conversation

pietern commented Mar 11, 2024 • edited Loading

Changes

Tests

codecov-commenter commented Mar 18, 2024 • edited Loading

Codecov Report

pietern Mar 18, 2024

Choose a reason for hiding this comment

pietern Mar 18, 2024

Choose a reason for hiding this comment

pietern Mar 18, 2024

Choose a reason for hiding this comment

pietern Mar 18, 2024

Choose a reason for hiding this comment

pietern commented Mar 18, 2024

Rewrite relative paths using `dyn.Location` of the underlying value #1273

Rewrite relative paths using `dyn.Location` of the underlying value #1273

pietern commented Mar 11, 2024 •

edited

Loading

codecov-commenter commented Mar 18, 2024 •

edited

Loading