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

[minor] Allow sky check to set enabled_clouds to empty #3174

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

Michaelvll
Copy link
Collaborator

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • 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

@concretevitamin
Copy link
Collaborator

Hey @Michaelvll could you remind me why this fix is necessary?

@Michaelvll
Copy link
Collaborator Author

Hey @Michaelvll could you remind me why this fix is necessary?

This is to keep the behavior align for the output of sky check and the enabled_clouds in database. In the master, after removing all the cloud credentials and sky check, although it says all clouds are disabled, the enabled_clouds remains unchanged in the database.

It can cause some minor misalignment of the behavior for sky launch. When there was no cloud enabled before, sky launch will fail quickly. If a user removes all their clouds that were enabled and do a sky check, it will show that all the clouds are disabled, but if they sky launch, it will still show the optimizer table instead of failing quickly.

To reproduce on master:

  1. sky check having aws enabled
  2. mv ~/.aws ~/.aws.bk to disable the aws credential
  3. sky check showing AWS being disabled already
  4. sky launch will show the optimizer table, instead of just saying there is no cloud enabled.

@cblmemo
Copy link
Collaborator

cblmemo commented Feb 18, 2024

Hey @Michaelvll could you remind me why this fix is necessary?

If the user has one enabled cloud previously and the access token expired, then we should set enabled_clouds to empty instead of keeping this stale record?

@concretevitamin
Copy link
Collaborator

Makes sense! LGTM

@Michaelvll Michaelvll merged commit 053d0ba into master Feb 18, 2024
19 checks passed
@Michaelvll Michaelvll deleted the allow-setting-empty-enabled-clouds branch February 18, 2024 04:15
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.

None yet

3 participants