Allow artifact path to be located outside the sync root #2128

pietern · 2025-01-13T14:43:24Z

Changes

We perform a check during path translation that the path being referenced is contained in the bundle's sync root. If it isn't, it's not a valid remote reference. However, this doesn't apply to paths that are always local, such as the artifact path. An artifact's build command is executed in its path. Files created by the artifact build (e.g. wheels or JARs) don't need to be in the sync root because they have a dedicated and different upload path into ${workspace.artifact_path}.

Therefore, this check that a path is contained in the bundle's sync root doesn't apply to artifact paths. This change modifies the structure of path translation to allow opting out of this check.

Fixes #1927.

Tests

Existing and new tests pass.
Manually confirmed that building and using a wheel built outside the sync root path works as expected.
No acceptance tests because we don't run build as part of validate.

pietern · 2025-01-13T14:44:45Z

bundle/tests/relative_path_with_includes/artifact_a/.gitkeep

Dummy file to keep the directory around.

This is needed now that path translation verifies that artifact paths exist.

pietern · 2025-01-13T14:44:56Z

bundle/tests/relative_path_with_includes_test.go

+	assert.Equal(t, "artifact_a", relA)
+	relB, err := filepath.Rel(b.SyncRootPath, b.Config.Artifacts["test_b"].Path)
+	require.NoError(t, err)
+	assert.Equal(t, filepath.Join("subfolder", "artifact_b"), relB)


Artifact paths are now absolute.

assert.Equal(t, filepath.Join("subfolder", "artifact_b"), relB)

Artifact paths are now absolute.

What are you referring to? the variables in the test are relative.

Simplified in latest.

pietern · 2025-01-13T14:45:45Z

bundle/config/mutator/translate_paths_pipelines.go

+				}
+			}
+
+			return dyn.InvalidValue, err


Denormalize the previous rewriteRelativeTo function into the body for jobs and pipelines.

It is not relevant outside these two resources.

pietern · 2025-01-13T14:46:08Z

bundle/config/mutator/translate_paths_jobs.go

+			}
+		}
+
+		return dyn.InvalidValue, err


Denormalize the previous rewriteRelativeTo function into the body for jobs and pipelines.

It is not relevant outside these two resources.

Can this and below comments be incorporated into source? Seems like valuable context.

It's included (vs the previous), here:

// If we failed to rewrite the path, try to rewrite it relative to the fallback directory. // We only do this for jobs and pipelines because of the comment in [gatherFallbackPaths].

From gatherFallbackPaths the rest of the context is clear.

I would like to deprecate it but we don't know how much usage is out there. Another good thing to track when we can capture some telemetry.

pietern · 2025-01-13T14:47:03Z

bundle/config/mutator/translate_paths.go

+	} else {
+		t.remoteRoot = t.b.Config.Workspace.FilePath
+	}
+


This assignment now happens once instead of for every path.

denik · 2025-01-13T15:23:58Z

No acceptance tests because we don't run build as part of validate.

You can also do deploy in acceptance tests. You would just need to implement a few more dummy handlers in server_test.go.

Another option is to extract 'build' into separate command.

…r-artifact-paths

pietern · 2025-01-14T08:34:47Z

Manually confirmed with pending acceptance tests that this does not change behavior or output.

New feature announcement. You can now manage Databricks Apps using DABs by defining an `app` resource in your bundle configuration. For more information see Databricks documentation https://docs.databricks.com/en/dev-tools/bundles/resources.html#app CLI: * Filter out system clusters in cluster picker ([#2131](#2131)). * Process all the fields in top level request object even if it contains request body ([#2155](#2155)). Bundles: * Added support for Databricks Apps in DABs ([#1928](#1928)). * Allow artifact path to be located outside the sync root ([#2128](#2128)). * Retry app deployment if there is an active deployment in progress ([#2153](#2153)). * Resolve variables in a loop ([#2164](#2164)). * Improve resolution of complex variables within complex variables ([#2157](#2157)). * Added output message to warn about slower deployments with apps ([#2161](#2161)). * Patch references to UC schemas to capture dependencies automatically ([#1989](#1989)). * Format default-python template ([#2110](#2110)). * Encourage the use of root_path in production to ensure single deployment ([#1712](#1712)). * Log warnings to stderr for "bundle validate -o json" ([#2109](#2109)). Internal: * Move merge fix-ups after variable resolution ([#2125](#2125)). * Enable linter 'unconvert' and fix the issues found ([#2136](#2136)). * Coverage for acceptance tests ([#2123](#2123)). * Add acceptance tests for builtin templates ([#2135](#2135)). * Add a unique schema for recreate pipeline test ([#2159](#2159)). * Migrate resolution tests to acceptance tests ([#2143](#2143)). * Update runner for the publish-winget job ([#2105](#2105)). * Add a test for complex variable resolution with 3 levels ([#2163](#2163)). API Changes: * Changed `databricks account federation-policy update` command with new required argument order. * Changed `databricks account service-principal-federation-policy update` command with new required argument order. OpenAPI commit 779817ed8d63031f5ea761fbd25ee84f38feec0d (2025-01-08) Dependency updates: * Upgrade TF provider to 1.63.0 ([#2162](#2162)). * Bump golangci-lint version to v1.63.4 from v1.63.1 ([#2114](#2114)). * Bump astral-sh/setup-uv from 4 to 5 ([#2116](#2116)). * Bump golang.org/x/oauth2 from 0.24.0 to 0.25.0 ([#2080](#2080)). * Bump github.com/hashicorp/hc-install from 0.9.0 to 0.9.1 ([#2079](#2079)). * Bump golang.org/x/term from 0.27.0 to 0.28.0 ([#2078](#2078)). * Bump github.com/databricks/databricks-sdk-go from 0.54.0 to 0.55.0 ([#2126](#2126)).

### New feature announcement #### Databricks Apps support You can now manage Databricks Apps using DABs by defining an `app` resource in your bundle configuration. For more information see Databricks documentation https://docs.databricks.com/en/dev-tools/bundles/resources.html#app #### Referencing complex variables in complex variables You can now reference complex variables within other complex variables. For more details see #2157 CLI: * Filter out system clusters in cluster picker ([#2131](#2131)). * Add command line flags for fields that are not in the API request body ([#2155](#2155)). Bundles: * Added support for Databricks Apps in DABs ([#1928](#1928)). * Allow artifact path to be located outside the sync root ([#2128](#2128)). * Retry app deployment if there is an active deployment in progress ([#2153](#2153)). * Resolve variables in a loop ([#2164](#2164)). * Improve resolution of complex variables within complex variables ([#2157](#2157)). * Added output message to warn about slower deployments with apps ([#2161](#2161)). * Patch references to UC schemas to capture dependencies automatically ([#1989](#1989)). * Format default-python template ([#2110](#2110)). * Encourage the use of root_path in production to ensure single deployment ([#1712](#1712)). * Log warnings to stderr for "bundle validate -o json" ([#2109](#2109)). API Changes: * Changed `databricks account federation-policy update` command with new required argument order. * Changed `databricks account service-principal-federation-policy update` command with new required argument order. OpenAPI commit 779817ed8d63031f5ea761fbd25ee84f38feec0d (2025-01-08) Dependency updates: * Upgrade TF provider to 1.63.0 ([#2162](#2162)). * Bump golangci-lint version to v1.63.4 from v1.63.1 ([#2114](#2114)). * Bump astral-sh/setup-uv from 4 to 5 ([#2116](#2116)). * Bump golang.org/x/oauth2 from 0.24.0 to 0.25.0 ([#2080](#2080)). * Bump github.com/hashicorp/hc-install from 0.9.0 to 0.9.1 ([#2079](#2079)). * Bump golang.org/x/term from 0.27.0 to 0.28.0 ([#2078](#2078)). * Bump github.com/databricks/databricks-sdk-go from 0.54.0 to 0.55.0 ([#2126](#2126)). --------- Co-authored-by: shreyas-goenka <[email protected]>

pietern added 11 commits January 13, 2025 09:10

Pass along context during path translation

ef2853e

Replace pointer mutation with proper return value

e4770c6

Denormalize fallback logic into jobs and pipelines functions

12943db

Use filepath.IsLocal over prefix check

aa92264

Define translation mode and options

9c8c85d

Move remote root joining logic into functions that need it

246e10c

Rename modes to more accurately capture what they do

70a0dd8

Allow artifact paths to be located outside the sync root path

6856a39

Translate artifact paths to local absolute directory path

de011c4

Comment

ecfe545

Update TestRelativePathsWithIncludes for absolute artifact paths

a78e3a2

pietern temporarily deployed to test-trigger-is January 13, 2025 14:43 — with GitHub Actions Inactive

pietern commented Jan 13, 2025

View reviewed changes

pietern marked this pull request as ready for review January 13, 2025 14:51

pietern requested review from andrewnester, shreyas-goenka and denik as code owners January 13, 2025 14:51

Simplify

77e159f

andrewnester approved these changes Jan 13, 2025

View reviewed changes

pietern temporarily deployed to test-trigger-is January 13, 2025 16:37 — with GitHub Actions Inactive

pietern added 2 commits January 13, 2025 20:03

Merge remote-tracking branch 'origin/main' into no-sync-root-check-fo…

7a633ec

…r-artifact-paths

Update apps path translation

9ce30f3

pietern temporarily deployed to test-trigger-is January 13, 2025 19:07 — with GitHub Actions Inactive

pietern added this pull request to the merge queue Jan 14, 2025

Merged via the queue into main with commit 5d9bc3b Jan 14, 2025
9 checks passed

pietern deleted the no-sync-root-check-for-artifact-paths branch January 14, 2025 08:40

andrewnester mentioned this pull request Jan 16, 2025

[Release] Release v0.239.0 #2167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow artifact path to be located outside the sync root #2128

Allow artifact path to be located outside the sync root #2128

pietern commented Jan 13, 2025 •

edited

Loading

pietern Jan 13, 2025

pietern Jan 13, 2025

denik Jan 13, 2025

pietern Jan 13, 2025

pietern Jan 13, 2025

pietern Jan 13, 2025

denik Jan 13, 2025

pietern Jan 13, 2025

pietern Jan 13, 2025

denik commented Jan 13, 2025

pietern commented Jan 14, 2025 •

edited

Loading

Allow artifact path to be located outside the sync root #2128

Allow artifact path to be located outside the sync root #2128

Conversation

pietern commented Jan 13, 2025 • 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

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

denik commented Jan 13, 2025

pietern commented Jan 14, 2025 • edited Loading

pietern commented Jan 13, 2025 •

edited

Loading

pietern commented Jan 14, 2025 •

edited

Loading