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

Smth reverts package.json changes after prepare step #1

Closed
antongolub opened this issue Mar 5, 2020 · 10 comments · Fixed by #2
Closed

Smth reverts package.json changes after prepare step #1

antongolub opened this issue Mar 5, 2020 · 10 comments · Fixed by #2
Labels
bug Something isn't working released

Comments

@antongolub
Copy link
Member

antongolub commented Mar 5, 2020

qiwi/substrate@5e6b5d2

@antongolub antongolub added the bug Something isn't working label Mar 5, 2020
@antongolub
Copy link
Member Author

antongolub commented Apr 24, 2020

@dhoulb, could you take a look?

I seems that updated manifest deps were reverted before the publish, but package version was bumped as expected.

  1. semrel-extra/demo-msr-cicd@aaff470
  2. semrel-extra/demo-msr-cicd@6565409
  3. semrel-extra/demo-msr-cicd@9061436

@dhoulb
Copy link
Contributor

dhoulb commented May 4, 2020

@antongolub Sorry, I took a quick look at this, but it was taking too long to understand your setup and I couldn't justify any more time on it.

The multi-semantic-release package was only intended as a proof of concept. I didn't consider it to be production ready — I wrote up a plan for semantic-release to allow them to support monorepos natively but I guess they don't have the time to implement it.

Since I created multi-semantic-release I've changed my opinion on semantic-release and monorepos. I think the step of coordinating and inter-relating the packages is too complicated to ever work well (and would be a maintenance nightmare).

As an alternative I now publish the entire repo to NPM and reference it using e.g. import { myFunc } from "my-package/my-sub-package"; — I find this hugely simplifies things.

@dhoulb
Copy link
Contributor

dhoulb commented May 11, 2020

@antongolub I've thought about this a bit further and I think it's an actual issue in multi-semantic-release

In the prepare step, each package writes its deps then immediately calls the prepare step for all other plugins (including the @semantic-release/git plugin's prepare step which is the one that commits the changes back to the step).

This means that when the prepare step in the git plugin is called for first package, the changes are committed even though not all packages have written their package.json yet.

It needs reworking to so that ALL package.json files are written before the prepare step is called for any of the plugins.

The way I would do this is adjust updateManifestDeps() in createInlinePluginCreator.js so that it loops through all packages and writes them all (on the first run only) before any plugins have their prepare step called.

Unfortunately I don't have time to do this right now — but if you have a little time to fix it please do!

@antongolub
Copy link
Member Author

@dhoulb, Hi Dave.

Thanks for the feedback. We'll try to check this conjecture asap.

@antongolub
Copy link
Member Author

antongolub commented May 14, 2020

@dhoulb, Dave

semrel runs its own git plugin prepare before the synthetic inlinePlugin, so I
attached the deps updater right after the last commitAnalyzer run. Dirty hack right here, right now.

Anyway, it looks to be working: semrel-extra/demo-msr-cicd@834daff

UPD#1
I guess, notesGenerator is even a better place. It goes right before the prepare step:

  await plugins.verifyRelease(context);

  nextRelease.notes = await plugins.generateNotes(context);

  await plugins.prepare(context);

  if (options.dryRun) {

UDP#2
And finally it woks as expected. Here's the fix: #2. Result: semrel-extra/demo-msr-cicd@e14b6d4
I need some extra time to prepare PR.

antongolub added a commit that referenced this issue May 15, 2020
@antongolub
Copy link
Member Author

antongolub commented May 19, 2020

@dhoulb, Dave

I'm also trying to replace execa hook and setImmediate loops with a signal bus. I found a good one — yanickrochon/promise-events.

But I was faced with another strange problem. The first published package release somehow completes the whole multi-sem-rel session, as like it triggers process.exit(0) and ignores the rest promises

await Promise.all(packages.map((pkg) => releasePackage(pkg, createInlinePlugin, multiContext)));

If you have free time, could you take a look?

Here's the code: pull/2
Here's the build log: builds/166994513

Screenshot 2020-05-20 at 11 36 55

@antongolub
Copy link
Member Author

It seems that semrel behaviour in CI differs from local. Logged: Inline plugin publish step was not loaded and was not triggered. So I just moved signal point to pre-success/fail stage and finally multi-sem-rel works now: builds/167111295. Unbelievable 🚀

antongolub added a commit that referenced this issue May 20, 2020
qiwibot added a commit that referenced this issue May 20, 2020
# [2.5.0](v2.4.3...v2.5.0) (2020-05-20)

### Bug Fixes

* publish updated deps ([791f55a](791f55a)), closes [#1](#1)

### Features

* add execa queued hook ([042933e](042933e))
* apply queuefy to plugin methods instead of execa ([9ae7d0d](9ae7d0d))
@qiwibot
Copy link
Member

qiwibot commented May 20, 2020

🎉 This issue has been resolved in version 2.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dhoulb
Copy link
Contributor

dhoulb commented May 21, 2020

@antongolub Great work!

@antongolub
Copy link
Member Author

This is not the end. It's necessary to fix semrel plugin config resolver.

dhoulb pushed a commit to dhoulb/multi-semantic-release that referenced this issue Jun 2, 2020
# [1.1.0](qiwi/multi-semantic-release@v1.0.3...v1.1.0) (2019-11-03)

### Bug Fixes

* **package:** add missed sem-rel plugins ([f3c9318](qiwi/multi-semantic-release@f3c9318))
* **package:** update execa to be compatible with sem-rel 15.13.28 ([069bb4e](qiwi/multi-semantic-release@069bb4e)), closes [#7](qiwi/multi-semantic-release#7)
* force a release ([1e3ece5](qiwi/multi-semantic-release@1e3ece5))

### Features

* add execasync CLI flag to make execa calls be always synchronous ([693438c](qiwi/multi-semantic-release@693438c)), closes [#1](qiwi/multi-semantic-release#1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants