Add an option to allow waiting until an analysis has been processed before finishing the Action. - #781
Add an option to allow waiting until an analysis has been processed before finishing the Action.#781chrisgavin merged 8 commits into
Conversation
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Looks reasonable to me.
Sorry, something went wrong.
cannist
left a comment
There was a problem hiding this comment.
I wonder if we should decouple the check from the actual upload. If we did that we could, for example, upload the codeql database in the analyze action before we start checking the status.
Sorry, something went wrong.
…efore finishing the Action.
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
cannist
left a comment
There was a problem hiding this comment.
LGTM, just some minor details.
Sorry, something went wrong.
| default: "true" | ||
| wait-for-processing: | ||
| description: If true, the Action will wait for the uploaded SARIF to be processed before completing. | ||
| required: true |
There was a problem hiding this comment.
Should this be false since we're providing a default value?
Sorry, something went wrong.
There was a problem hiding this comment.
I believe it being required means it cannot be null, which it won't be if we're providing a default.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, I always understood required here as "must be specified by the user". If it does not matter if we use true or false in the presence of a default, I think false still is less confusing to the reader.
That would also be in line with the example given in the docs.
Sorry, something went wrong.
There was a problem hiding this comment.
There are also other docs that show the opposite pattern; a required argument with a default value.
I tried in a test repository, and it seems like Actions doesn't really care about whether the value is "required" or not. Possibly it's a planned future feature, but without knowing how it will interpret the property it's hard to say if one way is any better than the other.
Sorry, something went wrong.
There was a problem hiding this comment.
OK, it is your PR. 🙂
I still prefer the other value but if actions do not fall over if the parameter is left unspecified by the user I do not mind too much.
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Looks reasonable to me, but maybe @cannist should have a look, too.
Sorry, something went wrong.
This adds an option to allow waiting until the analysis has succeeded before continuing. This will be used to allow surfacing errors that have occurred while processing the uploaded SARIF file.
When this option is enabled, after uploading the SARIF file the Action will call the status endpoint every 5 seconds until the processing is complete. After 2 minutes, it will give up and continue anyway. I felt that there should be a timeout in case there is some issue with processing and that it shouldn't fail the Action since it's possible the analysis will eventually be processed.
I've not added the option to enable this behavior to the readme, as our hope is to test this out a bit and eventually make it the default at which point we'll remove the option.
Merge / deployment checklist