Support splitting of DB creation and query execution by edoardopirovano · Pull Request #602 · github/codeql-action · GitHub

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:

The analyze step now supports a skip-queries parameter that tells it to not run any queries.
The analyze step 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

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

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.

Nice.

I think it's great that we're running more unit-like integration tests on different options for the action. The pr-checks workflow is getting long. I wonder if we should be splitting these into a separate workflow.

Two comments:

getRequiredInput will 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.

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.

Ah...got it. So, ignore my first comment. :)

In response to 2: I definitely agree we should support more variants of true. Let's leave it for a separate PR.

So, we are not adding this functionality to the runner? This should be called out in the changelog.

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.

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.

Done, though it is a rather long changelog entry now.

LGTM.

Nit: If you wanted to, you could turn this into 2 entries. This would make each entry shorter.

Let's keep as is, it seems weird to have two entries referring to the same PR.

edoardopirovano requested review from aeisenberg and esbena June 28, 2021 10:42

edoardopirovano requested a review from a team as a code owner June 28, 2021 10:42

robertbrignull reviewed Jun 28, 2021

View reviewed changes

aeisenberg reviewed Jun 28, 2021

View reviewed changes

Support splitting of DB creation and query execution

ef852c0

edoardopirovano force-pushed the split-create-analysis branch from a7e9641 to ef852c0 Compare June 28, 2021 16:15

aeisenberg approved these changes Jun 28, 2021

View reviewed changes

Merge branch 'main' into split-create-analysis

93214ec

edoardopirovano enabled auto-merge June 28, 2021 17:25

edoardopirovano merged commit 53cf5d9 into github:main Jun 28, 2021

github-actions Bot mentioned this pull request Jul 5, 2021

Merge main into v1 #614

Closed

5 tasks

edoardopirovano deleted the split-create-analysis branch July 12, 2021 08:20

github-actions Bot mentioned this pull request Jul 12, 2021

Merge main into v1 #616

Merged

5 tasks

+                if (runStats && uploadStats) {
+                  await sendStatusReport(startedAt, { ...runStats, ...uploadStats });
+                } else if (runStats) {
+                  await sendStatusReport(startedAt, { ...runStats });

                     logger
                   );
+                  await runFinalize(outputDir, threads, config, logger);
+                  if (actionsUtil.getRequiredInput("skip-queries") !== "true") {

-                    await runAnalyze(
+                    const threads = getThreadsFlag(cmd.threads, logger);
+                    await runFinalize(outputDir, threads, config, logger);
+                    await runQueries(

               ## [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.

               ## [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)

Support splitting of DB creation and query execution by edoardopirovano · Pull Request #602 · github/codeql-action · GitHub

Support splitting of DB creation and query execution - #602

Support splitting of DB creation and query execution#602edoardopirovano merged 2 commits intogithub:maingithub/codeql-action:mainfrom edoardopirovano:split-create-analysisedoardopirovano/codeql-action:split-create-analysisCopy head branch name to clipboard

Conversation

edoardopirovano commented Jun 28, 2021

Merge / deployment checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Support splitting of DB creation and query execution#602
edoardopirovano merged 2 commits into
github:mainfrom
edoardopirovano:split-create-analysis