Only delete SARIF in PR check if not running on a fork #2084
Only delete SARIF in PR check if not running on a fork #2084angelapwen merged 5 commits intogithub:mainfrom
Conversation
The `Submit SARIF after failure` PR Check was failing when opened on a fork because of a permissions problem when deleting the uploaded SARIF. This change should fix this by only deleting the SARIF when the owner of the current repository is `github`.
a0afcce to
7a76543
Compare
0f8b25a to
30f279c
Compare
c0d63fd to
98fa0c0
Compare
98fa0c0 to
b0b38b2
Compare
|
Okay, I've finally worked out how to differentiate the behavior between fork and non-fork. The Actions runs linked to the PR are a bit confused because the branch in my fork is named the same as the branch in this repo, but here are the correct associated runs: Run from a fork (this PR), where the Run from not-a-fork, where the |
src/init-action-post-helper.ts
Outdated
| if ( | ||
| process.env["CODEQL_ACTION_EXPECT_UPLOAD_FAILED_SARIF"] === "true" && | ||
| repositoryNwo.owner !== "github" | ||
| github.context.payload.pull_request?.head.repo.fork === false |
There was a problem hiding this comment.
Nice. That's easy. Though, this will also fail to run for push and workflow_dispatch triggers. I think that neither of these would ever run in a fork.
So, this should be more generic since it also captures non-pr triggered runs:
| github.context.payload.pull_request?.head.repo.fork === false | |
| !github.context.payload.pull_request?.head.repo.fork |
There was a problem hiding this comment.
And now that I think about it a little more, can you restructure this so that there is a log message emitted explaining that we don't delete the SARIF on forks when CODEQL_ACTION_EXPECT_UPLOAD_FAILED_SARIF is true?
if (process.env["CODEQL_ACTION_EXPECT_UPLOAD_FAILED_SARIF"] === "true") {
if (github.context.payload.pull_request?.head.repo.fork === false) {
// do delete
} else {
// log
}
}There was a problem hiding this comment.
That makes sense 👍 the log message emitted also means I can remove the code comment ✨
I've pushed the changes here and to the non-fork PR. I'll re-request review once the checks pass as expected on both.
|
Interesting...the check run for the SARIF failure CI job on this PR is matching to the CI job for #2085. See https://github.com/github/codeql-action/actions/runs/7548705896/job/20551327490 The code looks fine. I just clicked to re-run all the jobs. But, actions may pick up that it's running from main, not from a fork. |
aeisenberg
left a comment
There was a problem hiding this comment.
Anyway, I think this is good to go in.
Yes, Henry and I picked that up earlier 🤔 we think it's because I named the branches the same on both the fork and this repo, and they have the exact same commits... (and this may be something that the Actions folks didn't anticipate) |
The
Submit SARIF after failurePR Check was failing when opened on a fork because of a permissions problem when deleting the uploaded SARIF. This change should fix this by only deleting the SARIF when the head of the pull request is explicitly not a fork.Tested because this PR was opened on a fork and the PR check is passing, and the SARIF deletion step is not entered 😸 I also pushed the same commits to a branch on this repo and the deletion behavior is preserved: #2085 (draft PR)
Also fixed some linting errors in the
analyze-action.tsfile as a drive-by second commit.Merge / deployment checklist