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

[Lms24]

came up in: getsentry/sentry-docs#11716

This PR adds dump to our pre-release detection regex, so that isPreviewRelease() check returns true for versions like 1.2.3-dump(.)1.

We had an internal discussion about choosing a more "wholistic" approach to detecting preview releases, like for example checking if there's any suffix to a typical semver <major>.<minor>.<patch> version. I'm also happy to implement this but I'm probably missing some context as to why we chose to opt in specific strings in the past. Perhaps the answer is that we can also handle non-semver version strings? Anyway, this should at least avoid the case brought up in getsentry/sentry-docs#11716.

[Lms24]

added some more tests here for other suffixes

[asottile-sentry]

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)

[Lms24]

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?

[asottile-sentry]

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

[asottile-sentry]

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

[Lms24]

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.

[Lms24]

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',
     });
   });

[asottile-sentry]

seems fine to me!

[Lms24]

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.

[Lms24]

Closing as dump is not a standardized pre-release identifier. We can think about a more versatile check for arbitrary identifiers (in a new PR) or we stick to using well-established identifiers.