add error-catching wrapper around calls to toolrunner #174
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # lib/codeql.js # lib/codeql.js.map # src/codeql.ts
nickfyson
changed the title
alexrford
approved these changes
Sep 10, 2020
Contributor
alexrford
left a comment
alexrford
left a comment
There was a problem hiding this comment.
LGTM - I'm wondering now in review if it might be clearer to define ErrorMatcher as:
interface ErrorMatcher {
returnCode?: number;
outputRegex?: RegExp;
message: string;
}
rather than as a tuple, but I don't think it's a big deal in the context of how we're using the matchers.
Contributor
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
A fair number of comments but all of them minor, so generally looks good to me. Nothing major that I think will cause problems with this approach.
nickfyson
force-pushed
the
nickfyson/error_wrapper
branch
from
1b5cf58 to
b104d6e
Compare
robertbrignull
approved these changes
Sep 14, 2020
Contributor
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Had another quick look at the fixes and LGTM
Unfortunately looks like there are some conflicts from the merge of #181, but you may hopefully be able to just run npm run lint-fix, or maybe you'll need to fix conflicts manually first.
# Conflicts: # lib/codeql.js # lib/codeql.js.map # src/codeql.ts
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a wrapper around specific
exec.execcalls that are known to sometimes fail in specific ways, and captures both the exit code and stdout/err in order to match against a pre-defined list of conditions. Matching can be either by an exit code that thecodeql-cliuses to indicate a specific condition, or by regex that matches against either stdout or stderr. When a match is found the wrapper throws a custom error message, which can be used to give snippets of documentation to users.In this initial PR there is just a single matcher, intended to catch cases where no source code was seen during the build.
You can see the matchers in action in the extra (deliberately failing) integration tests here...
https://github.com/github/codeql-action/actions/runs/243679881
The testing story for this behaviour isn't entirely complete – ideally we would have integration tests like those above that deliberately trigger the errors in question and check they are caught as expected. Perhaps this could be achieved by some additional wrapper to test the wrapper, but that's all getting a little bit inception. I don't think there's a way to run to 'expect' a job to fail, but obviously that would be ideal.
Closes https://github.com/github/code-scanning/issues/1559
Merge / deployment checklist