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.

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

Confirm the order of prereleases #859

Open
mtojek opened this issue Aug 4, 2022 · 5 comments
Open

Confirm the order of prereleases #859

mtojek opened this issue Aug 4, 2022 · 5 comments
Labels
Team:Ecosystem Label for the Packages Ecosystem team

Comments

@mtojek
Copy link
Contributor

mtojek commented Aug 4, 2022

While working on elastic/apm-server#8491, I found an interesting problem: are all prerelease tags comparable?

The package-spec allows for defining the prerelease part as tag + timestamp. Which rules should EPR follow to figure out which revision is the newest one.

Few "tricky" comparisons:

8.5.0-preview-1111111111111 vs 8.5.0-preview-2111111
8.5.0-preview-1111111111111 vs 8.5.0-beta-1111111111111

I guess that we have to apply more rules (first thoughts below) to the IsNewerOrEqual function like:

  1. Compare timestamp when the prerelease starts with rc/beta/preview.
  2. rc/beta/preview has higher priority than SNAPSHOT/next.

BTW the APM team selected the preview tag as it seemed to be the most accurate for them.

Feel free to chime in, @jsoriano @mrodm.

@jlind23 jlind23 added the Team:Ecosystem Label for the Packages Ecosystem team label Aug 4, 2022
@jsoriano
Copy link
Member

jsoriano commented Aug 4, 2022

I think we should follow semver rules. Semver has clear rules for precedence of prerelease tags. If you want prerelease identifiers to be compared numerically, they must be separated by dots, that is:

  • 8.5.0-preview-1111111111111 < 8.5.0-preview-2111111
  • 8.5.0-preview.1111111111111 > 8.5.0-preview.2111111

For timestamps, if they are in the YYYYMMDD format, they are ordered the same lexicographically and numerically.

@mtojek
Copy link
Contributor Author

mtojek commented Aug 4, 2022

For timestamps, if they are in the YYYYMMDD format

Currently, timestamps are represented in the Unix form, so it should work until somebody (or a buggy CI job?) accidentally posts a shorter timestamp.

to be compared numerically, they must be separated by dots

I understand that it should work out of the box, right? We shouldn't need to patch the library code.

--

I'm wondering if we need to apply a patch to ensure the correct order of tags (beta/rc... preview).

@mtojek
Copy link
Contributor Author

mtojek commented Aug 4, 2022

I made more experiments to learn about the dotted approach.

We need to remember that if the package versioning pattern switches from preview-12345 to preview.12345, the first one will always be treated as newer (lex order, I guess?).

This is the pattern that is currently applied for the APM package: https://epr-v2.ea-web.elastic.dev/search?prerelease=true&package=apm. We don't plan to mix prerelease tags, so I guess that this form is safe.

@jsoriano
Copy link
Member

jsoriano commented Aug 4, 2022

I understand that it should work out of the box, right? We shouldn't need to patch the library code.

Yes, this should work with current semver library. If not, this is a bug in the library.

I'm wondering if we need to apply a patch to ensure the correct order of tags (beta/rc... preview).

With current labels and ordering (1.0.0-alpha < 1.0.0-beta < 1.0.0-preview < 1.0.0-rc < 1.0.0), it seems that the only problem is preview, maybe we should use the more industry-standard "alpha" instead of preview. Or completely remove the "preview", 1.0.0-1659529701 would be lower than any prerelease with letters, and would be still clear that is not an stable version.
I would prefer to change the prerelease tags before adding exceptions to the semver implementation.

Currently, timestamps are represented in the Unix form, so it should work until somebody (or a buggy CI job?) accidentally posts a shorter timestamp.

Well, or until timestamps need a new digit to be represented, but this won't happen till UNIX time 10000000000, a.k.a. November of 2086 😃

We need to remember that if the package versioning pattern switches from preview-12345 to preview.12345, the first one will always be treated as newer (lex order, I guess?).

If a project wants to change this, I would consider doing it after a version change, so prereleases are consistent inside a version.

This is the pattern that is currently applied for the APM package: https://epr-v2.ea-web.elastic.dev/search?prerelease=true&package=apm. We don't plan to mix prerelease tags, so I guess that this form is safe.

Yes, this looks safe, at least till 2086 🙂 But in any case it may be worth to change to the dotted form on the next version, to be more explicit on the numeric order of the versions. Or to remove the "preview" part, as commented above.

@jlind23
Copy link
Contributor

jlind23 commented Aug 9, 2022

Repurposing this issue - As discussed with @jsoriano and @mrodm we shouldn't do anything there and rather check if our guidelines do recommend format versioned for pre-release tags as stated in the semver docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Ecosystem Label for the Packages Ecosystem team
Projects
None yet
Development

No branches or pull requests

3 participants