-
Notifications
You must be signed in to change notification settings - Fork 510
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
[UX] Print useful message when image id not found #2535
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.
Thanks @cblmemo. Minor UX nits. I guess it's fine to leave docker:xxx
to the future?
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Co-authored-by: Zongheng Yang <zongheng.y@gmail.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.
Thanks @cblmemo, LGTM.
@@ -83,15 +84,18 @@ | |||
# TODO(zhwu): Move the default AMI size to the catalog instead. | |||
DEFAULT_GCP_IMAGE_GB = 50 | |||
|
|||
USER_PORTS_FIREWALL_RULE_NAME = 'sky-ports-{}' |
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.
Short comment? Add a blank line to separate it from L88
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.
Sorry wdym by 'short comment'? Did you mean to short the following string so we could get rid of the # pylint: disable=line-too-long
?
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.
# Describe this var briefly.
USER_PORTS_FIREWALL_RULE_NAME = 'sky-ports-{}'
:)
* add aws, gcp * fix * fix * add IBM message * Update sky/clouds/gcp.py Co-authored-by: Zongheng Yang <zongheng.y@gmail.com> * Update sky/clouds/aws.py Co-authored-by: Zongheng Yang <zongheng.y@gmail.com> * apply suggestion from code review * add newline * add blank line * add comments for constants --------- Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Fixes #2519 by adding some messages to help the user debug. Most of the messages are pasted from our
yaml-spec.rst
.Current docker implementation won't check whether the docker image exists, so no message for docker is added in this PR.
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh