Skip to content
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

Merged
merged 5 commits into from Jan 17, 2024

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Jan 12, 2024

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

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

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`.
@angelapwen angelapwen changed the title Only delete SARIF if not running on a fork Only delete SARIF in PR check if not running on a fork Jan 12, 2024
@angelapwen angelapwen marked this pull request as ready for review January 12, 2024 23:48
@angelapwen angelapwen requested a review from a team as a code owner January 12, 2024 23:48
@angelapwen
Copy link
Contributor Author

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 Deleting failed SARIF upload step does not run: https://github.com/github/codeql-action/actions/runs/7547884719/job/20548780850

Run from not-a-fork, where the Deleting failed SARIF upload step does run: https://github.com/github/codeql-action/actions/runs/7547885210/job/20548798454

@@ -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
Copy link
Contributor

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:

Suggested change
github.context.payload.pull_request?.head.repo.fork === false
!github.context.payload.pull_request?.head.repo.fork

Copy link
Contributor

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
  }
}

Copy link
Contributor Author

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.

@aeisenberg
Copy link
Contributor

aeisenberg commented Jan 16, 2024

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.

Copy link
Contributor

@aeisenberg aeisenberg left a 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.

@angelapwen
Copy link
Contributor Author

angelapwen commented Jan 17, 2024

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

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)

@angelapwen angelapwen merged commit f65ecd0 into github:main Jan 17, 2024
632 of 633 checks passed
@angelapwen angelapwen deleted the stop-deleting-sarif-forks branch January 17, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants