-
Notifications
You must be signed in to change notification settings - Fork 793
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
fix: cloud login now also updates context #4699
base: main
Are you sure you want to change the base?
Conversation
@@ -41,7 +41,7 @@ def get_email(self) -> str: | |||
"Unable to get current user from yatai server" | |||
) | |||
self.email = user.email | |||
add_context(self, ignore_warning=True) | |||
_ = add_context(self, ignore_warning=True) |
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.
What is the purpose of using a underscore to keep the result?
@@ -61,7 +61,8 @@ def login(shared_options: SharedOptions, endpoint: str, api_token: str) -> None: | |||
email=user.email, | |||
) | |||
|
|||
add_context(ctx) | |||
new_config = add_context(ctx) | |||
_ = new_config.set_current_context(ctx.name) |
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.
Why do we update the current context? This would be a behavior change.
I often do bentoml cloud login ... --context dev
and then bentoml push --context dev
, because I want to use the dev environment for testing but don't want to update the current context.
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.
I see. Because if we have multiple context:
doing bentoml cloud login ...
as copied from the UI doesn't actually log you in the context. As a user I would expect when i login my context will change to the one i just logged in.
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.
doing
bentoml cloud login ...
as copied from the UI doesn't actually log you in the context.
Okay, but the approach to fixing it is wrong. We should instead, login using the current active context when no --context
is given.
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.
I agree with @frostming here, we shouldn't updating the context automatically like this, which is confusing.
Probably better to be explicit at this case.
What does this PR address?
currently, using
bentoml cloud login
will not change the context, so it is not exactly "logging in".Fixes #(issue)
Before submitting:
guide on how to create a pull request.
pre-commit run -a
script has passed (instructions)?those accordingly? Here are documentation guidelines and tips on writting docs.