Remove CLI getAccountId util #1331
Conversation
brandenrodgers
requested review from
camden11,
kemmerle and
joe-yeager
as code owners
| /** | ||
| * Obtains accountId using supplied --account flag or from environment variables | ||
| */ | ||
| export function getAccountId( |
There was a problem hiding this comment.
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.
| @@ -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( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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( | |||
There was a problem hiding this comment.
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
Description and Context
This still needs to be tested thoroughly, but I'm fairly certain that we can remove this getAccountId() util in the CLI in favor of the getAccountId in local-dev-lib because now we have the
injectAccountIdMiddlewaremiddleware that does everything this old util in the CLI was doing. I'll leave inline comments to explain my thinking.Screenshots
TODO
Who to Notify