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

Refresh login on all remote infra commands #374

Merged
merged 9 commits into from
Oct 6, 2021
Merged

Refresh login on all remote infra commands #374

merged 9 commits into from
Oct 6, 2021

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Oct 1, 2021

If session is expired when user runs infra list, start login flow. Once user is logged in, continue with the previous command.

  • Make logout idempotent and remove as much as possible.
  • Some wording changes in login status messages
  • Logout of specified registry. If none specified, log out of current registry. Does not activate another registry after logging out of current registry

Fixes #367

@mxyng mxyng changed the title Refresh login Refresh login on all remote infra commands Oct 1, 2021
internal/cmd/cmd.go Outdated Show resolved Hide resolved
@mxyng
Copy link
Collaborator Author

mxyng commented Oct 1, 2021

I wonder if user should be prompted if they want to login when using commands like infra list as opposed to automatically logging them in?

e.g.

$ infra list
Your session has expired. Do you want to login? [Yn]

@mxyng mxyng requested a review from BruceMacD October 1, 2021 20:28
@jmorganca
Copy link
Contributor

I wonder if user should be prompted if they want to login when using commands like infra list as opposed to automatically logging them in?

e.g.

$ infra list
Your session has expired. Do you want to login? [Yn]

Great Q! Yes, in this case we'd want to show them the regular login prompt – otherwise it may be unexpected behavior to just pop open a window. Thoughts? I don't think we have to give them an additional y/n prompt to continue, but perhaps a message stating that their session has expired.

func login(registry string, interactive bool) error {
config, err := readConfig()
if err != nil {
config = &Config{Host: registry}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Configuration does not exist so create a new Config

}

if interactive {
config = &Config{Host: config.Host}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Commands wants interactive login so discard all configuration except Host

}

if config.Host == "" {
return errors.New("Registry host is required")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be unlikely since login will provide a host while other commands will be reading from Config

@@ -459,7 +474,7 @@ func login(config *Config) error {
}

if !proceed {
return nil
return errors.New("Could not continue with login")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is necessary to short circuit the login flow in commands like list, users, and groups

@@ -755,6 +336,14 @@ func NewRootCmd() (*cobra.Command, error) {
return nil, err
}

rootCmd := &cobra.Command{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@mxyng
Copy link
Collaborator Author

mxyng commented Oct 5, 2021

Waiting for #377 to be merged then I'll rebase on top.

@mxyng mxyng force-pushed the refresh-login branch 4 times, most recently from d84014b to 05cb702 Compare October 6, 2021 01:06
@mxyng
Copy link
Collaborator Author

mxyng commented Oct 6, 2021

I wonder if user should be prompted if they want to login when using commands like infra list as opposed to automatically logging them in?
e.g.

$ infra list
Your session has expired. Do you want to login? [Yn]

Great Q! Yes, in this case we'd want to show them the regular login prompt – otherwise it may be unexpected behavior to just pop open a window. Thoughts? I don't think we have to give them an additional y/n prompt to continue, but perhaps a message stating that their session has expired.

This isn't possible right now since the new login will automatically login to the current registry and, if there's a single source, not prompt at all.

@jmorganca
Copy link
Contributor

jmorganca commented Oct 6, 2021

That makes sense! Then this is great since the user is running a command. LGTM!

@mxyng mxyng merged commit 9eafc4a into main Oct 6, 2021
@mxyng mxyng deleted the refresh-login branch October 6, 2021 15:50
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.

Consistent login refresh experience for all commands infra list
3 participants