Handle `${workspace.file_path}` references in source-linked deployments #2046

ilyakuz-db · 2024-12-23T11:19:52Z

Changes

Updates workspace.file_path during variable resolution phase in source-linked deployment to address cases like this https://github.com/databricks/bundle-examples/blob/main/default_python/resources/default_python_pipeline.yml#L13
Updates workspace.file_path in metadata.json
Prints warning for users when workspace.file_path is explicitly set but deploy is running in source-linked mode

Tests

Unit test

…ment

bundle/config/mutator/resolve_variable_references.go

…ked-deployment-file-path

ilyakuz-db · 2025-01-02T13:56:30Z

I moved source-linked deployment preset handler in separate mutator and now it runs before variable resolution step

One caveat is that this mutator uses development mode setting but it runs before ProcessTargetMode so it checks b.Config.Bundle.Mode == config.Development without any additional validations

github-actions · 2025-01-02T13:58:37Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

PR number: 2046
Commit SHA: a65dc982a955cde543eb7503de86f683dd30c499

Checks will be approved automatically on success.

…icitly set

ilyakuz-db · 2025-01-06T14:13:32Z

bundle/phases/initialize.go

@@ -41,6 +41,10 @@ func Initialize() bundle.Mutator {
 			mutator.PopulateCurrentUser(),
 			mutator.LoadGitDetails(),

+			// This mutator needs to be run before variable interpolation and defining default workspace paths
+			// because it affects how workspace variables are resolved.
+			mutator.ApplySourceLinkedDeploymentPreset(),


I moved all source-linked initialization logic to separate mutator which will is running before workspace paths is defined and variables are resolved

This is concerning -- it decentralizes how we deal with mode: development and I'd like to keep that in one place. I understand why you need to do this here, and we can merge the PR to unblock the issue in the workspace, but we should revert to the original place.

@pietern what do you think would be the best approach here?

We have several constraint that are not super compatible

we need to enable/disable source-linked before variable resolution phase (to properly compute file_path)

source-linked deployment needs to be enabled in dev mode

dev mode validation relies on workspace paths, which may include variables (see findNonUserPath)

Instead of intercepting interpolation of ${workspace.file_path} we could perform a modification pass after variable resolution where we replace instances of the value of this variable. Would that work?

cc @denik

Let's keep on hold until @denik starts work on variable resolution v2 to see what would be the better approach

ilyakuz-db · 2025-01-06T14:15:50Z

bundle/config/mutator/apply_source_linked_deployment_preset.go

+		b.Config.Presets.SourceLinkedDeployment = &enabled
+	}
+
+	if b.Config.Workspace.FilePath != "" && config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) {


Added warning for cases when workspace.file_path is defined by user but deploy is running in source-linked mode

Please include a comment saying that this mutator runs before these fields are defaulted. I was puzzled by this but then found it is run before the mutators that populate its default value.

andrewnester

LGTM @pietern @denik can you take a look as well?

andrewnester · 2025-01-06T15:01:22Z

bundle/config/mutator/apply_source_linked_deployment_preset.go

+			)
+
+			disabled := false
+			b.Config.Presets.SourceLinkedDeployment = &disabled


SInce you're mutating the config here, you'd have to call b.Config.Mutate and do all the mutation inside the callback there

Thanks, I will update it! Could you elaborate what is the difference between this mutation and normal assignment to struct. Seems that it works good in both cases but I may miss some context

Discussed with Pieter that it's better to skip dynamic assignment here in favor of simplicity, but I can update it in another PR if it doesn't feel right

…ked-deployment-file-path

pietern · 2025-01-08T11:51:04Z

bundle/config/mutator/apply_source_linked_deployment_preset.go

+
+	if config.IsExplicitlyEnabled((b.Config.Presets.SourceLinkedDeployment)) {
+		if !isDatabricksWorkspace {
+			path := dyn.NewPath(dyn.Key("targets"), dyn.Key(target), dyn.Key("presets"), dyn.Key("source_linked_deployment"))


This won't work -- the targets field is cleared after selecting it.

But because the target is merged into the main configuration, you can use presets.source_linked_deployment directly and it will return the original location.

pietern · 2025-01-08T11:53:58Z

bundle/config/mutator/apply_source_linked_deployment_preset.go

+		b.Config.Presets.SourceLinkedDeployment = &enabled
+	}
+
+	if b.Config.Workspace.FilePath != "" && config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) {


Please include a comment saying that this mutator runs before these fields are defaulted. I was puzzled by this but then found it is run before the mutators that populate its default value.

pietern · 2025-01-08T11:54:31Z

bundle/config/mutator/apply_source_linked_deployment_preset.go

+		diags = diags.Append(
+			diag.Diagnostic{
+				Severity: diag.Warning,
+				Summary:  "workspace.file_path setting will be ignored in source-linked deployment mode",


This warning should have a Detail section explaining the issue further.

pietern · 2025-01-08T11:54:45Z

bundle/config/mutator/apply_source_linked_deployment_preset.go

+	}
+
+	if b.Config.Workspace.FilePath != "" && config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) {
+		path := dyn.NewPath(dyn.Key("targets"), dyn.Key(target), dyn.Key("workspace"), dyn.Key("file_path"))


Same issue as above w.r.t. the path not existing.

Seems like it works fine, or do we expect other behavior here?

Ah, I was confused by the path including targets.<target>.

This call site and the one above index it with path[2:].

It looks like it is not used in full, so the first two elements can be removed from the assignment.

pietern · 2025-01-08T12:38:39Z

bundle/phases/initialize.go

@@ -41,6 +41,10 @@ func Initialize() bundle.Mutator {
 			mutator.PopulateCurrentUser(),
 			mutator.LoadGitDetails(),

+			// This mutator needs to be run before variable interpolation and defining default workspace paths
+			// because it affects how workspace variables are resolved.
+			mutator.ApplySourceLinkedDeploymentPreset(),


This is concerning -- it decentralizes how we deal with mode: development and I'd like to keep that in one place. I understand why you need to do this here, and we can merge the PR to unblock the issue in the workspace, but we should revert to the original place.

pietern · 2025-01-08T12:43:35Z

bundle/deploy/metadata/compute.go

@@ -54,5 +54,8 @@ func (m *compute) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics {

 	// Set file upload destination of the bundle in metadata
 	b.Metadata.Config.Workspace.FilePath = b.Config.Workspace.FilePath
+	if config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) {


Missing comment why this is done.

Bundles: * Fix finding Python within virtualenv on Windows ([#2034](#2034)). * Include missing field descriptions in JSON schema ([#2045](#2045)). * Add validation for volume referenced from `artifact_path` ([#2050](#2050)). * Handle `${workspace.file_path}` references in source-linked deployments ([#2046](#2046)). * Set the write bit for files written during template initialization ([#2068](#2068)).

denik · 2025-01-14T21:15:27Z

bundle/config/mutator/resolve_variable_references.go

+func lookup(v dyn.Value, path dyn.Path, b *bundle.Bundle) (dyn.Value, error) {
+	if config.IsExplicitlyEnabled(b.Config.Presets.SourceLinkedDeployment) {
+		if path.String() == "workspace.file_path" {
+			return dyn.V(b.SyncRootPath), nil


Why could not we just set bundle.workspace.file_path to b.SyncRootPath and let it be resolved naturally?

I explained it here, tldr: overriding file path is nice but with new changes there is a risk to affect source files accidentally

but we can revisit that

…th specified (#2119) ## Changes Resolves remaining comments from here #2046 [This](#2046 (comment)) and [this](#2046 (comment)) are on hold until Pieter's response ## Tests

ilyakuz-db added 2 commits December 23, 2024 12:14

fix: Handle ${workspace.file_path references} in source-linked deploy…

d7fbf8a

…ment

fix: Cleanup

35b3921

ilyakuz-db temporarily deployed to test-trigger-is December 23, 2024 11:19 — with GitHub Actions Inactive

ilyakuz-db requested a review from andrewnester December 23, 2024 11:19

ilyakuz-db temporarily deployed to test-trigger-is December 23, 2024 11:20 — with GitHub Actions Inactive

andrewnester reviewed Dec 23, 2024

View reviewed changes

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

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

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

andrewnester requested review from pietern and denik December 23, 2024 12:11

ilyakuz-db added 2 commits December 23, 2024 13:31

fix: Move source-linked logic to lookup

31ff82f

fix: Move source-linked preset before variable resolution step

74f51d6

ilyakuz-db requested a review from shreyas-goenka as a code owner January 2, 2025 11:55

ilyakuz-db temporarily deployed to test-trigger-is January 2, 2025 11:55 — with GitHub Actions Inactive

ilyakuz-db added 2 commits January 2, 2025 13:06

Merge branch 'main' of github.com:databricks/cli into feat/source-lin…

85959ac

…ked-deployment-file-path

fix: Add dbr runtime context to renderer test

cf19166

ilyakuz-db temporarily deployed to test-trigger-is January 2, 2025 13:48 — with GitHub Actions Inactive

fix: Add dbr context mock to pipeline glob paths test

a65dc98

ilyakuz-db temporarily deployed to test-trigger-is January 2, 2025 13:58 — with GitHub Actions Inactive

ilyakuz-db requested a review from andrewnester January 2, 2025 14:19

feat: Change metadata.file_path, print warning when file_path is expl…

42cad14

…icitly set

ilyakuz-db had a problem deploying to test-trigger-is January 6, 2025 14:07 — with GitHub Actions Failure

ilyakuz-db commented Jan 6, 2025

View reviewed changes

andrewnester approved these changes Jan 6, 2025

View reviewed changes

fix: Test name update

53c1e18

ilyakuz-db temporarily deployed to test-trigger-is January 7, 2025 17:53 — with GitHub Actions Inactive

Merge branch 'main' of github.com:databricks/cli into feat/source-lin…

fcdc3ea

…ked-deployment-file-path

ilyakuz-db temporarily deployed to test-trigger-is January 7, 2025 17:54 — with GitHub Actions Inactive

pietern changed the title ~~fix: Handle ${workspace.file_path references} in source-linked deploy…~~ Handle ${workspace.file_path} references in source-linked deployments Jan 8, 2025

pietern changed the title ~~Handle ${workspace.file_path} references in source-linked deployments~~ Handle ${workspace.file_path} references in source-linked deployments Jan 8, 2025

pietern approved these changes Jan 8, 2025

View reviewed changes

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

Merged via the queue into main with commit 0289bec Jan 8, 2025
9 checks passed

pietern deleted the feat/source-linked-deployment-file-path branch January 8, 2025 12:50

pietern mentioned this pull request Jan 8, 2025

[Release] Release v0.238.0 #2096

Merged

ilyakuz-db mentioned this pull request Jan 10, 2025

fix: Detailed message for using source-linked deployment with file_path specified #2119

Merged

denik reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `${workspace.file_path}` references in source-linked deployments #2046

Handle `${workspace.file_path}` references in source-linked deployments #2046

ilyakuz-db commented Dec 23, 2024 •

edited

Loading

ilyakuz-db commented Jan 2, 2025

github-actions bot commented Jan 2, 2025

ilyakuz-db Jan 6, 2025

pietern Jan 8, 2025

ilyakuz-db Jan 10, 2025

pietern Jan 14, 2025

ilyakuz-db Jan 15, 2025

ilyakuz-db Jan 6, 2025

pietern Jan 8, 2025

andrewnester left a comment

andrewnester Jan 6, 2025

ilyakuz-db Jan 6, 2025 •

edited

Loading

ilyakuz-db Jan 8, 2025

pietern Jan 8, 2025

pietern Jan 8, 2025

pietern Jan 8, 2025

pietern Jan 8, 2025

ilyakuz-db Jan 9, 2025

pietern Jan 14, 2025

pietern Jan 8, 2025

pietern Jan 8, 2025

denik Jan 14, 2025

ilyakuz-db Jan 15, 2025

ilyakuz-db Jan 15, 2025

Handle ${workspace.file_path} references in source-linked deployments #2046

Handle ${workspace.file_path} references in source-linked deployments #2046

Conversation

ilyakuz-db commented Dec 23, 2024 • edited Loading

Changes

Tests

ilyakuz-db commented Jan 2, 2025

github-actions bot commented Jan 2, 2025

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

andrewnester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakuz-db Jan 6, 2025 • edited Loading

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

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

Handle `${workspace.file_path}` references in source-linked deployments #2046

Handle `${workspace.file_path}` references in source-linked deployments #2046

ilyakuz-db commented Dec 23, 2024 •

edited

Loading

ilyakuz-db Jan 6, 2025 •

edited

Loading