Visitar URL original
Delete analysis after uploading by aeisenberg · Pull Request #1981 · github/codeql-action · GitHub
Skip to content

Delete analysis after uploading - #1981

Merged
aeisenberg merged 3 commits into
mainfrom
aeisenberg/delete-analysis-after-upload
Nov 16, 2023
Merged

Delete analysis after uploading#1981
aeisenberg merged 3 commits into
mainfrom
aeisenberg/delete-analysis-after-upload

Conversation

@aeisenberg

@aeisenberg aeisenberg commented Nov 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

The analysis is purposefully failing. We don't want a failed analysis sitting in the security center since this can cause some internal checks to erroneously fail.

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.

@aeisenberg
aeisenberg requested a review from a team as a code owner November 4, 2023 02:11
@aeisenberg
aeisenberg marked this pull request as draft November 4, 2023 02:11
@aeisenberg
aeisenberg force-pushed the aeisenberg/delete-analysis-after-upload branch 2 times, most recently from 6cc7ec8 to d1108ba Compare November 5, 2023 22:40
github-advanced-security[bot]
github-advanced-security AI found potential problems Nov 5, 2023
View reviewed changes
Comment thread src/init-action-post-helper.ts Fixed
@aeisenberg
aeisenberg force-pushed the aeisenberg/delete-analysis-after-upload branch 6 times, most recently from c81ea3f to 76b9cda Compare November 7, 2023 23:31
@aeisenberg

aeisenberg commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

I think this is working now, but I am confused by something. The call to /repos/github/codeql-action/code-scanning/analyses?sarif_id=XXX seems to be returning multiple analyses when I call it in the workflow. Some of the results are overlapping.

For example, you can see here https://github.com/github/codeql-action/actions/runs/6791589351/job/18463369144?pr=1981#step:22:44 that there are a number of analyses that we attempt to delete, but they don't exist. They have already been deleted by another job in the workflow, ie- https://github.com/github/codeql-action/actions/runs/6791589351/job/18463373448?pr=1981#step:18:53.

Two questions:

  1. Why is the single SARIF upload generating so many analyses?
  2. Why are two calls to retrieve analyses from different SARIF uploads retrieving a partially overlapping set of analyses?

@aeisenberg
aeisenberg force-pushed the aeisenberg/delete-analysis-after-upload branch from 1c978c8 to cac19f3 Compare November 8, 2023 00:54
@aeisenberg
Delete analysis after uploading
04451e0 
The analysis is purposefully failing. We don't want a failed analysis
sitting in the security center since this can cause some internal
checks to erroneously fail.
@aeisenberg
aeisenberg force-pushed the aeisenberg/delete-analysis-after-upload branch from cac19f3 to 04451e0 Compare November 10, 2023 21:26
@aeisenberg
aeisenberg changed the base branch from main to aeisenberg/fix-debug-integration-tests November 10, 2023 21:26
@aeisenberg
aeisenberg marked this pull request as ready for review November 10, 2023 21:26
@aeisenberg

aeisenberg commented Nov 10, 2023

Copy link
Copy Markdown
Contributor Author

This PR is now ready for review. I separated out the changes for the debug-artifacts-upload workflows, since we can and should merge that first in order to get main passing again.

Base automatically changed from aeisenberg/fix-debug-integration-tests to main November 13, 2023 11:42
henrymercer
henrymercer reviewed Nov 13, 2023
View reviewed changes

@henrymercer henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, a bunch of suggestions, but this looks good.

Comment thread .github/workflows/python312-windows.yml Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
Comment thread src/init-action-post-helper.ts Resolved
henrymercer
henrymercer reviewed Nov 13, 2023
View reviewed changes
Comment thread src/init-action-post-helper.ts Resolved
@aeisenberg

aeisenberg commented Nov 15, 2023

Copy link
Copy Markdown
Contributor Author

Looks like we can't use the cause option in Error yet.

@aeisenberg
Address comments from review
df9b50e 
- Change error messages.
- Use logger instead of core
- throw Error instead of write error message
@aeisenberg
aeisenberg force-pushed the aeisenberg/delete-analysis-after-upload branch from 6f43872 to df9b50e Compare November 15, 2023 20:54
henrymercer
henrymercer previously approved these changes Nov 15, 2023
View reviewed changes

@henrymercer henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one minor comment about the changes.

Comment thread src/util.ts Resolved
@aeisenberg
Use delay instead of wait
4e80a80 
Need to also change the signature of delay to allow this to happen.
henrymercer
henrymercer approved these changes Nov 15, 2023
View reviewed changes

@henrymercer henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@aeisenberg
aeisenberg enabled auto-merge November 15, 2023 21:42
@aeisenberg
aeisenberg merged commit 10f0515 into main Nov 16, 2023
@aeisenberg
aeisenberg deleted the aeisenberg/delete-analysis-after-upload branch November 16, 2023 12:32
@github-actions github-actions Bot mentioned this pull request Nov 16, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henrymercer henrymercer henrymercer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aeisenberg @henrymercer @github-advanced-security