-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat: added a timeout to the login command #991
Conversation
@craicoverflow have you had a chance to review the changes? |
The build is failing - this needs to be fixed.
We have a number of tasks to complete, but don't worry - this is on the backlog. |
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.
So, the best way to do this would be to use context.WithTimeout
, but only on the auth requests.
I believe that the timeout should be focused on the network request, not the command itself.
Currently we are using context.WithCancel
here, this could be replaced with context.WithTimeout
.
Since one context is created for each OpenID client, using 30 seconds for each might be satisfactory (as when combined it gives 60 seconds) - looking for @wtrocki's opinion before we agree with this approach.
So first callback requires human interaction (if you need to login). Second callback uses SSO so it is almost instant. |
Global timeout makes sense then. We should try to avail of the built-in The context can be passed to app-services-cli/pkg/auth/login/login.go Line 41 in b4128d5
|
Needs rebase. |
Rebase needed. |
@wtrocki , @craicoverflow please see a new PR #1027. This one can be closed as I lost my old local repo. |
Closes #989