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 private registry proxy for API requests when available by mbg · Pull Request #4007 · github/codeql-action Conversation
|
|
As an example of this in action, see https://github.com/github/codeql-action/actions/runs/29347975179/job/87138397597?pr=4007#step:8:312 for the expected log message and the proxy log artifact at https://github.com/github/codeql-action/actions/runs/29347975179/artifacts/8317105753 |
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.
Warning
Adds feature-flagged registry-proxy support for retrieving remote CodeQL configuration files from private repositories.
Changes:
| File | Description |
|---|---|
src/api-client.ts |
Implements proxy-aware API clients. |
src/api-client.test.ts |
Tests proxy configuration. |
src/config/file.ts |
Proxies remote configuration requests. |
src/config/file.test.ts |
Tests feature-flagged proxy selection. |
src/environment.ts |
Defines proxy variables and environment cloning. |
src/feature-flags.ts |
Adds the proxy feature flag. |
src/testing-utils.ts |
Isolates environments between test-builder clones. |
pr-checks/checks/start-proxy.yml |
Exercises proxy-backed initialization. |
package.json |
Adds Undici. |
package-lock.json |
Locks the dependency update. |
lib/entry-points.js |
Generated artifact; excluded from review. |
.github/workflows/__start-proxy.yml |
Generated workflow; excluded from review. |
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.
| // Should use it when the FF is enabled and the environment variables are set. | ||
| await target | ||
| .withFeatures([Feature.ProxyApiRequests, Feature.NewRemoteFileAddresses]) | ||
| .withEnv((env) => { | ||
| env.set(RegistryProxyVars.PROXY_HOST, "localhost"); | ||
| env.set(RegistryProxyVars.PROXY_PORT, "1234"); | ||
| }) | ||
| .logs(t, "Using private registry proxy at 'http://localhost:1234'") | ||
| .passes(t.truthy); | ||
|
|
||
| // But not when the FF is not enabled. | ||
| await target | ||
| .withFeatures([Feature.NewRemoteFileAddresses]) | ||
| .withEnv((env) => { | ||
| env.set(RegistryProxyVars.PROXY_HOST, "localhost"); | ||
| env.set(RegistryProxyVars.PROXY_PORT, "1234"); | ||
| }) | ||
| .notLogs(t, "Using private registry proxy at 'http://localhost:1234'") | ||
| .throws(t, { message: errorMessage }); | ||
|
|
||
| // And not when the environment variables aren't set. | ||
| await target | ||
| .withFeatures([Feature.ProxyApiRequests, Feature.NewRemoteFileAddresses]) | ||
| .notLogs(t, "Using private registry proxy at 'http://localhost:1234'") | ||
| .throws(t, { message: errorMessage }); | ||
| }); |
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.
I think that these should be three individual unit tests—per best-practice. But, that's quite a lot of boilerplate setup code to duplicate. You could put it into a helper setup method, but not crazy about that either. I'll leave that up to you whether to keep as-is, or split/refactor.
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.
I have no strong feelings here. I can break this up into three separate tests, but these can also be viewed as three separate assertions. Not duplicating the boilerplate isn't hard.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
This makes that function testable.
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.