Fix bundle validate creating the remote file path#5528
Open
pietern wants to merge 1 commit into
Open
Conversation
Validation ran files_to_sync with a regular sync object, whose setup creates workspace.file_path if it is missing. Validate must not have side effects, so construct the sync in dry-run mode: the read-only path checks still run, but a missing directory is no longer created. Deploy now performs the mkdir itself instead of inheriting the directory from a prior validate, as reflected in the regenerated request recordings. Removes files.GetSync; files_to_sync was its only caller and now needs to set DryRun on the options before constructing the sync. Co-authored-by: Isaac
pietern
force-pushed
the
pieter.noordhuis/validate-dryrun
branch
from
ad60e6f to
4b0eafb
Compare
Contributor
Approval status: pending
|
Collaborator
|
Commit: 4b0eafb
24 interesting tests: 15 SKIP, 7 KNOWN, 2 flaky
Top 29 slowest tests (at least 2 minutes):
|
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
2 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
Before,
bundle validatecreatedworkspace.file_pathin the workspace if it was missing; now validation performs only read-only API calls.files_to_syncvalidator constructs its sync in dry-run mode, building on Fix sync --dry-run mutating the remote workspace #5495 which made dry-run skip creating a missing remote directory. The read-only checks still run: afile_pathpointing at an existing file, or nested under a nonexistent repo, still fails validation.files.GetSync; this validator was its only caller.One behavioral consequence, visible in the regenerated request recordings:
bundle deploynow issues themkdirsitself instead of inheriting the directory from a prior validate.Stacked on #5495; will be retargeted to
mainonce that merges.Why
Validation promises to be side-effect free, but issued
POST /workspace/mkdirswhenever the remote file path didn't exist yet.Tests
acceptance/bundle/validate/no_writesrecords HTTP requests and asserts thatbundle validateagainst a missing remote path issues no write requests at all. Before the fix it recorded aworkspace/mkdirs.bundle/config/validate,bundle/deploy,libs/syncpass;fmt-q,lint-q,checksclean.This pull request and its description were written by Isaac.