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

Delete analysis after uploading #1981

Merged
merged 3 commits into from Nov 16, 2023
Merged

Delete analysis after uploading #1981

merged 3 commits into from Nov 16, 2023

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 4, 2023 •
edited

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

@henrymercer henrymercer left a comment

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.

.github/workflows/python312-windows.yml Show resolved Hide resolved
src/init-action-post-helper.ts Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Show resolved Hide resolved
@aeisenberg
Copy link
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
Copy link
Contributor

@henrymercer henrymercer left a comment

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.

src/util.ts Outdated Show resolved Hide 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
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Nice

@aeisenberg aeisenberg merged commit 10f0515 into main Nov 16, 2023
337 checks passed
@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.

None yet
2 participants
@aeisenberg @henrymercer