Changes pre-ET - #48
Conversation
| } catch (err) { | ||
| throw err; | ||
| } | ||
| return result.results; |
There was a problem hiding this comment.
Since we just throw here we can remove the catch and rethrow
| }; | ||
|
|
||
| module.exports = { | ||
| getAuthType, |
There was a problem hiding this comment.
Moving this to config to be useable elsewhere
| } = require('./preview/previewUtils'); | ||
| const { markRemoteFsDirty } = require('./preview/routes/meta'); | ||
| const { startShadowDevServer } = require('./preview/shadowDevServer'); | ||
| const { startSprocketMenuServer } = require('./preview/sprocketMenuServer'); |
There was a problem hiding this comment.
shadowDevServer -> sprocketMenuServer to be a little more obvious what it's for
| const { portalId, src, dest } = sessionInfo; | ||
|
|
||
| return uploadFolder(portalId, src, dest, fileMapperArgs, {}, filePaths); | ||
| const { accountId, src, dest } = sessionInfo; |
There was a problem hiding this comment.
Updated the initial upload to be less spammy and only draw attention if there's actually a problem - instead display a progress bar & print out any errors at the end
Screen.Recording.2024-02-07.at.3.17.21.PM.mov
There was a problem hiding this comment.
I generally like this pattern better, because it is cleaner in the terminal -- but I'm not sure if there is discussion to be had about this pattern being globalized as the standard 'upload' pattern for the CLI
cc @brandenrodgers - we have the cli guidelines being formed right now and I saw on the doc that the component library was slotted to include progress indicators - We are somewhat straying from what is the current pattern for uploading here for the moment, do you have any concerns about that for the interim? I'm assuming that once the guidelines are established, we can circle back and match the decided upon pattern.
| unlinkSync(`${__dirname}/key.pem`); | ||
| unlinkSync(`${__dirname}/cert.pem`); | ||
| unlinkSync(`${os.tmpdir()}/hstmp/hsLocalSshKey.pem`); | ||
| unlinkSync(`${os.tmpdir()}/hstmp/hsLocalSshCert.pem`); |
There was a problem hiding this comment.
Put these in the os temp dir feels a little cleaner pattern for the millisecond they exist before they get deleted. Should be cross platform but I'll make sure
| nextWait *= BACKOFF_RATIO; | ||
| if (nextWait * BACKOFF_RATIO <= MAX_WAIT) { | ||
| nextWait *= BACKOFF_RATIO; | ||
| } |
There was a problem hiding this comment.
Figured I'd cap this while I'm in here and close an open issue
| ${ domains.length ? | ||
| getSiteList(domains, PORT) : | ||
| "<p>No domains found. You either don't have any domains set up in your portal or your personal access key is missing a scope 'cms.domains.read' required for this feature.</p>" | ||
| } |
There was a problem hiding this comment.
We log the error to console if the scope is missing but good to put one here too.
| filePaths = [] | ||
| ) { | ||
| const { saveOutput, convertFields } = commandOptions; | ||
| const { |
There was a problem hiding this comment.
Adjusted this to allow overriding the places that would otherwise log so that we can hook in the progress bar hooks. Should have no effect to current usage if these aren't passed in.
| } | ||
|
|
||
| async function getNewAccessToken(accountId, personalAccessKey, expiresAt, env) { | ||
| async function getNewAccessToken(accountId, personalAccessKey, expiresAt, env, fullAPIResponse=false) { |
There was a problem hiding this comment.
Previous implementation to get the enabledFeatures fetched to refresh the access key but didn't write it to the config if it got a new one - fixes that
j-malt
left a comment
There was a problem hiding this comment.
One minor style point but looks good!
TanyaScales
left a comment
There was a problem hiding this comment.
Should go back through and i18n all the strings in here -- however, you might find on porting to local-dev-lib that these errors will be thrown there and i18n might occur on that end, while you just display/log it on this side.
| const { portalId, src, dest } = sessionInfo; | ||
|
|
||
| return uploadFolder(portalId, src, dest, fileMapperArgs, {}, filePaths); | ||
| const { accountId, src, dest } = sessionInfo; |
There was a problem hiding this comment.
I generally like this pattern better, because it is cleaner in the terminal -- but I'm not sure if there is discussion to be had about this pattern being globalized as the standard 'upload' pattern for the CLI
cc @brandenrodgers - we have the cli guidelines being formed right now and I saw on the doc that the component library was slotted to include progress indicators - We are somewhat straying from what is the current pattern for uploading here for the moment, do you have any concerns about that for the interim? I'm assuming that once the guidelines are established, we can circle back and match the decided upon pattern.
Co-authored-by: Tanya Scales <4976331+TanyaScales@users.noreply.github.com>
Lots of changes - I'll comment throughout
See also HubSpot/hubspot-cli#996