Skip to content

Fix parsing of GHES pre-release versions #2969

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

Merged
koesie10 merged 2 commits into from
Jul 14, 2025
Merged

Fix parsing of GHES pre-release versions #2969

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59d67fc
Fix parsing of GHES pre-release versions
koesie10 Jul 14, 2025
c6a6c14
Move comment to JSDoc
koesie10 Jul 14, 2025
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
4 changes: 2 additions & 2 deletions lib/upload-lib.js
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion lib/upload-lib.js.map
View file
Open in desktop

Large diffs are not rendered by default.

22 changes: 18 additions & 4 deletions lib/upload-lib.test.js
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion lib/upload-lib.test.js.map
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions lib/util.js
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion lib/util.js.map
View file
Open in desktop

Large diffs are not rendered by default.

28 changes: 28 additions & 0 deletions src/upload-lib.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,18 @@ test("shouldShowCombineSarifFilesDeprecationWarning when on GHES 3.14", async (t
);
});

test("shouldShowCombineSarifFilesDeprecationWarning when on GHES 3.16 pre", async (t) => {
t.true(
await uploadLib.shouldShowCombineSarifFilesDeprecationWarning(
[createMockSarif("abc", "def"), createMockSarif("abc", "def")],
{
type: GitHubVariant.GHES,
version: "3.16.0.pre1",
},
),
);
});

test("shouldShowCombineSarifFilesDeprecationWarning with only 1 run", async (t) => {
t.false(
await uploadLib.shouldShowCombineSarifFilesDeprecationWarning(
Expand Down Expand Up @@ -454,6 +466,10 @@ test("throwIfCombineSarifFilesDisabled when on dotcom with feature flag", async
type: GitHubVariant.DOTCOM,
},
),
{
message:
/The CodeQL Action does not support uploading multiple SARIF runs with the same category/,
},
);
});

Expand Down Expand Up @@ -518,6 +534,10 @@ test("throwIfCombineSarifFilesDisabled when on GHES 3.18 pre", async (t) => {
version: "3.18.0.pre1",
},
),
{
message:
/The CodeQL Action does not support uploading multiple SARIF runs with the same category/,
},
);
});

Expand All @@ -531,6 +551,10 @@ test("throwIfCombineSarifFilesDisabled when on GHES 3.18 alpha", async (t) => {
version: "3.18.0-alpha.1",
},
),
{
message:
/The CodeQL Action does not support uploading multiple SARIF runs with the same category/,
},
);
});

Expand All @@ -544,6 +568,10 @@ test("throwIfCombineSarifFilesDisabled when on GHES 3.18", async (t) => {
version: "3.18.0",
},
),
{
message:
/The CodeQL Action does not support uploading multiple SARIF runs with the same category/,
},
);
});

Expand Down
5 changes: 3 additions & 2 deletions src/upload-lib.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
getRequiredEnvParam,
GitHubVariant,
GitHubVersion,
parseGhesVersion,
SarifFile,
SarifRun,
} from "./util";
Expand Down Expand Up @@ -132,7 +133,7 @@ export async function shouldShowCombineSarifFilesDeprecationWarning(
// Do not show this warning on GHES versions before 3.14.0
if (
githubVersion.type === GitHubVariant.GHES &&
semver.lt(githubVersion.version, "3.14.0")
semver.lt(parseGhesVersion(githubVersion.version), "3.14.0")
) {
return false;
}
Expand Down Expand Up @@ -177,7 +178,7 @@ async function shouldDisableCombineSarifFiles(
) {
if (githubVersion.type === GitHubVariant.GHES) {
// Never block on GHES versions before 3.18.
if (semver.lt(githubVersion.version, "3.18.0-0")) {
if (semver.lt(parseGhesVersion(githubVersion.version), "3.18.0-0")) {
return false;
}
} else {
Expand Down
8 changes: 8 additions & 0 deletions src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,14 @@ export function checkActionVersion(
}
}

export function parseGhesVersion(version: string): semver.SemVer {
// GHES pre-release versions are in the format "3.18.0.pre1", which is not a valid semver version.
Comment thread
koesie10 marked this conversation as resolved.
Outdated
if (version.includes(".pre")) {
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

The condition version.includes(".pre") may be too broad and could match unintended patterns. Consider using a more specific pattern like /\.pre\d+$/ to ensure it only matches the expected GHES pre-release format at the end of the version string.

Suggested change
if (version.includes(".pre")) {
if (/\.pre\d+$/.test(version)) {

Copilot uses AI. Check for mistakes.
version = version.replace(".pre", "-pre");
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

Using replace(".pre", "-pre") only replaces the first occurrence. If there could be multiple .pre patterns in a version string, consider using replaceAll() or a global regex. However, if only one occurrence is expected, this is acceptable.

Suggested change
version = version.replace(".pre", "-pre");
version = version.replaceAll(".pre", "-pre");

Copilot uses AI. Check for mistakes.
}
return new semver.SemVer(version);
Comment on lines +1135 to +1149
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

The function doesn't handle potential errors from new semver.SemVer(version). If the version string is still invalid after transformation, this will throw an exception. Consider adding error handling or documenting this behavior.

Suggested change
export function parseGhesVersion(version: string): semver.SemVer {
// GHES pre-release versions are in the format "3.18.0.pre1", which is not a valid semver version.
if (version.includes(".pre")) {
version = version.replace(".pre", "-pre");
}
return new semver.SemVer(version);
export function parseGhesVersion(version: string): semver.SemVer | null {
// GHES pre-release versions are in the format "3.18.0.pre1", which is not a valid semver version.
if (version.includes(".pre")) {
version = version.replace(".pre", "-pre");
}
try {
return new semver.SemVer(version);
} catch (error) {
core.warning(`Invalid GHES version string: ${version}. Error: ${error}`);
return null;
}

Copilot uses AI. Check for mistakes.
}

/**
* Supported build modes.
*
Expand Down
Loading