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

Jump to bottom

Use local dev lib config #947

Merged
merged 8 commits into from
Nov 2, 2023
Merged

Use local dev lib config #947

merged 8 commits into from
Nov 2, 2023

Conversation

camden11
Copy link
Contributor

Description and Context

This switches the CLI from using @hubspot/cli-lib for config to using @hubspot/local-dev-lib. The config module is functionally identical between both, so everything should behave exactly the same. Once this PR is merged, we'll be able to start porting everything else over to `local-dev-lib

TODO

  • This PR will need to be merged and released in tandem with this cli-lib PR Move config module to local-dev-lib cli-lib#22
    As well as a PR for the VSCode extension
  • It would probably be good to stress test this pretty thoroughly before doing a full release.

Who to Notify

@brandenrodgers

brandenrodgers
brandenrodgers reviewed Oct 27, 2023
View reviewed changes
packages/cli/commands/project/deploy.js Outdated
@@ -18,7 +18,7 @@ const { projectNamePrompt } = require('../../lib/prompts/projectNamePrompt');
const { buildIdPrompt } = require('../../lib/prompts/buildIdPrompt');
const { i18n } = require('../../lib/lang');
const { uiBetaTag } = require('../../lib/ui');
const { getAccountConfig } = require('@hubspot/cli-lib');
const { getAccountConfig } = require('@hubspot/cli-lib/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need the trailing slash?

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'm not sure how that got there. This should actually be a local-dev-lib import anyway

@brandenrodgers
Copy link
Contributor

The code in here lgtm (looks like tests are failing though). I'll hammer on this locally for a bit before approving though

@camden11 camden11 force-pushed the use-local-dev-lib-config branch from 9bb4c4c to ace5f3c Compare October 27, 2023 16:33
@camden11 camden11 force-pushed the use-local-dev-lib-config branch from ace5f3c to 194af24 Compare October 27, 2023 16:36
brandenrodgers
brandenrodgers reviewed Nov 1, 2023
View reviewed changes
packages/cli/commands/project/deploy.js Outdated
@@ -18,7 +18,7 @@ const { projectNamePrompt } = require('../../lib/prompts/projectNamePrompt');
const { buildIdPrompt } = require('../../lib/prompts/buildIdPrompt');
const { i18n } = require('../../lib/lang');
const { uiBetaTag } = require('../../lib/ui');
const { getAccountConfig } = require('@hubspot/cli-lib');
const { getAccountConfig } = require('@hubspot/local-dev-lib');
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing /config here I think

@brandenrodgers
Copy link
Contributor

brandenrodgers commented Nov 1, 2023 •
edited
Loading

I cross checked all of the exports from local-dev-lib/config. There are a few more exports that you may want to update so they pull from local-dev-lib:

  • getAccountId (exported from the old commonOpts file in cli-lib)
    • Edit: I realized this is pulling from cli/lib/commonOpts 😨. We should probably look into fixing up that file to remove confusion between those utils and the ones exported from local-dev-lib
  • isConfigFlagEnabled (I think I saw one or two missing updates for this one)
  • getEnv (One or two missing updates here too)

@camden11
Copy link
Contributor Author

camden11 commented Nov 1, 2023

Ah good catch. Missed one or two because the had a trailing / and didn't show up in my search. These config functions are imported from so many different places.

And yeah might be worth renaming some of the stuff in commonOpts

brandenrodgers
brandenrodgers approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@brandenrodgers brandenrodgers left a comment

Choose a reason for hiding this comment

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

Tested locally and this lgtm! We should make sure there isn't any other WIP PR's that need to be merged soon because this should probably sit on the beta release for a bit so we can be sure that there aren't any bugs

@camden11 camden11 merged commit 9530d08 into master Nov 2, 2023
1 check passed
@camden11 camden11 deleted the use-local-dev-lib-config branch November 2, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandenrodgers brandenrodgers brandenrodgers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@camden11 @brandenrodgers