Allows to use a custom environment variable for Slack webhook #36
Conversation
|
Only thing I'm missing are some tests. 😊 |
| @@ -16,7 +16,8 @@ module.exports = async (pluginConfig, context) => { | |||
| env: { SEMANTIC_RELEASE_PACKAGE, npm_package_name } | |||
| } = context | |||
| const { | |||
| slackWebhook = process.env.SLACK_WEBHOOK, | |||
| slackWebhookEnVar = 'SLACK_WEBHOOK', | |||
There was a problem hiding this comment.
Where is this variable used again in this file? Apart from row 20 obviously. 😊 Or maybe this is need for the const magic with defaults to work? 🤔
There was a problem hiding this comment.
Yep, it is only used at line 20. It is needed for the default slackWebhook value 👌.
I can create the slackWebhook constant outside the destructuring assignement if you think it is clearer
README.md
Outdated
| | `onFailTemplate` | Provides a template for the slack message object on fail when `notifyOnFail` is `true`. See [templating](#templating). | undefined | | ||
| | `markdownReleaseNotes` | Pass release notes through markdown to slack formatter before rendering. | false | | ||
| | `slackWebhook` | Slack webhook created when adding app to workspace. | SLACK_WEBHOOK | | ||
| | `slackWebhookEnVar` | Environment variable to use to select the default Slack webhook value. | SLACK_WEBHOOK | |
There was a problem hiding this comment.
Just to clarify so that I understand it correctly. The default value of slackWebhook is the value of SLACK_WEBHOOK, and for slackWebhookEnVar it is actually the string 'SLACK_WEBHOOK'?
If so, do you think there is a good way to be more clear about this?
There was a problem hiding this comment.
That is exactly it. I tried to improve the documentation, is it clearer ?
There was a problem hiding this comment.
I think it was much clearer after your update. I did, however, have some difficulties with understanding the description for slackWebhookEnVar. You can see the comment further down 🙂
|
@BeyondEvil I wasn't sure where you wanted me to add the tests, so I've tested |
|
For this PR to be merged I:
|
|
This is particularly useful when using CircleCI and their `context` system. In our organisation, the context has a default SLACK_WEBHOOK value but we need to override it for some repositories (but still use the context). Since in CircleCI the context override custom environment variables we need this parameter to use another variable.
|
@juliuscc Sorry forgot to push the README modification. I changed the description of |
|
Great! LGTM! Thanks for the feature @KillianHmyd and thanks for the help reviewing the PR @BeyondEvil |
This is particularly useful when using CircleCI and their `context` system. In our organisation, the context has a default SLACK_WEBHOOK value but we need to override it for some repositories (but still use the context). Since in CircleCI the context override custom environment variables we need this parameter to use another variable.
This is particularly useful when using CircleCI and their
contextsystem.In our organisation, the context has a default SLACK_WEBHOOK value but we need to override it for some repositories (but still use the context).
Since in CircleCI the context override custom environment variables we need this parameter to use another variable.