- Notifications
You must be signed in to change notification settings - Fork373
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 ourterms of service andprivacy 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 theparseGhesVersion function with a newsatisfiesGHESVersion function that usessemver.coerce and explicitly strips pre-release components before performing version comparisons.
- Replace
parseGhesVersionfunction withsatisfiesGHESVersionfor cleaner version range checking - Use
semver.coerceto 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 | ReplacesparseGhesVersion withsatisfiesGHESVersion 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 |
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 toe30db30CompareThanks for the reviews! I've rebased and squashed all three commits into one to make this easier to cherry-pick for the backport. |
07455ed intomainUh oh!
There was an error while loading.Please reload this page.
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