Don't treat x in pre-release and build metadata identifiers as a wild…

[mamachanko]

…card

I don't think we would want to extend logic to wildcards inside pre-release and build metadata identifiers.

Would need flow up through vendir into kapp-controller.

refs:

• blang/semver ParseRange fails to handle prereleases containing x vendir#123
• Cannot install package with prerelease identifier containing x kapp-controller#1518

[mamachanko]

In the absence of CI

pushd v4 && go test -v ./... && popd

passes ✅

[praveenrewar]

I am not an expert at semver, however the changes looks good to me.
Out of curiosity, any reason not to use the changes from original PR in blang/semver?

[Zebradil]

In the upstream PR this test fails:

?       github.com/blang/semver/v4/examples [no test files]
--- FAIL: TestParseRange (0.00s)
    range_test.go:511: Invalid for case "1.2.3+experimental.2" matching "1.2.3+experimental.1": Expected false, got: true
    range_test.go:511: Invalid for case "1.2.3+experimental.2" matching "1.2.3+experimental.3": Expected false, got: true
FAIL
FAIL    github.com/blang/semver/v4  0.002s
FAIL

[mamachanko]

In the upstream PR this test fails:

?       github.com/blang/semver/v4/examples [no test files]
--- FAIL: TestParseRange (0.00s)
    range_test.go:511: Invalid for case "1.2.3+experimental.2" matching "1.2.3+experimental.1": Expected false, got: true
    range_test.go:511: Invalid for case "1.2.3+experimental.2" matching "1.2.3+experimental.3": Expected false, got: true
FAIL
FAIL    github.com/blang/semver/v4  0.002s
FAIL

Thank you for bringing that up, @Zebradil! I wasn't aware of the pending (since 3y! 😆) blang#76. However, after studying it I partially incorporated the suggested changes here.

The test for .x as suggested by blang#76 is nice and simple, but the pre-release and build metadata identifiers need to be trimmed first. That's because either may contain .x. I've added tests for those scenarios and refactored things a little.

@Zebradil @praveenrewar ready for review! 🙇🏻

Fwiw, I don't think we really consider github.com/blang/semver "upstream upstream" anymore since it is no longer maintained and github.com/carvel-dev/semver is ahead by one or two fixes and features. I may be wrong here.

[Zebradil]

LGTM

By the way, I think it worth considering switching to a more popular and better maintained semver library, for example this one: https://github.com/Masterminds/semver

[joaopapereira]

LGTM

[joaopapereira]

@Zebradil I would tend to agree but we would have to ensure that the features that exist in this repo also exist in that semver library, so some work needs to be done to ensure it.

[Zebradil]

@Zebradil I would tend to agree but we would have to ensure that the features that exist in this repo also exist in that semver library, so some work needs to be done to ensure it.

Agree, it'd probably require a lot of effort without immediate benefits.