-
Notifications
You must be signed in to change notification settings - Fork 236
feat: update accounts:set to work with keychain managers #3696
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
base: feat/credential-mgr-integration
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,21 +6,24 @@ import AccountsModule from '../../lib/accounts/accounts.js' | |
|
|
||
| export default class Set extends Command { | ||
| static args = { | ||
| name: Args.string({description: 'name of account to set', required: true}), | ||
| name: Args.string({description: 'name or username of account to set', required: true}), | ||
| } | ||
|
|
||
| static description = 'set the current Heroku account from your cache' | ||
| static description = 'set the current Heroku account from your accounts cache or system keychain' | ||
|
|
||
| static example = `${color.command('heroku accounts:set my-account')}` | ||
|
|
||
| async run() { | ||
| const {args} = await this.parse(Set) | ||
| const {name} = args | ||
|
|
||
| if (!(await AccountsModule.list()).some(account => account.name === name)) { | ||
| ux.error(`${name} does not exist in your accounts cache.`) | ||
| const accounts = await AccountsModule.list() | ||
| const accountExists = accounts.some(account => account.name === name || account.username === name) | ||
|
|
||
| if (!(accountExists)) { | ||
| ux.error(`${name} does not exist in your accounts cache or system keychain.`) | ||
| } | ||
|
|
||
| AccountsModule.set(name) | ||
| await AccountsModule.set(name, this.config.dataDir) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The match checks both name and username, but the raw input is passed to set() where the netrc path does a file lookup by that string. Should we resolve the matched account to ensure the correct identifier reaches each storage path? |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import {APIClient, listKeychainAccounts, getStorageConfig} from '@heroku-cli/command' | ||
| import {APIClient, listKeychainAccounts, getStorageConfig, writeLoginState} from '@heroku-cli/command' | ||
| import * as Heroku from '@heroku-cli/schema' | ||
| import fs from 'node:fs' | ||
| import os from 'node:os' | ||
|
|
@@ -15,7 +15,8 @@ export interface IAccountsWrapper { | |
| current(heroku: APIClient): Promise<string | null> | ||
| add(name: string, username: string, password: string): void | ||
| remove(name: string): void | ||
| set(name: string): Promise<void> | ||
| set(name: string, dataDir: string): Promise<void> | ||
| writeLoginState(configDir: string, name: string): Promise<void> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like something IAccountsWrapper wouldn't need to implement. Should we keep it off the interface and stub it a different way in tests? |
||
| } | ||
|
|
||
| export class AccountsWrapper implements IAccountsWrapper { | ||
|
|
@@ -64,6 +65,10 @@ export class AccountsWrapper implements IAccountsWrapper { | |
| return getStorageConfig() | ||
| } | ||
|
|
||
| async writeLoginState(dataDir: string, name: string): Promise<void> { | ||
| return writeLoginState(dataDir, name) | ||
| } | ||
|
|
||
| async list(): Promise<AccountEntry[]> { | ||
| const config = this.getStorageConfig() | ||
| if (config.credentialStore) { | ||
|
|
@@ -123,7 +128,13 @@ export class AccountsWrapper implements IAccountsWrapper { | |
| fs.unlinkSync(path.join(basedir, name)) | ||
| } | ||
|
|
||
| async set(name: string): Promise<void> { | ||
| async set(name: string, dataDir: string): Promise<void> { | ||
| const config = this.getStorageConfig() | ||
| if (config.credentialStore) { | ||
| await this.writeLoginState(dataDir, name) | ||
| return | ||
| } | ||
|
|
||
| const netrcInstance = await this.initNetrc() | ||
| const current = this.account(name) | ||
| netrcInstance.machines['git.heroku.com'] = {login: current.username, password: current.password} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,46 @@ | ||
| import {expect} from 'chai' | ||
| import runCommand from '../../../helpers/runCommand.js' | ||
| import * as sinon from 'sinon' | ||
| import {restore, SinonStub, stub} from 'sinon' | ||
|
|
||
| import Cmd from '../../../../src/commands/accounts/set.js' | ||
| import AccountsModule from '../../../../src/lib/accounts/accounts.js' | ||
| import runCommand from '../../../helpers/runCommand.js' | ||
|
|
||
| describe('accounts:set', function () { | ||
| let listStub: sinon.SinonStub | ||
| let setStub: sinon.SinonStub | ||
| let listStub: SinonStub | ||
| let setStub: SinonStub | ||
|
|
||
| beforeEach(function () { | ||
| listStub = sinon.stub(AccountsModule, 'list') | ||
| setStub = sinon.stub(AccountsModule, 'set') | ||
| listStub = stub(AccountsModule, 'list') | ||
| setStub = stub(AccountsModule, 'set').resolves() | ||
| }) | ||
|
|
||
| afterEach(function () { | ||
| sinon.restore() | ||
| restore() | ||
| }) | ||
|
|
||
| it('calls the set function with the account name when the account exists', async function () { | ||
| it('calls set with the account name and dataDir when matched by name', async function () { | ||
| listStub.resolves([{name: 'test-account', username: 'user1'}, {name: 'test-account-2', username: 'user2'}]) | ||
| await runCommand(Cmd, ['test-account-2']) | ||
| expect(setStub.calledWith('test-account-2')) | ||
| expect(setStub.calledOnce).to.be.true | ||
| expect(setStub.firstCall.args[0]).to.equal('test-account-2') | ||
| expect(setStub.firstCall.args[1].toLowerCase()).to.contain('local') | ||
| expect(setStub.firstCall.args[1]).to.contain('heroku') | ||
| }) | ||
|
|
||
| it('calls set with the account name and dataDir when matched by username', async function () { | ||
| listStub.resolves([{username: 'user1@example.com'}, {username: 'user2@example.com'}]) | ||
| await runCommand(Cmd, ['user1@example.com']) | ||
| expect(setStub.calledOnce).to.be.true | ||
| expect(setStub.firstCall.args[0]).to.equal('user1@example.com') | ||
| expect(setStub.firstCall.args[1].toLowerCase()).to.contain('local') | ||
| expect(setStub.firstCall.args[1]).to.contain('heroku') | ||
| }) | ||
|
|
||
| it('should return an error if the selected account name is not included in the account list', async function () { | ||
| it('returns an error if the account is not in the list', async function () { | ||
| listStub.resolves([{name: 'test-account', username: 'user1'}, {name: 'test-account-2', username: 'user2'}]) | ||
| await runCommand(Cmd, ['test-account-3']) | ||
| .catch((error: Error) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not major but the .catch() pattern validates the error message but doesn't fail if no error occurs. Should we adopt the try/catch + expect.fail() pattern used in newer tests like rake and data/pg to ensure the error path is actually exercised? |
||
| expect(error.message).to.contain('test-account-3 does not exist in your accounts cache.') | ||
| expect(error.message).to.contain('test-account-3 does not exist in your accounts cache or system keychain.') | ||
| }) | ||
| }) | ||
| }) | ||
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.
So small but probably dont need double parens here. Might be simplified for clarity.