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

Invalid canary versions for certain SHAs #1118

Closed
mleonh opened this issue Nov 15, 2017 · 13 comments
Closed

Invalid canary versions for certain SHAs #1118

mleonh opened this issue Nov 15, 2017 · 13 comments
Assignees
Milestone

Comments

@mleonh
Copy link

@mleonh mleonh commented Nov 15, 2017

Expected Behavior

--canary should always generate valid version strings (in accordance with the generally accepted semver rules).

Current Behavior

--canary generates invalid versions if the current SHA's first eight characters are numeric and the first digit is zero. For example, 1.1.0-alpha.21636f26 is valid, but 1.1.0-alpha.08213312 isn't – see rule 9 of the specification ("Numeric identifiers MUST NOT include leading zeroes").

Note that this is not just an issue of just being pedantic – we're using JFrog's Artifactory, and without going into too much detail, it really, really doesn't like encountering a version string like that. We're publishing a lot of packages, multiple times a day, and we're hitting this issue quite regularly.

Possible Solution

Change the template string in PublishCommand/getCanaryVersion() to be: ${nextVersion}-${preid}${hash}, or ${nextVersion}-${preid}.commit${hash}, or something similar.

Steps to Reproduce

  1. Commit until the latest SHA starts with a zero followed by seven digits. This might take a while :)
  2. Publish using --canary

Your Environment

Executable Version
lerna --version 2.2.0
npm --version 5.5.1
yarn --version 1.3.2
node --version 8.9.1
OS Version
macOS Sierra 10.12.6
@evocateur
Copy link
Member

@evocateur evocateur commented Nov 16, 2017

This is kinda-sorta a duplicate of #277. I can certainly appreciate the pain. Does JFrog support "properly" incrementing pre-release versions (basically, shunting the git sha into the "build metadata" slot)?

  1. 1.1.0-alpha.0+deadbeef
  2. 1.1.0-alpha.1+08213312
  3. 1.1.0-alpha.2+21636f26
  4. et cetera

We can get those from some fancy git describe options (might not work in shallow-cloned CI), or queries of the registry directly using https://www.npmjs.com/package/package-json to get the gitHead... wow I just went down quite a rabbit-hole. So, like, it's definitely something I've been thinking about.

@mleonh
Copy link
Author

@mleonh mleonh commented Nov 16, 2017

While 1.1.0-alpha.0-deadbeef should work, shouldn't it be 1.1.0-alpha.0+deadbeef, since deadbeef is the build metadata in this case?

@evocateur
Copy link
Member

@evocateur evocateur commented Nov 16, 2017

Yep, you're right! I brainfarted on the spec.

@mleonh
Copy link
Author

@mleonh mleonh commented Nov 16, 2017

I just published 1.1.0-alpha.0+deadbeef to Artifactory (v5.3.2) as a test, and it seems to like that just fine.

@evocateur
Copy link
Member

@evocateur evocateur commented Nov 16, 2017

Thanks, that's good to know.

@achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Jan 4, 2018

@mleonh I think it depends which version of Artifactory you are running. We're stuck on v4.8.0 at the moment and see lots of:

2018-01-03 19:05:17,271 [art-exec-1487] [WARN ] (o.j.r.n.u.NpmUtils  :88) - Failed validating version: '0.1.0-beta.02883375' of package: '@org/internal-package': Numeric identifier MUST NOT contain leading zeroes

Pre-release versions can have multiple identifiers, but the spec says that any numeric identifiers cannot start with a leading 0. I guess 'numeric' is interpreted as base 10 so 1.0.0-beta.0392abc is valid (as 0392abc is treated as a string) but 1.0.0-beta.0392123 is not.

Build metadata on the other hand can contain multiple identifiers but does not have the numeric item restriction, so 1.0.0-beta+0392123 is valid.

But... Build metadata is ignored when calculating version precedence so 1.0.0-beta+0392123 and 1.0.0-beta+abc23498 would be seen as equal so you need a different way to calculate precedence.

On my current project we're using build numbers for version precedence and git commits for metadata, so something like:

1.0.0-beta.473+commit.0392123

We have to put the commit separator in because lerna adds .0392123 to the string - the spec says identifiers cannot be empty so 1.0.0-beta+.0392123 would be invalid.

Our publish step looks like this:

lerna publish \
  --canary=beta.$BUILD_NUMBER+commit

Maybe lerna could add an option for which character to use to separate the commit id from the version number?

Then we could do something like:

lerna publish \
  --canary=beta.$BUILD_NUMBER \
  --commit-separator=+

and end up with:

1.0.0-beta.473+0392123

@evocateur evocateur added this to the v3.0.0 milestone Jan 18, 2018
@evocateur evocateur self-assigned this Jan 18, 2018
@corsen2000
Copy link

@corsen2000 corsen2000 commented Jul 18, 2018

This is causing us problems as well. Is it acceptable to modify:

predicate = ({ version }) => `${semver.inc(version, release)}-${keyword}.${shortHash}`;

By adding a non-numeric character such as a H in front of the hash so that it is not interpreted as numerical in any case, which should avoid the issue?

@hutson
Copy link

@hutson hutson commented Jul 18, 2018

@corsen2000 that may be a breaking change if there are other tools out there parsing the tag created by lerna and assuming the H is part of the commit hash (without, say, validating that it's a proper short-hash).

@hutson
Copy link

@hutson hutson commented Jul 18, 2018

@achingbrain would it be alright to just skip the extra argument for setting the commit separator and instead, just always append the short commit hash as the first segment of the build metadata string?

+ is always the build metadata separator, and the short hash will, on occasion, fail to meet the requirements for the pre-release section.

Therefore, I believe the only safe place for the commit hash is appended after +.

@hutson
Copy link

@hutson hutson commented Jul 18, 2018

Therefore, how do we manage the canary portion of the string?

If we move the hash after +, then setting --canary to beta gives you -beta+0392123 as demonstrated by @achingbrain.

Should Lerna just expect you to set the canary version using the same --canary argument, like --canary=beta.$BUILD_NUMBER, as demonstrated by @achingbrain?

Or would you expect Lerna to infer the version on its own so that you only need to set --canary?

@hutson
Copy link

@hutson hutson commented Jul 18, 2018

Personally I like the idea of setting the entire pre-release string using --canary. It's simple, and it gives me flexibility to leverage external CI tooling, such as the build number injected into Jenkins CI jobs, or $CIRCLE_BUILD_NUM in Circle CI.

Though, perhaps, the CLI argument name is not intuitive. While canary is how we use these releases, we're really setting the pre-release string in a Semantic Version 2 string.

@hutson
Copy link

@hutson hutson commented Jul 19, 2018

PR is pending here - #1507

evocateur added a commit that referenced this issue Jul 27, 2018
BREAKING CHANGE:
* `--preid` now defaults to "alpha" during prereleases:

  The previous default for this option was undefined, which led to an awkward "1.0.1-0" result when passed to `semver.inc()`.

  The new default "alpha" yields a much more useful "1.0.1-alpha.0" result. Any previous prerelease ID will be preserved, just as it was before.

* `--no-verify` is no longer passed to `git commit` by default, but controlled by the new `--commit-hooks` option:

  The previous behavior was too overzealous, and the new option operates exactly like the corresponding [npm version](https://docs.npmjs.com/cli/version#commit-hooks) option of the same name.

  As long as your pre-commit hooks are properly scoped to ignore changes in package.json files, this change should not affect you. If that is not the case, you may pass `--no-commit-hooks` to restore the previous behavior.

Fixes #277
Fixes #936
Fixes #956
Fixes #961
Fixes #1056
Fixes #1118
Fixes #1385
Fixes #1483
Fixes #1494
nicolo-ribaudo pushed a commit to babel/lerna that referenced this issue Dec 18, 2018
BREAKING CHANGE:
* `--preid` now defaults to "alpha" during prereleases:

  The previous default for this option was undefined, which led to an awkward "1.0.1-0" result when passed to `semver.inc()`.

  The new default "alpha" yields a much more useful "1.0.1-alpha.0" result. Any previous prerelease ID will be preserved, just as it was before.

* `--no-verify` is no longer passed to `git commit` by default, but controlled by the new `--commit-hooks` option:

  The previous behavior was too overzealous, and the new option operates exactly like the corresponding [npm version](https://docs.npmjs.com/cli/version#commit-hooks) option of the same name.

  As long as your pre-commit hooks are properly scoped to ignore changes in package.json files, this change should not affect you. If that is not the case, you may pass `--no-commit-hooks` to restore the previous behavior.

Fixes lerna#277
Fixes lerna#936
Fixes lerna#956
Fixes lerna#961
Fixes lerna#1056
Fixes lerna#1118
Fixes lerna#1385
Fixes lerna#1483
Fixes lerna#1494
@lock
Copy link

@lock lock bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants