feat: added a global timeout to the login command #1027
Conversation
| @@ -53,6 +52,7 @@ func (a *AuthorizationCodeGrant) Execute(ctx context.Context, ssoCfg *SSOConfig, | |||
| if err := a.loginMAS(ctx, masSSOCfg); err != nil { | |||
| return err | |||
| } | |||
| a.Logger.Info(a.Localizer.MustLocalize("login.log.info.loggedIn")) | |||
There was a problem hiding this comment.
I've moved this here because we may end up in a situation like this
Logging in...
Logged in successfully
Logging in to identity.api.openshift.com...
Error: login operation took too long. Please try again. Run the command in verbose mode using the -v flag to see more information
After I did that, everything looks much better
Logging in...
Logging in to identity.api.openshift.com...
Error: login operation took too long. Please try again. Run the command in verbose mode using the -v flag to see more information
There was a problem hiding this comment.
a.Logger.Info(a.Localizer.MustLocalize("login.log.info.loggingInMAS", localize.NewEntry("Host", masSSOHost)))Could you make this into a debug message instead? The user does not need to see two login statuses whyen it should happen automatically. The new ordering might be a bit confusing but that's just an effect of repeated login messages
|
Also, it seems like global timeout is enough and createClientContext don't need to be changed. |
| ctx, cancel := context.WithTimeout(context.Background(), build.DefaultLoginTimeout) | ||
| defer cancel() | ||
|
|
||
| if err = loginExec.Execute(ctx, ssoCfg, masSsoCfg); err != nil { |
There was a problem hiding this comment.
| if err = loginExec.Execute(ctx, ssoCfg, masSsoCfg); err != nil { | |
| if err = loginExec.Execute(ctx, ssoCfg, masSsoCfg); err != nil && errors.Is(err, context.DeadlineExceeded) { |
This could work, to save a nested if statement.
| @@ -53,6 +52,7 @@ func (a *AuthorizationCodeGrant) Execute(ctx context.Context, ssoCfg *SSOConfig, | |||
| if err := a.loginMAS(ctx, masSSOCfg); err != nil { | |||
| return err | |||
| } | |||
| a.Logger.Info(a.Localizer.MustLocalize("login.log.info.loggedIn")) | |||
| a.Logger.Info(a.Localizer.MustLocalize("login.log.info.loggedInMAS", localize.NewEntry("Host", masSSOHost))) | |||
There was a problem hiding this comment.
| a.Logger.Info(a.Localizer.MustLocalize("login.log.info.loggedInMAS", localize.NewEntry("Host", masSSOHost))) | |
| a.Logger.Debug(a.Localizer.MustLocalize("login.log.info.loggedInMAS", localize.NewEntry("Host", masSSOHost))) |
While you are here, would you mind making this a debug message? I think the new ordering might be a little confusing, and the user does not really need to see two log in messages, now that I think about it :)
Closes #989