-
Notifications
You must be signed in to change notification settings - Fork 27
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(create-mc-app): add cloudIdentifier flag #3564
Conversation
🦋 Changeset detectedLatest commit: 8bda236 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Some general questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this! Congrats on your first PR 🥳 It looks really good 🤗
Some general questions:
- Is the pipeline failing because of my changes or is it supposed to fail in the development environment. The error message that shows up is "Error: The operation was canceled."
Yes, the pipeline step runs scripts/install-template.mjs
script.
The cli prompts for cloud env, which is not provided in the script.
merchant-center-application-kit/scripts/install-template.mjs
Lines 56 to 67 in 67a35e2
const createAppCmdResult = shelljs.exec( | |
[ | |
binaryPath, | |
applicationName, | |
`--template=${templateName}`, | |
`--template-version=${branchName}`, | |
`--application-type=${applicationType}`, | |
`--initial-project-key=${initialProjectKey}`, | |
`--yes`, | |
'--skip-install', | |
].join(' '), | |
{ cwd: sandboxPath } |
- I tried generating a changeset file, however, I was not completely sure on the options to select while creating one for these changes.
We use semantic versioning. A new feature (a non-breaking change) corresponds to a minor
package update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job so far, thanks! 🙌
…tion-config and added CLOUD_IDENTIFIER env variable
…updated lock file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, everything looks good to me 🙌
Here are my final comments
…nt code Co-authored-by: Kacper Krzywiec <[email protected]>
… CLOUD_IDENTIFIERS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great 😊
2 final notes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks a lot! 🙌
All good from my side, just a minor naming suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @mustafaasif2 🙌🏽
Summary
Added a cloudIdentifier flag for Custom Applications and Custom Views
Description
The create-mc-app CLI now prompts the user about the region of their choice, defaulting to gcp-eu.
The option to define the region is also be possible via a CLI option, e.g. --cloud-identifier=gcp-au for programmatic usage.
Background
Installing one of the Merchant Center starter templates (customizations) results by default in using the GCP EU environment.
However, for customers using a different CoCo region it might get confusing if they forget to change the cloudIdentifier to the region of their choice.
Corresponding Jira Ticket: SHIELD-1300