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

cli: don't error if source id doesn't exist #476

Merged
merged 7 commits into from
Oct 21, 2021
Merged

Conversation

jmorganca
Copy link
Contributor

Fixes #408

@jmorganca jmorganca requested a review from mxyng October 18, 2021 15:26
@jmorganca jmorganca changed the title cli: dont error if source id doesnt exist cli: don't error if source id doesn't exist Oct 18, 2021
@mxyng
Copy link
Collaborator

mxyng commented Oct 18, 2021

As mentioned offline, need to be careful with this since the same code path is used by infra tokens create to if the user session is timed out. Since that's generally invoked by kubectl, the non-interactiveness would cause the command line to be blocked indefinitely. Could try changing to Interactive client but that's a larger change.

For the time being, it might be sufficient to use the source domain as the selector instead of ID.

@jmorganca
Copy link
Contributor Author

jmorganca commented Oct 20, 2021

As of the latest change:

  • Added the constraint that we won't support non-interactive logins (yet) Support non-interactive login #488 . This should remove the need for a lock (and potential blocking) since all auth flows should be in the user's foreground/terminal now.
  • kubectl will now correctly prompt users for data if needed at login. Usually, users won't need this, but worst case it works great as well now (survey can print to stderr)

Follow up changes that I've left out to keep this PR small:

internal/cmd/config0dot2.go Outdated Show resolved Hide resolved
internal/cmd/login.go Show resolved Hide resolved
if err != nil {
return err
func login(options LoginOptions) error {
// TODO (https://github.com/infrahq/infra/issues/488): support non-interactive login
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the definition of non-interactive in this context?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This means to me adding support for command line parameters to fill in the fields we would other prompt user. Not sure if it will address the issues with desktop apps needing to reauth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's correct, more specifically when no TTY is available for stdin. For Desktop apps we should consider refresh tokens or another mechanism that allows for transparently renewing session duration

case errors.Is(err, terminal.InterruptErr):
return nil
case len(sources) == 0:
return errors.New("Zero sources have been configured.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once we have APIs to add sources, this should prompt user to input a source rather than error out

internal/cmd/config0dot2.go Outdated Show resolved Hide resolved
internal/cmd/config0dot2.go Outdated Show resolved Hide resolved
internal/cmd/login.go Show resolved Hide resolved
@jmorganca jmorganca merged commit d3b95d1 into main Oct 21, 2021
@jmorganca jmorganca deleted the login-source-error branch October 30, 2021 00:52
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.

Using cache infra configs after reinstalling registry will block login
4 participants