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.

Sign up for GitHub

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

Jump to bottom

Only delete SARIF in PR check if not running on a fork #2084

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Only delete SARIF in PR check if not running on a fork #2084

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a76543
Only delete SARIF in PR check if not running on a fork
angelapwen Jan 12, 2024
a6427b3
Drive-by: fix linting errors
angelapwen Jan 12, 2024
30f279c
Remove print debugging
angelapwen Jan 13, 2024
b0b38b2
Use `github.context.payload...` to determine if running on fork
angelapwen Jan 16, 2024
648a31a
Emit log message, capture non-PR runs
angelapwen Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/init-action-post-helper.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/init-action-post-helper.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion src/init-action-post-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import * as github from "@actions/github";

import * as actionsUtil from "./actions-util";
import { getApiClient } from "./api-client";
import { getCodeQL } from "./codeql";
Expand Down Expand Up @@ -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.

) {
await removeUploadedSarif(uploadFailedSarifResult, logger);
}
Expand Down