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.

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

kemmerle
Copy link
Contributor

@kemmerle kemmerle commented Jan 22, 2025

Description and Context

Relies on HubSpot/hubspot-local-dev-lib#228

In this PR, I'm implementing support for default account overrides with the new centralized configuration file.

Now, customers can place a .hs-account file in any directory where they would like the default account overridden. The .hs-account file will contain a single numerical entry: 1234567, or the account ID. Any changes made in the directory and its subdirectories will apply in the account stipulated in the .hs-account file. This will work very similarly to our nest configs currently do.

The advantages are:

  1. We will still have one centralized configuration file and therefore one single source of truth.
  2. We will create a command to auto-generate the .hs-account file, so that customers need never have to manually interact with the config.
  3. It's much more secure, if a customer accidentally uploads the file to GitHub--it contains no sensitive information besides an account ID.

TODO

  • Implement the hs accounts create-override command
  • Address feedback

Who to Notify

@brandenrodgers @camden11 @joe-yeager

@kemmerle kemmerle changed the title Add error messages for default account override Feature: Add support for default account override Jan 22, 2025
@kemmerle kemmerle changed the title Feature: Add support for default account override Feature (WIP): Add support for default account override Jan 22, 2025
@kemmerle kemmerle marked this pull request as ready for review January 23, 2025 15:35
bin/cli.js Outdated Show resolved Hide resolved
bin/cli.js Outdated Show resolved Hide resolved
@@ -202,7 +206,27 @@ const injectAccountIdMiddleware = async options => {
if (options.useEnv && process.env.HUBSPOT_ACCOUNT_ID) {
options.derivedAccountId = parseInt(process.env.HUBSPOT_ACCOUNT_ID, 10);
} else {
options.derivedAccountId = getAccountId(account);
Copy link
Contributor

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 that getAccountId 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?

Copy link
Contributor Author

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 in injectAccountIdMiddleware.

Apart from the init and auth commands, injectAccountMiddleware will run in the middleware before every command handler. Actually, the work you did to eliminate multiple getAccountId 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.


export const command = 'create-override [account]';

type AccountInfoArgs = CommonArgs &
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a copy/paste leftover here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 hs account info command but had to make some changes. Should I move this to Types, so we can reuse between commands?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants