Skip to content

internal: Refactor immutable folder upload as an internal resource in direct engine - #6084

Open
andrewnester wants to merge 5 commits into
mainfrom
feat/upload-resource
Open

internal: Refactor immutable folder upload as an internal resource in direct engine#6084
andrewnester wants to merge 5 commits into
mainfrom
feat/upload-resource

Conversation

@andrewnester

Copy link
Copy Markdown
Contributor

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

@andrewnester
andrewnester requested a review from denik July 28, 2026 11:29
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Approval status: pending

/acceptance/bundle/ - needs approval

8 files changed
Suggested: @denik
Also eligible: @pietern, @janniklasrose, @shreyas-goenka, @lennartkats-db, @anton-107

/bundle/ - needs approval

20 files changed
Suggested: @denik
Also eligible: @pietern, @janniklasrose, @shreyas-goenka, @lennartkats-db, @anton-107

/cmd/bundle/ - needs approval

Files: cmd/bundle/plan.go
Suggested: @denik
Also eligible: @pietern, @janniklasrose, @shreyas-goenka, @lennartkats-db, @anton-107

General files (require maintainer)

Files: libs/snapshot/client.go, libs/snapshot/hash.go, libs/testserver/handlers.go
Based on git history:

  • @denik -- recent work in bundle/direct/dresources/, bundle/direct/, bundle/phases/

Any maintainer (@anton-107, @denik, @pietern, @shreyas-goenka, @simonfaltum, @renaudhartert-db, @janniklasrose, @lennartkats-db) can approve all areas.
See OWNERS for ownership rules.

@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: aa73e34

Run: 30368923813

Env ❌​FAIL 🟨​KNOWN 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
❌​ aws linux 3 1 3 4 319 1062 5:57
❌​ aws windows 3 1 3 4 321 1060 9:28
❌​ azure linux 3 1 3 4 319 1061 6:27
❌​ azure windows 3 1 3 4 321 1059 8:51
💚​ gcp linux 1 5 321 1063 6:16
❌​ gcp windows 3 1 5 320 1061 9:58
11 interesting tests: 4 SKIP, 3 FAIL, 3 RECOVERED, 1 KNOWN
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
🟨​ TestAccept 🟨​K 🟨​K 🟨​K 🟨​K 💚​R 🟨​K
❌​ TestAccept/bundle/deploy/mlops-stacks ❌​F ❌​F ❌​F ❌​F ✅​p ❌​F
❌​ TestAccept/bundle/deploy/mlops-stacks/DATABRICKS_BUNDLE_ENGINE=direct ❌​F ❌​F ❌​F ❌​F ✅​p ❌​F
❌​ TestAccept/bundle/deploy/mlops-stacks/DATABRICKS_BUNDLE_ENGINE=terraform ❌​F ❌​F ❌​F ❌​F ✅​p ❌​F
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R
Top 1 slowest tests (at least 2 minutes):
duration env testname
2:46 gcp linux TestAccept

@denik denik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not have edge case for "internal" resources. We can just create resources.internal_files, populate it ourselves (forbid users from touching it) and then just let the framework handle it as a regular resource.

Deploying resources...
Updating deployment state...
Deployment complete!
\nSnapshot did not change as expected

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: \n is printed verbatim rather than as newline.

// For regular resources like "resources.jobs.foo.name", returns ("resources.jobs.foo", "name").
// For sub-resources like "resources.jobs.foo.permissions[0].level", returns ("resources.jobs.foo.permissions", "[0].level").
func splitResourcePath(path *structpath.PathNode) (string, *structpath.PathNode) {
// Internal resources: "internal.<name>.<field>" — key is first two components.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep using resources. format? Could be resources.internal_files or resources.files. We control the namespace so we can ensure they don't conflict.

"deployments": [
{
"command_path": "${workspace.snapshot_path}/files/src/main.py",
"command_path": "${internal.snapshot.full_path}/files/src/main.py",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// NewAdapterFromInstance creates an Adapter from an already-initialized resource
// instance. Use this for internal resources whose New() method doesn't take a
// workspace client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear why do we need it a custom New there. SnapshotClient is a wrapper for WorkspaceClient, so we can just create it normally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants