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

Restore the naming of the published npm packages to be suffixed with a commit sha #10233

Closed
azatsarynnyy opened this issue Oct 5, 2021 · 9 comments
Labels
Team: Che-Editors issues regarding the che-editors team

Comments

@azatsarynnyy
Copy link
Member

Description:

Recently, we've noticed that the Theia packages that are published to npmjs are not suffixed with the commit hash anymore (e.g. 1.18.0-next.aed98766), but with just sequential numbers, like the amount of the commits contributed after the latest release or something like that (e.g. 1.18.0-next.72). For example, see https://www.npmjs.com/package/@theia/plugin-ext
After discussing that topic on Dev call, we learned that it's a side-effect of lerna upgrade, within #9710

In Che-Theia, we rely on these sha suffixes to detect the commit that a specific package corresponds to.
Considering that a new naming pattern is not something that others are relying on, we would like to work on restoring the previous naming scheme.

Additional Info:

Some helpful information on the topic: WordPress/gutenberg#29379

@azatsarynnyy azatsarynnyy added the Team: Che-Editors issues regarding the che-editors team label Oct 5, 2021
@vince-fugnitto
Copy link
Member

cc @marcdumais-work @paul-marechal

@marcdumais-work
Copy link
Contributor

like the amount of the commits contributed after the latest release or something like that (e.g. 1.18.0-next.72).

That's the theory, but it seems that lerna has not noticed we did the 1.18.0 release yet - I expect that it should have switched to 1.19.0-next.1 right after :)

@paul-marechal
Copy link
Member

paul-marechal commented Oct 6, 2021

@azatsarynnyy do you need the sha being part of the version string, or is it ok for you to go read the gitHead property of the installed package.json file?

image

@azatsarynnyy
Copy link
Member Author

@paul-marechal our flow is something like that:

  1. Fetch https://registry.npmjs.org/-/package/@theia/core/dist-tags
  2. Get the package name that corresponds to next
  3. Extract the commit sha that corresponds to the specific package
  4. Checkout to the specific commit

So, we need the sha to be part of the version string.

@paul-marechal
Copy link
Member

paul-marechal commented Oct 6, 2021

Would the following steps work for you too?

  1. Fetch https://registry.npmjs.org/-/package/@theia/core/dist-tags
  2. Read the next field that contains the latest version string
  3. Fetch https://registry.npmjs.org/@theia/core/[next-pkg-version]
  4. Read the gitHead field that contains the exact and non-truncated git sha
  5. Checkout that commit

Using those steps it shouldn't break in the future even if we keep changing the versioning scheme.

The reason I insist is that:

  1. The commit from which the next release was done isn't lost
  2. The new naming makes reasoning about the next releases a lot easier.

I don't know how yarn/npm decided to pull the latest next release, but according to semver alone the latest two next releases that include the commit sha would be compared the wrong way:

last two next releases

> semver.compare('1.18.0-next.a73a746b', '1.18.0-next.de7b81be')
-1

Here -1 means that semver thinks 1.18.0-next.de7b81be has precedence over 1.18.0-next.a73a746b which is wrong as the latter is in fact more recent.

Another problem that just happened today: It seems that yarn/npm advertise the latest published when asking about next but our CI published our latest commit before the previous publish job. So now we see something like:

versions published in the wrong order

Notice how the most recent published version is .77 rather than .84... And when querying npm for next it does seem to return the most recent one.

But thanks to this new versioning scheme I was able to spot this oddity by just looking at the history.

All in all I think there are still issues with our next releases but this new scheme is a step in the right direction,

If you still think that it would be better to stick to putting the sha, then let's revert back to do this.

@tsmaeder
Copy link
Contributor

tsmaeder commented Oct 7, 2021

I can understand wanting to map commit SHA's to a release, but depending on the exact format of the release name seems like undue coupling to details of the release process. If this is something we want to support, we need a more robust mechanism.

@marcdumais-work
Copy link
Contributor

Independent of whether Che should depend on the SHA, I think it might make sense to go forward with this suggestion, or find out why next releases have not stepped-up last release. If that happens again tomorrow, it could be confusing: latest will be 1.19.0 and next will be something like 1.18.0-next.130

@azatsarynnyy
Copy link
Member Author

Hello @paul-marechal
The solution you've proposed (reading the gitHead field) should work great for our case.
Thanks a lot for providing that!

@azatsarynnyy
Copy link
Member Author

I'm closing the issue. Will be reopened if we face more obstacles with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team: Che-Editors issues regarding the che-editors team
Projects
None yet
Development

No branches or pull requests

5 participants