fix: remove dash from environment variables #195
Conversation
Codecov Report
@@ Coverage Diff @@
## main #195 +/- ##
=======================================
Coverage 63.15% 63.15%
=======================================
Files 21 21
Lines 589 589
Branches 221 221
=======================================
Hits 372 372
Misses 176 176
Partials 41 41
Continue to review full report at Codecov.
|
| @@ -47,7 +47,7 @@ function getEnv() { | |||
| return envCopy | |||
| }, | |||
| { | |||
| [`SCRIPTS_${script.toUpperCase()}`]: true, | |||
| [`SCRIPTS_${script.toUpperCase().replace(/-/g, '_')}`]: true, | |||
There was a problem hiding this comment.
If previous SCRIPTS_ environment variables were considered as public API it wouldn't harm to also include them here - in addition to the new ones.
There was a problem hiding this comment.
It seems like it never really worked in the first place so I don't think they could have been considered public either way 🙃
| @@ -47,7 +47,7 @@ function getEnv() { | |||
| return envCopy | |||
| }, | |||
| { | |||
| [`SCRIPTS_${script.toUpperCase()}`]: true, | |||
| [`SCRIPTS_${script.toUpperCase().replace(/-/g, '_')}`]: true, | |||
There was a problem hiding this comment.
It seems like it never really worked in the first place so I don't think they could have been considered public either way 🙃
|
@all-contributors please add @AriPerkkio for code and tests |
|
I've put up a pull request to add @AriPerkkio! 🎉 |
|
🎉 This PR is included in version 8.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
I'm not sure why, but this patch update has caused our build to suddenly fail with lint errors. I'm happy to fix up the errors and warnings, but just a FYI about it. I'm not even entirely sure it's caused by this change, but it's the only thing referenced in the bump. |
|
I think that is actually a real issue in the code base. Previously your CI did not run Now that CI runs the lint step it is actually finding real issues. kcd-scripts/src/scripts/validate.js Line 21 in 5c69945 Edit: Oh right there is actually a lint step in the earlier run too. Missed that single line in verbose output. 🤦♂️ |
|
Yeah, I'm not sure why it's an issue now, the rules it's complaining about have been around for at least a few months. I think it's something along the lines what you're saying and however this was broken before, it's now doing the right thing so we should fix it (although my fix it to disable the rules because they're too stylistic for my taste). |
What:
Fixes #194.
Replaces dash characters (
-) fromSCRIPTS_environment variables with_.In practice
SCRIPTS_PRE-COMMITis nowSCRIPTS_PRE_COMMIT.Why:
Bash does not support environment variables with dash characters (
-).This cannot be reproduced with git-bash on windows but appears in Debian 10.
How:
When
SCRIPTS_environment variables are added replace all dashes with underscores.Searched and checked all usage of
parseEnvandprocess.env.Checklist:
Manual test:
Without this fix I was unable to pass pre-commit hook of this repository. With the fix applied pre-commit hook runs successfully.