Split upload method into two mode-specific ones #323

sampart · 2020-11-26T11:53:25Z

Continuing to think about reducing our long argument lists, this PR splits the upload method into two mode-specific ones, which are called from the runner- or actions-specific code so that it's obvious which one to use without requiring a mode parameter. uploadFiles still has the optional parameters, but at least that's not an exported method.

runAnalyze now just does one thing - running an analysis, rather than having a side-effect of uploading.
11(!) arguments removed from runAnalyze

Merge / deployment checklist

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

sampart · 2020-11-26T11:55:34Z

src/analyze.ts

+  let uploadStats: upload_lib.UploadStatusReport;
+  if (mode == "actions") {
+    uploadStats = await upload_lib.uploadFromActions(
+      outputDir,
+      repositoryNwo,
+      commitOid,
+      ref,
+      analysisKey!,
+      analysisName!,
+      workflowRunID!,
+      checkoutPath,
+      environment!,
+      apiDetails,
+      logger
+    );
+  } else if (mode == "runner") {
+    uploadStats = await upload_lib.uploadFromRunner(
+      outputDir,
+      repositoryNwo,
+      commitOid,
+      ref,
+      checkoutPath,
+      apiDetails,
+      logger
+    );
+  } else {
+    throw new Error(`Unknown mode "${mode}"`);
+  }


This conditional (and the user of !) bothers me. I could avoid it by moving the calls to the upload* methods into analyze-action.ts and runner.ts. That would have the additional advantage of reducing the number of arguments for runAnalyze. Would you be happy with that change, @robertbrignull?

Yeah as you say, unfortunately splitting up the upload* methods hasn't really helped as it's just pushed the problem one method earlier so now we have a big switch in analyze.ts and have to worry about values being undefined here.

We could try moving the calls to upload to being in analyze-action.ts and runner.ts. You'd need to return some data from the analyze code but that should be fine. I don't know if it'll end up better overall but it would be good to try it and see how it looks.

I've moved the calls up in ff28c8d. I'm not sure how easy the overall diff of this PR is to read, I'm afraid.

Here are some of my thoughts:

Advantages

runAnalyze now just does one thing - running an analysis, rather than having a side-effect of uploading.

11(!) arguments removed from runAnalyze 🎊

We now see the benefits of the split out upload* methods - fewer parameters (and no undefineds) in the argument list in the runner (uploadFromRunner does have a similar situation, but that doesn't feel so bad to me).

Disadvantages

runAnalyze might only do one thing now, but the side effect has only moved as far as the calling methods. I think it's less bad that the runner method (as opposed to run in src/analyze-action.ts) does multiple things, though, since that's basically procedural code that does everything all in one place anyway.

Similarly, run in src/analyze-action.ts has now got even longer. It would be an interesting exercise to try and decompose it into smaller functions (not in this PR)

Likewise, the action in the runner is even longer too. Again decomposition would be an interesting exercise

What are your thoughts?

Robert and I discussed this on a call, and we're going to go with it. I've tweaked the PR description accordingly

Likewise, the action in the runner is even longer too. Again decomposition would be an interesting exercise

I've added this to https://github.com/github/code-scanning/issues/1874#issuecomment-728215299

#323 (comment)

sampart · 2020-12-22T11:37:23Z

src/analyze-action.ts

+      logger
+    );
+
+    if (actionsUtil.getRequiredInput("upload") !== "true") {


The uploading happens here now. Previously, it was a side-effect of runAnalyze, which was one of the reasons it needed so many params.

Likewise in runner.ts below.

sampart · 2020-12-22T11:54:11Z

src/upload-lib.ts

@@ -117,22 +160,7 @@ export async function upload(
  } else {
    sarifFiles.push(sarifPath);
  }
-
-  return await uploadFiles(


The diff is confusing here - this uploadFiles call has been replaced by the one on line 128 above. The differ doesn't realise that I've extracted the SARIF file path stuff into a new method (getSarifFilePaths)

robertbrignull

I had a few comments with minor points, but generally this is looking good. I do like splitting the upload method up and I think that part is a big benefit. Just some minor details to make sure we avoid changing behaviour.

robertbrignull · 2021-01-05T14:00:30Z

src/analyze-action.ts

+
+    if (actionsUtil.getRequiredInput("upload") !== "true") {
+      logger.info("Not uploading results");
+      return;


If you return here then it wont send the status report. Probably need to add an else branch instead of returning early.

Ooh, good spot. Fixed in 1da4ce5

robertbrignull · 2021-01-05T14:06:11Z

src/analyze-action.ts

@@ -68,7 +69,23 @@ async function run() {
      auth: actionsUtil.getRequiredInput("token"),
      url: actionsUtil.getRequiredEnvParam("GITHUB_SERVER_URL"),
    };
-    stats = await runAnalyze(


The AnalysisStatusReport type is still defined in analyze.ts but it doesn't need to be there as it's now only used from this file. You could just move that type definition to this file, or in fact if you want you could get rid of it and replace all uses with upload_lib.UploadStatusReport & QueriesStatusReport and it isn't too onerous.

Nice one. I've kept the type but moved it in 4bc186c

robertbrignull · 2021-01-05T14:08:02Z

src/upload-lib.ts

+  );
+}
+
+function getSarifFilePaths(sarifPath: string) {


There's a merge conflict here. You want to solve it by taking your version but then deleting the const sarifFiles: string[] = []; line, so it looks like

// Uploads a single sarif file or a directory of sarif files // depending on what the path happens to refer to. // Returns true iff the upload occurred and succeeded export async function uploadFromRunner( sarifPath: string, repositoryNwo: RepositoryNwo, commitOid: string, ref: string, checkoutPath: string, apiDetails: api.GitHubApiDetails, logger: Logger ): Promise<UploadStatusReport> { return await uploadFiles( getSarifFilePaths(sarifPath), repositoryNwo, commitOid, ref, undefined, undefined, undefined, checkoutPath, undefined, apiDetails, "runner", logger ); } function getSarifFilePaths(sarifPath: string) { if (!fs.existsSync(sarifPath)) { throw new Error(`Path does not exist: ${sarifPath}`); } let sarifFiles: string[]; if (fs.lstatSync(sarifPath).isDirectory()) { sarifFiles = findSarifFilesInDir(sarifPath); if (sarifFiles.length === 0) { throw new Error(`No SARIF files found to upload in "${sarifPath}".`); } } else { sarifFiles = [sarifPath]; } return sarifFiles; }

Thanks so much for this. I'd have been scratching my head for ages over the conflict otherwise!

robertbrignull · 2021-01-05T14:20:13Z

src/upload-lib.ts

@@ -276,6 +303,7 @@ async function uploadFiles(

  const toolNames = util.getToolNames(sarifPayload);

+  const gitHubVersion = await util.getGitHubVersion(apiDetails);


I can see the desire to move this here and avoid passing it through a few methods, however it does change behaviour slightly in that we'll query the github api for its version a second time instead of using the version we already obtained in the init action. That's why from the analyze action it would pass in the github version from the config, and from the upload-sarif action it would fetch a new copy because we don't have a config around.

Admittedly it's a very small cost to make one extra HTTP request but it's also a small cost to have an extra argument so it's hard to compare those two points. I don't think this argument comes under the original intent of the PR so unless you feel strongly I'd like to put this argument back for now.

We could look at ways of improving this situation but as another PR. Perhaps it could be included into GitHubApiDetails, but then that would complicate the construction of that object, so I think it's worth doing as a separate change. I'm also already suggesting changing that type in #357 though that PR may not be merged.

Reverted in 54e0c67

#323 (comment)

This reverts commit 6de1b75. #323 (comment)

54e0c67#r45693026 54e0c67#r45693030

github#323 (comment)

This reverts commit 6de1b75. github#323 (comment)

sampart added 2 commits November 26, 2020 11:18

Extract filepath retrieval to a method

7ae9b0d

Split upload method into two mode-specific ones

57514f3

sampart requested a review from robertbrignull November 26, 2020 11:53

sampart commented Nov 26, 2020

View reviewed changes

sampart added 4 commits November 26, 2020 11:57

Use better comparison operator

9532bda

Move uploading side-effect out of runAnalyze

ff28c8d

#323 (comment)

Merge branch 'main' into split-upload-method

82a8fa4

Fix linter violations

45dd5ee

sampart commented Dec 22, 2020

View reviewed changes

Retrieve GitHubVersion when used rather than passing around

6de1b75

sampart commented Dec 22, 2020

View reviewed changes

sampart marked this pull request as ready for review December 22, 2020 11:54

robertbrignull reviewed Jan 5, 2021

View reviewed changes

sampart added 6 commits January 6, 2021 10:52

Ensure stats get sent when not uploading results

1da4ce5

#323 (comment)

Move type definition to where it's used

4bc186c

#323 (comment)

Revert "Retrieve GitHubVersion when used rather than passing around"

54e0c67

This reverts commit 6de1b75. #323 (comment)

Merge branch 'main' into split-upload-method

4e46a49

Fix linter violations

023add5

Don't re-retrieve GitHub version when already have it

c7275a7

54e0c67#r45693026 54e0c67#r45693030

robertbrignull approved these changes Jan 6, 2021

View reviewed changes

sampart merged commit a0c4707 into main Jan 6, 2021

sampart deleted the split-upload-method branch January 6, 2021 12:02

github-actions bot mentioned this pull request Jan 11, 2021

Merge main into v1 #359

Merged

aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021

Move uploading side-effect out of runAnalyze

08f57b3

github#323 (comment)

aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021

Move type definition to where it's used

358599d

github#323 (comment)

aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021

Revert "Retrieve GitHubVersion when used rather than passing around"

6ab565c

This reverts commit 6de1b75. github#323 (comment)

This was referenced Apr 24, 2023

[Snyk] Fix for 21 vulnerabilities aliscco/codeql-action#85

Open

[Snyk] Fix for 23 vulnerabilities aliscco/codeql-action#232

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split upload method into two mode-specific ones #323

Split upload method into two mode-specific ones #323

sampart commented Nov 26, 2020 •

edited

Loading

sampart Nov 26, 2020 •

edited

Loading

robertbrignull Nov 27, 2020

sampart Nov 27, 2020

sampart Dec 22, 2020

sampart Dec 22, 2020 •

edited

Loading

sampart Dec 22, 2020

robertbrignull left a comment

robertbrignull Jan 5, 2021

sampart Jan 6, 2021

robertbrignull Jan 5, 2021

sampart Jan 6, 2021

robertbrignull Jan 5, 2021

sampart Jan 6, 2021

robertbrignull Jan 5, 2021

sampart Jan 6, 2021

		@@ -276,6 +303,7 @@ async function uploadFiles(

		const toolNames = util.getToolNames(sarifPayload);

		const gitHubVersion = await util.getGitHubVersion(apiDetails);

Split upload method into two mode-specific ones #323

Split upload method into two mode-specific ones #323

Conversation

sampart commented Nov 26, 2020 • edited Loading

Merge / deployment checklist

sampart Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampart Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbrignull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampart commented Nov 26, 2020 •

edited

Loading

sampart Nov 26, 2020 •

edited

Loading

sampart Dec 22, 2020 •

edited

Loading