Add mutator documentation in initialize phase by denik · Pull Request #2560 · databricks/cli

denik · 2025-03-25T12:00:02Z

Changes

Add comments to every mutator call in initialize.go. Some of them written manually, most are AI generated.

The script & prompt are not included available on separate branch:
https://github.com/databricks/cli/blob/denik/mutator-documentation-with-script/tools/update_phases_comments.py
https://github.com/databricks/cli/blob/denik/mutator-documentation-with-script/bundle/phases/mutator_documentation.md

Why

This helps to see what changes are mutators making to bundle (if any), without reading every source file.

pietern · 2025-03-28T12:26:52Z


+ // Reads (dynamic): resources.jobs.*.{notebook_task.notebook_path,spark_jar_task.main_class_name,spark_python_task.python_file}, resources.pipelines.*.{libraries.notebook.path,libraries.file.path}, resources.dashboards.*.definition, resources.apps.*.{package,resources.*.path}
+ // Updates (dynamic): resources.jobs.*.{notebook_task.notebook_path,spark_jar_task.main_class_name,spark_python_task.python_file}, resources.pipelines.*.{libraries.notebook.path,libraries.file.path}, resources.dashboards.*.definition, resources.apps.*.{package,resources.*.path} (converts local paths to workspace paths)
+ // Translates local file paths to workspace paths for notebooks, files, and directories


The list of paths is incorrect; lists items that are not updated and omits others.

This one is quite complex, no wonder LLM could not write correct comment. I also don't see what exactly it updates from a glance.
Can you suggest authoritative description? Can also be a follow up PR.

For now, I removed wrong comment completely.

shreyas-goenka · 2025-03-31T16:16:53Z

 // Figure out if the sync root path is identical or an ancestor of the bundle root path.
 // If it is an ancestor, this updates all paths to be relative to the sync root path.
+ // Reads (typed): b.Config.Sync.Paths (calculates longest common parent together with bundle root).
+ // Updates (typed) b.{SyncRoot,SyncRootPath} (set to calculate sync root, which is either bundle root or some parent of bundle root)


Suggested change
// Updates (typed) b.{SyncRoot,SyncRootPath} (set to calculate sync root, which is either bundle root or some parent of bundle root)
// Updates (typed) b.{SyncRoot,SyncRootPath} (set to inferred sync root, which is either bundle root or some parent of bundle root)

Not sure why the comment was disregarded as it seems like inferred makes more sense here

"inferred" to be implies some guessing or really deep reasoning. We just calculate the closest common parent, nothing fancy.

shreyas-goenka · 2025-03-31T16:22:39Z

- // searches for strings with variable references in them.
+ // Reads (dynamic): * (strings) (searches for strings with workspace path variables prefixed with "/Workspace")
+ // Updates (dynamic): * (strings) (removes "/Workspace" prefix from workspace path variables)
+ // Finds and removes "/Workspace" prefix from all strings in bundle configuration


Suggested change
// Finds and removes "/Workspace" prefix from all strings in bundle configuration
// Finds and removes "/Workspace" prefix from all references to `workspace.root_path`, `workspace.file_path` etc in bundle configuration

andrewnester · 2025-04-01T13:53:02Z

 // Figure out if the sync root path is identical or an ancestor of the bundle root path.
 // If it is an ancestor, this updates all paths to be relative to the sync root path.
+ // Reads (typed): b.Config.Sync.Paths (calculates longest common parent together with bundle root).
+ // Updates (typed) b.{SyncRoot,SyncRootPath} (set to calculate sync root, which is either bundle root or some parent of bundle root)


Not sure why the comment was disregarded as it seems like inferred makes more sense here

andrewnester · 2025-04-01T13:59:13Z

 // searches for strings with variable references in them.
 mutator.RewriteWorkspacePrefix(),

+ // Reads (dynamic): variables.* (checks for existing values, defaults, and lookup references)


Not sure what this part means, should it be clarified?

How would you write it?

andrewnester · 2025-04-01T14:00:34Z


+ // Reads (dynamic): variables.* (checks for existing values, defaults, and lookup references)
+ // Updates (dynamic): variables.*.value (sets values from environment variables, variable files, or defaults)
+ // Resolves and sets values for bundle variables from environment variables, variable files, or defaults


Maybe worth clarifying the order how we set the variables?

How would you write it?

andrewnester · 2025-04-01T14:05:01Z

+
+ // Reads (typed): b.Config.RunAs, b.Config.Workspace.CurrentUser (validates run_as configuration)
+ // Reads (dynamic): run_as (checks if run_as is specified)
+ // Updates (typed): b.Config.Resources.Jobs[].RunAs (sets job run_as fields to bundle run_as)


It only does it when Experimental.UseLegacyRunAs is set which is imprortant part here. It also does it for pipelines and mostly just validates that we don;t use run_as with unsupported resources

andrewnester · 2025-04-02T13:53:06Z

+
+ // Reads (typed): b.Config.Experimental.PythonWheelWrapper, b.Config.Presets.SourceLinkedDeployment (checks Python wheel wrapper and deployment mode settings)
+ // Reads (dynamic): resources.jobs.*.tasks (checks for tasks with local libraries and incompatible DBR versions)
+ // Provides warnings when Python wheel tasks require DBR 13.3+ or when wheel wrapper is incompatible with source-linked deployment


This seems incorrect, the warning is for cases when python wheel tasks are used on DBRs < 13.3

Co-authored-by: Andrew Nester <andrew.nester.dev@gmail.com>

## Changes - Add comments to every mutator call in initialize.go. Some of them written manually, most are AI generated. The script & prompt are not included available on separate branch: https://github.com/databricks/cli/blob/denik/mutator-documentation-with-script/tools/update_phases_comments.py https://github.com/databricks/cli/blob/denik/mutator-documentation-with-script/bundle/phases/mutator_documentation.md ## Why This helps to see what changes are mutators making to bundle (if any), without reading every source file. --------- Co-authored-by: Andrew Nester <andrew.nester.dev@gmail.com>

denik requested review from andrewnester, anton-107, pietern and shreyas-goenka as code owners March 25, 2025 12:00

denik temporarily deployed to test-trigger-is March 25, 2025 12:00 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is March 25, 2025 12:08 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is March 25, 2025 13:57 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is March 25, 2025 16:16 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is March 25, 2025 16:27 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is March 25, 2025 16:30 — with GitHub Actions Inactive

pietern reviewed Mar 28, 2025
View reviewed changes

andrewnester reviewed Mar 28, 2025
View reviewed changes

Comment threadbundle/phases/initialize.go Outdated
Comment threadbundle/phases/initialize.go Outdated

denik force-pushed the mutator-documentation branch from 175631e to a6eea15Compare March 28, 2025 12:44

denik temporarily deployed to test-trigger-is March 28, 2025 12:44 — with GitHub Actions Inactive

denik changed the title ~~AI generated mutator documentation in initialize phase~~Add mutator documentation in initialize phaseMar 28, 2025

denik temporarily deployed to test-trigger-is March 28, 2025 16:23 — with GitHub Actions Inactive

denik force-pushed the mutator-documentation branch from a6e9049 to 1fcad7aCompare March 28, 2025 16:23

denik temporarily deployed to test-trigger-is March 28, 2025 16:23 — with GitHub Actions Inactive

denik requested review from andrewnester and pietern March 28, 2025 16:26

shreyas-goenka reviewed Mar 31, 2025
View reviewed changes

shreyas-goenka self-requested a review March 31, 2025 16:26

denik temporarily deployed to test-trigger-is March 31, 2025 17:34 — with GitHub Actions Inactive

denik force-pushed the mutator-documentation branch from d1a0669 to 1ed2ec0Compare April 1, 2025 13:22

denik temporarily deployed to test-trigger-is April 1, 2025 13:26 — with GitHub Actions Inactive

andrewnester reviewed Apr 2, 2025
View reviewed changes

Add comments
b35785d

denik force-pushed the mutator-documentation branch from eac2aa0 to b35785dCompare April 3, 2025 07:34

denik temporarily deployed to test-trigger-is April 3, 2025 07:34 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is April 3, 2025 07:35 — with GitHub Actions Inactive

address comments and clean up
b5ae417

denik force-pushed the mutator-documentation branch from 7aafea4 to b5ae417Compare April 3, 2025 07:42

denik temporarily deployed to test-trigger-is April 3, 2025 07:42 — with GitHub Actions Inactive

denik requested a review from andrewnester April 3, 2025 07:44

andrewnester approved these changes Apr 4, 2025
View reviewed changes

Comment threadbundle/phases/initialize.go Outdated
Comment threadbundle/phases/initialize.go Outdated
Comment threadbundle/phases/initialize.go Outdated

denikand others added 3 commits April 4, 2025 14:49

Update bundle/phases/initialize.go
21b78cf
Co-authored-by: Andrew Nester <andrew.nester.dev@gmail.com>

Update bundle/phases/initialize.go
e52f43c
Co-authored-by: Andrew Nester <andrew.nester.dev@gmail.com>

Merge branch 'main' into mutator-documentation
d9f372a

denik temporarily deployed to test-trigger-is April 4, 2025 12:52 — with GitHub Actions Inactive

Update bundle/phases/initialize.go
153db1c
Co-authored-by: Andrew Nester <andrew.nester.dev@gmail.com>

denik temporarily deployed to test-trigger-is April 4, 2025 12:53 — with GitHub Actions Inactive

denik enabled auto-merge April 4, 2025 12:54

denik disabled auto-merge April 4, 2025 13:34

denik merged commit 0821f08 into mainApr 4, 2025

denik deleted the mutator-documentation branch April 4, 2025 13:34

denik mentioned this pull request Apr 17, 2025
Add NewPatternFromString and bundle.SetDefault; use it for volumes & dashboards #2734
Merged

	// Updates (typed) b.{SyncRoot,SyncRootPath} (set to calculate sync root, which is either bundle root or some parent of bundle root)
	// Updates (typed) b.{SyncRoot,SyncRootPath} (set to inferred sync root, which is either bundle root or some parent of bundle root)

	// Finds and removes "/Workspace" prefix from all strings in bundle configuration
	// Finds and removes "/Workspace" prefix from all references to `workspace.root_path`, `workspace.file_path` etc in bundle configuration

Uh oh!

Conversation

denik commented Mar 25, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Why

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

denik commented Mar 25, 2025•
edited
Loading