-
Notifications
You must be signed in to change notification settings - Fork 4
Config Revamp #231
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
Config Revamp #231
Conversation
|
|
||
| Returns config data for a specific account, given the account's ID or name. | ||
|
|
||
| ## Example config |
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 updated the existing content of the readme to reflect the changes I made, but it may make sense to rethink it completely
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.
Yeah good call. That could always be done as a follow up
| @@ -1,268 +1,406 @@ | |||
| import * as config_DEPRECATED from './config_DEPRECATED'; | |||
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.
This file is more or less net new, but because it has the same name as the old one, GH is showing a diff.
| } | ||
| return config_DEPRECATED.deleteEmptyConfigFile(); | ||
| export function getConfigFilePath(): string { | ||
| const { configFilePathFromEnvironment } = getConfigPathEnvironmentVariables(); |
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.
getConfigFilePath checks for a HUBSPOT_CONFIG_PATH environment variable if the user is using a custom path - this variable will need to be set by the CLI if the user passes in the --config flag
config/index.ts
Outdated
| if (CLIConfiguration.isActive()) { | ||
| return CLIConfiguration.config; | ||
| export function getConfig(): HubSpotConfig { | ||
| const { useEnvironmentConfig } = getConfigPathEnvironmentVariables(); |
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.
Same with environment config - a user passes in the --use-env flag to the CLI, the CLI should set the USE_ENVIRONMENT_CONFIG env variable to tell LDL to use environment variables. These new environment variables let us avoid needing to have configPath and useEnv arguments for every function that accesses the config
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.
Is this (and the HUBSPOT_CONFIG_PATH) documented anywhere? It makes sense in terms of implementation, but it wouldn't be obvious behavior to someone coming into this.
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.
Good call, I can add something to the readme for these. On the CLI side, we'll be setting these automatically when users use --use-env or --config paths so nothing should change there
| } else { | ||
| config_DEPRECATED.updateHttpTimeout(timeout); | ||
| export function updateConfigAccount( | ||
| updatedAccount: HubSpotConfigAccount |
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.
This function now accepts a full updated account rather than just fields to update. This makes it easier to use TS to confirm that updates are valid (for example, TS would have no way to know the auth type of the account you're trying to update, and therefore would allow updating accounts to have invalid auth)
config/index.ts
Outdated
| export function updateConfigAccount( | ||
| updatedAccount: HubSpotConfigAccount | ||
| ): void { | ||
| if (!isConfigAccountValid(updatedAccount)) { |
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.
We still manually check validity just in case though!
| hsAccountFile: defaultAccountOverrideFilePath, | ||
| }), | ||
| { | ||
| // TODO: This is improper use of cause, we should create a custom error class |
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.
Do these todos need to be addressed before merging?
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 think they can be addressed separately. This is from existing code that I ported over to this new file - I just wanted to call it out so we remember to get around to it at some point
config/README.md
Outdated
| #### getConfig() | ||
|
|
||
| Locates, parses, and stores the `hubspot.config.yml` file in memory. This should be the first thing that you do if you plan to access any of the config file values. If the config has already been loaded, this function will simply return the already-parsed config values. | ||
| Locates and parses the hubspot config file. This function will automatically find the correct config file. Typically, it defaults to the nearest config file by working up the direcotry tree. Custom config locations can be set using the following environment variables |
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.
This part "Typically, it defaults to the nearest config file by working up the direcotry tree." is only true if the user doesn't also have a global config file. If they have a global config, we'll prioritize using that one.
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.
Good catch, will update that
|
Updated the docs to more clearly explain the environment variable options + behavior of |
config/index.ts
Outdated
| export function loadConfigFromEnvironment(): boolean { | ||
| if (CLIConfiguration.isActive()) { | ||
| return CLIConfiguration.useEnvConfig; | ||
| if (fs.existsSync(globalConfigFilePath)) { |
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 wonder if we should be catching and wrapping these in the new HubSpotConfigError and providing more context
fs.existsSync doesn't throw
It can if there are access issues
5f3bc4b to
eaf7883
Compare
c4cd7ad to
c1e6e04
Compare
Description and Context
This PR rebuilds the
configmodule from the ground up. The newconfigmodule has a variety of improvements over the old one:config/configandconfig/CLIConfigurationinto a single file.accountfields and deprecatedportalfields, but converts them into a standardized format (no need to check portalId vs accountId anymore)HubSpotConfigandHubSpotConfigAccounttypes are guaranteed to exist. No more checking fornullandundefinedeverywhere!twothree config files (added one for defaultAccountOverride stuff), down from five.config/indexcontains all exported functionality, andconfig/utilscontains all non-exported helper functionsNote: the majority of new functionality is in
config/indexandconfig/utils, both of which are hidden by defaultTODO
I haven't yet merged in the account override work since it appears to still be in flux. Will be working with @kemmerle on that.Default account override functionality addedOAuth2Managerandlogger. We will need to take care of those too, but I figured this PR is big enough as isWho to Notify
@joe-yeager @brandenrodgers @kemmerle