Skip to content

Use getDefaultCliVersion for start-proxy #3512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mbg wants to merge 1 commit into
base: main
Choose a base branch
from
+139 −51
Open

Use getDefaultCliVersion for start-proxy #3512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions lib/start-proxy-action.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/start-proxy-action.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as core from "@actions/core";

import * as actionsUtil from "./actions-util";
import { getGitHubVersion } from "./api-client";
import { Feature, FeatureEnablement, initFeatures } from "./feature-flags";
import { FeatureEnablement, initFeatures } from "./feature-flags";
import { KnownLanguage } from "./languages";
import { getActionsLogger, Logger } from "./logging";
import { getRepositoryNwo } from "./repository";
Expand Down Expand Up @@ -98,7 +98,7 @@ async function run(startedAt: Date) {
};

// Start the Proxy
const proxyBin = await getProxyBinaryPath(logger);
const proxyBin = await getProxyBinaryPath(logger, features);
const proxyInfo = await startProxy(
proxyBin,
proxyConfig,
Expand Down
145 changes: 110 additions & 35 deletions src/start-proxy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,27 @@ import sinon from "sinon";

import * as apiClient from "./api-client";
import * as defaults from "./defaults.json";
import { setUpFeatureFlagTests } from "./feature-flags/testing-util";
import { KnownLanguage } from "./languages";
import { getRunnerLogger, Logger } from "./logging";
import * as startProxyExports from "./start-proxy";
import { parseLanguage } from "./start-proxy";
import * as statusReport from "./status-report";
import {
checkExpectedLogMessages,
createFeatures,
getRecordingLogger,
makeTestToken,
RecordingLogger,
setupTests,
withRecordingLoggerAsync,
} from "./testing-utils";
import { ConfigurationError } from "./util";
import {
ConfigurationError,
GitHubVariant,
GitHubVersion,
withTmpDir,
} from "./util";

setupTests(test);

Expand Down Expand Up @@ -347,8 +355,18 @@ test("parseLanguage", async (t) => {
t.deepEqual(parseLanguage(""), undefined);
});

function mockGetReleaseByTag(assets?: Array<{ name: string; url?: string }>) {
const mockClient = sinon.stub(apiClient, "getApiClient");
function mockGetApiClient(endpoints: any) {
return (
sinon
.stub(apiClient, "getApiClient")
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
.returns({ rest: endpoints } as any)
);
}

type ReleaseAssets = Array<{ name: string; url?: string }>;

function mockGetReleaseByTag(assets?: ReleaseAssets) {
const getReleaseByTag =
assets === undefined
? sinon.stub().rejects()
Expand All @@ -359,21 +377,28 @@ function mockGetReleaseByTag(assets?: Array<{ name: string; url?: string }>) {
url: "GET /repos/:owner/:repo/releases/tags/:tag",
});

// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
mockClient.returns({
rest: {
repos: {
getReleaseByTag,
},
},
} as any);
return mockClient;
return mockGetApiClient({ repos: { getReleaseByTag } });
}

function mockOfflineFeatures(tempDir: string, logger: Logger) {
// Using GHES ensures that we are using `OfflineFeatures`.
const gitHubVersion = {
type: GitHubVariant.GHES,
version: "3.0.0",
};
sinon.stub(apiClient, "getGitHubVersion").resolves(gitHubVersion);

return setUpFeatureFlagTests(tempDir, logger, gitHubVersion);
}

test("getDownloadUrl returns fallback when `getLinkedRelease` rejects", async (t) => {
test("getDownloadUrl returns fallback when `getReleaseByVersion` rejects", async (t) => {
mockGetReleaseByTag();

const info = await startProxyExports.getDownloadUrl(getRunnerLogger(true));
const features = createFeatures([]);
const info = await startProxyExports.getDownloadUrl(
getRunnerLogger(true),
features,
);

t.is(info.version, startProxyExports.UPDATEJOB_PROXY_VERSION);
t.is(
Expand All @@ -383,33 +408,48 @@ test("getDownloadUrl returns fallback when `getLinkedRelease` rejects", async (t
});
Comment on lines 394 to 408
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Mock getGitHubVersion() and use mockOfflineFeatures() or another proper feature implementation (like the other tests do), OR
  2. Mock getDefaultCliVersion on the features object to return a valid result, so that the test actually verifies the behavior when getReleaseByVersion rejects (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();
  });

Copilot uses AI. Check for mistakes.

test("getDownloadUrl returns fallback when there's no matching release asset", async (t) => {
const logger = new RecordingLogger();
const testAssets = [[], [{ name: "foo" }]];

for (const assets of testAssets) {
const stub = mockGetReleaseByTag(assets);
const info = await startProxyExports.getDownloadUrl(getRunnerLogger(true));
await withTmpDir(async (tempDir) => {
const features = mockOfflineFeatures(tempDir, logger);

t.is(info.version, startProxyExports.UPDATEJOB_PROXY_VERSION);
t.is(
info.url,
startProxyExports.getFallbackUrl(startProxyExports.getProxyPackage()),
);
for (const assets of testAssets) {
const stub = mockGetReleaseByTag(assets);
const info = await startProxyExports.getDownloadUrl(
getRunnerLogger(true),
features,
);

stub.restore();
}
t.is(info.version, startProxyExports.UPDATEJOB_PROXY_VERSION);
t.is(
info.url,
startProxyExports.getFallbackUrl(startProxyExports.getProxyPackage()),
);

stub.restore();
}
});
});

test("getDownloadUrl returns matching release asset", async (t) => {
const logger = new RecordingLogger();
const assets = [
{ name: "foo", url: "other-url" },
{ name: startProxyExports.getProxyPackage(), url: "url-we-want" },
];
mockGetReleaseByTag(assets);

const info = await startProxyExports.getDownloadUrl(getRunnerLogger(true));
await withTmpDir(async (tempDir) => {
const features = mockOfflineFeatures(tempDir, logger);
const info = await startProxyExports.getDownloadUrl(
getRunnerLogger(true),
features,
);

t.is(info.version, defaults.cliVersion);
t.is(info.url, "url-we-want");
t.is(info.version, defaults.cliVersion);
t.is(info.url, "url-we-want");
});
});

test("credentialToStr - hides passwords", (t) => {
Expand Down Expand Up @@ -560,13 +600,15 @@ test(
);

test("getProxyBinaryPath - returns path from tool cache if available", async (t) => {
const logger = new RecordingLogger();
mockGetReleaseByTag();

await withRecordingLoggerAsync(async (logger) => {
await withTmpDir(async (tempDir) => {
const toolcachePath = "/path/to/proxy/dir";
sinon.stub(toolcache, "find").returns(toolcachePath);

const path = await startProxyExports.getProxyBinaryPath(logger);
const features = mockOfflineFeatures(tempDir, logger);
const path = await startProxyExports.getProxyBinaryPath(logger, features);

t.assert(path);
t.is(
Expand All @@ -577,12 +619,31 @@ test("getProxyBinaryPath - returns path from tool cache if available", async (t)
});

test("getProxyBinaryPath - downloads proxy if not in cache", async (t) => {
const logger = new RecordingLogger();
const expectedTag = "codeql-bundle-v2.20.1";
const expectedParams = {
owner: "github",
repo: "codeql-action",
tag: expectedTag,
};
const downloadUrl = "url-we-want";
mockGetReleaseByTag([
{ name: startProxyExports.getProxyPackage(), url: downloadUrl },
]);
const assets = [
{
name: startProxyExports.getProxyPackage(),
url: downloadUrl,
},
];

await withRecordingLoggerAsync(async (logger) => {
const getReleaseByTag = sinon.stub();
getReleaseByTag.withArgs(sinon.match(expectedParams)).resolves({
status: 200,
data: { assets },
headers: {},
url: "GET /repos/:owner/:repo/releases/tags/:tag",
});
mockGetApiClient({ repos: { getReleaseByTag } });

await withTmpDir(async (tempDir) => {
const toolcachePath = "/path/to/proxy/dir";
const find = sinon.stub(toolcache, "find").returns("");
const getApiDetails = sinon.stub(apiClient, "getApiDetails").returns({
Expand All @@ -603,8 +664,22 @@ test("getProxyBinaryPath - downloads proxy if not in cache", async (t) => {
.resolves(extractedPath);
const cacheDir = sinon.stub(toolcache, "cacheDir").resolves(toolcachePath);

const path = await startProxyExports.getProxyBinaryPath(logger);

const gitHubVersion: GitHubVersion = {
type: GitHubVariant.DOTCOM,
};
sinon.stub(apiClient, "getGitHubVersion").resolves(gitHubVersion);

const features = setUpFeatureFlagTests(tempDir, logger, gitHubVersion);
const getDefaultCliVersion = sinon
.stub(features, "getDefaultCliVersion")
.resolves({ cliVersion: "2.20.1", tagName: expectedTag });
const path = await startProxyExports.getProxyBinaryPath(logger, features);

t.assert(getDefaultCliVersion.calledOnce);
sinon.assert.calledOnceWithMatch(
getReleaseByTag,
sinon.match(expectedParams),
);
t.assert(find.calledOnce);
t.assert(getApiDetails.calledOnce);
t.assert(getAuthorizationHeaderFor.calledOnce);
Expand Down
25 changes: 18 additions & 7 deletions src/start-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import {
getApiClient,
getApiDetails,
getAuthorizationHeaderFor,
getGitHubVersion,
} from "./api-client";
import * as artifactScanner from "./artifact-scanner";
import { Config } from "./config-utils";
import * as defaults from "./defaults.json";
import { FeatureEnablement } from "./feature-flags";
import { KnownLanguage } from "./languages";
import { Logger } from "./logging";
import {
Expand Down Expand Up @@ -391,15 +393,15 @@ export function getFallbackUrl(proxyPackage: string): string {

/**
* Uses the GitHub API to obtain information about the CodeQL CLI bundle release
* that is pointed at by `defaults.json`.
* that is tagged by `version`.
*
* @returns The response from the GitHub API.
*/
async function getLinkedRelease() {
async function getReleaseByVersion(version: string) {
return getApiClient().rest.repos.getReleaseByTag({
owner: "github",
repo: "codeql-action",
tag: defaults.bundleVersion,
tag: version,
});
}

Expand All @@ -412,12 +414,18 @@ async function getLinkedRelease() {
*/
export async function getDownloadUrl(
logger: Logger,
features: FeatureEnablement,
): Promise<{ url: string; version: string }> {
const proxyPackage = getProxyPackage();

try {
// Try to retrieve information about the CLI bundle release pointed at by `defaults.json`.
const cliRelease = await getLinkedRelease();
// Retrieve information about the CLI version we should use. This will be either the linked
// version, or the one enabled by FFs.
const gitHubVersion = await getGitHubVersion();
const versionInfo = await features.getDefaultCliVersion(gitHubVersion.type);

// Try to retrieve information about the CLI bundle release identified by `versionInfo`.
const cliRelease = await getReleaseByVersion(versionInfo.tagName);

// Search the release's assets to find the one we are looking for.
for (const asset of cliRelease.data.assets) {
Expand Down Expand Up @@ -548,9 +556,12 @@ export function getProxyFilename() {
* @param logger The logger to use.
* @returns The path to the proxy binary.
*/
export async function getProxyBinaryPath(logger: Logger): Promise<string> {
export async function getProxyBinaryPath(
logger: Logger,
features: FeatureEnablement,
): Promise<string> {
const proxyFileName = getProxyFilename();
const proxyInfo = await getDownloadUrl(logger);
const proxyInfo = await getDownloadUrl(logger, features);

let proxyBin = toolcache.find(proxyFileName, proxyInfo.version);
if (!proxyBin) {
Expand Down
Loading