Move logs, SARIF, database bundle actions uploads to post: hooks

[angelapwen]

Previously, even with debug mode on, if the init step failed we did not upload the appropriate Actions artifacts. This was because the artifacts were only uploaded in the analyze step.

This change:

• moves the uploading of log files to a post: hook in the init step. Regardless of whether the entire workflow was successful or if any steps after init failed, we will upload whatever logfiles we have as an artifact.
• moves the uploading of database bundles to a post: hook in the init step. If the database has not been finalized, instead of running the CLI database bundle command, the action directly zips up everything in the database directory.
• moves the uploading of SARIF results (if they exist) to a post: hook in the analyze step.

So far the change has been manually tested so that:

• when init fails, partial logs from init are uploaded as artifacts, and the partial database bundle is uploaded.
• when analyze fails, logs from init and analyze are uploaded as artifacts; the partial database bundle is uploaded; and the SARIF upload succeeds if there is a SARIF file generated in the output folder of the action.
• on workflow success, all file types are successfully uploaded.

Testing strategy:

• unit tests for the scripts called by post: hooks
• integration tests showing what happens when init and analyze fail; will write after Bump @actions/core from 1.8.0 to 1.9.1 in /packages/artifact actions/toolkit#1160 is merged.

Merge / deployment checklist

• Confirm this change is backwards compatible with existing workflows.
• [N/A] Confirm the readme has been updated if necessary.
• Confirm the changelog has been updated if necessary.

[angelapwen]

I'm correctly hitting the case where the database isn't finalized now, and just need to implement the zipping of the partial database files. I see documentation for the CLI command but am still not exactly sure which files I'm looking to zip. I'll take a look at the CLI source code but would appreciate any pointers!

Also, I see I have a CI failure on debug artifact which looks legitimate, as that's the code I'm changing. It only fails on ubuntu, as for some reason the step to download the artifact doesn't begin on the Mac runners. It looks like this is because the step to download the artifacts is executed before the post: hooks that upload them, so I'll have to find out how to reorder these.

I see the PR check comes from

codeql-action/pr-checks/checks/debug-artifacts.yml

Line 16 in b100b75

- uses: actions/download-artifact@v3

which executes directly after the analyze step. Not entirely sure how to make sure it executes only after the post: hooks as I guess the entire point is that the post: hooks happen after everything else. 🤔

[angelapwen]

I think perhaps splitting the PR check file into 2 jobs, one for running everything before checking artifact downloads, and one for just checking artifact downloads with a needs dependency on the former, might force things into the right order. Similar to the code blocks in the docs at https://docs.github.com/en/actions/learn-github-actions/essential-features-of-github-actions. I'll try to do this in a separate PR and see if it's at least a no-op without these new post: hook changes.

[aeisenberg]

What happens if matrix is not valid json? How is the error handled?

[angelapwen]

Hm — I actually didn't write this (just moved it to another section of the codebase) but that's a good question. I had thought that it was forced to be JSON from where it was declared in inputs:

codeql-action/init/action.yml

Lines 14 to 15 in 19d025e

	matrix:
	default: ${{ toJson(matrix) }}

and

https://github.com/github/codeql-action/blob/main/analyze/action.yml#L67-L68

but it seems like that's the default input and the user could theoretically have passed non-valid JSON.

I am adding a catch block with an error message indicating that there was an error parsing the matrix input and the debug artifact will be uploaded without the matrix input in its name. It seems like the only thing this block of code does is add a suffix to the artifact name, for every value in the matrix input, so it should be able to still upload the artifact correctly (just without the appropriate suffix(es).

[aeisenberg]

Same comment here about making this module as small as possible, and passing in actionsUtil.uploadDebugArtifacts as a function parameter.

[angelapwen]

I've made most of the requested changes, and will work on the refactoring for unit tests.

I also made the change to zip up the database bundle and it seems to be working as expected on a failed run.

The core.info logs state:

db-javascript is not finalized. Uploading partial database bundle at /home/runner/work/_temp/codeql_databases/db-javascript-partial.zip...

and then the zip file is uploaded as part of the artifacts. I can't attach it here due to file size limits but here is the directory structure of the partial database bundle file when unzipped:

[angelapwen]

I added //TODOs in locations where I am planning to unit test, with the description of what each unit test should do, to get feedback on the planned tests before/while I write them.

[adityasharad]

Good work! The overall logic looks sensible -- suggestions mostly about clarity and readability.

Let me know when you've added some tests and would like another look.

[adityasharad]

We already use the zlib library; I think we could reuse that in this situation rather than adding a dependency. One difference is that zlib produces gzip.

[angelapwen]

Ah interesting. I will give this a try!

[angelapwen]

Ah, I remembered that I had some trouble with zlib when I tried it out earlier because it seems to only compress streams of data, and it makes preserving the directory structure difficult. I looked around 👀 and see another package that can create zipped directories using zlib for compression: https://github.com/archiverjs/node-archiver

But as this would also add a new dependency I'm not sure if it's worthwhile 🤔

[adityasharad]

Let's stick with what you've got for now. There is an astounding number of ways to do this in the npm ecosystem, and I don't have the expertise/familiarity to know whether any one is best, so I won't hold this up. adm-zip is widely used and has no other dependencies, which is nice. (In the VS Code extension I see we use zip-a-folder.)

[angelapwen]

Quickly closing and re-opening to trigger new PR checks from actions/toolkit#1160

[adityasharad]

That looks good. The other option I considered was turning this workflow off on PRs, and only running it on push events. Let's keep this for now, and if the "failing" check confuses other contributors we can adjust it.

[adityasharad]

Some minor suggestions while you polish up the tests.

[adityasharad]

Let's stick with what you've got for now. There is an astounding number of ways to do this in the npm ecosystem, and I don't have the expertise/familiarity to know whether any one is best, so I won't hold this up. adm-zip is widely used and has no other dependencies, which is nice. (In the VS Code extension I see we use zip-a-folder.)

[aeisenberg]

For easier testing, you can move this function to a different file and test that file instead. This way, you avoid invoking runWrapper by default.

[angelapwen]

I've done this, though I didn't know what to call the other file (not sure what patterns we want to follow). I called it analyze-action-post-helper but would like to hear what you think!

[aeisenberg]

That's unfortunate and avoids a good use case for codeql database bundle. Do you know why it throws? I wonder if we could add a --force option (or something similar) to allow the command to succeed even if the database is malformed.

[angelapwen]

I did a little digging into the CLI source code, but I thought it'd be too complex to somehow catch the error thrown in the action and then do something different. A --force option would be interesting though and isn't something I'd considered.

We'd like to get this change in this week but I can write up a follow-up issue on the --force option on the CLI side for our backlog.

[aeisenberg]

Right...there's no need to get this done now.

[angelapwen]

The remaining unit tests are up, requesting final review now 😄

After this is merged, I will add the "Download and check debug artifacts after failure in analyze" PR check/job to the required PR checks for each branch. I'll also write up an issue on improving the codeql database bundle command to handle non-finalized databases.

[adityasharad]

Looks good!

[adityasharad]

You may want to save the old value and restore it at the end of this test, so it doesn't affect the environment for later tests. Or turn it into a stub.

[angelapwen]

Hm — I see it used this way without restoring in several other existing tests like

codeql-action/src/config-utils.test.ts

Line 626 in a6d0901

process.env["RUNNER_TEMP"] = tmpDir;

Is there a reason this one would act differently from other tests?

[aeisenberg]

Since it's used elsewhere in this way, it's probably fine. I would recommend in the future that we do what Aditya suggests in all the tests. The danger if we don't do this is there may be hard to spot dependencies between tests that use this environment variable.

[angelapwen]

I see we also have this helper function that is called quite a lot

codeql-action/src/testing-utils.ts

Line 102 in a6d0901

export function setupActionsVars(tempDir: string, toolsDir: string) {

although we don't need the other two environment variables here.

[angelapwen]

Hm, does this line

codeql-action/src/testing-utils.ts

Line 96 in cade2b5

process.env = t.context.env;

effectively reset the environment vars after each test?

[aeisenberg]

Yes...it does. I also see sinon.restore();, which makes my comment about using a sandbox irrelevant.

[angelapwen]

On a related note, I see that in some places we use ${STUB}.restore() to clean up at the end of a test, but in others we don't. Is this necessary or best practice?

[angelapwen]

Same as above, I think this line

codeql-action/src/testing-utils.ts

Line 93 in cade2b5

sinon.restore();

resets the sinon stubs after each test?

[aeisenberg]

Actually...we should do that everywhere. The vscode extension uses a different (and better) idiom, which we should move to in the action.

Instead of creating stubs directly using sinon.stub, use a sandbox:

  let sandbox: sinon.SinonSandbox;
  beforeEach(() => {
    sandbox = sinon.createSandbox();
  });
  afterEach(() => {
    sandbox.restore();
  });

And create stubs using: sandbox.stub(). This ensures all stubs are removed after every test runs. In the action, we are not doing this. So far, it hasn't caused any problems, but it might later.

[angelapwen]

For now it looks like as long as we call

codeql-action/src/testing-utils.ts

Line 49 in cade2b5

export function setupTests(test: TestFn<any>) {

in each test file it should call sinon.restore() and also restore the environment variables, right?

[aeisenberg]

Yes....I missed that. So, you can ignore my comment.

[angelapwen]

Ok, great! This was educational 🧑‍🎓

[aeisenberg]

Since it's used elsewhere in this way, it's probably fine. I would recommend in the future that we do what Aditya suggests in all the tests. The danger if we don't do this is there may be hard to spot dependencies between tests that use this environment variable.

[aeisenberg]

Right...there's no need to get this done now.