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

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.

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)

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

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

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!

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