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.

Sign up for GitHub

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

Remove CLI getAccountId util #1331

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+42 −73
Open

Remove CLI getAccountId util #1331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa7a06c
Remove CLI getAccountId util
brandenrodgers Jan 10, 2025
5dc138b
this can be simplified since derivedAccountID is already the id
brandenrodgers Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions commands/account/remove.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const {
getConfigPath,
deleteAccount,
getConfigDefaultAccount,
getAccountId: getAccountIdFromConfig,
getAccountId,
updateDefaultAccount,
} = require('@hubspot/local-dev-lib/config');

Expand All @@ -23,7 +23,7 @@ exports.handler = async options => {
const { account } = options;
let accountToRemove = account;

if (accountToRemove && !getAccountIdFromConfig(accountToRemove)) {
if (accountToRemove && !getAccountId(accountToRemove)) {
logger.error(
i18n(`${i18nKey}.errors.accountNotFound`, {
specifiedAccount: accountToRemove,
Expand All @@ -32,17 +32,13 @@ exports.handler = async options => {
);
}

if (!accountToRemove || !getAccountIdFromConfig(accountToRemove)) {
if (!accountToRemove || !getAccountId(accountToRemove)) {
accountToRemove = await selectAccountFromConfig(
i18n(`${i18nKey}.prompts.selectAccountToRemove`)
);
}

trackCommandUsage(
'accounts-remove',
null,
getAccountIdFromConfig(accountToRemove)
);
trackCommandUsage('accounts-remove', null, getAccountId(accountToRemove));

const currentDefaultAccount = getConfigDefaultAccount();

Expand Down
10 changes: 3 additions & 7 deletions commands/account/use.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const { logger } = require('@hubspot/local-dev-lib/logger');
const {
getConfigPath,
updateDefaultAccount,
getAccountId: getAccountIdFromConfig,
getAccountId,
} = require('@hubspot/local-dev-lib/config');

const { trackCommandUsage } = require('../../lib/usageTracking');
Expand All @@ -20,7 +20,7 @@ exports.handler = async options => {

if (!newDefaultAccount) {
newDefaultAccount = await selectAccountFromConfig();
} else if (!getAccountIdFromConfig(newDefaultAccount)) {
} else if (!getAccountId(newDefaultAccount)) {
logger.error(
i18n(`${i18nKey}.errors.accountNotFound`, {
specifiedAccount: newDefaultAccount,
Expand All @@ -30,11 +30,7 @@ exports.handler = async options => {
newDefaultAccount = await selectAccountFromConfig();
}

trackCommandUsage(
'accounts-use',
null,
getAccountIdFromConfig(newDefaultAccount)
);
trackCommandUsage('accounts-use', null, getAccountId(newDefaultAccount));

updateDefaultAccount(newDefaultAccount);

Expand Down
4 changes: 2 additions & 2 deletions commands/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const {
getConfigPath,
loadConfig,
getConfigDefaultAccount,
getAccountId,
} = require('@hubspot/local-dev-lib/config');
const {
commaSeparatedValues,
Expand All @@ -39,7 +40,6 @@ const {
const {
addConfigOptions,
setLogLevel,
getAccountId,
addTestingOptions,
addGlobalOptions,
} = require('../lib/commonOpts');
Expand Down Expand Up @@ -197,7 +197,7 @@ exports.handler = async options => {
'accountsListCommand',
]);

const accountId = getAccountId({ account: accountName });
const accountId = getAccountId(accountName);
await trackAuthAction('auth', authType, TRACKING_STATUS.COMPLETE, accountId);

process.exit(EXIT_CODES.SUCCESS);
Expand Down
5 changes: 1 addition & 4 deletions commands/project/__tests__/installDeps.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const { logger } = require('@hubspot/local-dev-lib/logger');
const { getProjectConfig } = require('../../../lib/projects');
const { EXIT_CODES } = require('../../../lib/enums/exitCodes');
const { trackCommandUsage } = require('../../../lib/usageTracking');
const { getAccountId } = require('../../../lib/commonOpts');
const {
installPackages,
getProjectPackageJsonLocations,
Expand Down Expand Up @@ -56,10 +55,8 @@ describe('commands/project/installDeps', () => {

it('should track the command usage', async () => {
const accountId = 999999;
getAccountId.mockReturnValue(accountId);
await installDepsCommand.handler({});
await installDepsCommand.handler({ derivedAccountId: accountId });

expect(getAccountId).toHaveBeenCalledTimes(1);
expect(trackCommandUsage).toHaveBeenCalledTimes(1);
expect(trackCommandUsage).toHaveBeenCalledWith(
'project-install-deps',
Expand Down
6 changes: 2 additions & 4 deletions commands/project/installDeps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const { promptUser } = require('../../lib/prompts/promptUtils');
const path = require('path');
const { i18n } = require('../../lib/lang');
const { trackCommandUsage } = require('../../lib/usageTracking');
const { getAccountId } = require('../../lib/commonOpts');
const { uiBetaTag } = require('../../lib/ui');

const i18nKey = `commands.project.subcommands.installDeps`;
Expand All @@ -19,10 +18,9 @@ exports.command = 'install-deps [packages..]';
exports.describe = uiBetaTag(i18n(`${i18nKey}.help.describe`), false);

exports.handler = async options => {
const { packages } = options || {};
const { derivedAccountId, packages } = options || {};
try {
const accountId = getAccountId(options);
trackCommandUsage('project-install-deps', null, accountId);
trackCommandUsage('project-install-deps', null, derivedAccountId);

const projectConfig = await getProjectConfig();
if (!projectConfig || !projectConfig.projectDir) {
Expand Down
18 changes: 7 additions & 11 deletions commands/sandbox/delete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ const {
addAccountOptions,
addConfigOptions,
addUseEnvironmentOptions,
getAccountId,
addTestingOptions,
} = require('../../lib/commonOpts');
const { logger } = require('@hubspot/local-dev-lib/logger');
Expand All @@ -17,7 +16,7 @@ const {
getEnv,
removeSandboxAccountFromConfig,
updateDefaultAccount,
getConfigDefaultAccount,
getAccountId,
getConfigAccounts,
} = require('@hubspot/local-dev-lib/config');
const {
Expand Down Expand Up @@ -61,11 +60,10 @@ exports.handler = async options => {
}
}

const sandboxAccountId = getAccountId({
account: providedAccountId || accountPrompt.account,
});
const isDefaultAccount =
sandboxAccountId === getAccountId(getConfigDefaultAccount());
const sandboxAccountId = getAccountId(
providedAccountId || accountPrompt.account
);
const isDefaultAccount = sandboxAccountId === getAccountId();

const baseUrl = getHubSpotWebsiteOrigin(
getValidEnv(getEnv(sandboxAccountId))
Expand All @@ -79,9 +77,7 @@ exports.handler = async options => {
parentAccountId = portal.parentAccountId;
} else if (!force) {
const parentAccountPrompt = await deleteSandboxPrompt(true);
parentAccountId = getAccountId({
account: parentAccountPrompt.account,
});
parentAccountId = getAccountId(parentAccountPrompt.account);
} else {
logger.error(i18n(`${i18nKey}.failure.noParentAccount`));
process.exit(EXIT_CODES.ERROR);
Expand All @@ -94,7 +90,7 @@ exports.handler = async options => {
getEnv(sandboxAccountId) === 'qa' ? '--qa' : ''
} --account=${parentAccountId}`;

if (parentAccountId && !getAccountId({ account: parentAccountId })) {
if (parentAccountId && !getAccountId(parentAccountId)) {
logger.log('');
logger.error(
i18n(`${i18nKey}.failure.noParentPortalAvailable`, {
Expand Down
4 changes: 0 additions & 4 deletions lib/__tests__/commonOpts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const {
} = require('@hubspot/local-dev-lib/constants/files');
const {
getAndLoadConfigIfNeeded,
getAccountId,
getAccountConfig,
loadConfigFromEnvironment,
} = require('@hubspot/local-dev-lib/config');
Expand Down Expand Up @@ -40,7 +39,6 @@ describe('lib/commonOpts', () => {

afterEach(() => {
getAndLoadConfigIfNeeded.mockReset();
getAccountId.mockReset();
getAccountConfig.mockReset();
loadConfigFromEnvironment.mockReset();
});
Expand Down Expand Up @@ -68,7 +66,6 @@ describe('lib/commonOpts', () => {
getAndLoadConfigIfNeeded.mockReturnValue(
configWithDefaultCmsPublishMode
);
getAccountId.mockReturnValue(accounts.DEV);
getAccountConfig.mockReturnValue(devAccountConfig);
loadConfigFromEnvironment.mockReturnValue(undefined);
expect(getCmsPublishMode({ account: accounts.DEV })).toBe(
Expand All @@ -81,7 +78,6 @@ describe('lib/commonOpts', () => {
getAndLoadConfigIfNeeded.mockReturnValue(
configWithDefaultCmsPublishMode
);
getAccountId.mockReturnValue(accounts.PROD);
getAccountConfig.mockReturnValue(prodAccountConfig);
loadConfigFromEnvironment.mockReturnValue(undefined);
expect(getCmsPublishMode({ account: accounts.PROD })).toBe(
Expand Down
6 changes: 4 additions & 2 deletions lib/__tests__/validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// @ts-nocheck
const { getAccountConfig } = require('@hubspot/local-dev-lib/config');
const {
getAccountId,
getAccountConfig,
} = require('@hubspot/local-dev-lib/config');
const { getOauthManager } = require('@hubspot/local-dev-lib/oauth');
const {
accessTokenForPersonalAccessKey,
} = require('@hubspot/local-dev-lib/personalAccessKey');
const { getAccountId } = require('../commonOpts');
const { validateAccount } = require('../validation');

jest.mock('@hubspot/local-dev-lib/config');
Expand Down
26 changes: 6 additions & 20 deletions lib/commonOpts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
} from '@hubspot/local-dev-lib/constants/files';
import { CmsPublishMode } from '@hubspot/local-dev-lib/types/Files';
import {
getAccountId as getAccountIdFromConfig,
getAccountConfig,
getAndLoadConfigIfNeeded,
} from '@hubspot/local-dev-lib/config';
Expand Down Expand Up @@ -102,33 +101,20 @@ export function getCommandName(argv: Arguments<{ _?: string[] }>): string {
return (argv && argv._ && argv._[0]) || '';
}

/**
* Obtains accountId using supplied --account flag or from environment variables
*/
export function getAccountId(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This util is just a wrapper around the local-dev-lib's getAccountId, except it also checks for any account env variables.

Now that we have injectAccountIdMiddleware doing this same thing, we should be able to remove this util. Commands can get this same behavior by looking at options.derivedAccountId.

The benefit of removing this is that it cuts down on the number of options for getting account ID's in the project.

options: Arguments<{ account?: number | string }>
): number | null {
const { account } = options || {};

if (options?.useEnv && process.env.HUBSPOT_ACCOUNT_ID) {
return parseInt(process.env.HUBSPOT_ACCOUNT_ID, 10);
}

return getAccountIdFromConfig(account);
}

export function getCmsPublishMode(
options: Arguments<{ cmsPublishMode?: CmsPublishMode }>
options: Arguments<{
cmsPublishMode?: CmsPublishMode;
derivedAccountId?: number;
}>
): CmsPublishMode {
// 1. --cmsPublishMode
const { cmsPublishMode } = options;
if (cmsPublishMode && typeof cmsPublishMode === 'string') {
return cmsPublishMode.toLowerCase() as CmsPublishMode;
}
// 2. config[account].defaultCmsPublishMode
const accountId = getAccountId(options);
if (accountId) {
const accountConfig = getAccountConfig(accountId);
if (options.derivedAccountId) {
const accountConfig = getAccountConfig(options.derivedAccountId);
if (accountConfig && accountConfig.defaultCmsPublishMode) {
return accountConfig.defaultCmsPublishMode;
}
Expand Down
24 changes: 13 additions & 11 deletions lib/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { commaSeparatedValues } from '@hubspot/local-dev-lib/text';
import {
getConfigPath,
getAccountConfig,
getAccountId,
loadConfigFromEnvironment,
} from '@hubspot/local-dev-lib/config';
import { getOauthManager } from '@hubspot/local-dev-lib/oauth';
Expand All @@ -22,23 +23,24 @@ import {
getCwd,
getExt,
} from '@hubspot/local-dev-lib/path';
import { getAccountId, getCmsPublishMode } from './commonOpts';
import { getCmsPublishMode } from './commonOpts';
import { logError } from './errorHandlers/index';

export async function validateAccount(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that these validation checks weren't working correctly. I'm actually not sure if we should even keep them around. Now that I fixed these I'm starting to see them showing up. For example, I'm getting an error if I have env variables set and I try to run a CLI command without using --use-env. But I don't think we need to show an error in that case. The command still works as expected.

I think these checks might be too aggressive, but curious to hear what others think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yeah that seems unnecessary, you may still want to use your config file after setting env variables. The only thing we really need to check for is using --account and --use-env at the same time, which we already do

options: Arguments<{ account?: string; accountId?: string }>
options: Arguments<{
account?: string;
accountId?: string;
derivedAccountId?: number;
providedAccountId?: string;
}>
): Promise<boolean> {
const accountId = getAccountId(options);
const { accountId: accountIdOption, account: accountOption } = options;
const { derivedAccountId, providedAccountId } = options;
const accountId = getAccountId(derivedAccountId);

if (!accountId) {
if (accountOption) {
if (providedAccountId) {
logger.error(
`The account "${accountOption}" could not be found in the config`
);
} else if (accountIdOption) {
logger.error(
`The account "${accountIdOption}" could not be found in the config`
`The account "${providedAccountId}" could not be found in the config`
);
} else {
logger.error(
Expand All @@ -48,7 +50,7 @@ export async function validateAccount(
return false;
}

if (accountOption && loadConfigFromEnvironment()) {
if (providedAccountId && loadConfigFromEnvironment()) {
throw new Error(
'Cannot specify an account when environment variables are supplied. Please unset the environment variables or do not use the "--account" flag.'
);
Expand Down
Loading