Add an option to allow waiting until an analysis has been processed before finishing the Action. #781
Conversation
chrisgavin
force-pushed
the
wait-for-processing
branch
3 times, most recently
from
dd9d5e4
to
d423c8d
Compare
chrisgavin
force-pushed
the
wait-for-processing
branch
from
d423c8d
to
218d7f0
Compare
…efore finishing the Action.
Co-authored-by: Andrew Eisenberg <[email protected]>
Co-authored-by: Andrew Eisenberg <[email protected]>
chrisgavin
force-pushed
the
wait-for-processing
branch
3 times, most recently
from
9688a32
to
a2b0a22
Compare
chrisgavin
force-pushed
the
wait-for-processing
branch
from
a2b0a22
to
e0b9b9a
Compare
| @@ -52,6 +52,10 @@ inputs: | |||
| description: Whether to upload the resulting CodeQL database | |||
| required: false | |||
| 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?
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.
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.
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.
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.
There was a problem hiding this comment.
Looks reasonable to me, but maybe @cannist should have a look, too.
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