FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add an option to upload some debugging artifacts by edoardopirovano · Pull Request #798 · github/codeql-action · GitHub

/ codeql-action Public

Add an option to upload some debugging artifacts - #798

Merged
edoardopirovano merged 1 commit into
github:mainfrom
edoardopirovano:debug-mode
Nov 1, 2021
ConversationCommits (1)ChecksFiles changed
Merged

Add an option to upload some debugging artifacts#798
edoardopirovano merged 1 commit into
github:mainfrom
edoardopirovano:debug-mode

Conversation

edoardopirovano commented Oct 28, 2021

Copy link
Copy Markdown
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 added the enhancement New feature or request label Oct 28, 2021
edoardopirovano requested a review from a team as a code owner October 28, 2021 17:21

adityasharad commented Oct 28, 2021

Copy link
Copy Markdown
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 commented Oct 28, 2021

Copy link
Copy Markdown
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 reviewed Nov 1, 2021
View reviewed changes
Comment thread src/analyze-action.ts
logger
);

if (config.debugMode) {

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

edoardopirovano Nov 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread src/analyze-action.ts
import * as fs from "fs";
import * as path from "path";

import * as artifact from "@actions/artifact";

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

edoardopirovano Nov 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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/%")

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aeisenberg Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

edoardopirovano Nov 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 1, 2021

Copy link
Copy Markdown
Contributor

Updating the query can happen in a different PR.

aeisenberg reviewed Nov 1, 2021
View reviewed changes

aeisenberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

edoardopirovano commented Nov 1, 2021

Copy link
Copy Markdown
Contributor Author

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

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

adityasharad commented Nov 2, 2021

Copy link
Copy Markdown
Contributor

I think this needs a changelog entry.

github-actions Bot mentioned this pull request Nov 4, 2021
Merged
5 tasks
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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.

4 participants

Back | FazBrowse Home | New Git URL