feat: Allow warning only if secrets aren't available #22
Conversation
|
I'm pretty sure this is due to fork PRs having no access to github secrets. Let's see what CI says. Though github actions seem to be down at the moment. |
|
Though this might be a bit hidden under "Example usage" in the README.md:
|
|
Yeah I just found that https://docs.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token as well |
Yeah. I'd prefer a more helpful error message and an option to fallback to a warning. Warnings by default are usually missed. |
|
done. Some context on checking both on 403 and 404 is because of that https://developer.github.com/v3/troubleshooting/ |
sources/main.ts
Outdated
| throw new Error(`error adding "${label}": ${error}`); | ||
| if ((error.status === 403 || error.status === 404) && error.message.endsWith(`Resource not accessible by integration`)) { | ||
| core.warning(`could not remove label`); | ||
| core.info(`Worflows can't access secrets and have read-only access to upstream when they are triggered by a pull request from a fork, [more information](https://docs.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token)`); |
There was a problem hiding this comment.
Is there a reason for two separate messages on different levels? Can we squash these into a single one?
I'd still prefer a failure by default with an option to warn-only. PRs not having access to secrets isn't commonly know which is apparent from this PR.
There was a problem hiding this comment.
just to be clear, you want everything in the warning? The reason I had separated it is because I didn't want the thing to be too verbose, but I'm happy to review that.
I'm not sure if I'd fail by default, maybe warn by default (warning shows in the gates) with an option to fail. I feel like failing by default is a huge blocker for people for something they can't do anything about at the moment. What about warn by default with an option to fail?
There was a problem hiding this comment.
@eps1lon any opinion on the above? :)
There was a problem hiding this comment.
Yep everything in one message if it is related.
I do have a strong opinion on the failure. A warning will mark the check as green, no? If something isn't working then I'd always prefer a fail because warnings are usually missed.
There was a problem hiding this comment.
As far as I remember the check mark is green in the gate (PR recap) but you get some orange markers in the worklow run (once you click on details)
There was a problem hiding this comment.
Yeah. The default assumption is that it adds a label. If something isn't functioning then it should throw not warn. I'll definitely accept an option that downgrades it to a warning (or simply extends the error message). But the failure by default should stay.
There was a problem hiding this comment.
fair enough, updated
…g in some context" This reverts commit 5f6b63a.
eps1lon
changed the title
|
Actually makes sense to enable this flag on this repository for integration test. |
done! |
|
Works like a charm. Passes with the action on this branch but fails on master as expected. Thanks for sticking with it! |
thanks for driving it to it's best version! When do you expect it to be live? |
Should be live. You could always use the latest version on |
As you can see in https://github.com/eps1lon/actions-label-merge-conflict/runs/890095723?check_suite_focus=true#step:9:7 the action fails when a PR which comes from a fork is updated (push to the source branch).
It seems to be because in that case the github.context.repo reference points to the fork and not the upstream, making the subsequent queries wrong.
This pull request checks for the issue's information first (if any) and falls back on the repo information