Skip to content
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] Add currently activated account for the sky check and check for disk-tier options #2114

Merged
merged 6 commits into from
Jul 10, 2023

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Jun 21, 2023

This PR adds an --all option for the sky check, so that the user could see more details about the enabled cloud, such as the currently activated account.

This PR also adds checks for the --disk-tier option

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky check -a
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@Michaelvll Michaelvll requested a review from cblmemo July 10, 2023 16:57
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a small UX concern: the name of the argument --all sounds a little bit confusing and could be misinterpreted as checking all clouds (which implies that the sky check would only check certain default clouds without this argument). Should we consider using -v / --verbose or exploring other options? 🤔 If there are no better alternatives, the --all option should be fine.

@cblmemo
Copy link
Collaborator

cblmemo commented Jul 10, 2023

BTW, shall we update the docs in docs/source/reference/cli.rst to support this newly added flag?

@Michaelvll Michaelvll changed the title [UX] Add currently activated account for the sky check [UX] Add currently activated account for the sky check and check for disk-tier options Jul 10, 2023
@Michaelvll
Copy link
Collaborator Author

Thanks for the review @cblmemo! -v sonuds good to me. Just updated the option name.

BTW, shall we update the docs in docs/source/reference/cli.rst to support this newly added flag?

The cli document is automatically generated. The new option should be automatically show up after merging.

Also, I just add a option check for the --disk-tier so that it will only allow the three options when it is passed by the sky launch --disk-tier

Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried it out. LGTM!

sky/cli.py Outdated Show resolved Hide resolved
Co-authored-by: Tian Xia <[email protected]>
@Michaelvll Michaelvll merged commit 7262e21 into master Jul 10, 2023
@Michaelvll Michaelvll deleted the check-a branch July 10, 2023 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants