feat: add accounts commands #3257
Conversation
| } | ||
|
|
||
| if (current() === name) { | ||
| ux.error(`${name} is the current account`) |
There was a problem hiding this comment.
Is this error when a logged in user tries to remove themselves from the cache? @k80bowman
There was a problem hiding this comment.
Yes, this error is when a user tries to remove the account they are currently logged into from the cache.
There was a problem hiding this comment.
@claire-riley @eablack should we update this error message to read something like ${name} is the current account. It cannot be removed.?
Requested work will be handled in a separate PR.
| async run() { | ||
| const accounts = list() | ||
| if (accounts.length === 0) { | ||
| ux.error('No accounts found in cache.') |
There was a problem hiding this comment.
I noticed this was new error text (it's a very good sanity check, btw). And it looks like cx reviewed it, but we resolved the conversation without change. Was that intentional?
There was a problem hiding this comment.
Claire let me know that I'd missed this one, it's been updated. I must have resolved it by accident.
| async run() { | ||
| const {args} = await this.parse(Add) | ||
| const {name} = args | ||
| const logInMessage = 'You must be logged in to run this command.' |
There was a problem hiding this comment.
I don't actually encounter this message when i'm not logged in:
There was a problem hiding this comment.
Thank you. I will check on this.
There was a problem hiding this comment.
This should be fixed now. Instead of an error message you should be prompted to log in.
| import * as Heroku from '@heroku-cli/schema' | ||
| import {add, list} from '../../lib/accounts/accounts' | ||
|
|
||
| export default class Add extends Command { |
There was a problem hiding this comment.
In the plugin, there's an sso flag that is required to facilitate login for an sso user, and I don't see that here. Actually, I don't see us performing the login in the new command.. did we change the behavior here? Like, with the plugin, if a user is not logged in, we're starting the login for them I think. Here, I think we are just telling them they need to log in. Is that right?
There was a problem hiding this comment.
Yes, I made that behavior change here. The actual login command has several flags that I did not want to reintroduce here, beyond just sso. We could do this a different way and start the login process here, but I thought it would be better to keep this command focused on adding an account.
There was a problem hiding this comment.
Actually, I could get rid of the {retry: false} in the API call and it would prompt the user to login. I'm not sure why I added that, but it's not necessary. I'm going to remove it and then I can remove some of the error logic as well. If you're not logged in, it should prompt you to log in the way most of the rest of our commands do.
| if (account) { | ||
| ux.styledHeader(`Current account is ${account}`) | ||
| } else { | ||
| ux.error(`You haven't set an account. Run ${color.cmd('heroku login')} to confirm you're logged in to Heroku.`) |
There was a problem hiding this comment.
This error message is possibly misleading. So, if I'm logged in and run this command I'll get this command, suggesting I need to run heroku login. But the problem isn't that I need to log in, it's that I need to run accounts:add.
There was a problem hiding this comment.
I will check on this, thank you.
There was a problem hiding this comment.
@claire-riley could we remove the portion of the message about logging in and have it just say You haven't set an account.? The error appears both when the user isn't logged and when the user has not added any accounts.
There was a problem hiding this comment.
@k80bowman would it be possible to suggest both commands? If we only have "You haven't set an account." I think as a user I'd feel at a dead end.
What is the response that accounts:add gives if user isn't logged in?
| } | ||
|
|
||
| if (current() === name) { | ||
| ux.error(`${name} is the current account.`) |
There was a problem hiding this comment.
I had the same thought as claire when I tested this scenario.
There was a problem hiding this comment.
Ah another one I missed. Sorry, I will check on this.
Co-authored-by: Claire Riley <claire.riley@salesforce.com> Signed-off-by: Katy Bowman <katy.bowman@salesforce.com>
Work item
This PR moves the commands from heroku/heroku-accounts to the CLI core, migrates them to TypeScript and oclif/core v2, updates and adds descriptions and error text, and adds tests.
These commands made use of functionality from heroku-cli-utils that is implemented differently in heroku-cli/command and oclif. Because of this, the migrated commands may be implemented a little differently than the originals. The overall functionality should be roughly the same and should accomplish the same tasks.
Testing
yarn && yarn build./bin/run accounts:add ACCOUNT_NAME(the account name can be whatever makes sense to you)./bin/run accountsand verify that your new account is listed./bin/run accounts:add ACCOUNT_NAMEusing a different account name./bin/run accountsand verify that both accounts are now listed./bin/run accounts:currentand verify that the account you are currently logged into is listed./bin/run accounts:set ACCOUNT_NAMEusing the name of the account you are not currently logged into./bin/run accounts:currentand verify that it lists the account you set./bin/run accounts:remove ACCOUNT_NAMEusing the name of the account you are not currently logged into./bin/run accountsand verify that only one account is now listed