Use setup-codeql logic for finding release assets in start-proxy

[mbg]

The start-proxy Action currently uses its own getReleaseByVersion function to get hold of the release that we expect to find the proxy assets at. That function has hard-coded owner and repo values for the default github/codeql-action repository. This repository may not be available in all environments. In such environments, getReleaseByVersion fails and we fall-back to the hard-coded release obtained from getFallbackUrl instead.

Broadly, this PR does the following:

• getCodeQLActionRepository and getCodeQLBundleDownloadURL are refactored out of setup-codeql into a new setup/repository module.
• We then use getCodeQLAssetDownloadURL (renamed from getCodeQLBundleDownloadURL) in the start-proxy action to apply the same logic for finding a suitable release as used by setup-codeql.
• The hard-coded fallback has been removed. The default URL returned by getCodeQLBundleDownloadURL is essentially equivalent, except that it uses the linked version rather than a hard-coded one. We have been shipping proxy artifacts successfully with releases for a good while now, so the fallback shouldn't be needed anymore.

There are some drive-by improvements to logging and assertions for sinon stubs.

Risk assessment

For internal use only. Please select the risk level of this change:

• Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

• Managed - Impacts users with dynamic workflows (Default Setup, Code Quality, ...).

Products:

• Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
• Code Quality - The changes impact analyses when analysis-kinds: code-quality.
• Other first-party - The changes impact other first-party analyses.

Environments:

• Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.
• GHES - Impacts CodeQL workflows on GitHub Enterprise Server.

How did/will you validate this change?

• Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
• End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

• Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

• Telemetry - I rely on existing telemetry or have made changes to the telemetry.
  • Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
  • Alerts - New or existing monitors will trip if something goes wrong with this change.

Are there any special considerations for merging or releasing this change?

• No special considerations - This change can be merged at any time.

Merge / deployment checklist

• Confirm this change is backwards compatible with existing workflows.
• Consider adding a changelog entry for this change.
• Confirm the readme and docs have been updated if necessary.

[henrymercer]

This is just a URL, not an API URL, i.e. it doesn't start with api.

[mbg]

The name was just copied from setup-codeql.ts, but I have renamed this to instanceURL to address the feedback. While doing that I also noticed that apiURL /instanceURL isn't actually used at all beyond the short-circuit check and in log messages. That seems like it may have been an oversight in your PR when you added this, because without this we just end up checking for the repos on whatever instance getApiClient decides to use instead of the intended apiURL /instanceURL?

[henrymercer]

without this we just end up checking for the repos on whatever instance getApiClient decides to use instead of the intended apiURL /instanceURL?

Sorry, could you explain what you mean by this? The intent is to check the called Action repo on the same GH instance, the canonical github/codeql-action on the same GH instance, followed by github/codeql-action on Dotcom.

[mbg]

Suppose that we are on GHE with GITHUB_SERVER_URL being something like https://my-org.ghe.com and that GITHUB_ACTION_REPOSITORY is test-org/my-codeql-action-fork, then we have the following (expected/intended) value for potentialDownloadSources:

[
  ["https://my-org.ghe.com", "test-org/my-codeql-action-fork"],
  ["https://my-org.ghe.com", "github/codeql-action"],
  ["https://github.com", "github/codeql-action"]
]

The for (const [instanceURL, repository] of uniqueDownloadSources) loop, however, never explicitly uses instanceURL for getReleaseByTag. The getApiClient call makes its own call to getApiDetails(env) which will always get it the GitHubApiDetails based on GITHUB_SERVER_URL and GITHUB_API_URL.

This happens to work as expected, because the only entry in potentialDownloadSources that would want different GitHubApiDetails is the last element of the array and we short-circuit the getReleaseByTag call there to exit the loop and return the result of getDefaultDotcomDownloadURL.

However, there are two problems with this:

1. It is not immediately obvious that this happens to work out for any potentialDownloadSources that is currently possible.
2. If we ever make a change that allows an additional entry in potentialDownloadSources where the instance URL doesn't match GITHUB_SERVER_URL, or the repository name is not GITHUB_ACTION_REPOSITORY if the instance URL is GITHUB_DOTCOM_URL, then this leads to a hard-to-troubleshoot issue where the short-circuit check doesn't apply and we instead call getReleaseByTag with the wrong GitHubApiDetails.

I'd suggest we either change this so that we configure the api client correctly in the loop for the instance given by instanceURL, to avoid shooting ourselves in the foot in the future. Alternatively, we should document this very explicitly and perhaps add a test to make sure we never end up with this scenario. I'll have a look.

[henrymercer]

Beyond your later fix in f9a9962, I think this should just be calledWith, since unlike calledOnce / calledOnceWithExactly / calledOnceWithMatch we don't validate that the stub was only called once.

[mbg]

I am unsure if your comment here is a request to change anything, since f9a9962 has addressed this in the way I had intended.

[henrymercer]

The matchers calledOnce, calledOnceWithExactly, and calledOnceWithMatch in sinon each require exactly one call in total, irrespective of the match, whereas this matcher permits the function to be called multiple times. So for consistency I think we should drop the once from the name of this matcher in favour of something like calledWith or calledAtLeastOnceWith.

[mbg]

[..] whereas this matcher permits the function to be called multiple times.

It doesn't: the t.deepEqual(actual, [expected]) puts expected into a singleton array. It should only match if stub was called exactly once.

To test this as well, I made a local change to the "cleanup removes only old CodeQL TRAP caches" test which has an existing t.is(deleteStub.callCount, 2) assertion. Adding the following:

// Existing assertion
t.is(deleteStub.callCount, 2);

// Locally added new assertion
const st = addSinonAssertions(t);
st.onceWith(deleteStub, 4);

Results in a test failure, as expected:

cleanup removes only old CodeQL TRAP caches

  src/trap-caching.test.ts:281

   280:     const st = addSinonAssertions(t);
   281:     st.onceWith(deleteStub, 4);
   282:

  Difference (- actual, + expected):

    [
      [
        4,
      ],
  -   [
  -     3,
  -   ],
    ]

[henrymercer]

Couple of additional comments