Skip to content
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

Jump to bottom

fix(utils): Ensure isPreviewRelease identifies -dump as a preview release #561

Closed
wants to merge 1 commit into from
Closed

fix(utils): Ensure isPreviewRelease identifies -dump as a preview release #561

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
13 changes: 10 additions & 3 deletions src/utils/__tests__/version.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,21 @@
});

describe('isPreviewRelease', () => {
test('accepts semver preview release', () => {
expect(isPreviewRelease('2.3.4-preview1')).toBe(true);
});
test.each(['preview', 'pre', 'alpha.0', 'beta', 'rc.1', 'dev'])(
Copy link
Member Author

Choose a reason for hiding this comment

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

added some more tests here for other suffixes

'accepts semver preview release',
previewSuffix => {
expect(isPreviewRelease(`2.3.4-${previewSuffix}1`)).toBe(true);
}
);

test('accepts Python-style preview release', () => {
expect(isPreviewRelease('2.3.4rc0')).toBe(true);
});

test('accepts dump preview release', () => {
expect(isPreviewRelease('2.3.4-dump1')).toBe(true);
});

test('does not accept non-preview release', () => {
expect(isPreviewRelease('2.3.4')).toBe(false);
});
Expand Down Expand Up @@ -183,8 +190,8 @@
});

test('can compare pre parts', () => {
const v1 = parseVersion('1.2.3-1')!;

Check warning on line 193 in src/utils/__tests__/version.test.ts

View workflow job for this annotation

GitHub Actions / Lint fixes 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
const v2 = parseVersion('1.2.3-2')!;

Check warning on line 194 in src/utils/__tests__/version.test.ts

View workflow job for this annotation

GitHub Actions / Lint fixes 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
expect(versionGreaterOrEqualThan(v1, v2)).toBe(false);
expect(versionGreaterOrEqualThan(v2, v1)).toBe(true);
});
Expand Down
2 changes: 1 addition & 1 deletion src/utils/version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export function versionGreaterOrEqualThan(v1: SemVer, v2: SemVer): boolean {
/**
* A regular expression to detect that a version is a pre-release version.
*/
export const PREVIEW_RELEASE_REGEX = /(?:[^a-z])(preview|pre|rc|dev|alpha|beta|unstable|a|b)(?:[^a-z]|$)/i;
export const PREVIEW_RELEASE_REGEX = /(?:[^a-z])(preview|pre|rc|dev|alpha|beta|unstable|a|b|dump)(?:[^a-z]|$)/i;
Copy link
Member

Choose a reason for hiding this comment

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

I've never heard of "dump" as a prerelease naming scheme -- is this standardized anywhere? I'd prefer we use actual semver names rather than making up our own here (as that would be more expected from users as well when reasoning about versions)

Copy link
Member Author

@Lms24 Lms24 Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I understand, dump was chosen because it described a feature that was added to this preview release. So likely not standardized. I agree that this is a niche/edge case but "dump" kinda seems likely enough to come up again. Also noticed while removing the entries of this version from the registry, we had similar naming schemes being used in the past but they are even more specific, so not worth to add to this list IMO.

I can also close this as I get your point. WDYT about checking for any suffix after the semver patch number instead?

Copy link
Member

Choose a reason for hiding this comment

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

ok then we definitely shouldn't add this to the regex

I think craft should reject any non-semver versions -- I thought it already did this but maybe that's only for python?

the dotnet sdk release should have named it -pre1 or one of the standard naming schemes

Copy link
Member

Choose a reason for hiding this comment

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

one important case either way is 1.2.3-1 is standardized in python as equivalent to 1.2.3.post1 and we use that intentionally so we have to make sure that still works

Copy link
Member Author

@Lms24 Lms24 Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So it looks like we would throw an error in the release registry target if we encounter a version string we can't parse to semver:

craft/src/utils/symlink.ts

Lines 43 to 45 in 413a603

if (!parsedNewVersion) {
throw new ConfigurationError(`Cannot parse version: "${parsedNewVersion}"`);
}

the prepare command also throws if the new version is not semver-compliant:

craft/src/commands/prepare.ts

Lines 115 to 129 in 413a603

export function checkVersionOrPart(argv: Arguments<any>, _opt: any): boolean {
const version = argv.newVersion;
if (['major', 'minor', 'patch'].indexOf(version) > -1) {
throw Error('Version part is not supported yet');
} else if (isValidVersion(version)) {
return true;
} else {
let errMsg = `Invalid version or version part specified: "${version}"`;
if (version.startsWith('v')) {
errMsg += '. Removing the "v" prefix will likely fix the issue';
}
throw Error(errMsg);
}
}

So theoretically, we could change the pre-release check to use parseVersion and check for the existence of the returned .pre property. WDYT?

I'll also double-check the python case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the 1.2.3-1 case:

  • isPreviewRelease returns false. I guess this is correct, given this is a post release?
  • parseVersion can parse this string and identifies the -1 as .pre: 
     test('parses a Python-style post release version', () => {
   expect(parseVersion('1.2.3-1')).toEqual({
     major: 1,
     minor: 2,
     patch: 3,
     pre: '1',
   });
 });

Copy link
Member

Choose a reason for hiding this comment

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

seems fine to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok now the specific list of pre-releases makes sense: If we want to ensure Python post releases are not identified as pre-releases, we can't check against arbitrary pre-release version identifiers. So just checking for !!parseVersion(...).pre would not cover the python post release correctly.

So I guess, we don't change anything 👍 I can open a PR with some tests I wrote to have the Python case covered.


/**
* Checks that the provided string is a pre-release version.
Expand Down
Loading