-
Notifications
You must be signed in to change notification settings - Fork 302
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
Add leave-archive
flag to pd join
#4607
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!! this is a simple, impactful ux improvement 💪
crates/bin/pd/src/testnet/join.rs
Outdated
if !leave_archive { | ||
// Post-extraction, clean up the downloaded tarball. | ||
std::fs::remove_file(archive_filepath)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to complement this with an else { tracing::info!(?path = archive_filepath, "leaving downloaded archive on disk"); }
branch to point users at the location of the tarball, for the sake of facilitating copy-pasting and saving someone the trouble of going looking for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion, I tacked this on just now after testing locally.
includes a log message that displays the path if the flag is set.
deff2d4
to
cad7320
Compare
Downloading gigabytes of archive data while testing locally then seeing it deleted and having to do it again is a powerful motivator 😅 |
Is there a way to point the join command at the existing archive file on disk? I didn't check that |
Wow yeah I would really like being able to install an archive from disk |
Right now loading archives from a local doesn't work:
but we should be able to tweak the scheme, give me a moment... |
Done. I overloaded the |
Saves the need to download a multi-gigabyte file, especially while testing.
4252af1
to
8f2e870
Compare
Follow-up to #4607. This fix is required to preserve the original downloading from URL behavior. Both file:// and https:// URLs must remain supported. Must have accidentally rebased this diff out while collborating on 4607.
Follow-up to #4607. This fix is required to preserve the original downloading from URL behavior. Both file:// and https:// URLs must remain supported. Must have accidentally rebased this diff out while collborating on 4607.
Follow-up to #4607. This fix is required to preserve the original downloading from URL behavior. Both file:// and https:// URLs must remain supported. Must have accidentally rebased this diff out while collborating on 4607.
Describe your changes
Small change to add a
--leave-archive
flag topd join
(defaults to false) to optionally leave behind the downloaded archive file.Issue ticket number and link
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: