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

fix(version): Prioritize --preid over existing prerelease ID #1568

Merged
merged 2 commits into from
Aug 15, 2018

Conversation

moe971
Copy link
Contributor

@moe971 moe971 commented Aug 14, 2018

in order to switch a prerelease id from alpha version to rc I needed to change condition to prioritize preid insteed of an existing preid

Description

I juste change order of OR statement to take first preid
const resolvePrereleaseId = existingPreid => preid || (isPrerelease && existingPreid) || "alpha";

Motivation and Context

On my continuous integration workflow version my app on alpha prerelease, then on it goes to release candidate, so I try tu use lerna version prerelease --preid rc but because my previous tag was dam/library-client@0.1.1-alpha.10 lerna 3.0.4 used my existing preid.
The solution was to change order on the test statement.

How Has This Been Tested?

It has been test on my project, it works 👍

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

in order to switch from alpha version to rc for exemple change condition to take first preid insteed
of existing preid
Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks reasonable, but it needs a test. Copy this test, and pass "--preid", "rc" after the "prerelease" argument to lernaVersion(), then validate that the changes are as expected.

@moe971
Copy link
Contributor Author

moe971 commented Aug 14, 2018

Thanks for the review, I add the test as you asked me.
I ran npm test -- -u to update snapshots, everything is ok on my side but fail on CI..

@moe971 moe971 force-pushed the fix/prioritize-preid branch from 3282dff to 317a1ca Compare August 14, 2018 19:34
Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the snapshots need updating, as you're not even adding a new snapshot. The lockfile should also not be changed (you're not adding or removing dependencies)

"version": "1.3.3",
"resolved": "https://registry.npmjs.org/JSONStream/-/JSONStream-1.3.3.tgz",
"integrity": "sha512-3Sp6WZZ/lXl+nTDoGpGWHEpTnnC6X5fnkolYZR6nwIfzbxxvA8utPWe1gCt7i0m9uVGsSz2IS8K8mJ7HmlduMg==",
"version": "1.3.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lockfile should not have changed (npm ci should be sufficient). Please revert this file

--- a/packages/package-1/package.json
+++ b/packages/package-1/package.json
--- c/packages/package-1/package.json
+++ w/packages/package-1/package.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why these snapshots needed to be updated. Are you using a non-English localization of git?

Copy link
Contributor Author

@moe971 moe971 Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on an english localization of git, just my OSX is in French. I revert my package-lock, ran npm ci the only diff is the new test. I have the same problems with snapshots on my computer, but it pass on CI 👍

revert package-lock file
@moe971 moe971 force-pushed the fix/prioritize-preid branch from 5384039 to c9f12ec Compare August 15, 2018 04:56
@evocateur
Copy link
Member

evocateur commented Aug 15, 2018 via email

@moe971
Copy link
Contributor Author

moe971 commented Aug 15, 2018

As you said after revert everything is green on CI, thanks for your help.

@evocateur evocateur changed the title refactor(version): prioritize preid on existingPreid fix(version): Prioritize --preid over existing prerelease ID Aug 15, 2018
Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@evocateur
Copy link
Member

I don't think this solves #1214, as it's actually a separate code path (but certainly accesses similar properties).

@evocateur evocateur merged commit f2c470a into lerna:master Aug 15, 2018
@evocateur
Copy link
Member

I think passing an explicit --src-prefix=a/ and --dst-prefix=b/ will solve your local snapshot problems. Can you pull from master and execute npm run ci to verify for me? (no rush, just not certain how to reproduce on my own machine)

@moe971
Copy link
Contributor Author

moe971 commented Aug 16, 2018

I sync my repo,
My current commit match with 3.0.6 tag, I run npm i && npm run ci I've got 526 lint error for import/unresolved, did I miss something ?

@evocateur
Copy link
Member

evocateur commented Aug 16, 2018 via email

@lock
Copy link

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 this pull request may close these issues.

2 participants