Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add an option to upload some debugging artifacts #798

Merged
merged 1 commit into from
Nov 1, 2021
Merged

Add an option to upload some debugging artifacts #798

merged 1 commit into from
Nov 1, 2021

Conversation

edoardopirovano
Copy link
Contributor

This PR adds a new option debug-artifacts to the analyze step which will result in the following files being uploaded as artifacts in order to aid debugging:

  1. Logs, including the tracer log for pre-multilanguage tracing. (After multi language tracing this is in the database logs, so does not need to be special cased).
  2. The bundle(s) for the CodeQL database(s) produced.
  3. The SARIF file(s) produced.

@edoardopirovano edoardopirovano added the enhancement New feature or request label Oct 28, 2021
@edoardopirovano edoardopirovano requested a review from a team as a code owner October 28, 2021 17:21
@adityasharad
Copy link
Contributor

What do you think about making this a catch-all debug option, potentially including other changes like increasing the verbosity of CLI commands?

@edoardopirovano
Copy link
Contributor Author

What do you think about making this a catch-all debug option, potentially including other changes like increasing the verbosity of CLI commands?

That sounds like a sensible idea, yes. For this PR let's stick to just uploading artifacts, but we can name it debug to give us room to have that flag do more things in future.

Follow-up question - if it's a general debugging flag should it go in the init step instead? I think that would make sense in case we want to debug something from before the analyze step.

aeisenberg
aeisenberg reviewed Nov 1, 2021
View reviewed changes
src/analyze-action.ts
@@ -106,6 +108,42 @@ async function run() {
config,
logger
);

if (config.debugMode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a danger of running with debugMode === true on the runner? Presumably, if so, the runner would fail on the upload stage, but it would be nice if we could detect this and fail faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, this will be caught (if it is indeed a problem) by updating the unguarded-action-lib.ql query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should never be possible to be in debug mode when using the runner. In particular, note that the call to initConfig in runner.ts has gained a hard-coded value of false for the parameter specifying whether we are in debug mode.

src/analyze-action.ts
@@ -1,6 +1,7 @@
import * as fs from "fs";
import * as path from "path";

import * as artifact from "@actions/artifact";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the unguarded-action-lib.ql query should be updated to include calls to @actions/artifact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm misunderstanding that query, it will already be including these calls as they match

codeql-action/queries/unguarded-action-lib.ql

Line 44 in 4293754

getImportedPath().getValue().matches("@actions/%") and

and are not specifically included in the whitelist

codeql-action/queries/unguarded-action-lib.ql

Lines 17 to 20 in 4293754

lib = "@actions/http-client" or
lib = "@actions/exec" or
lib = "@actions/io" or
lib.matches("@actions/exec/%")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep...sorry...you're right here.

src/analyze-action.ts Outdated Show resolved Hide resolved
src/analyze-action.ts
async function uploadDebugArtifact(toUpload: string[], rootDir: string) {
await artifact.create().uploadArtifact(
DEBUG_ARTIFACT_NAME,
toUpload.map((file) => path.normalize(file)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm...artifacting lots of little files (eg- a database) can take a really long time. It looks like you're doing this in several places. Much better to zip things up first and then upload.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look and see if this really is a problem in the action, but I've encountered it before in semmle-code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you may already be zipping things, so maybe this is a noop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed a no-op, I think. The only place this a concern is databases since, as you say, they contain lots of little files. These are being zipped up using codeql database bundle. The SARIF and some of the logs (those that exist outside of a database) are being uploaded unzipped, but that is okay because they are only a couple of files.

@aeisenberg
Copy link
Contributor

Updating the query can happen in a different PR.

aeisenberg
aeisenberg reviewed Nov 1, 2021
View reviewed changes
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. All my comments turned into noops. Thanks for explaining. The code looks fine to me. Just need to address merge conflicts and 🚢 .

@edoardopirovano
Copy link
Contributor Author

Thanks! Have squashed and rebased to address the merge conflicts.

@edoardopirovano edoardopirovano merged commit 3ba4184 into github:main Nov 1, 2021
@edoardopirovano edoardopirovano deleted the debug-mode branch November 1, 2021 16:46
@adityasharad
Copy link
Contributor

I think this needs a changelog entry.

@github-actions github-actions bot mentioned this pull request Nov 4, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeisenberg aeisenberg aeisenberg approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edoardopirovano @adityasharad @aeisenberg