GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — Use `setup-codeql` logic for finding release assets in `start-proxy` by mbg · Pull Request #4198 · github/codeql-action Conversation
|
| }, | ||
| ); | ||
|
|
||
| for (const [apiURL, repository] of uniqueDownloadSources) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is just a URL, not an API URL, i.e. it doesn't start with api.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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:
potentialDownloadSources that is currently possible.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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, thanks for the explanation! I agree the code indeed currently does the right thing, but that hardening makes sense.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Couple of additional comments
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Couple of small things, and there are some conflicts to address, but otherwise this looks good to me!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
This reverts commit 909fda1.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Repository NWO validation accepts malformed values that can fail later outside the intended validation handling.
🧠 Review effort: Balanced
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.