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

feat(auth): adds support for non-interactive login #32

Merged

Conversation

ankitrgadiya
Copy link
Member

@ankitrgadiya ankitrgadiya commented May 18, 2022

Resolves #24

┆Issue is synchronized with this Wrike task

@ankitrgadiya ankitrgadiya added the 🔨 refactor Improvements in an existing feature label May 18, 2022
@ankitrgadiya ankitrgadiya added this to the v0.4.0 milestone May 18, 2022
@ankitrgadiya ankitrgadiya self-assigned this May 18, 2022
@ankitrgadiya ankitrgadiya linked an issue May 18, 2022 that may be closed by this pull request
2 tasks
@ankitrgadiya ankitrgadiya force-pushed the feature/non-interactive-login branch from 61a85a7 to d67eaa0 Compare May 20, 2022 04:28
@ankitrgadiya ankitrgadiya marked this pull request as ready for review May 20, 2022 04:29
@ankitrgadiya ankitrgadiya added the ⏳ awaiting-approval Requires approval label May 20, 2022
@ankitrgadiya ankitrgadiya force-pushed the feature/non-interactive-login branch from d67eaa0 to 1b2f8ad Compare May 20, 2022 08:34
@wiresurfer wiresurfer self-requested a review May 25, 2022 09:06
Copy link
Contributor

@wiresurfer wiresurfer left a comment

Choose a reason for hiding this comment

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

looks good now.

@@ -47,6 +51,9 @@ def login(ctx: click.Context, email: str, password: str):
click.echo("[Warning] rio already has a config file present")
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple of comments.
I am assuming the project variable is implicitly setting the project key in ctx.obj.data.

  • Is that true? If yes, then we should still explicitly set it just like we set set email_id, password, auth_token, for readability.

  • Line 47-49. Can we have the ctx.obj.save before the echo. in case the save fails, we will have a very confusing order of messages. Ideally with spinner: ctx.obj.save() click.echo

  • instead of --no-project flag. can we call it something more indicative. --interactive/--no-interactive flag. then we can pseudo-code it with. if interactive. then ask email, if not given, ask password if not give, select project if not given. if no-interactive, check the three args [email, password, project] raise error if anything is missing. set the config.

@wiresurfer wiresurfer merged commit 21730c5 into feature/shell-improvements May 25, 2022
@ankitrgadiya ankitrgadiya removed the ⏳ awaiting-approval Requires approval label May 26, 2022
rr-github-ci-user pushed a commit that referenced this pull request Oct 3, 2022
# [0.4.0](v0.3.1...v0.4.0) (2022-10-03)

### Bug Fixes

* **network:** handles network not found correctly ([#22](#22)) ([b38c7a0](b38c7a0))
* **shell:** Fixed a bug which causes REPL to close in case of exception. ([e8dc6f0](e8dc6f0))

### Features

* **apply:** adds support for apply command ([#30](#30)) ([f6ae40d](f6ae40d)), closes [#39](#39)
* **auth:** adds support for non-interactive login ([#32](#32)) ([8c8c460](8c8c460))
* **project:** adds highlight for current project in list output ([ce348da](ce348da))
* **shell:** adds improvements in repl session ([b7a481e](b7a481e))
@ankitrgadiya ankitrgadiya deleted the feature/non-interactive-login branch April 20, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 refactor Improvements in an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add support for non-interactive login experience
2 participants