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.

Sign up for GitHub

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

Jump to bottom

Add a timeout when logging in via the web browser #989

Closed
craicoverflow opened this issue Aug 30, 2021 · 5 comments · Fixed by #1027
Closed

Add a timeout when logging in via the web browser #989

craicoverflow opened this issue Aug 30, 2021 · 5 comments · Fixed by #1027
Assignees
@alexal
Labels
feedback good first issue Good for newcomers priority/backlog A nice-to-have, but no one’s available to work on it anytime soon.

Comments

@craicoverflow
Copy link
Contributor

The login command should have a timeout so that it does not "hang" if the command fails.
@alexal
Copy link
Contributor

alexal commented Aug 30, 2021

@craicoverflow I would like to work on this if you do not mind.

@craicoverflow
Copy link
Contributor Author

That would be great @alexal!

@alexal
Copy link
Contributor

alexal commented Aug 30, 2021

@craicoverflow , the code is ready. I have three questions for you:

  1. How big should be the default timeout? 60 seconds will be enough?
  2. What error message should be returned when timeout is reached?
  3. Do you want to me wait until refactor: converted i18n files from TOML to YAML #982 is merged to avoid changing TOML files?

@craicoverflow
Copy link
Contributor Author

@craicoverflow , the code is ready. I have three questions for you:

  1. How big should be the default timeout? 60 seconds will be enough?

I think 60 is probably too much. Let's go with 30.

  1. What error message should be returned when timeout is reached?

"login attempt has expired"

  1. Do you want to me wait until refactor: converted i18n files from TOML to YAML #982 is merged to avoid changing TOML files?

You can go ahead with this PR, we have decided to not go ahead with the YAML i18n files. Thanks.

@alexal alexal mentioned this issue Aug 30, 2021
Closed
@alexal
Copy link
Contributor

alexal commented Aug 30, 2021

@craicoverflow thanks, please review PR #991 .

@craicoverflow craicoverflow added priority/backlog A nice-to-have, but no one’s available to work on it anytime soon. and removed needs-triage labels Sep 2, 2021
@alexal alexal mentioned this issue Sep 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alexal alexal

Labels
feedback good first issue Good for newcomers priority/backlog A nice-to-have, but no one’s available to work on it anytime soon.
Projects
None yet
Milestone
No milestone
3 participants
@alexal @craicoverflow @app-services-ci