internal: Refactor immutable folder upload as an internal resource in direct engine - #6084
Merged
Merged
Conversation
Collaborator
Integration test reportCommit: 3ad3160
Top 20 slowest tests (at least 2 minutes):
|
denik
reviewed
Jul 29, 2026
| "deployments": [ | ||
| { | ||
| "command_path": "${workspace.snapshot_path}/files/src/main.py", | ||
| "command_path": "${internal.snapshot.full_path}/files/src/main.py", |
Contributor
There was a problem hiding this comment.
Instead of custom path, can we keep using ${workspace.file_path}, just make it an alias internally to internal resource's snapshot path.
That way:
- users know what this means
- users can use the variable themselves and it'll work both in regular and immutable case.
Contributor
Author
There was a problem hiding this comment.
${workspace.file_path} is ${internal.snapshot.full_path}/files and because we resolve the variables ${workspace.file_path} resolved to this pass. I'd prefer not to change the way variable reslution works and introduce some custom aliases
denik
reviewed
Aug 3, 2026
| @@ -35,12 +36,15 @@ type ACLEntry struct { | |||
| // the same workspace location. | |||
| // This interface exists so the implementation can later be replaced with a Go SDK call. | |||
Contributor
There was a problem hiding this comment.
This does not explain why we need the interface - do we plan on supporting multiple implementation at the same time?
Contributor
Author
There was a problem hiding this comment.
Strictly speaking we don't need it for SDK replacement later but we need this now in unit tests to replace the uploader with mock one
denik
reviewed
Aug 5, 2026
pietern
reviewed
Sep 8, 2026
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
| return path, &SnapshotRemote{RelativePath: path, FullPath: info.Path}, nil |
Contributor
There was a problem hiding this comment.
The path argument is still unused, per the comment in Upload, but we use it as the ID here.
Are you sure that DoRead works and that we don't accidentally recreate on every deploy?
Contributor
Author
There was a problem hiding this comment.
Yes, the path isn't used yet as it is not in API yet, so the snapshot path is identified by it's snapshot ID right now.
We do have tests which checks that we reuse and don't recreate here: https://github.com/databricks/cli/pull/6084/changes#diff-9549fa666ea57d5cde279a9fb3fe70f47fe2f7bebf72dbd8d5c2a31f4b261d13R31-R33
|
|
||
| trace title "Check snapshot plan after deploy" | ||
| trace $CLI bundle plan -o json > plan.json | ||
| trace jq '.plan["resources.internal_immutable_snapshots.immutable"]' plan.json |
Contributor
There was a problem hiding this comment.
This now recreates when it shouldn't.
Contributor
There was a problem hiding this comment.
Possible this is because of job.json. We should confirm with an acc test that it doesn't recreate when no files have changed and the snapshot can be confirmed to exist remotely.
Contributor
Author
There was a problem hiding this comment.
This test is expected to recreated (there's an assertion below). It builds the wheel so it should be a new snapshot.
This test has no recreate https://github.com/databricks/cli/pull/6084/changes#diff-9549fa666ea57d5cde279a9fb3fe70f47fe2f7bebf72dbd8d5c2a31f4b261d13R31-R33
pietern
approved these changes
Sep 8, 2026
Collaborator
Integration test reportCommit: 5909ef6
795 interesting tests: 606 MISS, 188 FAIL, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
denik
added a commit
that referenced
this pull request
Sep 11, 2026
#6084 refactored the immutable folder upload into a direct-engine resource, adding internal_immutable_snapshots to SupportedResources. The drivenTypes guard demanded a fixture for it -- which is the guard working, a new supported type must be covered -- but this one is marked bundle:"internal" on config.Resources: an engine construct with no user-authored config, the same category as permissions and grants. So it is excluded the same way, by the bundle:"internal" tag rather than a name-prefix guess, reading it off config.Resources with structwalk the way resources_test.go does. Co-authored-by: Isaac
denik
added a commit
that referenced
this pull request
Sep 11, 2026
#6084 refactored the immutable folder upload into a direct-engine resource, adding internal_immutable_snapshots to SupportedResources. The drivenTypes guard demanded a fixture for it -- which is the guard working, a new supported type must be covered -- but this one is marked bundle:"internal" on config.Resources: an engine construct with no user-authored config, the same category as permissions and grants. So it is excluded the same way, by the bundle:"internal" tag rather than a name-prefix guess, reading it off config.Resources with structwalk the way resources_test.go does. Co-authored-by: Isaac
denik
added a commit
that referenced
this pull request
Sep 11, 2026
#6084 refactored the immutable folder upload into a direct-engine resource, adding internal_immutable_snapshots to SupportedResources. The drivenTypes guard demanded a fixture for it -- which is the guard working, a new supported type must be covered -- but this one is marked bundle:"internal" on config.Resources: an engine construct with no user-authored config, the same category as permissions and grants. So it is excluded the same way, by the bundle:"internal" tag rather than a name-prefix guess, reading it off config.Resources with structwalk the way resources_test.go does. Co-authored-by: Isaac
denik
added a commit
that referenced
this pull request
Sep 15, 2026
#6084 refactored the immutable folder upload into a direct-engine resource, adding internal_immutable_snapshots to SupportedResources. The drivenTypes guard demanded a fixture for it -- which is the guard working, a new supported type must be covered -- but this one is marked bundle:"internal" on config.Resources: an engine construct with no user-authored config, the same category as permissions and grants. So it is excluded the same way, by the bundle:"internal" tag rather than a name-prefix guess, reading it off config.Resources with structwalk the way resources_test.go does. Co-authored-by: Isaac
janniklasrose
pushed a commit
that referenced
this pull request
Sep 15, 2026
… direct engine (#6084) ## Changes Refactor immutable folder upload as an internal resource in direct engine ## Why This enables us: 1. Keep track of upload state and upload paths, so next deployment can check the status of the uploaded folder. Needed for breakglass 2. Easier path resolution: now all resources can just reference to internal resource like `${internal.snapshot.full_path}` and it's automatically resolved during deploy 3. Plan is correct now and can easily skip uploads if nothing changed and no need to upload ## Tests Existing tests pass <!-- If your PR needs to be included in the release notes for next release, add a changelog fragment: create .nextchanges/<section>/<name>.md with a one-line description (e.g. .nextchanges/cli/quickstart.md). See .nextchanges/README.md. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Refactor immutable folder upload as an internal resource in direct engine
Why
This enables us:
${internal.snapshot.full_path}and it's automatically resolved during deployTests
Existing tests pass