-
Notifications
You must be signed in to change notification settings - Fork 374
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the parsing of GHES (GitHub Enterprise Server) pre-release versions to handle the non-standard semver format used by GHES. The main issue is that GHES pre-release versions like 3.18.0.pre1
are not valid semver format, so they need to be transformed to 3.18.0-pre1
before being parsed.
- Adds a new
parseGhesVersion
utility function to normalize GHES version strings - Updates version comparison logic to use the new parser for GHES versions
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/util.ts | Adds parseGhesVersion function to normalize GHES pre-release version format |
src/upload-lib.ts | Updates version comparisons to use the new parser for GHES versions |
src/upload-lib.test.ts | Adds test for GHES 3.16 pre-release version handling and improves test assertions |
lib/util.js | Generated JavaScript for the new utility function |
lib/upload-lib.js | Generated JavaScript for the updated upload library |
lib/upload-lib.test.js | Generated JavaScript for the updated tests |
@@ -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. | |||
if (version.includes(".pre")) { |
There was a problem hiding this comment.
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.
if (version.includes(".pre")) { | |
if (/\.pre\d+$/.test(version)) { |
Copilot uses AI. Check for mistakes.
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"); |
There was a problem hiding this comment.
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.
version = version.replace(".pre", "-pre"); | |
version = version.replaceAll(".pre", "-pre"); |
Copilot uses AI. Check for mistakes.
src/util.ts
Outdated
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); |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable fix for this scenario! Thanks for implementing and thanks to @cklin for spotting it.
Just one minor suggestion, otherwise LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding in the thorough documentation! ✨
This fixes parsing of GHES pre-release versions for the combining SARIF files check. The version
3.18.0.pre1
is not valid semver, so this just transforms it to3.16.0-pre1
.Merge / deployment checklist