Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add an option to allow waiting until an analysis has been processed before finishing the Action. #781

Merged
merged 8 commits into from
Nov 18, 2021
Merged

Add an option to allow waiting until an analysis has been processed before finishing the Action. #781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
316ad9d
Add an option to allow waiting until an analysis has been processed b…
chrisgavin Oct 18, 2021
21a786f
Improve a log message.
chrisgavin Nov 17, 2021
49fc4c9
Reduce log message level.
chrisgavin Nov 17, 2021
823bb21
Add a default value for `wait-for-processing`.
chrisgavin Nov 17, 2021
e0b9b9a
Check for errors in the analysis status response.
chrisgavin Nov 17, 2021
4eef7ef
Split out waiting for processing.
chrisgavin Nov 17, 2021
215c4f5
Move the delay to the end of the loop.
chrisgavin Nov 17, 2021
b9bd459
Add a clarifying comment to a `break`.
chrisgavin Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions analyze/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ inputs:
description: Whether to upload the resulting CodeQL database
required: false
default: "true"
wait-for-processing:
chrisgavin marked this conversation as resolved.
Show resolved Hide resolved
description: If true, the Action will wait for the uploaded SARIF to be processed before completing.
required: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be false since we're providing a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it being required means it cannot be null, which it won't be if we're providing a default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

default: "false"
token:
default: ${{ github.token }}
matrix:
Expand Down
15 changes: 11 additions & 4 deletions lib/analyze-action.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/analyze-action.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading