-
Notifications
You must be signed in to change notification settings - Fork 309
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
Add expect-error
input to force PR check green on expected failure
#1177
Conversation
analyze/action.yml
Outdated
@@ -66,6 +66,10 @@ inputs: | |||
default: ${{ github.token }} | |||
matrix: | |||
default: ${{ toJson(matrix) }} | |||
expect-error: | |||
description: For PR checks. If true, the Action will not set Actions status to failure. |
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.
May need to make this more descriptive so that our users don't set this to true
when they don't mean to? I don't think I see a hidden
option for input parameters.
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.
How about this?
description: For PR checks. If true, the Action will not set Actions status to failure. | |
description: [Internal] If true, the Action will not set Actions status to failure. |
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.
Hm, this caused some failures — I wonder if the bracket symbols aren't able to be parsed correctly.
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.
I think adding double quotes around it resolved it.
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.
Consider throwing an error inside the code if this is set but the repo is not github/codeql-action
or a fork. (You should be able to get that information from the Actions event context.)
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.
I've added this code — still doing a final test of:
- that if it's set in another repo, the error throws
- that if it's set in this repo, the test passes
- that if it's set in a fork of this repo, the test passes
- that if it's set in this repo but no error is found, an error indicating this is thrown
analyze/action.yml
Outdated
@@ -66,6 +66,10 @@ inputs: | |||
default: ${{ github.token }} | |||
matrix: | |||
default: ${{ toJson(matrix) }} | |||
expect-error: | |||
description: For PR checks. If true, the Action will not set Actions status to failure. |
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.
How about this?
description: For PR checks. If true, the Action will not set Actions status to failure. | |
description: [Internal] If true, the Action will not set Actions status to failure. |
@@ -183,7 +183,10 @@ async function run() { | |||
} catch (origError) { |
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.
Note that if an error isn't thrown, we need to set the job to a failure if this input is set to true.
It's a little more complicated than that since I see there are two places where core.setFailed
is being called. Do we need to call both of them? It looks like the one in runWrapper
is higher up the call chain. Maybe we don't need the one in run
.
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.
Note that if an error isn't thrown, we need to set the job to a failure if this input is set to true.
I'm a little confused: if an error isn't thrown, then why do we need to set the job to a failure? In that case, the job should succeed and we want that, right? Or do you mean that if we expect-error
that there must be an error, otherwise there was a problem? That's interesting. I had interpreted it more like force-success
It's a little more complicated than that since I see there are two places where core.setFailed is being called. Do we need to call both of them? It looks like the one in runWrapper is higher up the call chain. Maybe we don't need the one in run.
I'm not sure either. I can play around with it.
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.
Or do you mean that if we
expect-error
that there must be an error, otherwise there was a problem? That's interesting. I had interpreted it more likeforce-success
That's what I mean. If we expect an error and there is none, then that's an error. :)
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.
I've added this logic at the end of the outer try
block (indicating that an error was not thrown). Thanks!!
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.
Interestingly, I found that the outer try
block wasn't very useful. This catch block with origError
is where I was able to throw/catch/stop errors appropriately. I removed the duplicate logic from the outer try
block because it wasn't doing anything.
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
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.
I think all the bits are correct floating around somewhere, but there must have been a bad merge and there are some syntax errors.
} catch (origError) { | ||
const error = | ||
origError instanceof Error ? origError : new Error(String(origError)); | ||
if ( | ||
actionsUtil.getOptionalInput("expect-error") === "false" || | ||
actionsUtil.getOptionalInput("expect-error") !== "true" || |
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.
Maybe a bad merge here?
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.
I think the failures are expected — I removed the ram: 1
line in the last commit to make sure that if expect-error
was true and no error was thrown, that it would throw Error: expect-error input was set to true but no error was thrown.
I'll put the line back now that I have that run 👍
So far I have tested to confirm the following cases:
I will test to make sure it does the right things in a fork and update. |
The fork logic works 👍 (run) I will do some comment and commit cleanup. |
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.
This is good. It's a nice and simple solution you came up with in the end.
Thank you! I forgot to remove a couple of |
In #1159 we introduced a PR check to confirm that when the
analyze
workflow fails, partial debug artifacts are still uploaded. This caused 2 PR checks that always failed with a red X, which was not ideal for user experience.This change adds an
expect-error
optional input to theanalyze
workflow, which is set by default to false. Only in the PR Check that intentionally causes a failure inanalyze
is it set to true (though it could also be used in future cases where we expect a failing job). If this parameter is true, we do not callcore.setFailed(...)
on the job so it succeeds.If
expect-error
is set in any run not created by this repo, or a fork of it, we throw an error.If
expect-error
is set to true, but no error in the run was found, the job fails.Merge / deployment checklist