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 description to include limit on number of results #369

Merged
merged 2 commits into from
Jan 19, 2021
Merged

Update description to include limit on number of results #369

merged 2 commits into from
Jan 19, 2021

Conversation

felicitymay
Copy link
Contributor

👋🏻 I'm proposing this change alongside some similar changes to the documentation. We want to make sure that people are aware of the limit on the number of results that will be processed per SARIF file. This is rarely a problem for results generated by CodeQL but a few people have encountered it when running 3rd party tools that may be "noisy".

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@@ -3,7 +3,7 @@ description: 'Upload the analysis results'
author: 'GitHub'
inputs:
sarif_file:
description: The SARIF file or directory of SARIF files to be uploaded.
description: The SARIF file or directory of SARIF files to be uploaded. Each SARIF file should contain a maximum of 1000 results, any additional results are ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry saying "each SARIF file" is misleading. If you upload a directory of files then it doesn't upload them separately but instead first concatenated them into one big SARIF file and uploads that. It would be more accurate to say "A maximum of 1000 results will be processed and any additional results will be ignored."

Does that seem ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Thanks. I hadn't realized that you could have more than one file before looking at the action, so wondered if this was a way to work around the 1000 result limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the description with Alistair's suggestion.

upload-sarif/action.yml Outdated Show resolved Hide resolved
@felicitymay
Copy link
Contributor Author

Many thanks for the rapid turnaround on reviews @hubwriter and @robertbrignull

@felicitymay
Copy link
Contributor Author

I don't have permission to merge this. Can I leave it in your hands @robertbrignull?

@robertbrignull robertbrignull merged commit a1bfa76 into github:main Jan 19, 2021
@felicitymay felicitymay deleted the patch-1 branch January 19, 2021 18:13
@github-actions github-actions bot mentioned this pull request Jan 25, 2021
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.

3 participants