qiwi / multi-semantic-release

Proof of concept that wraps semantic-release to work with monorepos.
BSD Zero Clause License
86 stars 34 forks source link

Flagged prerelease bump fix #97

Closed hanseltime closed 10 months ago

hanseltime commented 11 months ago

New PR

This is a new PR since it fundamentally changes the branch I already merged to my fork.

The Changes address this comment about flagging the old bugged behavior so old users have a way to sustain any of their work arounds. https://github.com/qiwi/multi-semantic-release/pull/96#issuecomment-1723479859

Fixes Issue 75

On Extensive debugging, the prerelease "next" logic that we use for bumping dependencies is disregarding the "_lastRelease" on a package and biasing on the git tags. This is an issue though, because we always end up having run the previous package's release thanks to the topo library, so there is a tag indicating the current release. Our desired release is this tag though and not the next increment. It is, however, the desired bump of the "_lastRelease" of that package.

Changes

To fix this, we keep a light touch by augmenting getNextPreVersion to:

  1. In config, supply a dep.skipEvaluateTags flag that can trigger the legacy bug behavior again
    1. Default is set to true since this is a true bug
  2. Removing the "process.cwd()" call for getting tags to be pkg.dir
    1. this was the reason existing prerelease tests passed despite that behavior not working (they would try catch the failed tag retrieval)
  3. With skipEvaluateTags:
    1. still consider tags if an tags set is provided to the getNextPrerelease function
    2. Only consider lastRelease if not. (Which is the only way that it is used currently).

The flag and README are updating since there is a path where users of this library may have already provided work arounds based off of the behavior. It is opt-in though, so that other users do not get the bugged behavior on new usage.

hanseltime commented 11 months ago

@antongolub - I have opened this new PR with a flagged changed and will close my old one. Please let me know if you have any further concerns. Thanks!

antongolub commented 11 months ago

@hanseltime,

Could you refer the new flag in bin/cli.js? And meow api will do the type cast str → bool on its side.

hanseltime commented 11 months ago

@antongolub - Sorry about the slow turn around on this. I updated the flags under the same commit.

I did realize that I hadn't written or used the correct naming for a boolean flag so I changed to deps.useTagsForBump so that the presence of the flag is is true.

Thanks for your patience!

antongolub commented 11 months ago

@hanseltime,

No rush required. Can we use the one param name for the entire flow? useSmth!skipSmth transformation creates ambiguity.

deps.tags — uses repo tags for version bumping, defaults to true. If \<case description> pass --no-deps.tags to disable.

hanseltime commented 11 months ago

@antongolub -

Just to clarify, are you asking for us to update the switch to deps.tags? and then push that through the pipeline? (Another note here, is that we never used the tags for stable release bumps, only prerelease. Hence the bug not being so pronounced, so I'm not sure we want to get the indication that "tags" is a simple mode.

Secondly, it looks like you're asking for that to default to true. I would push back on that request if it wasn't just an example. When I wrote the tests for multi-semantic release, I simulated the bug in an existing test because I found out that the tests weren't actually letting the tags get added due to a try-catch. Once that was fixed, the test was failing. Because of that, this is definitely a bug fix so I would make the fix opt-out. I do agree that the flag should be provided as an escape hatch for anyone with their own customized work arounds.

Let me know your thoughts and I'll get them swapped.

antongolub commented 11 months ago

@hanseltime,

I'm just speculating about what this fix will look like a half year later) I confess that perhaps I do not fully understand the current impl, and I believe you've figured it out better than me, so if you say both ´use´ and `skip` parameters are needed, I'll accept. If there is at least some possibility of getting by with just one, I would be grateful for such an improvement.

Thanks for you patience and efforts.

hanseltime commented 10 months ago

Thanks for the clarification! I have a welding class tonight but will try to get this finished tomorrow.

To your point about use and skip, I'm pretty sure I just got lazy due to the iterations. I can clean that up so it will be standard.

hanseltime commented 10 months ago

@antongolub - It should be all there now. We are just using useTagsForBump

antongolub commented 10 months ago

@hanseltime,

We're at the finish line. I renamed the new param to pullTagsForPrerelease to make it more self-describing, if you don't mind. Everything LGTM. Shall we merge now?

hanseltime commented 10 months ago

@antongolub - sounds good by me. Merge whenever you'd like!

qiwibot commented 10 months ago

:tada: This PR is included in version 7.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: