Added hs config commands #464

miketalley · 2021-04-05T22:17:23Z

Description and Context

#27 Finally getting around to this

Adds six new commands

hs config set default-account [accountNameOrId] - selects portal to use as defaultPortal from a list or by specifying accountNameOrId
hs config set default-mode [mode] - selects mode to use as defaultMode in config, also supports specifying mode as argument
hs config set http-timeout <timeout> - sets httpTimeout in config
hs config set allow-usage-tracking - selects value for allowUsageTracking in config
hs accounts list (alias hs accounts ls) - lists portal names, portalIds, and authTypes in table format as well as defaultPortal
hs accounts rename <currentNameOrId> <newName> - allows naming/renaming a portal name in the config (will also update defaultPortal if that is the one that is renamed)

Screenshots

hs config set default-account

hs config set default-mode

hs config set http-timeout

hs config set allow-usage-tracking

hs accounts list

hs accounts rename

Notes:

Fixed a bug where if defaultPortal was a number getAccountId was returning null
Fixed table output to be borderless see https://www.npmjs.com/package/table#user-content-borderless-table

miketalley · 2021-04-06T13:32:13Z

@abelb @jasminetudor do you have any suggestions on this?

gcorne

From a UX perspective, I wonder if we should assume that the only actions that can be taken are to interact with the accounts in the config. I say this because there are other global config options that could have corresponding config commands for updating them, which makes me wonder if we should have commands like hs config rename-account and hs config default-account or similar.

I also didn't comment on all the places where portal is used instead of account in user-facing copy.

packages/cli-lib/lib/config.js

packages/cli/commands/config.js

packages/cli/commands/config/default.js

packages/cli/commands/config/list.js

packages/cli-lib/lib/config.js

packages/cli/commands/config/rename.js

anthmatic · 2021-04-06T19:38:52Z

packages/cli/commands/config/list.js

+
+  const config = getConfig();
+  const portalData = config.portals.reduce((acc, portal) => {
+    acc.push([portal.name, portal.portalId, portal.authType]);


Wondering if authType should be an enum?

We could definitely make it an enum though it's not elsewhere in the code. I think doing this may be out of scope for this PR though.

Is there something that you're worried about with not using an enum here?

Ah, sorry I was thinking that we could validate that a correct authType exists but might not be a big deal

packages/cli-lib/lib/config.js

gcorne · 2021-04-14T21:44:46Z

packages/cli/commands/config.js

+    .command({
+      ...list,
+      aliases: 'ls',
+    })


Instead of adding the alias here, can we add to the command in config/list?

If we list aliases of subcommands in the parent command we don't have to sift through each subcommand to find every alias and avoid collisions. It also matches the current pattern for the hs list (hs ls) and hs functions list (hs functions ls).

brandenrodgers · 2021-04-15T16:45:37Z

When running hs config default, should we have special handling if there is only one available account to choose from?

In this scenario I only have one account so maybe it could instead show something like "You only have one account configured. Add more accounts to your hubspot.config.yml to see more options here". My phrasing there probably isn't the best, but I'm trying to think of ways to help guide the user to their intended result.

brandenrodgers · 2021-04-15T16:48:35Z

I'm seeing an issue in the hs config rename command where it's not correctly logging the original account name:

packages/cli/commands/config/rename.js

packages/cli-lib/lib/config.js

anthmatic · 2021-04-23T14:47:51Z

packages/cli/commands/config/list.js

+  portalData.unshift(getTableHeader(['Name', 'Portal ID', 'Auth Type']));
+
+  logger.log(getTableContents(portalData));
+  logger.log('Default Portal: ', config.defaultPortal);


I'm not sure if this is the best place for this, but it would be helpful to print out the path of the config file, which might be helpful for folks to know which config the CLI is actually loading

Good call, I updated the output from this command to include the config path and made the accounts list a bit nicer too

erincouse · 2021-04-23T16:22:31Z

@gcorne can you give me an example of the global config commands that would conflict with these?

gcorne · 2021-04-23T18:40:56Z

@gcorne can you give me an example of the global config commands that would conflict with these?

Since hs config is being introduced here, there isn't a conflict. It is more of a design issue in that the structure of the commands don't match the structure of the configuration. The configuration consists of two sections: 1) global config options and 2) config for each account that the CLI has been set up to access.

Global configuration options:

httpTimeout - controls how long the CLI waits for network requests to the backend to complete. This is an advanced feature.
allowUserTracking - controls whether or not CLI usage activity is being tracked
defaultPortal - default account used when running commands without passing --acount option
defaultMode - determines whether files will automatically be overwritten or not when running hs fetch

Account configuration:

Name
Authentication type

jasminetudor · 2021-04-23T18:53:10Z

@miketalley we are thinking...
Great:
hs config list
hs config rename

but we would change:

hs config rename <currentNameOrId> <newName>
to
hs config set --default= <accountId>

to stick with the format of:

hs 
	command	noun (feature you need to use) 
	subcommand	verb (what you want to do w it)
	flags 	parameter (limits or specifics)

miketalley · 2021-04-26T18:58:21Z

@miketalley we are thinking...
Great:
hs config list
hs config rename

but we would change:

hs config rename <currentNameOrId> <newName>
to
hs config set --default= <accountId>

to stick with the format of:
hs 
	command	noun (feature you need to use) 
	subcommand	verb (what you want to do w it)
	flags 	parameter (limits or specifics)

To clarify -- hs config list and hs config rename should remain as-is in the current PR and hs config default should be renamed to hs config set with 0 positional arguments and 1 option (default) that can be used to specify the new defaultName value?

For example:
hs config set --default=accountNameToSetAsDefault -- updates defaultPortal in config to accountNameToSetAsDefault
hs config set -- display error? display help? -- this is the part that seems a bit strange (see below)

Having 0 positional arguments for a command is a bit strange to me. I don't think that there are currently any other commands in the CLI that do not function unless an --option is passed. If hs config default were to be renamed to hs config set default <newDefaultValue> that would fit the current command paradigm better I think.

What are other people's thoughts?

miketalley · 2021-04-26T21:52:05Z

After discussion with @erincouse and @jasminetudor on the hs config default command we decided to use hs config set default [newDefaultAccountNameOrId].

It can be used to select from a list by running hs config set default or a name or portalId value can be specified as a positional argument -- hs config set default MyAccountName / hs config set default 1234567.

packages/serverless-dev-runtime/lib/server.js

gcorne · 2021-04-28T16:01:24Z

Maybe it is just me, but I struggle with the expectations when it comes to hs config list. I expect it to list the current config values not just the list of accounts. I also feel like "default" needs a qualifier because the config also stores other defaults like the default mode used when running hs fetch.

miketalley · 2021-04-28T19:46:31Z

Maybe it is just me, but I struggle with the expectations when it comes to hs config list. I expect it to list the current config values not just the list of accounts. I also feel like "default" needs a qualifier because the config also stores other defaults like the default mode used when running hs fetch.

We can definitely expand the output to include defaultMode, httpTimeout and allowUsageTracking (not sure if I missed any others). Are you thinking we should also make the output friendly to pipe to other commands? I'm interested to know which use cases you'd like to cover. I'm not sure that outputting the tokens/access keys would be a good idea.

I think changing the hs config set default command to hs config set default-account would be a good idea to avoid ambiguity. Adding hs config set default-mode, hs config set http-timeout, and hs config set allow-usage-tracking would also be good to completely flesh out the hs config set subcommands.

gcorne · 2021-04-28T20:15:04Z

I think I am stuck on hs config list and hs config rename being operations on accounts in a config not the config itself. It makes me wonder if hs accounts might be a clearer top-level command for the account operations.

miketalley · 2021-04-29T14:11:19Z

I think I am stuck on hs config list and hs config rename being operations on accounts in a config not the config itself. It makes me wonder if hs accounts might be a clearer top-level command for the account operations.

I'd be fine with having this functionality nested as hs accounts instead. How do you see hs config working, then?

gcorne · 2021-05-04T13:06:47Z

How do you see hs config working, then?

I'd imagine that it would be used for the "global" config options that are not related to a specific account like defaultMode.

…ortal is a portalId

… about circular dependency

…put table

…ed' as the new table version will do

…em from hs config command

erincouse · 2021-05-13T19:59:20Z

Small comment: I think hs accounts rename makes more sense as hs config rename (but understand that that may add complexity down the line, so not super hung up on it). Other is a general content nit: There's a lot of interchangeable use of portal and account, and it should really always be account as much as possible. It causes a lot of internal and external confusion because we aren't consistent with our own language.

abelb · 2021-05-13T21:09:56Z

One idea is to maybe do this?
hs config accounts rename <currentNameOrId> <newName>
hs config accounts list

It makes the commands longer but it nests them underneath config and renaming them AND the list are all data that lives in config.

Thoughts @jasminetudor @erincouse @miketalley?

Small comment: I think hs accounts rename makes more sense as hs config rename (but understand that that may add complexity down the line, so not super hung up on it).

abelb · 2021-05-13T21:10:13Z

Otherwise @miketalley everything looks good to me!

gcorne · 2021-05-14T16:55:20Z

Small comment: I think hs accounts rename makes more sense as hs config rename (but understand that that may add complexity down the line, so not super hung up on it).

@erincouse can you elaborate on why? Is your expectation that hs accounts rename would make changes to the name of the account inside HubSpot instead of the name that the developer chose when setting it up using hs init or hs auth? I think from an ergonomics perspective that hs accounts rename makes sense to me because all the other commands use the name as the value for --account. I also think we should consider deprecating hs auth in favor of hs accounts add since I think that is clearer. It will also make documenting how a developer can juggle multiple accounts (such as DEV, QA, PROD) via the CLI more straightforward.

gcorne · 2021-05-14T16:59:49Z

In addition to hs accounts add, I could see hs accounts set-default and hs accounts remove being valuable to round things out.

miketalley · 2021-05-19T20:45:59Z

I think there are two ways to organize the commands that are being thought about...

Commands that modify the hubspot.config.yml file are nested as hs config <command>
Commands that modify root level config values within hubspot.config.yml are hs config <command> and commands that modify the portals array values are hs accounts <command>

I can see good reasons for each. Having the rename command as hs accounts rename makes more sense to me because it doesn't leave any question about what is being renamed. It also leaves us the open possibility of having other rename commands to rename something else in the config in the future and nesting those separately. I can't think of a drawback to having multiple commands that can make changes to the config file values. 🤔

That said, I think @gcorne's idea (and as @erincouse, @abelb, @jasminetudor, and I have discussed separately) of deprecating hs auth in favor of hs accounts add. I think that work should be done in a separate PR -- as well as potentially adding hs accounts remove.

miketalley marked this pull request as ready for review April 5, 2021 22:21

miketalley requested review from drewjenkins, gcorne, anthmatic and brandenrodgers April 5, 2021 22:21

gcorne suggested changes Apr 6, 2021

View reviewed changes

anthmatic reviewed Apr 6, 2021

View reviewed changes

packages/cli/commands/config/rename.js Outdated Show resolved Hide resolved

anthmatic reviewed Apr 6, 2021

View reviewed changes

packages/cli-lib/lib/config.js Show resolved Hide resolved

gcorne reviewed Apr 14, 2021

View reviewed changes

brandenrodgers reviewed Apr 15, 2021

View reviewed changes

packages/cli/commands/config/rename.js Outdated Show resolved Hide resolved

brandenrodgers reviewed Apr 15, 2021

View reviewed changes

packages/cli-lib/lib/config.js Show resolved Hide resolved

anthmatic reviewed Apr 23, 2021

View reviewed changes

miketalley requested a review from gcorne April 27, 2021 13:54

gcorne reviewed Apr 27, 2021

View reviewed changes

packages/serverless-dev-runtime/lib/server.js Outdated Show resolved Hide resolved

miketalley requested review from gcorne and brandenrodgers May 6, 2021 14:55

miketalley added 20 commits May 10, 2021 09:57

switched to using map as we don't need reduce here anymore

200bac1

added test for getAccountId to return the defaultPortal when defaultP…

6ffb6dd

…ortal is a portalId

corrected option name from portalName to accountName

2343454

renamed hs config default to hs config set default

c243f22

updated table lib to allow for more customizability and added comment…

f61a263

… about circular dependency

added mergeDeep util for table customization

f98d5c8

added config path to hs config list output and formatted accounts out…

1534839

…put table

Moved table into cli-lib

af32999

fixed dependency path

4527010

updated table and schema logSchemas to handle not outputting 'undefin…

bb2a4f8

…ed' as the new table version will do

added hs account command, moved rename and list to it, and removed th…

cd67f31

…em from hs config command

added hs config set default-mode command

822d401

renamed files to use camelCase

e70eb46

removed unused var

6c61d28

added hs config set http-timeout command

d65677c

corrected description for httpTimeout

883824d

added hs config set allow-usage-tracking command

3d4e0a9

removed duplicate dep

02ccfd9

fixed typo

2bafb86

removed unused dep

f8bd2ed

miketalley force-pushed the issues/27-hs-config branch from 66028cc to f8bd2ed Compare May 10, 2021 14:12

ran lerna bootstrap which regenerated yarn.lock due to merge conflict

73a34ce

miketalley merged commit 146d82d into master May 26, 2021

miketalley deleted the issues/27-hs-config branch May 26, 2021 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added hs config commands #464

Added hs config commands #464

miketalley commented Apr 5, 2021 •

edited

Loading

miketalley commented Apr 6, 2021 •

edited

Loading

gcorne left a comment

anthmatic Apr 6, 2021

miketalley Apr 7, 2021

anthmatic Apr 23, 2021

gcorne Apr 14, 2021

miketalley Apr 20, 2021

brandenrodgers commented Apr 15, 2021

brandenrodgers commented Apr 15, 2021

anthmatic Apr 23, 2021

miketalley Apr 27, 2021

erincouse commented Apr 23, 2021

gcorne commented Apr 23, 2021

jasminetudor commented Apr 23, 2021 •

edited

Loading

miketalley commented Apr 26, 2021

miketalley commented Apr 26, 2021 •

edited

Loading

gcorne commented Apr 28, 2021

miketalley commented Apr 28, 2021 •

edited

Loading

gcorne commented Apr 28, 2021

miketalley commented Apr 29, 2021

gcorne commented May 4, 2021

erincouse commented May 13, 2021

abelb commented May 13, 2021

abelb commented May 13, 2021

gcorne commented May 14, 2021

gcorne commented May 14, 2021

miketalley commented May 19, 2021

Added hs config commands #464

Added hs config commands #464

Conversation

miketalley commented Apr 5, 2021 • edited Loading

Description and Context

Screenshots

Notes:

miketalley commented Apr 6, 2021 • edited Loading

gcorne left a comment

Choose a reason for hiding this comment

anthmatic Apr 6, 2021

Choose a reason for hiding this comment

miketalley Apr 7, 2021

Choose a reason for hiding this comment

anthmatic Apr 23, 2021

Choose a reason for hiding this comment

gcorne Apr 14, 2021

Choose a reason for hiding this comment

miketalley Apr 20, 2021

Choose a reason for hiding this comment

brandenrodgers commented Apr 15, 2021

brandenrodgers commented Apr 15, 2021

anthmatic Apr 23, 2021

Choose a reason for hiding this comment

miketalley Apr 27, 2021

Choose a reason for hiding this comment

erincouse commented Apr 23, 2021

gcorne commented Apr 23, 2021

jasminetudor commented Apr 23, 2021 • edited Loading

miketalley commented Apr 26, 2021

miketalley commented Apr 26, 2021 • edited Loading

gcorne commented Apr 28, 2021

miketalley commented Apr 28, 2021 • edited Loading

gcorne commented Apr 28, 2021

miketalley commented Apr 29, 2021

gcorne commented May 4, 2021

erincouse commented May 13, 2021

abelb commented May 13, 2021

abelb commented May 13, 2021

gcorne commented May 14, 2021

gcorne commented May 14, 2021

miketalley commented May 19, 2021

miketalley commented Apr 5, 2021 •

edited

Loading

miketalley commented Apr 6, 2021 •

edited

Loading

jasminetudor commented Apr 23, 2021 •

edited

Loading

miketalley commented Apr 26, 2021 •

edited

Loading

miketalley commented Apr 28, 2021 •

edited

Loading