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

feat: add new mc-scripts config:sync command #2524

Merged
merged 33 commits into from
Apr 1, 2022

Conversation

Rhotimee
Copy link
Contributor

@Rhotimee Rhotimee commented Mar 24, 2022

Implement a new config:sync command for mc-scripts.

See changelog for more information

@changeset-bot
Copy link

changeset-bot bot commented Mar 24, 2022

🦋 Changeset detected

Latest commit: fe05ab7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@commercetools-frontend/mc-scripts Minor
@commercetools-frontend/application-config Minor
merchant-center-application-template-starter Patch
playground Patch
@commercetools-frontend/application-shell Minor
@commercetools-frontend/cypress Minor
@commercetools-frontend/mc-html-template Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Mar 24, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/2AMw91rpjrtHxXfpLSpgUjvLz2DH
✅ Preview: https://merchant-center-application-kit-git-shield-711575-commercetools.vercel.app

@vercel vercel bot temporarily deployed to Preview March 24, 2022 15:33 Inactive
@vercel vercel bot temporarily deployed to Preview March 25, 2022 12:05 Inactive
@vercel vercel bot temporarily deployed to Preview March 25, 2022 12:24 Inactive
@Rhotimee Rhotimee force-pushed the SHIELD-448-mc-script-config-sync branch from 04055fb to e700bc8 Compare March 25, 2022 12:35
@vercel vercel bot temporarily deployed to Preview March 25, 2022 12:42 Inactive
@vercel vercel bot temporarily deployed to Preview March 28, 2022 16:30 Inactive
@vercel vercel bot temporarily deployed to Preview March 28, 2022 16:36 Inactive
@vercel vercel bot temporarily deployed to Preview March 28, 2022 16:59 Inactive
@Rhotimee Rhotimee marked this pull request as ready for review March 28, 2022 16:59
@Rhotimee Rhotimee requested a review from a team March 28, 2022 17:00
@vercel vercel bot temporarily deployed to Preview March 29, 2022 11:37 Inactive
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks, functionality wise looks good. I left some feedback to improve different minor things.

@@ -62,6 +62,16 @@ const explorer = cosmiconfigSync(moduleName, {
},
});

export const getCustomApplicationConfigPath = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think we can simply name this getConfigPath (as we have the other functions loadConfig, processConfig)

Comment on lines 7 to 11
This command allows users to synchronize the local custom application config with the one available on the merchant center organization.

It creates a new custom application if there is no custom application with the entryPointUriPath in the local config file.

If there is a custom application with the entryPointUriPath available in the merchant center, it is updated with the local configuration upon confirmation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This command allows users to synchronize the local custom application config with the one available on the merchant center organization.
It creates a new custom application if there is no custom application with the entryPointUriPath in the local config file.
If there is a custom application with the entryPointUriPath available in the merchant center, it is updated with the local configuration upon confirmation.
This command allows users to synchronize the local Custom Application config with the Merchant Center. The sync implies that a new Custom Application will be created or an existing one will be updated.
Developers can use the `config:sync` command to automatically manage the configuration of a Custom Application from the config file instead of having to manually fill out the information in the Merchant Center.
If a new Custom Application needs to be created, an interactive prompt will ask the user to select the Organization where the Custom Application should be configured to.
Additionally, the Custom Application ID is automatically synced when running the `config:sync` command.
Note that this command requires a valid API token. You can get one by using the `mc-scripts login` command.

@@ -1,3 +1,4 @@
export { default as processConfig } from './process-config';
export { getCustomApplicationConfigPath } from './load-config';
Copy link
Member

Choose a reason for hiding this comment

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

Please also add a separate changeset for the new changes in this package.

const permissionKeys = entryPointUriPathToPermissionKeys(
const permissionKeys = entryPointUriPathToResourceAccesses(
Copy link
Member

Choose a reason for hiding this comment

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

Thanks

@@ -44,6 +44,7 @@ Commands:

login Log in to your Merchant Center account through the CLI, using the cloud environment information from the Custom Application config file. An API token is generated and stored in a configuration file for the related cloud environment, and valid for 36 hours.

config:sync Synchronizes the local custom application config with the one stored on the merchant center cloud.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
config:sync Synchronizes the local custom application config with the one stored on the merchant center cloud.
config:sync Synchronizes the local Custom Application config with the Merchant Center. A new Custom Application will be created if none existed, otherwise it will be updated.

Comment on lines 40 to 66
const userOrganizations = await fetchUserOrganizations({ mcApiUrl, token });

let organizationId, organizationName;

if (userOrganizations.total > 1) {
const organizationChoices = userOrganizations.results.map(
(organization) => ({
title: organization.name,
value: organization.id,
})
);

organizationId = await prompts({
type: 'select',
name: 'organizationId',
message: 'Select Organization',
choices: organizationChoices,
initial: 0,
})['organizationId'];

organizationName = organizationChoices.find(
({ value }) => value === organizationId
).title;
} else {
organizationId = userOrganizations.results[0].id;
organizationName = userOrganizations.results[0].name;
}
Copy link
Member

Choose a reason for hiding this comment

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

Prompting for the organization is only required for creating a new Custom App.

If the app exists, we already have the organizationId from the response.

const { confirmation } = await prompts({
type: 'text',
name: 'confirmation',
message: `You are about to create the configuration in the "${organizationName}" organization in environment ${location}. Are you sure you want to proceed?`,
Copy link
Member

@emmenko emmenko Mar 29, 2022

Choose a reason for hiding this comment

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

Suggested change
message: `You are about to create the configuration in the "${organizationName}" organization in environment ${location}. Are you sure you want to proceed?`,
message: `You are about to create a new Custom Application in the "${organizationName}" organization for the ${mcApiUrl} environment. Are you sure you want to proceed?`,

Copy link
Contributor

Choose a reason for hiding this comment

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

Nicola recommendation is great, but bear in mind that perhaps we have an extra "environment" word over there:

You are about to create a new Custom Application in the "${organizationName}" organization for the environment ${mcApiUrl} environment. Are you sure you want to proceed?

Maybe we could use only one:

You are about to create a new Custom Application in the "${organizationName}" organization for the ${mcApiUrl} environment. Are you sure you want to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

Yes thanks, it's a typo.

);
console.log(
chalk.green(
`You have successfully created the configuration in the "${organizationName}" organization in environment ${location}.\nYour local configuration has also been updated with the newly created application identifier: ${createdCustomApplication.id}.\nLink to the created custom application is ${customAppLink}`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`You have successfully created the configuration in the "${organizationName}" organization in environment ${location}.\nYour local configuration has also been updated with the newly created application identifier: ${createdCustomApplication.id}.\nLink to the created custom application is ${customAppLink}`
`Custom Application created.\nThe "applicationId" in your local Custom Application config file has been updated with the application ID.\nYou can see the Custom Application data in the Merchant Center at ${customAppLink}.`

const { confirmation } = await prompts({
type: 'text',
name: 'confirmation',
message: `You are about to update the configuration in the "${organizationName}" organization in environment ${location}. Are you sure you want to proceed?`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message: `You are about to update the configuration in the "${organizationName}" organization in environment ${location}. Are you sure you want to proceed?`,
message: `You are about to update the Custom Application "${entryPointUriPath}" in the ${mcApiUrl} environment. Are you sure you want to proceed?`,

);
console.log(
chalk.green(
`You have successfully updated the configuration in the "${organizationName}" organization in environment ${location}.\nLink to the created custom application is ${customAppLink}`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`You have successfully updated the configuration in the "${organizationName}" organization in environment ${location}.\nLink to the created custom application is ${customAppLink}`
`You have successfully updated the configuration in the "${organizationName}" organization in environment ${location}.\nLink to the created custom application is ${customAppLink}`
`Custom Application updated.\nYou can see the Custom Application data in the Merchant Center at ${customAppLink}.`

@@ -0,0 +1,138 @@
const omit = require('lodash/omit');
const prompts = require('prompts');
const chalk = require('react-dev-utils/chalk');
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are relying in the fact that react-dev-utils dependency currently export chalk.
Given that we are going to use that dependency directly I guess it would be safer to explicitly depend on it by itself declaring it in our package.json.

initial: 'yes',
});
if (confirmation.toLowerCase().charAt(0) !== 'y') {
console.log('Aborted.');
Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT Mar 29, 2022

Choose a reason for hiding this comment

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

Perhaps we could use chalk (red?) here to be more explicit with the message.

initial: 'yes',
});
if (confirmation.toLowerCase().charAt(0) !== 'y') {
console.log('Aborted.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could use chalk (red?) here to be more explicit with the message.

};

configSync().catch((error) => {
console.error(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could use chalk (red?) here to be more explicit with the message.

getCustomApplicationConfigPath,
} = require('@commercetools-frontend/application-config');

function updateApplicationIdInCustomApplicationConfig(applicationId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if we could maybe just update the applicationId using a regular expression to make this process simpler.

Maybe something like:

configFileContents.replace(/("env":.*"production":.*"applicationId":\s?)"(\w*)"/, '$1"[REPLACER]"')

(perhaps this is more complex than I can see)

Copy link
Member

Choose a reason for hiding this comment

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

FYI: we use this also int he create-mc-app CLI, as we need to update more fields.

https://github.com/commercetools/merchant-center-application-kit/blob/main/packages/create-mc-app/src/tasks/update-custom-application-config.js

Also to note, that this is only for JS files. For JSON we can update it directly. @Rhotimee I think I mentioned this last time but I forgot to follow up with you. We need to adjust the update logic in case the config is a JSON file.

@vercel vercel bot temporarily deployed to Preview March 30, 2022 14:37 Inactive
@vercel vercel bot temporarily deployed to Preview March 30, 2022 14:59 Inactive
@Rhotimee
Copy link
Contributor Author

@emmenko @CarlosCortizasCT, thanks for the feedback. All feedback above has been Implemented. Kindly take another look.

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Awesome, looks much better now thanks!

Just a few minor adjustments and then we're good to go 🚀

'@commercetools-frontend/application-config': minor
---

Implement and expose getConfigPath
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Implement and expose getConfigPath
Expose a new function `getConfigPath` to return the absolute file path of the Custom Application config file.

Comment on lines 44 to 66
if (userOrganizations.total > 1) {
const organizationChoices = userOrganizations.results.map(
(organization) => ({
title: organization.name,
value: organization.id,
})
);

organizationId = await prompts({
type: 'select',
name: 'organizationId',
message: 'Select Organization',
choices: organizationChoices,
initial: 0,
})['organizationId'];

organizationName = organizationChoices.find(
({ value }) => value === organizationId
).title;
} else {
organizationId = userOrganizations.results[0].id;
organizationName = userOrganizations.results[0].name;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would also check if there are no organizations. In this case we need to throw an error.

And maybe invert the condition.

Suggested change
if (userOrganizations.total > 1) {
const organizationChoices = userOrganizations.results.map(
(organization) => ({
title: organization.name,
value: organization.id,
})
);
organizationId = await prompts({
type: 'select',
name: 'organizationId',
message: 'Select Organization',
choices: organizationChoices,
initial: 0,
})['organizationId'];
organizationName = organizationChoices.find(
({ value }) => value === organizationId
).title;
} else {
organizationId = userOrganizations.results[0].id;
organizationName = userOrganizations.results[0].name;
}
if (userOrganizations.total === 0) {
throw new Error(`It seems you are not an admin of any Organization. Please make sure to be part of the Administrators team of the Organization you want the Custom Application to be configured to.`);
}
if (userOrganizations.total === 1) {
organizationId = userOrganizations.results[0].id;
organizationName = userOrganizations.results[0].name;
} else {
const organizationChoices = userOrganizations.results.map(
(organization) => ({
title: organization.name,
value: organization.id,
})
);
organizationId = await prompts({
type: 'select',
name: 'organizationId',
message: 'Select Organization',
choices: organizationChoices,
initial: 0,
})['organizationId'];
organizationName = organizationChoices.find(
({ value }) => value === organizationId
).title;
}

Comment on lines +23 to +25
'x-graphql-target': target,
'x-mc-cli-access-token': token,
'x-user-agent': userAgent,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'x-graphql-target': target,
'x-mc-cli-access-token': token,
'x-user-agent': userAgent,
Accept: 'application/json',
'Content-Type': 'application/json',
'x-graphql-target': target,
'x-mc-cli-access-token': token,
'x-user-agent': userAgent,

@@ -0,0 +1,113 @@
const { GraphQLClient } = require('graphql-request');
const { GRAPHQL_TARGETS } = require('@commercetools-frontend/constants');
Copy link
Member

Choose a reason for hiding this comment

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

Please define this dependency in the package.json.

customApplicationConfig.env.production.applicationId = applicationId;
fs.writeFileSync(
filePath,
JSON.stringify(customApplicationConfig, null, 2),
Copy link
Member

Choose a reason for hiding this comment

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

Let's format this with Prettier as well.


If a new Custom Application needs to be created, an interactive prompt will ask the user to select the Organization where the Custom Application should be configured to.

Additionally, the Custom Application ID is automatically synced when running the `config:sync` command.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Additionally, the Custom Application ID is automatically synced when running the `config:sync` command.
Additionally, the [Custom Application ID](/api-reference/application-config#envproductionapplicationid) is automatically synced when running the `config:sync` command.

```console
mc-scripts config:sync
```
-->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-->
<Info>
The command uses the [cloud Region](/concepts/merchant-center-api#cloud-regions) environment information from the Custom Application config.
</Info>
-->

Comment on lines 98 to 101
// update applicationID in the custom-application-config file
updateApplicationIdInCustomApplicationConfig(
fetchedCustomApplication.application.id
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually possible for the ID of a Custom Application to change after its creation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to my knowledge. It's a Prisma generated cuid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. But, it is possible that the user created the custom app on the merchant center and not with the cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is to update the local custom-application-config with the applicationId.

Copy link
Member

Choose a reason for hiding this comment

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

Speaking of the applicationId, we need to consider this use case: the config might use an env placeholder for the application id, e.g. ${env:APPLICATION_ID}.

In this case, we can't really replace the value.

I suppose we should only do that when we create a new Custom App. When updating, we shouldn't rewrite the value.

@Rhotimee Could you adjust the logic? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure 👍🏽

@vercel vercel bot temporarily deployed to Preview March 31, 2022 08:58 Inactive
Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT left a comment

Choose a reason for hiding this comment

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

lgtm

@vercel vercel bot temporarily deployed to Preview March 31, 2022 10:08 Inactive
@emmenko
Copy link
Member

emmenko commented Mar 31, 2022

I noticed that the build bundle does not contain the .graphql files. I'll push a commit to fix that.

I'll also add a --dry-run option to help testing the command.

@vercel vercel bot temporarily deployed to Preview March 31, 2022 15:53 Inactive
@vercel vercel bot temporarily deployed to Preview April 1, 2022 07:29 Inactive
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

💯 🚀

@vercel vercel bot temporarily deployed to Preview April 1, 2022 08:57 Inactive
Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Awesome work 👏

@Rhotimee Rhotimee merged commit ea1bee4 into main Apr 1, 2022
@Rhotimee Rhotimee deleted the SHIELD-448-mc-script-config-sync branch April 1, 2022 09:19
@ghost ghost mentioned this pull request Apr 1, 2022
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.

5 participants