Support splitting of DB creation and query execution #602
Conversation
| if (runStats && uploadStats) { | ||
| await sendStatusReport(startedAt, { ...runStats, ...uploadStats }); | ||
| } else if (runStats) { | ||
| await sendStatusReport(startedAt, { ...runStats }); |
There was a problem hiding this comment.
Ideally still send a successful status report here, even if we don't have runStats. You'll probably have to amend to types to in order to do that.
| logger | ||
| ); | ||
| await runFinalize(outputDir, threads, config, logger); | ||
| if (actionsUtil.getRequiredInput("skip-queries") !== "true") { |
There was a problem hiding this comment.
Two comments:
getRequiredInputwill fail if the input isn't there. I don't think that's what we want, since this input is optional.- Just a nit: Not sure how we are doing it for other boolean inputs, but do we want to support variants like
True,TRUE,yes,1, etc? Hmmm...and I see that you are using the same pattern we are using elsewhere. We should consider changing this, but in a separate PR.
There was a problem hiding this comment.
I think using getRequiredInput is fine here because the input is defined as
skip-queries:
description: If this option is set, the CodeQL database will be built but no queries will be run on it. Thus, no results will be produced.
required: false
default: "false"
so if not given by the user then the actions runtime inserts the default value, so our code will always see a value present.
I did run into a problem during tests because then the value really will be missing and getRequiredInput will crash.
There was a problem hiding this comment.
Ah...got it. So, ignore my first comment. :)
There was a problem hiding this comment.
In response to 2: I definitely agree we should support more variants of true. Let's leave it for a separate PR.
| await runAnalyze( | ||
| const threads = getThreadsFlag(cmd.threads, logger); | ||
| await runFinalize(outputDir, threads, config, logger); | ||
| await runQueries( |
There was a problem hiding this comment.
So, we are not adding this functionality to the runner? This should be called out in the changelog.
There was a problem hiding this comment.
Yep, I don't think we need this in the runner (or at least, nobody has asked for it). I will address this in the changelog.
CHANGELOG.md
Outdated
| @@ -3,6 +3,8 @@ | |||
| ## [UNRELEASED] | |||
|
|
|||
| - Fix `RUNNER_TEMP environment variable must be set` when using runner. | |||
| - The `analyze` step now supports a `skip-queries` option to merely build the CodeQL database without analyzing. | |||
There was a problem hiding this comment.
Can you add a link to this PR in the changelog entry for consistency?
Also, I think you should add something about handling already finalized databases.
There was a problem hiding this comment.
Done, though it is a rather long changelog entry now.
edoardopirovano
force-pushed
the
split-create-analysis
branch
from
a7e9641
to
ef852c0
Compare
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| ## [UNRELEASED] | |||
|
|
|||
| No user facing changes. | |||
| - The `analyze` step of the Action now supports a `skip-queries` option to merely build the CodeQL database without analyzing. This functionality is not present in the runner. Additionally, the step will no longer fail if it encounters a finalized database, and will instead continue with query execution. [#602](https://github.com/github/codeql-action/pull/602) | |||
There was a problem hiding this comment.
Nit: If you wanted to, you could turn this into 2 entries. This would make each entry shorter.
There was a problem hiding this comment.
Let's keep as is, it seems weird to have two entries referring to the same PR.
This PR makes a couple of changes that make it possible to write workflows that either don't create a CodeQL database, or don't run any CodeQL queries. In particular:
analyzestep now supports askip-queriesparameter that tells it to not run any queries.analyzestep will now no longer error if a finalized database already exists. Instead, it will log an informational message, skip the finalize step, and proceed with query execution.Merge / deployment checklist