Skip to content

Conversation

@kemmerle
Copy link
Contributor

@kemmerle kemmerle commented May 13, 2025

Description and Context

(Draft because I'm currently experiencing issues with the API service; will mark as ready for review when those are addressed)

This PR relies on this LDL PR. I've created an experimental release of LDL for ease of testing.

This PR gives more detailed messaging about the migration/merge process to the global config file in the hs config migrate and hs account auth commands.

Screenshots

New migration prompt (Appears in both hs config migrate and hs account auth when deprecated config is present):

Screenshot 2025-05-13 at 4 26 09 PM

New merge prompt (Appears in both hs config migrate and hs account auth when both configs are present):

Screenshot 2025-05-13 at 4 31 54 PM

New verbose description of hs config migrate:

Screenshot 2025-05-09 at 9 33 46 AM

New verbose description of hs account auth:

Screenshot 2025-05-09 at 9 50 23 AM

TODO

  • Address feedback

Who to Notify

@brandenrodgers @camden11 @joe-yeager

package.json Outdated
"repository": "https://github.com/HubSpot/hubspot-cli",
"dependencies": {
"@hubspot/local-dev-lib": "3.5.6",
"@hubspot/local-dev-lib": "0.3.1-experimental.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to bump this, after the sister LDL PR is merged.

!configFileExists(true) &&
isTargetedCommand(argv._, {
account: { target: false, subCommands: { auth: { target: true } } },
config: { target: false, subCommands: { migrate: { target: true } } },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the hs config migrate command is validated and there is no deprecated config present, we warn the user that they need to supply an account ID. That would be ok for any other command, but with this command, we want to tell the user that no further action is required because the migration has already taken place.

@kemmerle kemmerle marked this pull request as ready for review May 13, 2025 18:52
@brandenrodgers
Copy link
Contributor

image

We're doing this prompt differently from how we do all of our other prompts. Making the entire thing bold is a little overwhelming IMO. In out other prompts, the only content in the actual prompt itself is the question. All of the other context goes above the prompt as a standard CLI log statement.

Example (not a great one, but the best I could quickly find):
image

We should put all the From/To context above the prompt as a standard CLI log, and then the prompt message should be something simple like: Migrate to the new config?

@kemmerle kemmerle requested a review from camden11 May 13, 2025 20:36
@kemmerle
Copy link
Contributor Author

@brandenrodgers, I refactored the prompts per your suggestions:

New migration prompt (Appears in both hs config migrate and hs account auth when deprecated config is present):

Screenshot 2025-05-13 at 4 26 09 PM

New merge prompt (Appears in both hs config migrate and hs account auth when both configs are present):

Screenshot 2025-05-13 at 4 31 54 PM

brandenrodgers
brandenrodgers previously approved these changes May 13, 2025
@kemmerle kemmerle merged commit c8a71a2 into main May 14, 2025
1 check passed
@kemmerle kemmerle deleted the add/better-migrate-messages branch May 14, 2025 19:17
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.

4 participants