-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port all binary build tooling to s5cmd, add sync tests, simplify removals #716
Conversation
This file contains 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
8916896
to
0033853
Compare
It can execute operations in parallel, which will massively speed up our repo generation, syncing, and removal scripts. CI needs it, too, since we do not run that inside a build Dockerfile, but there are tests for the build tooling. GUS-W-15794884
New bash quoting for echo-ing the command, too. GUS-W-15794884
The args a weird - other scripts, like deploy.sh, use only the env vars. It's more consistent this way. The bucket region is always returned by the "central" endpoint, even on a 403 response, so we can use that to auto-detect it if necessary.
We now first generate a "sync plan", from which operations that can be grouped together are then extracted, and executed in parallel using `s5cmd run`. First, all copies of new/changed dist archives - if anything fails during these, a re-run of the sync will perform them again. Second, all copies of new/changed manifests, as well as removals - if anything fails during these, only the failed ones will be performed again, including their dists. Finally, all dist archives of removed manifests - worst case, there will be some stray dist archives left behind, but nothing picks them up because their manifests are gone. GUS-W-15794884
Now that we're generating a "sync plan" for execution via `s5cmd run`, we can have some tests that assert the operations are correct, dist archive URL rewrites from source to destination bucket happen properly, etc. GUS-W-15794893
There are now local source and destination dir options for `sync.sh`, which will use the given directory of `.composer.json` manifests, instead of downloading them from the source or destination bucket. What we can now do is, upon removal, fetch all manifests from the repo, except the given list/wildcards of manifests to remove, and then tell `sync.sh` to use that filtered directory of `.composer.json` files as the local source directory for syncing. This moves all of the sanity checks, file deletion, error handling, confirmation etc logic into `sync.sh`, and lets us drop most of the logic in `remove.sh`. In addition, the core "sync plan" code of `sync.sh` is also covered by tests, which now extend to `remove.sh`. GUS-W-15794899
All scripts moved to s5cmd now GUS-W-15794884
edmorley
approved these changes
May 23, 2024
Now show the full sync output when really syncing, and only the sync.sh job summary (without the new-ish 'POTENTIALLY DESTRUCTIVE ACTION' warning at the end) when performing a dry-run, or when printing for info purposes at the end of a build
Not critical, but needs wheel installed so the bdist_wheel command during docopt (a bob-builder dependency) install works in the venv (making it a setup_requires dependency in bob-builder doesn't cut it).
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
s5cmd
can execute operations in parallel, which will massively speed up our repo generation, syncing, and removal scripts:Before (actual
➡️ ![Screenshot 2024-05-28 at 13 06 03](https://private-user-images.githubusercontent.com/27900/334390157-d2eafb3c-e3be-4ccd-b231-3002a2d12a0b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1MTY3MDAsIm5iZiI6MTcyNTUxNjQwMCwicGF0aCI6Ii8yNzkwMC8zMzQzOTAxNTctZDJlYWZiM2MtZTNiZS00Y2NkLWIyMzEtMzAwMmEyZDEyYTBiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA1VDA2MDY0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWYzNjBjNTVmNmUxMmE0ZTFlY2M1NjA5OTQ5OWI5YzQxMTVjMjg0MWVjNWQyNzExODIzZjVmYmE3YTNhNmY0OGMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0._djYBZ86J0IrgNDhEYPUJmA0FMaURjX6gTt3ykAYgVY)
mkrepo
step took 2 minutes 15 seconds) and after (actualmkrepo
step took 3 seconds):Before (actual
➡️ ![Screenshot 2024-05-23 at 16 34 39](https://private-user-images.githubusercontent.com/27900/333333137-1c08945f-d5a9-46ca-b876-e271634390c1.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1MTY3MDAsIm5iZiI6MTcyNTUxNjQwMCwicGF0aCI6Ii8yNzkwMC8zMzMzMzMxMzctMWMwODk0NWYtZDVhOS00NmNhLWI4NzYtZTI3MTYzNDM5MGMxLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA1VDA2MDY0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWZjYzk1NTBlNjA5OWQ5NDRiNDVjZmYwYTc5ZmRjY2U4OGVhYzExZGIwNjA0ODRmZjc2Njc4MjU1MjQ5ZTVkODMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.nci-ZfP3A69WzkqbmqfUZg7Yrw1-jvZK5KREwpWV1B8)
sync
dry-run step took 5 minutes 4 seconds) and after (actualsync
dry-run step took 4 seconds):GUS-W-15794884
For syncing, we now generate a "sync plan", from which operations that can be grouped together are then extracted, and executed in parallel using
s5cmd run
, in three groups:GUS-W-15794884
As the "sync plan" is now generated separately by a script, we can have some tests that assert the operations are correct, dist archive URL rewrites from source to destination bucket happen properly, etc.
GUS-W-15794893
There are now local source and destination dir options for
sync.sh
, which will use the given directory of.composer.json
manifests, instead of downloading them from the source or destination bucket.What we can now do is, upon removal, fetch all manifests from the repo, except the given list/wildcards of manifests to remove, and then tell
sync.sh
to use that filtered directory of.composer.json
files as the local source directory for syncing.This moves all of the sanity checks, file deletion, error handling, confirmation etc logic into
sync.sh
, and lets us drop most of the logic inremove.sh
. In addition, the core "sync plan" code ofsync.sh
is also covered by tests, which now extend toremove.sh
.GUS-W-15794899
Finally, there is currently a bit of a bug in
remove.sh
: when removing all files from a repository (e.g. for certain cleanups, testing, etc), there are no more.composer.json
files left behind that the code can match, and the subsequent call tomkrepo.sh
to re-generate the updated repository fails (as there are no files to pass to it), instead of either producing an empty repository file, or removing the repository file altogether.Instead, the tooling now detects this situation, and removes
packages.json
as well, so that repositories can be fully removed.GUS-W-15838870