-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only delete SARIF in PR check if not running on a fork #2084
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
@@ -186,7 +188,7 @@ export async function run( | |||
// appropriate permissions. | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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 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.ts
file as a drive-by second commit.Merge / deployment checklist