-
Notifications
You must be signed in to change notification settings - Fork 337
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
Mark limits errors from third-party SARIF uploads as configuration errors #2173
Mark limits errors from third-party SARIF uploads as configuration errors #2173
Conversation
Nitty: make it a little clearer when this shows up in the logs what type of request we mean
@@ -286,7 +286,6 @@ async function run() { | |||
actionsUtil.getRequiredInput("checkout_path"), | |||
actionsUtil.getOptionalInput("category"), | |||
logger, | |||
{ isThirdPartyUpload: false }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why we can remove isThirdPartyUpload
here: wouldn't this mean that we are also marking first-party upload failures as configuration errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the logic where we convert InvalidSarifUploadError
s to ConfigurationError
s to upload-sarif-action.ts
, which is the only place where we had isThirdPartyUpload: true
. So where we had { isThirdPartyUpload: false }
, we will not convert the InvalidSarifUploadError
s to ConfigurationError
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also pushed a couple of commits so that we don't convert InvalidSarifUploadError
s to ConfigurationError
s when we're a first-party analysis as determined by our existing status report functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 after this is merged I'll kick off an Action release to get this + the SyntaxError
changes in.
I think this was the intention of the original PR, but the place that we were converting
InvalidSarifUploadError
s toConfigurationErrors
wasn't including processing errors.Merge / deployment checklist