Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the start-proxy action to use getDefaultCliVersion from feature flags instead of hard-coding defaults.bundleVersion. This change allows the start-proxy action to dynamically determine the CodeQL CLI version to use, either from feature flags on GitHub.com or from defaults on GHES, making the behavior consistent with other actions in the codebase.
Changes:
- Modified
getDownloadUrlandgetProxyBinaryPathfunctions to accept aFeatureEnablementparameter - Updated
start-proxy-action.tsto initialize and pass feature flags to proxy-related functions - Added comprehensive test coverage with proper feature flag mocking
- Removed unused
Featureimport fromstart-proxy-action.ts
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/start-proxy.ts | Updated function signatures to accept FeatureEnablement; changed to use getDefaultCliVersion for dynamic version selection; added getGitHubVersion import |
| src/start-proxy.test.ts | Updated test mocking to properly handle feature flags; added mockOfflineFeatures helper; modified tests to pass features parameter |
| src/start-proxy-action.ts | Removed unused Feature import; updated getProxyBinaryPath call to pass features parameter |
| lib/start-proxy-action.js | Generated JavaScript reflecting TypeScript changes |
Comments suppressed due to low confidence (1)
src/start-proxy.ts:558
- The JSDoc comment for
getProxyBinaryPathis missing documentation for the newfeaturesparameter. Please add a@param featuresentry to document this parameter.
/**
* Gets a path to the proxy binary. If possible, this function will find the proxy in the
* runner's tool cache. Otherwise, it downloads and extracts the proxy binary,
* and stores it in the tool cache.
*
* @param logger The logger to use.
* @returns The path to the proxy binary.
*/
| @@ -383,33 +408,48 @@ test("getDownloadUrl returns fallback when `getLinkedRelease` rejects", async (t | |||
| }); | |||
There was a problem hiding this comment.
The test "getDownloadUrl returns fallback when getReleaseByVersion rejects" needs to be updated to properly test the new code path. The issue is that createFeatures([]) returns a feature object where getDefaultCliVersion throws "not implemented", but getGitHubVersion() is not mocked. This test should either:
- Mock
getGitHubVersion()and usemockOfflineFeatures()or another proper feature implementation (like the other tests do), OR - Mock
getDefaultCliVersionon the features object to return a valid result, so that the test actually verifies the behavior whengetReleaseByVersionrejects (which is what the test name claims to test).
Currently, this test may pass by accident because the exception thrown by getDefaultCliVersion is caught by the catch block at line 445 in start-proxy.ts, but it's not testing what it claims to test.
See below for a potential fix:
const logger = new RecordingLogger();
await withTmpDir(async (tempDir) => {
const features = mockOfflineFeatures(tempDir, logger);
const stub = mockGetReleaseByTag();
const info = await startProxyExports.getDownloadUrl(
getRunnerLogger(true),
features,
);
t.is(info.version, startProxyExports.UPDATEJOB_PROXY_VERSION);
t.is(
info.url,
startProxyExports.getFallbackUrl(startProxyExports.getProxyPackage()),
);
stub.restore();
});
| /** | ||
| * Determines the URL of the proxy release asset that we should download if its not | ||
| * already in the toolcache, and its version. | ||
| * | ||
| * @param logger The logger to use. | ||
| * @returns Returns the download URL and version of the proxy package we plan to use. | ||
| */ |
There was a problem hiding this comment.
The JSDoc comment for getDownloadUrl is missing documentation for the new features parameter. Please add a @param features entry to document this parameter.
This issue also appears on line 551 of the same file.
See below for a potential fix:
* @param logger The logger to use.
* @param features The feature enablement configuration used to determine the default CLI version.
henrymercer
requested review from
sam-robson
and removed request for
henrymercer
Changes the
start-proxyaction to usegetDefaultCliVersionrather than havingdefaults.bundleVersionhard-coded. Conveniently, this adds a permanent consumer of feature flags tostart-proxy.We could relatively easily add a FF as a gate for the new behaviour.
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