-
Notifications
You must be signed in to change notification settings - Fork 1
Remove config options from prompt, fix configure issue. #69
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
Conversation
sminot
left a comment
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.
Why are we asking the user to run a completely separate command to save the setting to cache the login? Wouldn't it be easier for everyone if we just asked when they first log in?
|
I added something to run the initial configuration if needed |
README.md
Outdated
| ``` | ||
| cirro-cli configure | ||
| ? Would you like to cache your login? Yes | ||
| ``` | ||
|
|
||
| Upon first use, it will give you a link to authenticate through the web browser. |
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.
If we're running _check_configure for all the functions, do users need to run the configure command themselves anymore? Can we remove this from the readme?
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.
It will still be available if they want to change the configuration. Or maybe we just make them edit the file directly. Open to suggestions.
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.
Okay, then I would keep the command in the readme but explicitly say something like "The Cirro client will automatically ask for your credentials the first time you run a command via a link to authenticate through the web browser. If you need to change your credentials after this point, you can run: command here" And maybe change the section title from "set up" to something like "authentication" and move it lower down in the readme since you don't have to do it to set up anymore?
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.
updated
Config options are still available if editing the file manually, but they are not really used.
Remove from the README.
Fix issue with transfer_max_retries argument.