Repository navigation
Use setup-codeql logic for finding release assets in start-proxy - #4198
Conversation
362e7d4 to
cd87ccd
Compare
cd87ccd to
133ffeb
Compare
| }, | ||
| ); | ||
|
|
||
| for (const [apiURL, repository] of uniqueDownloadSources) { |
There was a problem hiding this comment.
This is just a URL, not an API URL, i.e. it doesn't start with api.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
without this we just end up checking for the repos on whatever instance
getApiClientdecides to use instead of the intendedapiURL/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.
There was a problem hiding this comment.
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:
- It is not immediately obvious that this happens to work out for any
potentialDownloadSourcesthat is currently possible. - If we ever make a change that allows an additional entry in
potentialDownloadSourceswhere the instance URL doesn't matchGITHUB_SERVER_URL, or the repository name is notGITHUB_ACTION_REPOSITORYif the instance URL isGITHUB_DOTCOM_URL, then this leads to a hard-to-troubleshoot issue where the short-circuit check doesn't apply and we instead callgetReleaseByTagwith the wrongGitHubApiDetails.
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.
There was a problem hiding this comment.
Ah I see, thanks for the explanation! I agree the code indeed currently does the right thing, but that hardening makes sense.
henrymercer
left a comment
There was a problem hiding this comment.
Couple of additional comments
henrymercer
left a comment
There was a problem hiding this comment.
Couple of small things, and there are some conflicts to address, but otherwise this looks good to me!
This reverts commit 909fda1.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
🟡 Changes recommended
Repository NWO validation accepts malformed values that can fail later outside the intended validation handling.
1 open finding
1 resolved since last review
Files excluded by content exclusion policy (1)
- lib/entry-points.js
🧠 Review effort: Balanced

The
start-proxyAction currently uses its owngetReleaseByVersionfunction to get hold of the release that we expect to find the proxy assets at. That function has hard-codedownerandrepovalues for the defaultgithub/codeql-actionrepository. This repository may not be available in all environments. In such environments,getReleaseByVersionfails and we fall-back to the hard-coded release obtained fromgetFallbackUrlinstead.Broadly, this PR does the following:
getCodeQLActionRepositoryandgetCodeQLBundleDownloadURLare refactored out ofsetup-codeqlinto a newsetup/repositorymodule.getCodeQLAssetDownloadURL(renamed fromgetCodeQLBundleDownloadURL) in thestart-proxyaction to apply the same logic for finding a suitable release as used bysetup-codeql.getCodeQLBundleDownloadURLis 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:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist