-
Notifications
You must be signed in to change notification settings - Fork 64
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
Feature (WIP): Add support for default account override #1349
base: main
Are you sure you want to change the base?
Changes from all commits
47e238b
038a074
417ec76
76443f4
3dcae85
c65a6c1
ddc0984
1a54e99
db018c3
5b758d2
7d2b836
12b6284
d152eb4
ab16eff
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 |
---|---|---|
@@ -0,0 +1,77 @@ | ||
import fs from 'fs-extra'; | ||
import path from 'path'; | ||
import { Argv, ArgumentsCamelCase } from 'yargs'; | ||
import { getCwd } from '@hubspot/local-dev-lib/path'; | ||
import { logger } from '@hubspot/local-dev-lib/logger'; | ||
import { DEFAULT_ACCOUNT_OVERRIDE_FILE_NAME } from '@hubspot/local-dev-lib/constants/config'; | ||
import { getConfigPath, getAccountId } from '@hubspot/local-dev-lib/config'; | ||
import { addConfigOptions } from '../../lib/commonOpts'; | ||
import { i18n } from '../../lib/lang'; | ||
import { EXIT_CODES } from '../../lib/enums/exitCodes'; | ||
import { selectAccountFromConfig } from '../../lib/prompts/accountsPrompt'; | ||
import { logError } from '../../lib/errorHandlers/index'; | ||
import { CommonArgs, ConfigArgs } from '../../types/Yargs'; | ||
|
||
const i18nKey = 'commands.account.subcommands.createOverride'; | ||
|
||
export const describe = null; // i18n(`${i18nKey}.describe`); | ||
|
||
export const command = 'create-override [account]'; | ||
|
||
type AccountInfoArgs = CommonArgs & | ||
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. Looks like a copy/paste leftover here 😄 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 quite sure what you mean--I based the type off of the types in the |
||
ConfigArgs & { | ||
account: string | number; | ||
}; | ||
|
||
export async function handler( | ||
args: ArgumentsCamelCase<AccountInfoArgs> | ||
): Promise<void> { | ||
let overrideDefaultAccount = args.account; | ||
|
||
if (!overrideDefaultAccount) { | ||
overrideDefaultAccount = await selectAccountFromConfig(); | ||
} else if (!getAccountId(overrideDefaultAccount)) { | ||
logger.error( | ||
i18n(`${i18nKey}.errors.accountNotFound`, { | ||
configPath: getConfigPath() || '', | ||
}) | ||
); | ||
overrideDefaultAccount = await selectAccountFromConfig(); | ||
} | ||
const accountId = getAccountId(overrideDefaultAccount); | ||
|
||
try { | ||
const overrideFilePath = path.join( | ||
getCwd(), | ||
DEFAULT_ACCOUNT_OVERRIDE_FILE_NAME | ||
); | ||
await fs.writeFile(overrideFilePath, accountId!.toString(), 'utf8'); | ||
logger.success(i18n(`${i18nKey}.success`, { overrideFilePath })); | ||
kemmerle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
process.exit(EXIT_CODES.SUCCESS); | ||
} catch (e: unknown) { | ||
kemmerle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logError(e); | ||
process.exit(EXIT_CODES.ERROR); | ||
} | ||
} | ||
|
||
export function builder(yargs: Argv): Argv<AccountInfoArgs> { | ||
kemmerle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
addConfigOptions(yargs); | ||
|
||
yargs.positional('account', { | ||
describe: i18n(`${i18nKey}.options.account.describe`), | ||
type: 'string', | ||
}); | ||
yargs.example([ | ||
['$0 account create-override', i18n(`${i18nKey}.examples.default`)], | ||
[ | ||
'$0 account create-override 12345678', | ||
i18n(`${i18nKey}.examples.idBased`), | ||
], | ||
[ | ||
'$0 account create-override MyAccount', | ||
i18n(`${i18nKey}.examples.nameBased`), | ||
], | ||
]); | ||
|
||
return yargs as Argv<AccountInfoArgs>; | ||
} |
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.
I'm realizing the implication of
getAccountId
potentially throwing an error. Before, the worst case scenario was thatgetAccountId
could return null, but now it can throw an error. Does that mean we have to go through every usage of it in the app and wrap them all in try/catch blocks? I wonder if there's some way to get around having to do that 🤔Is it safe to just make the assumption that any potential thrown errors from
getAccountId()
would get caught here in the middleware, before any of the code in the commands can execute?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.
Yes, we can safely assume that any potential errors from
getAccountId()
will be caught ininjectAccountIdMiddleware
.Apart from the
init
andauth
commands,injectAccountMiddleware
will run in the middleware before every command handler. Actually, the work you did to eliminate multiplegetAccountId
functions guarantees that--now we call the same function every time, so we know that if it fails in the command, it will also fail in middleware.