Fix sync --dry-run mutating the remote workspace#5495
Open
simonfaltum wants to merge 5 commits into
Open
Conversation
A dry-run sync against a missing remote path created the directory, and applyPut opened local files before checking DryRun, so a file vanishing mid-run failed the preview. Skip the mkdir and the local open in dry-run mode. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: b8d71bf
22 interesting tests: 15 SKIP, 7 KNOWN
Top 28 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Git Bash rewrites the /Users/... argument to C:/Program Files/Git/Users/... before the CLI sees it, so the recorded request paths diverged; MSYS_NO_PATHCONV=1 disables that conversion. Also force LF on committed .py fixtures so the uploaded raw_body does not gain a CR on Windows checkouts. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
reviewed
Jun 10, 2026
| # uploading the file's content to a workspace. | ||
| *.txt text eol=lf | ||
| *.lvdash.json text eol=lf | ||
| *.py text eol=lf |
Member
Author
There was a problem hiding this comment.
Removed in b8d71bf. With the request bodies filtered out of the recorded output, the line ending of the .py fixture no longer matters.
| Uploaded .gitignore | ||
| Uploaded hello.py | ||
|
|
||
| >>> print_requests.py --sort //api/ |
Contributor
There was a problem hiding this comment.
Can use jq to filter to the method and path to omit the body from the output.
Member
Author
There was a problem hiding this comment.
Done in b8d71bf, the script now pipes print_requests.py through jq '{method, path}' so the body is omitted.
| @@ -0,0 +1 @@ | |||
| print("hello") | |||
Contributor
There was a problem hiding this comment.
Test should be nested under cmd/sync/<test name>.
Member
Author
There was a problem hiding this comment.
Moved to acceptance/cmd/sync/dryrun-missing-remote in b8d71bf. The parent cmd/sync test syncs its own directory, so its output now also lists the nested test files, same as the existing dryrun/out.test.toml entry.
| # A dry run must not create the missing remote dir or upload anything. | ||
| # Note: output line starting with Action lists files in non-deterministic order so we filter it out | ||
| # MSYS_NO_PATHCONV=1 prevents Git Bash on Windows from converting /Users/... to C:/Program Files/Git/Users/... | ||
| MSYS_NO_PATHCONV=1 trace $CLI sync . /Users/$CURRENT_USER_NAME/missing-dir --dry-run | grep -v "^Action" | sort |
Contributor
There was a problem hiding this comment.
Concurrent tests will interfere. Tip: use ephemeral remote directory and clean it up on exit.
Member
Author
There was a problem hiding this comment.
Done in b8d71bf. The test now syncs to /Users//missing-dir-$UNIQUE_NAME and the cleanup trap deletes the remote directory on exit.
…dryrun # Conflicts: # acceptance/.gitattributes
Move the test under acceptance/cmd/sync/dryrun-missing-remote, use an ephemeral remote directory based on UNIQUE_NAME with cleanup on exit, and filter recorded requests to method and path via jq so request bodies stay out of the output. Drop the .gitattributes *.py entry; with bodies omitted the line ending of the uploaded fixture no longer affects the output. Regenerate the parent cmd/sync output which now lists the nested test directory files. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
approved these changes
Jun 11, 2026
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
3 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.
Why
Found during a full-repo review of the CLI.
databricks sync --dry-run(andbundle sync --dry-run) promises to make no changes, but it still mutated the workspace: if the remote directory did not exist, the sync setup created it. Separately, the dry-run upload path opened every local file before checking the dry-run flag, so a file deleted between listing and upload failed the preview for no reason.Changes
Before, a dry run created the missing remote directory and opened every local upload candidate; now a dry run performs only read-only API calls and never touches local file contents.
EnsureRemotePathIsUsabletakes adryRunparameter. Read-only validation (path lookup, repo existence check) still runs, but creating a missing remote directory is skipped in dry-run mode.applyPutopens the local file inside the!DryRunbranch, next to the write it feeds.Test plan
acceptance/cmd/sync-dryrun-missing-remoterecords HTTP requests and asserts that a dry run against a missing remote path records no mkdirs or import requests, while the same sync without--dry-runrecords them.TestApplyPutDryRunSkipsLocalReadcovers the missing-local-file case in dry-run mode.cmd/sync,cmd/sync/dryrun,cmd/sync-from-file,cmd/sync-without-args,bundle/sync).libs/syncunit tests pass; integration test package compiles../task fmt-q,./task lint-q, and./task checkspass.This pull request and its description were written by Isaac.