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

Flagged prerelease bump fix #97

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

hanseltime
Copy link
Contributor

@hanseltime hanseltime commented Oct 1, 2023

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. #96 (comment)

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.

  • New code is covered by tests
  • All the changes are mentioned in docs (readme.md) - There are no external changes

@hanseltime
Copy link
Contributor Author

@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
Copy link
Member

@hanseltime,

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

* If you have a legacy fix where you are doing a work around and need the old behavior, please add useTagsForBump to config
@hanseltime hanseltime force-pushed the flagged-prerelease-bump-fix branch from 295951f to be24206 Compare October 8, 2023 03:43
@hanseltime
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@antongolub
Copy link
Member

antongolub commented Oct 13, 2023

@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
Copy link
Contributor Author

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

@antongolub antongolub merged commit a49d6b2 into qiwi:master Oct 16, 2023
5 checks passed
@qiwibot
Copy link
Member

qiwibot commented Oct 16, 2023

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prereleases not updating correctly
3 participants