-
Notifications
You must be signed in to change notification settings - Fork 376
Ignore pre-release parts when comparing GHES versions #2972
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 refactors the GHES version comparison logic to ignore pre-release version parts when determining compatibility. The change replaces the parseGhesVersion
function with a new satisfiesGHESVersion
function that uses semver.coerce
and explicitly strips pre-release components before performing version comparisons.
- Replace
parseGhesVersion
function withsatisfiesGHESVersion
for cleaner version range checking - Use
semver.coerce
to handle non-standard version formats and strip pre-release components - Update all GHES version comparisons to use the new function with range syntax
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/util.ts | Replaces parseGhesVersion with satisfiesGHESVersion function that strips pre-release parts |
src/upload-lib.ts | Updates version comparisons to use new function and removes unused semver import |
lib/util.js | Generated JavaScript for the util.ts changes |
lib/upload-lib.js | Generated JavaScript for the upload-lib.ts changes |
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 putting this together! This probably makes sense, since I can't imagine that we'd want to check for a particular pre-release version. Dropping the pre-release for the comparison makes sense to me.
One suggestion about the potential footgun that Copilot highlighted as well.
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! This looks good now.
@@ -1141,15 +1141,11 @@ export function checkActionVersion( | |||
* is invalid, this will return `defaultIfInvalid`. | |||
*/ | |||
export function satisfiesGHESVersion( | |||
githubVersion: GitHubVersion, | |||
ghesVersion: string, |
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.
Good idea!
There's a merge conflict with |
e080457
to
e30db30
Compare
Thanks for the reviews! I've rebased and squashed all three commits into one to make this easier to cherry-pick for the backport. |
This will change our GHES version comparison to always compare without taking into consideration the pre-release part of the version, and dropping any unrecognized semver parts.
Merge / deployment checklist