-
Notifications
You must be signed in to change notification settings - Fork 428
Send overall job status in init-post status report #2097
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
Conversation
2059487 to
dd51ee0
Compare
henrymercer
left a comment
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, a couple of thoughts:
Just to check my understanding here, if one of the Actions we maintain is called after we successfully uploaded a SARIF file and it fails, then this will set the job status environment variable, so we'll have a failure. If a third-party Action is called after we successfully uploaded a SARIF file, then we won't know about that so we'll mark this as a success. This seems fine as code scanning has succeeded in that situation at analyzing the repo and uploading the analysis results. |
Co-authored-by: Henry Mercer <[email protected]>
Yes! |
henrymercer
left a comment
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.
Nice!
Co-authored-by: Henry Mercer <[email protected]>
|
Will hold on merging this until the monolith PR is approved and merged. |
Co-authored-by: Henry Mercer <[email protected]>
|
Ah, rebuild doesn't have permission to write to forks. |
|
The failures are actually unrelated upstream API failures. The comment change doesn't affect the transpilation files (I think) |
|
Monolith PR is merged so I'll merge this now and keep an eye on our telemetry to make sure the change is looking okay! |
This PR sends a
job_statusfield in theinit-poststatus report so that we will be able to more accurately measure our SLOs (see internal backlinked issue for more).To populate this field, the change:
FailureorConfigurationErrorthe first time an error has been established. This is already checked for in thegetActionsStatusmethod that is called when we establish status of each individual action, so we additionally set the environment variable at this time.init-postAction, if theANALYZE_DID_COMPLETE_SUCCESSFULLYenvironment variable was not true, and the job status environment variable was not already set, we set it toConfigurationError. This catches the case shown in thesubmit SARIF after failurePR check, where the failure occurs within the job but outside of the Action steps where we send status reports. We consider this a configuration error for the purposes of telemetry as these steps are not owned by the Action.SuccessifANALYZE_DID_COMPLETE_SUCCESSFULLYis true. Note that in the case where analyze was successful, but some step betweenanalyzeand theinit-postaction fails, we'll incorrectly mark the job status asSuccess. Presumably this is a very uncommon scenario though 🤔Unknownif not previously populated, to theinit-poststatus report.At each step, if the environment variable already has a value, we never override it.
I logged the job status in a debug statement to confirm that the appropriate statuses are being sent as expected:
analyzestep) logMerge / deployment checklist