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

Update CHANGELOG.md with new limits on uploading SARIF #1493

Merged
merged 7 commits into from
Jan 25, 2023

Conversation

aeisenberg
Copy link
Contributor

Let's not merge this until the backend change is available.

@aeisenberg aeisenberg requested a review from a team as a code owner January 19, 2023 17:58
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.

I wonder if we can indicate that this is a server-side change in some way. Specifically, we don't want GHES users who use GitHub Connect pulling this in and expecting the maximum number of SARIF runs per file to increase.

@aeisenberg
Copy link
Contributor Author

Good point. I can specifically call this out to being dotcom only. Though, that raises a question: Will the change be available in the next release of GHES?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@JennyRockU
Copy link
Contributor

API change has been released, releasing the API docs' update in an hour (but I think that docs then take a couple more hours to be updated after the release).

aeisenberg and others added 2 commits January 23, 2023 08:35
@aeisenberg
Copy link
Contributor Author

Thanks for the reviews everyone. If this is good, I think it's safe to merge into main so it will be available for the next release. Can someone give this another review and approval?

@henrymercer
Copy link
Contributor

@JennyRockU Sorry for the double ping — would you mind confirming that this change will land in GHES 3.9? Thanks!

@JennyRockU
Copy link
Contributor

JennyRockU commented Jan 24, 2023

@JennyRockU Sorry for the double ping — would you mind confirming that this change will land in GHES 3.9? Thanks!

Yes, it should go into the GHES 3.9 version.
I'm also happy to give an approval, but just noticed that maybe this rest API docs URL should not include this apiVersion parameter (or wait until we have a new one with the update, should be available today)?

CHANGELOG.md Outdated Show resolved Hide resolved
Remove apiVersion parameter.
@aeisenberg
Copy link
Contributor Author

but just noticed that maybe this rest API docs URL should not include this apiVersion parameter

Good point. I removed it.

@JennyRockU
Copy link
Contributor

I can confirm that the API change in now live in the docs:

image

@aeisenberg aeisenberg merged commit ebf6415 into main Jan 25, 2023
@aeisenberg aeisenberg deleted the aeisenberg/upload-sarif-limits branch January 25, 2023 16:32
@patiupe
Copy link

patiupe commented Jan 25, 2023

I have been running into issues due to this change. My action to upload the sarif file, fails. Below is the action

  - name: Upload result to GitHub Code Scanning
    uses: github/codeql-action/upload-sarif@v2
    with:
      sarif_file: snyk.sarif

The error that I am seeing in the workflow run is

Error: Code Scanning could not process the submitted SARIF file:
rejecting SARIF, as there are more runs than allowed (30 > 20)
Error: Code Scanning could not process the submitted SARIF file:
rejecting SARIF, as there are more runs than allowed (30 > 20)

How do I fix this ?

@github-actions github-actions bot mentioned this pull request Jan 26, 2023
6 tasks
@aeisenberg
Copy link
Contributor Author

Hello @patiupe, can you please open a new issue for this? This change has bumped the limit up from 15 runs to 20 runs, so 30 runs is still too many runs for us to handle. In the new issue someone can help you around this issue.

@jwgmeligmeyling
Copy link

Great to see the limit increased, but its still too low. For us the limit of 20 is actually a dealbreaker as well. And my company is not alone. And after some Googling, I found folks that were much more patient than I am even split their pipeline up into an entire matrix of chunks to work around the same limitation: https://github.com/vmware-tanzu/community-edition/actions/runs/2253393437/workflow .

Monorepos are a thing and 20 is just too limited. And it's also just a very unnecessary constraint. It isn't actually considering the number of violations inside the check run or the file size or any meaningful metric that actually affects the “complexity” or “processing time”. It seems a typical case of premature optimization.

So far my support tickets and talks to my solutions engineer/sales rep have not been particularly fruitful. I know this is just the issue tracker for the action and not the backend, but hopefully we can get this constraint lifted one day.

@dorothymitchell
Copy link

Thanks for your feedback @jwgmeligmeyling. I understand the SARIF run limit can be difficult for monorepos and I've recorded your feedback for prioritisation in the future. We impose this limit for technical reasons, including scalability impact. We aren't currently working on increasing this limit, but I will keep you updated if this changes and we prioritize this request.

@madhavajay
Copy link

https://github.com/OpenMined/PySyft/actions/runs/5572810151/jobs/10179266350

Error: Code Scanning could not process the submitted SARIF file:
rejecting SARIF, as there are more runs than allowed (64 > 20)
Error: Code Scanning could not process the submitted SARIF file:
rejecting SARIF, as there are more runs than allowed (64 > 20)

I don't understand what I am supposed to do here?
What are runs?
Screenshot 2023-07-17 at 11 27 21 pm

Is it the number of errors in this one sarif file?
Is it the number of sarifs i did in this one action?
Is it the number of times I have run this action?
How can I clear this number?

Do I need to fix this bugs on my machine first, or clear the entire set of security issues in github first?

@jwgmeligmeyling
Copy link

Every sarif file contains one or more runs. Typically just a single run, so the issue is probably the number of sarif files uploaded. You can upload a generous 20 runs maximally (so 20 files with a single run). Sorry for bringing bad news! 😊maybe with the advancements in quantum computing, someday we can process 21 runs simultaneously.

@aeisenberg
Copy link
Contributor Author

@jwgmeligmeyling is correct. This is a restriction on the number or runs allowed in a SARIF file.

To get around this limitation, you can split the file into chunks up <= 20 runs and upload individually. For each upload, you should specify a unique and consistent category property.

@sacha-athias-wmx
Copy link

Hello, i got the same issue as @madhavajay i can not upload my 68 sarif files to github by using the github/codeql-action/upload-sarif@v2

image

So i tried to to merge my sarif files with Js code inside a custom action but this didn't worked because i received 422 response from the Github API. I also tried to use sarif-sdk from microsoft but without success. Might you have an alternative to this problem or no ? If you have nothing to propose, is it now in your plan to inscrease this uplaod limit ?

@jwgmeligmeyling Did you find a solution for your monorepo ?

@jwgmeligmeyling
Copy link

At this point I feel like you guys are just provoking me. 😉

No I did not find a solution, I cancelled GHAS after my trail period.

@aeisenberg
Copy link
Contributor Author

@sacha-athias-wmx, please create a new issue for this and we can help you out. As I mentioned above, I recommend that you split your SARIF into separate files and run the upload for each file separately. Each file should be uploaded with a unique category to disambiguate with other files.

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.

9 participants