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(mc-scripts): add new CLI options #2191

Merged
merged 12 commits into from
May 6, 2021
Merged

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented May 5, 2021

No description provided.

@vercel
Copy link

vercel bot commented May 5, 2021

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/Ay4thkSH9YTd6fWGH8Xt4heaiLbj
✅ Preview: https://merchant-cente-git-nm-mc-scripts-global-options-commer-fb468c.vercel.app

@changeset-bot
Copy link

changeset-bot bot commented May 5, 2021

🦋 Changeset detected

Latest commit: 0cb04e7

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

This PR includes changesets to release 4 packages
Name Type
merchant-center-application-template-starter Minor
@commercetools-frontend/mc-scripts Minor
playground Minor
@commercetools-website/custom-applications 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

@emmenko emmenko requested review from tdeekens and adnasa May 5, 2021 08:05
The options are as following:

- `--env <path>`: Parses the file path as a dotenv file and adds the variables to the environment. Multiple flags are allowed.
- `--env-type <environment>`: Supports cascading env variables from `.env`, `.env.local`, `.env.<environment>`, `.env.<environment>.local` files.
Copy link
Contributor

Choose a reason for hiding this comment

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

cascading env variables can seem tricky at first sight.
if we can point to a crash course article, let's do it ;)

Comment on lines 62 to 68
case 'start': {
// Special case to handle the `--match` option, where the user is prompted
// to select an application. We then use that as the `cwd` value when executing the command.
const applicationPath = await getApplicationPath(flags);
proxyCommand({ applicationPath });
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

dope.

Comment on lines 110 to 117
if (result.signal === 'SIGKILL') {
console.log(
`The command ${command} failed because the process exited too early. This probably means the system ran out of memory or someone called "kill -9" on the process.`
);
} else if (result.signal === 'SIGTERM') {
console.log(
`The command ${command} failed because the process exited too early. Someone might have called "kill" or "killall", or the system could be shutting down.`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

would be easiser flow to follow if we switch this up

Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

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

Nice!

Comment on lines 24 to 25
--env <path> (optional) Parses the file path as a dotenv file and adds the variables to the environment. Multiple flags are allowed.
--env-type <environment> (optional) Supports cascading env variables from ".env", ".env.local", ".env.<environment>", ".env.<environment>.local" files.
Copy link
Contributor

Choose a reason for hiding this comment

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

The below is more of a short hand of the before right. I could achieve --env-type with --env more inconvienetly?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the equivalent option of -c from dotenv-cli, I just named this differently. https://www.npmjs.com/package/dotenv-cli#cascading-env-variables

However, if you pass --env-type, the --env option(s) is ignored (see implementation of dotenv-cli).

@vercel vercel bot temporarily deployed to Preview May 5, 2021 09:32 Inactive
'@commercetools-website/custom-applications': minor
---

New CLI options! 🎉
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to 🎉
Celebrating 10 years of Nicola at commercetools!

process.exit(0);
}

const flags = mri(process.argv.slice(2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the flag for making 10 years at commercetools? 🎉 🎉 🎉 🔥

Congratulations @emmenko and thanks for being such an inspiration for your teammates!

You are the Nowitzki of tech, bunch of years with the same franchise! 😆


New CLI options! 🎉

## Loading dotenv files
Copy link

Choose a reason for hiding this comment

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

Maybe more info on this here

Congrats on 10 years Nicola!! 🎉
now to the next Goal!

} else if (Array.isArray(flags.env)) {
dotenvPath.push(...flags.env);
} else {
dotenvPath.push('.env');
Copy link
Contributor

Choose a reason for hiding this comment

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

Happy 10 years anniversary at CT 🥳
Keep pushing!


Previously, we recommended to use `dotenv-cli` to load environment variables before executing the `mc-scripts` command. For example:

```

Choose a reason for hiding this comment

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

🎉🎉🎉Auguri!!! 🎉🎉🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

wow such great new features congrats @emmenko

@vercel vercel bot temporarily deployed to Preview May 5, 2021 10:17 Inactive

Previously, we recommended to use `dotenv-cli` to load environment variables before executing the `mc-scripts` command. For example:

```
Copy link
Contributor

Choose a reason for hiding this comment

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

wow such great new features congrats @emmenko

'@commercetools-frontend/babel-preset-mc-app': patch
---

Fix babel warning
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, warnings are annoying, good fix 😄

Copy link

Choose a reason for hiding this comment

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

welcome back @montezume

@emmenko emmenko force-pushed the nm-mc-scripts-global-options branch from 0c615a7 to b80a432 Compare May 5, 2021 10:24
@vercel vercel bot temporarily deployed to Preview May 5, 2021 10:27 Inactive
NODE_ENV=production dotenv -e .env.gcp-production-eu -- mc-scripts compile-html
```

Now the `mc-scripts` CLI has the dotenv features built-in, so you don't need to install and use `dotenv-cli` anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

Huge congrats on 10 years Nicola! 🏆

I still remember watching CL and having burgers at Phil‘s place ages ago! ☺️

Copy link

Choose a reason for hiding this comment

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

It seems like a looong time ago @dferber90

Comment on lines 33 to 42
```diff
-dotenv -c development -- mc-scripts start
+mc-scripts --env-type development start

-NODE_ENV=production dotenv -- mc-scripts compile-html
+NODE_ENV=production mc-scripts compile-html

-NODE_ENV=production dotenv -e .env.gcp-production-eu -- mc-scripts compile-html
+NODE_ENV=production mc-scripts --env .env.gcp-production-eu compile-html
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful code sample!

You decennary is awesome, congratulations!

Copy link

@lg0m3s lg0m3s left a comment

Choose a reason for hiding this comment

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

UH LALAAAA! Looking dope af!
Congrats for the 1O years @ CT! 🔥🚀🙌🏻


If that's the case, you can now run the `mc-scripts start` command from the workspace root folder and pass the option `--match <glob>`. The option will attempt to gather a list of packages in the repository that match the glob pattern and show it as a prompt. You can then select the application that you want to start from that list.

We hope you find it useful.

Choose a reason for hiding this comment

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

Very useful, much wow 🐕

Congrats @emmenko to 10 years commercetools! 🎉

I had the most amazing time working with you and learning from you. I hope wholeheartedly that I'll have that privilege again in the future 🤗.

You are the best 🏆

Keep it on 🚀

@adnasa
Copy link
Contributor

adnasa commented May 5, 2021

whoa whoa...
some 🛡🛡SHIELD🛡 🛡 reunion and emoji blast going on here. @PhilippSpo @dferber90 @montezume @lg0m3s ... damn..

once again, I approve of this PR. 10/10 on the feature and 10/10 on 10 years.
Congrats buddy! 🙌🏽 💯

---
'merchant-center-application-template-starter': minor
'@commercetools-frontend/mc-scripts': minor
'playground': minor
Copy link
Member

Choose a reason for hiding this comment

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

Thanks that commercetools is your professional playground more than 10 years now @emmenko and we had even no "minor" issue with you 😄

Hope for some - of course 10! - real 🍻 soon!


## Loading dotenv files

The `mc-script` CLI now supports loading environment variables from [dotenv files](https://www.npmjs.com/package/dotenv).
Copy link
Contributor

Choose a reason for hiding this comment

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

That's great to have now as it is best practice nowadays 👍
Ten years of commercetools experience in this PR 🎉
@emmenko Thanks for all your contributions. Keep it up!

@emmenko emmenko force-pushed the nm-mc-scripts-global-options branch from b80a432 to ed8d690 Compare May 5, 2021 12:30
@vercel vercel bot temporarily deployed to Preview May 5, 2021 12:30 Inactive
@emmenko
Copy link
Member Author

emmenko commented May 5, 2021

Thanks to everyone for the kind words ❤️ Nice little surprise! 🤗

@vercel vercel bot temporarily deployed to Preview May 5, 2021 13:00 Inactive

## Prompt for selecting an application to start

If you are developing multiple Custom Applications in the same repository, chances are that you use a mono-repository setup.
Copy link

Choose a reason for hiding this comment

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

Mind blown That's huge! 😮

Congratulations on 10 years @emmenko! 😄

@vercel vercel bot temporarily deployed to Preview May 5, 2021 13:29 Inactive
@vercel vercel bot temporarily deployed to Preview May 5, 2021 14:14 Inactive
@vercel vercel bot temporarily deployed to Preview May 5, 2021 14:54 Inactive
@vercel vercel bot temporarily deployed to Preview May 5, 2021 16:06 Inactive
@emmenko
Copy link
Member Author

emmenko commented May 5, 2021

@tdeekens @adnasa I refactored a bit how dotenv files are loaded, after taking a deeper look at how react-scripts does it. See 0cb04e7

@emmenko emmenko requested review from tdeekens and adnasa May 5, 2021 16:07
@dirkhoerig
Copy link
Contributor

Who would have though that we go on a (so far) 10 year journey together @emmenko after Laura introduced us in Munich. What started with an idea in our english garden office started quite a ride :) Thanks for your trust, passion and support all the time. Let's see if we get it to another 10 years ;)

Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

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

Please disregard my comment above. Given that .local files should be git ignored the cascading should not have surprises. I was more worried that a .local env file would be used when one doesn't want to (e.g. when bundling on CI).

Copy link
Contributor

@jonnybel jonnybel left a comment

Choose a reason for hiding this comment

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

A delicious cake of new CLI options for the 10 year anniversary?

LGTM 🚀

Copy link
Contributor

@adnasa adnasa left a comment

Choose a reason for hiding this comment

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

🙌🏽

Comment on lines +168 to +176
const dotenvFiles = [
`.env.${environment}.local`,
// Don't include `.env.local` for `test` environment
// since normally you expect tests to produce the same
// results for everyone
process.NODE_ENV !== 'test' && `.env.local`,
`.env.${environment}`,
'.env',
].filter(Boolean);
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Comment on lines +41 to +52
By default, the following dotenv files are loaded according to the current environment values specified on each command: `process.MC_APP_ENV` or `process.NODE_ENV`. The priority of how the files are merged and overwritten goes from top to bottom (highest defined variable overrides lower).

- `.env.development.local`, `.env.test.local`, `.env.production.local`: Local overrides of environment-specific settings.
- `.env.development`, `.env.test`, `.env.production`: Environment-specific settings.
- `.env.local`: Local overrides. **This file is loaded for all environments except test.**
- `.env`

Please refer to the [dotenv documentation](https://github.com/motdotla/dotenv) for more details.

Furthermore, you can pass additional dotenv files by using the following option:

- `--env <path>`: Parses the file path as a dotenv file and adds the variables to the environment. Multiple flags are allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great documentation. thanks!

@emmenko emmenko merged commit b505813 into main May 6, 2021
@emmenko emmenko deleted the nm-mc-scripts-global-options branch May 6, 2021 09:54
@ghost ghost mentioned this pull request May 6, 2021
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.