-
Notifications
You must be signed in to change notification settings - Fork 334
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
Refactoring: Simplify upload SARIF flow and reuse loaded flags #2361
Conversation
eae5c66
to
bfdafbc
Compare
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 like a general improvement, but just want to make sure about the payload change.
src/upload-lib.ts
Outdated
analysisKey, | ||
analysisName, | ||
category, |
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.
Previously, this was analysisName
, but that doesn't look right since the util.getRequiredEnvParam("GITHUB_WORKFLOW")
was being passed in. Now you're using category
. This seems to be correct now, but will that mess up anything with our teleemtry since we're changing what we're stuffing into the analysis_name
field of the payload?
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.
You're right, let's keep it the same. My understanding is the category in the SARIF overrides what we have in the analysis_name
field, and GITHUB_WORKFLOW
gives the job name, which we could use as a fallback. We might be able to stop specifying the analysis_name
field if we're sure we're always specifying a category in the SARIF, but I'll leave that cleanup for later.
💭 The |
...and it passed! So, guessing the error came from the server side. |
This was due to an internal experiment on the API endpoints (unrelated to this PR). The relevant feature flag is now disabled. I've cced you in Slack. |
Minor refactoring to simplify the flow for uploading SARIF files, and to reuse the loaded feature flags if we've already loaded them from disk or from the API.
Merge / deployment checklist