Fix issue with "post_peek_screenshot" action failing when it's not supposed to #520
Conversation
| run: echo $PR_NUM > pr_num.txt | ||
|
|
||
| - name: Upload the pr num | ||
| uses: actions/upload-artifact@v2 |
There was a problem hiding this comment.
I think it's better to use the master version here. It's more updated than the v2 branch.
| uses: actions/upload-artifact@v2 | |
| uses: actions/upload-artifact@master |
There was a problem hiding this comment.
I think it might be better to stay with v2 or at least a specific tag. That way, if the upload-artifact repo is updated and breaking change occurs, we don't have to worry about it until we upgrade the action.
There was a problem hiding this comment.
@Thomas-Boi is right, (almost) always stick to a specific version and do not target a master or latest branch. Targeting a latest-release is not secure in terms of code injection attacks. You might wanna take a look at #395.
|
Weird, not sure why here it displays two
At the bottom of the PR, it only displays one. 🤔 |
Co-authored-by: David Leal <halfpacho@gmail.com>
Oh, I know this one. Currently, our peek action is triggered by the "bot:peek" label. To do this, we create an event whenever a label is added and check the name:
If the name doesn't match, it skips. Currently, we have 2 labels. Since they aren't "bot:peek", the action will be skipped, thus the above situation. |
There was a problem hiding this comment.
Looks good. Thank you! 🎉 👍
Let's wait on @amacado's approval. 🙂
This PR should fix the issue mentioned in #514 .
What's new:
The "post_peek_screenshot" will now check for three different states of the trigger "peek_icons" action: "skipped", "success", and "failure. These values have to be found by console logging the
github.eventobject. The docs don't mention this for some reason.Also, GitHub Actions will construct env variables before an if check by default:
Before, there was an env variable that relies on a skipped step => script was failing since it couldn't construct this variable.
That's pretty much it. I've tested it using my forked branch. The behaviours are:
I thought about separating user and system error but I don't think it's worth it. It'll require some extra code and the benefit (in my opinion) is not worth it. If I was to do it, it'll be something like the check SVG workflow.
Let me know what you think