-
Notifications
You must be signed in to change notification settings - Fork 322
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
Send overall job status in init-post status report #2097
Conversation
2059487
to
dd51ee0
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 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 <henry@henrymercer.name>
Yes! |
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 <henry@henrymercer.name>
Will hold on merging this until the monolith PR is approved and merged. |
Co-authored-by: Henry Mercer <henry@henrymercer.name>
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_status
field in theinit-post
status 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:
Failure
orConfigurationError
the first time an error has been established. This is already checked for in thegetActionsStatus
method that is called when we establish status of each individual action, so we additionally set the environment variable at this time.init-post
Action, if theANALYZE_DID_COMPLETE_SUCCESSFULLY
environment 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 failure
PR 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.Success
ifANALYZE_DID_COMPLETE_SUCCESSFULLY
is true. Note that in the case where analyze was successful, but some step betweenanalyze
and theinit-post
action fails, we'll incorrectly mark the job status asSuccess
. Presumably this is a very uncommon scenario though 🤔Unknown
if not previously populated, to theinit-post
status 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:
analyze
step) logMerge / deployment checklist