-
Notifications
You must be signed in to change notification settings - Fork 415
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
Added confirmation prompt for sky storage delete, and --yes flag to skip it #2726
Added confirmation prompt for sky storage delete, and --yes flag to skip it #2726
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.
@aseriesof-tubes Thanks for your contribution! Left some comments :)
Co-authored-by: Doyoung Kim <34902420+landscapepainter@users.noreply.github.com>
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.
Welcome to SkyPilot @aseriesof-tubes! I noticed some redundant messages and a bug when non-existent storage is specified, noted in comments below.
Thank you for the warm welcome @romilbhardwaj and @landscapepainter, your reviews are appreciated, and changes have been made! |
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.
Thanks for theq quick update @aseriesof-tubes ! Left some comments. This should be ready to merge after 👍
Co-authored-by: Doyoung Kim <34902420+landscapepainter@users.noreply.github.com>
Hi @landscapepainter @romilbhardwaj, the PR is ready for review again! Thank you for your patience :) |
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.
LGTM! Thanks for the contribution @aseriesof-tubes
Co-authored-by: Doyoung Kim <34902420+landscapepainter@users.noreply.github.com>
This is for issue #2635
Added a feature for
sky storage delete
where by default it shows a confirmation prompt before deleting, like insky down
orsky stop
. Like those commands, users can add a--yes
flag to skip the confirmation prompt. Also changed some tests in test_smoke for TestStorageWithCredentials for the tests to automatically use the --yes flag.Tested (run the relevant ones):
bash format.sh
Changed some
check_output
commands to include the--yes
flag, so previously if it wascheck_output( ['sky', 'storage', 'delete', storage_name])
, now it'scheck_output(['sky', 'storage', 'delete', storage_name, '--yes'])
pytest tests/test_smoke.py
pytest tests/test_smoke.py::TestStorageWithCredentials
bash tests/backward_comaptibility_tests.sh