Conversation
seratch
left a comment
There was a problem hiding this comment.
I haven't verified the behavior but the code changes look good to me and there is nothing to suggest so far. This is pretty cool!
zimeg
left a comment
There was a problem hiding this comment.
@seratch Thank you once again for such a fast review! I'll test this a few more times before merging, but will merge once things are feeling good.
Also leaving a few notes on some of the stranger changes, but am hopeful that tests cover the expected behaviors well 🙏
| ); | ||
| core.setSecret(this.inputs.token); | ||
| core.setSecret(this.inputs.webhook); | ||
| case !!core.getInput("token") && !!core.getInput("webhook"): |
There was a problem hiding this comment.
This prevents adding both token and webhook as inputs using with while still allowing both environment variables!
| case !!this.inputs.method: | ||
| if (!this.inputs.token) { |
There was a problem hiding this comment.
Special care was taken in flipping these cases with wording since we can't assume a set this.inputs.token means that technique is being used.
Instead, I think we must check for a method instead of token, and always check method before webhook 🤔
| async function post(config) { | ||
| switch (true) { | ||
| case !!config.inputs.token: | ||
| case !!config.inputs.method: |
There was a problem hiding this comment.
IMO an important change that might've been missed without testing a bit! Before changes, an environment variable might've set this and caused this case to run despite possible inputs preferring the webhook.
This is covered and confirmed in the send.spec.js tests!
|
🚢 💨 Merging now! But going to hold off on an immediate release with hopes of landing a few docs updates before the next tag. Planning to make those changes soon in another PR- 🫡 |
Summary
This PR allows both environment variables
SLACK_TOKENandSLACK_WEBHOOK_URLto exist in a workflow without causing steps to error due to an undefined technique! Fixes #373.Reviewers
Add the following to the workflow in
develop.ymlbefore running withnpm run dev:This should error before the change is applied but not after!
Requirements