FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Delete analysis after uploading by aeisenberg · Pull Request #1981 · github/codeql-action · GitHub

/ codeql-action Public

Delete analysis after uploading - #1981

Merged
aeisenberg merged 3 commits into
mainfrom
aeisenberg/delete-analysis-after-upload
Nov 16, 2023
ConversationCommits (3)ChecksFiles changed
Merged

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

Conversation

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 requested a review from a team as a code owner November 4, 2023 02:11
aeisenberg marked this pull request as draft November 4, 2023 02:11
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 AI found potential problems Nov 5, 2023
View reviewed changes
Comment thread src/init-action-post-helper.ts Fixed
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 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 force-pushed the aeisenberg/delete-analysis-after-upload branch from 1c978c8 to cac19f3 Compare November 8, 2023 00:54
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 force-pushed the aeisenberg/delete-analysis-after-upload branch from cac19f3 to 04451e0 Compare November 10, 2023 21:26
aeisenberg changed the base branch from main to aeisenberg/fix-debug-integration-tests November 10, 2023 21:26
aeisenberg marked this pull request as ready for review November 10, 2023 21:26

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 reviewed Nov 13, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

aeisenberg commented Nov 15, 2023

Copy link
Copy Markdown
Contributor Author

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

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good, just one minor comment about the changes.

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice

aeisenberg enabled auto-merge November 15, 2023 21:42
aeisenberg merged commit 10f0515 into main Nov 16, 2023
aeisenberg deleted the aeisenberg/delete-analysis-after-upload branch November 16, 2023 12:32
github-actions Bot mentioned this pull request Nov 16, 2023
Merged
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL