Add a CLI interface to the upload-sarif action #147

robertbrignull · 2020-08-10T17:11:23Z

This PR is a first attempt at adding a command lint interface to the codeql-action. This PR only deals with the upload-sarif action, and largely ignores the others except the minimum changes required to keep them working.

The main things done here are:

Introduce src/cli.ts which is the entrypoint to the CLI. I'm using the commander library here to do the CLI part, and it generates -h options and stuff like that. I'm open to other libraries instead if someone has a favourite that is better.
Introduce a new build-cli npm script, that uses webpack to bundle the cli.ts file and all its dependencies into a single file, that is output to cli/cli.js. This file is not checked into the repository as it isn't needed to run the action normally. I felt it therefore didn't belong in the lib directory, but I don't think the cli directory name is great either. I'd be very open to suggestions here.
Introduce logging.ts which mostly just wraps the @actions/core package, and replaces it with stuff like console.log when not on actions.
Adds lots of extra function parameters. I think the way we're going to have to do things, is pass parameters from the entrypoint all the way into the functions that need them. That way the shared code doesn't have to have different cases for actions vs the CLI. At the entrypoints we pass the inputs, either from command line args, or core.getInput, or the environment, and pass it onwards to where it is needed. Hopefully this won't get too ugly.

The diff looks huge because of node_modules changes, but if you ignore those and other generated files, the actual churn is only about 300 lines.

This code is largely untested in practice. I wanted to get the PR up soon so people could see what it looks like.

Merge / deployment checklist

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

joshhale · 2020-08-11T09:13:12Z

I can't jump to file to see the significant files because there are too many in the PR, and I can't see, e.g. src/cli.ts by searching in the files page. Would it help to open a dummy PR with only the significant files?

robertbrignull · 2020-08-11T09:32:42Z

I can't jump to file to see the significant files because there are too many in the PR, and I can't see, e.g. src/cli.ts by searching in the files page. Would it help to open a dummy PR with only the significant files?

I'll reorganise so the node_modules changes are in a separate commit. That way you should be able to view it in the UI.

robertbrignull · 2020-08-11T09:34:45Z

I'm doing some testing and have found some bits I've missed, so I'm fixing those up and will push a new version shortly.

robertbrignull · 2020-08-11T11:47:22Z

I've now tested this manually, which showed up a few bugs and odditites. I managed to upload to one of my testing repos with

node cli/cli.js upload --sarif-file path/to/example.sarif --repository dsp-testing/robertbrignull_test --commit 6548b467960719daec4cf3cca3e59b85f69fc0e1 --ref refs/heads/master --github-url https://api.github.com --github-auth robertbrignull:<PAT> --analysis-key foo

Unfortunately everything has become a bit more annoying since there are different upload endpoints for actions vs non-actions, so there's a bit of ugly code there to generate the right payload and upload it to the right place. I'm not sure what the best approach is to avoid duplicating too much code or keep it safe so we won't accidentally do the wrong thing in the wrong environment.

robertbrignull · 2020-08-11T11:48:26Z

@sampart would you be able to review this? You'll have to look at individual commits and ignore the one that's got all the node_modules and generated file changes in it.

robertbrignull · 2020-08-11T12:52:22Z

src/cli.ts

+  .requiredOption('--repository <repository>', 'Repository name')
+  .requiredOption('--commit <commit>', 'SHA of commit that was analyzed')
+  .requiredOption('--ref <ref>', 'Name of ref that was analyzed')
+  .requiredOption('--analysis-key <key>', 'Identifies the analysis, for use matching up equivalent analyses on different commits')


A bunch of these options are not used. That's a bug from changing definitions over time. I'll trim these down.

I've pushed a commit to remove all of these that aren't used in CLI mode. It makes the upload_lib.upload interface a bit silly though as lots of the parameters are nullable so the type system isn't giving us much.

Maybe we could do something better by passing all the options as a blob along the with mod actions/cli mode variable.

I like the idea of passing as an object, so you effectively get named parameters 👍

sampart · 2020-08-11T14:42:46Z

package.json

+    "ts-loader": "8.0.2",
+    "typescript": "^3.7.5",
+    "webpack": "4.44.1",
+    "webpack-cli": "3.3.12"


Suggested change

"ts-loader": "8.0.2",

"typescript": "^3.7.5",

"webpack": "4.44.1",

"webpack-cli": "3.3.12"

"ts-loader": "^8.0.2",

"typescript": "^3.7.5",

"webpack": "^4.44.1",

"webpack-cli": "^3.3.12"

sampart · 2020-08-11T14:45:01Z

package.json

    "@actions/github": "^2.2.0",
    "@actions/http-client": "^1.0.8",
    "@actions/tool-cache": "^1.5.5",
+    "commander": "6.0.0",


Suggested change

"commander": "6.0.0",

"commander": "^6.0.0",

Unless you've a reason to pin to specific versions, I would allow upgrades of minor versions. This will make it easier for people to upgrade in future - pinning to a specific version here is like saying "there's a reason we can't use a later version", IMHO.

As you noted this doesn't make too much difference to use day-to-day as we're using package-lock.json to pin to specific versions anyway and the node_modules folder is checked into the repo. So yes I suppose it makes the intention slightly more clear when it comes to upgrading.

sampart · 2020-08-11T14:49:21Z

src/cli.ts

+program
+  .command('upload')
+  .description('Uploads a SARIF file, or all SARIF files from a directory, to code scanning')
+  .requiredOption('--sarif-file <file>', 'SARIF file to upload')


Suggested change

.requiredOption('--sarif-file <file>', 'SARIF file to upload')

.requiredOption('--sarif-file <file>', 'SARIF file to upload; can also be a directory for uploading multiple')

sampart · 2020-08-11T15:05:30Z

src/cli.ts

+  .requiredOption('--repository <repository>', 'Repository name')
+  .requiredOption('--commit <commit>', 'SHA of commit that was analyzed')
+  .requiredOption('--ref <ref>', 'Name of ref that was analyzed')
+  .requiredOption('--analysis-key <key>', 'Identifies the analysis, for use matching up equivalent analyses on different commits')


I like the idea of passing as an object, so you effectively get named parameters 👍

sampart

Nice work! It's particularly gratifying how small cli.ts is. A fair few comments, but nothing major.

sampart · 2020-08-11T15:06:20Z

src/logging.ts

@@ -0,0 +1,26 @@
+import * as core from '@actions/core';
+
+export interface Logger {


I really like your approach here 🏅

sampart · 2020-08-11T15:13:46Z

src/upload-lib.ts

-  if (fs.lstatSync(input).isDirectory()) {
-    const sarifFiles = fs.readdirSync(input)
+export async function upload(
+  sarifFile: string,


How about calling this sarifPath, since it could be a file or a directory? I'm not particularly fussed about this, though, so happy to leave if you'd rather.

sampart · 2020-08-11T15:18:11Z

webpack.config.js

+		// We no not want to minimize our code.
+		minimize: false
+	},


Suggested change

// We no not want to minimize our code.

minimize: false

},

// We do not want to minimize our code.

minimize: false

},

You probably don't even need the comment:

Suggested change

// We no not want to minimize our code.

minimize: false

},

minimize: false

},

sampart · 2020-08-11T15:22:53Z

src/upload-lib.ts

-
-  const sentinelEnvVar = "CODEQL_UPLOAD_SARIF";
-  if (process.env[sentinelEnvVar]) {
-    throw new Error("Aborting upload: only one run of the codeql/analyze or codeql/upload-sarif actions is allowed per job");


You're now allowing multiple uploads in a job. Is that okay? Another option might be to clear this env var every time the CLI launches?

Thanks for spotting this. Removing this wasn't intentional.

It is worth pointing out the check won't work when running in CLI mode because we can't set env vars in the same way such that they are seen by subsequent processes. But it should be safe to leave in and it'll just only do something on actions.

That's what I thought 👍

sampart · 2020-08-11T15:25:26Z

src/upload-lib.ts

+      "sarif": zipped_sarif,
+      "workflow_run_id": workflowRunID,
+      "checkout_uri": checkoutURI,
+      "environment": environment,


You don't have started_at anymore. Is that deliberate?

Thanks. That also was not intentional.

sampart · 2020-08-11T15:26:37Z

src/upload-lib.ts

  // Log some useful debug info about the info
  const rawUploadSizeBytes = sarifPayload.length;
-  core.debug("Raw upload size: " + rawUploadSizeBytes + " bytes");
+  console.debug("Raw upload size: " + rawUploadSizeBytes + " bytes");


I think you want logger.debug here and below.

sampart · 2020-08-11T15:30:14Z

Because I added my comments to the commit rather than the PR-level diff, some comments are showing as "outdated". I don't think they are all genuinely redundant, though (although some are), so mind you don't miss them 😉

robertbrignull · 2020-08-12T16:56:28Z

I've added a simple workflow that builds and runs the CLI to upload an empty SARIF file. You can still run the CLI on actions, so I think this should be ok for making sure we don't accidentally break it too much.

robertbrignull · 2020-08-12T17:11:56Z

Ok, fixed the error handling so it actually now sets a non-zero exit code when there's an error, and fixed a typo in a filename. Good thing I made that typo otherwise might not have noticed the broken error handling.

With the exit code I tried a few variations of re-throwing the error, or putting the try-catch around the call to program.parse but they didn't work. So far putting the catch in the sub-action and setting process.exitCode was the only thing that worked.

swinton

👋 Hi, just added a few comments / suggestions around how an end-user would acquire this CLI 🙇

swinton · 2020-08-13T15:46:55Z

package.json

  "scripts": {
    "build": "tsc",
+    "build-cli": "webpack --mode production",
    "test": "ava src/** --serial --verbose",
    "lint": "tslint -p . -c tslint.json 'src/**/*.ts'",
    "removeNPMAbsolutePaths": "removeNPMAbsolutePaths . --force"


Suggestion: add a postinstall step that builds the CLI automatically after a successful npm install. This will make it easier for new users to acquire the CLI by reducing the number of installation steps.

E.g.

Suggested change

"scripts": {

"build": "tsc",

"build-cli": "webpack --mode production",

"test": "ava src/** --serial --verbose",

"lint": "tslint -p . -c tslint.json 'src/**/*.ts'",

"removeNPMAbsolutePaths": "removeNPMAbsolutePaths . --force"

"scripts": {

"build": "tsc",

"build-cli": "webpack --mode production",

"test": "ava src/** --serial --verbose",

"lint": "tslint -p . -c tslint.json 'src/**/*.ts'",

"removeNPMAbsolutePaths": "removeNPMAbsolutePaths . --force",

"postinstall": "npm run-script build-cli"

swinton · 2020-08-13T15:50:09Z

package.json

  "version": "0.0.0",
  "private": true,
  "description": "CodeQL action",
  "scripts": {


Suggestion: add a bin property to package.json, which resolves to the new CLI being introduced. This will allow new users to run the CLI directly, e.g. via npx ....

Suggested change

"scripts": {

"bin": {

"codeql-cli": "cli/code-scanning-cli.js"

},

"scripts": {

swinton · 2020-08-13T15:57:00Z

webpack.config.js

+  output: {
+    filename: 'cli.js',
+    path: path.resolve(__dirname, 'cli'),
+  },


Is is possible to make this output executable, so that it can be invoked directly from the command-line?

robertbrignull · 2020-08-13T17:03:39Z

I like the idea of passing as an object, so you effectively get named parameters

Sorry I forgot to do this. I still think it would be good, but perhaps better to delay to a separate PR so we can discuss it properly. It could be a pretty big change in direction.

robertbrignull · 2020-08-13T17:04:59Z

Thanks for the comments @swinton. How to distribute this is still largely an open question and not one we have to solve right now, so I think I'm going to merge this without any changes for now and we'll address it later. I've mentioned you on an issue for this so we can discuss there.

robertbrignull added 2 commits August 11, 2020 12:43

Add a CLI interface to the upload-sarif action

6d7a135

commit node_modules and generated files

34b3722

robertbrignull force-pushed the upload_cli branch from 3321083 to 34b3722 Compare August 11, 2020 11:44

robertbrignull assigned sampart Aug 11, 2020

robertbrignull commented Aug 11, 2020

View reviewed changes

remove unused arguments

cf08f5a

sampart reviewed Aug 11, 2020

View reviewed changes

sampart approved these changes Aug 11, 2020

View reviewed changes

robertbrignull added 2 commits August 12, 2020 17:42

address comments

5b0aafa

add test action

150e07f

robertbrignull added 2 commits August 12, 2020 18:06

Re-throw errors

4b56177

fix file location

2828c43

sampart approved these changes Aug 13, 2020

View reviewed changes

swinton reviewed Aug 13, 2020

View reviewed changes

robertbrignull merged commit 2527130 into main Aug 13, 2020

robertbrignull deleted the upload_cli branch August 13, 2020 17:05

github-actions bot mentioned this pull request Aug 17, 2020

Merge main into v1 #150

Merged

snyk-bot mentioned this pull request Feb 16, 2023

[Snyk] Upgrade sprintf-js from 1.0.3 to 1.1.2 Elsokary22/codeql-action#1

Open

aliscco mentioned this pull request Apr 25, 2023

[Snyk] Fix for 13 vulnerabilities aliscco/codeql-action#142

Open

	.requiredOption('--sarif-file <file>', 'SARIF file to upload')
	.requiredOption('--sarif-file <file>', 'SARIF file to upload; can also be a directory for uploading multiple')

		@@ -0,0 +1,26 @@
		import * as core from '@actions/core';

		export interface Logger {

Add a CLI interface to the upload-sarif action #147

Add a CLI interface to the upload-sarif action #147

Uh oh!

Conversation

robertbrignull commented Aug 10, 2020

Merge / deployment checklist

Uh oh!

joshhale commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertbrignull commented Aug 11, 2020

Uh oh!

robertbrignull commented Aug 11, 2020

Uh oh!

robertbrignull commented Aug 11, 2020

Uh oh!

robertbrignull commented Aug 11, 2020

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!

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!

sampart 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!

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!

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!

sampart commented Aug 11, 2020

Uh oh!

robertbrignull commented Aug 12, 2020

Uh oh!

robertbrignull commented Aug 12, 2020

Uh oh!

swinton 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!

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Aug 13, 2020

Uh oh!

robertbrignull commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

joshhale commented Aug 11, 2020 •

edited

Loading