VersionBot: Grouping PRs into a single version #16
Comments
|
As discussed on the call:
|
|
@lekkas @pimterry
By doing this, it means we can only version up NPM modules when we're satisfied that it's time for a new version. The maintainer creates a new issue milestone which they want the features in, adds the TL;DR:
|
|
@hedss Sounds like a sensible way to stop version updates, but I'm not sure about the process to trigger version updates on-demand from there. I'm a little confused by the detail of some of your description, but it sounds like there's three ways to trigger a version update when a PR is merged:
I think "are we ready to launch a new version" is something that's normally going to be answered either by:
Option 2 (labels on PRs) supports the first use case wonderfully. I'm not clear what use cases options 1 and 3 support (it's rare imo that I know a release will should definitely result at the moment I'm committing, before later discussion, or when I file the issue). I don't think we currently support the 2nd use case (releasing after out-of-band discussion). If I don't have any open PRs then there's no way to trigger versionistbot, but that is a case that comes up semi-frequently I think (because we merge when a PR is complete, not necessarily when we've thought through all the context about what's next). Doesn't mean that needs to block this, since releasing from PRs is the most common case, so we should start there, but it'd be nice to solve that too eventually. |
|
I guess to satisfy the 2nd use case, there effectively needs to be an "empty PR" with the |
|
@pimterry Re-reading my comment, I changed mind half-way through and didn't rewrite the last section. :-/ It was meant to convey that the 'procbots/versionbot/update-version` should either be on the PR or the linked issue. So essentially, 2. from your options (but with added issues as well). Thinking about it, I think you're absolutely right about not including issues in case something's added by mistake. So, yes, add the label explicitly when you want to version up. @lurch I really don't like the idea of phantom files. They make me sad. :-( |
|
Ok, so it's just option 2 from the above then: if you have That sounds great to me as the first step here. With that in place, this works for the typical npm module workflows (afaik), and we can look at extending it to not require a PR in various ways as a later step. I'm pretty sure we can come up with something not too hacky for that later step, but we don't need to worry about it right now. |
|
@pimterry @hedss just read this thread (sorry for being late to the party) and this sounds good. So when using the
I also agree that it'd be nice to have a sensical way to signal version bumping (and, soon, publishing?) for npm modules that's not tied to merging a PR and till then I'm also fine riding the PR-merge process to do this |
|
We could in fact have this as a config option now: (Forget the prop name for the moment, we can 'discuss' that later ;-) ). So this says 'I wish to, by default, group all my PRs and not run |
|
@hedss yes, these are exactly my thoughts on what this config option would do (and true i'd suggest a different name for this config option :p but we'll sort this out). We'd then also need to specify the label name that 'triggers' a version up, as it's already discussed in the thread |
|
Just a quick question: there's talk about 'merging' versionist and VersionBot (whatever that means). Will |
|
@lurch It's a good question, there's some back and forth on this. We really need an API VersionBot can hook into and just use it as an NPM module, so we've been mooting the idea of just rolling it wholesale into VersionBot as a utility library. |
|
Ahh, thanks. From what you've described above, it sounds like maybe I don't have evidence of versionist being used as a standalone tool outside of resin.io , but I remember that being one of the original intentions of the project ;-) |
|
If we're keeping it as a standalone tool too, I'm happy with just having a single |
|
Cool. Going back to the original topic of this PR... ;) However, if Am I making sense?? ;-) |
|
I guess we could morph the labels slightly. If Then again, you could just set the TBH, I have no strong opinion either way. Except there's more code to add without a manual merge. ;-) |
Hmm, my only concern with having it done via the existing labels, is that it might be too easy to accidentally apply both those labels at the same time, without realising that it means VersionBot will automatically merge without doing a version/changelog update? *shrug* Am I being overly-concerned about nothing, and people would actually be happy with multiple "typo fix" entries appearing in their Changelogs? |
|
Well if we're grouping PRs, I'm not sure why you wouldn't wait till the next grouping to fix these typos anyway? Or patches that don't need a rollout now. I thought that was part of the point of grouping in the first place? |
|
I guess in hindsight I'm specifically talking about changelog-updating (i.e. should there be a mechanism for merging in typo fixes without "typo fix" entries appearing in the changelog?), which is subtly different from version-updating. Sorry for confusing everybody - let me know if that ought to be split out to a separate issue. |
|
Yeah, I can see the use case for this, and that makes sense. Grouping PRs is a change to disable version releases for some merges, this is a change to disable version releases and changelog updates for some merges. Sounds sensible and fairly easy to do imo though. Another label is an ok solution, but an even more interesting one would be a different change type, e.g. Or of course you could just use no-checks, merge the PR by hand, and ignore versionbot entirely 😄 Separate issue for discussion on this one is probably sensible 👍. |
Hah, I was already considering suggesting exactly that (i.e. a change-type level 'below' Will create a new issue in the morning 👍. |
|
I want to bring @alexandrosm and @lekkas back into this, as there's now two different trains of thoughts on grouping and always bumping. |
|
@lurch Any |
|
So, with that little side-issue now split off to it's own discussion, I still wonder if, when Also, if |
|
I'm in general very negative to not publishing a version for
"psychological/aesthetic" reasons, as it's counter to the spirit of semver
and versionist. Semantic versions are not supposed to carry any marketing
meaning. On the particular point of not wanting to jump several major
versions very closely, I'd be willing to make a concession and say that
with an additional tag of the type `bump-version: false` to omit the
bumping, but only for major versions.
Happy to set up a call to discuss this further.
…--
*Alexandros Marinos*
Founder & CEO, Resin.io
+1 206-637-5498
@alexandrosm
On Wed, Mar 8, 2017 at 5:19 AM, Andrew Scheller ***@***.***> wrote:
So, with that little side-issue now split off to it's own discussion, I
still wonder if, when group-prs: false is set in .procbots.yml, there
should be a label you can apply to a PR to signify 'merge this PR (and
update the Changelog) but don't bump the version just yet'?
Also, if group-prs: true is set in .procbots.yml and the
procbots/versionbot/update-version label is set on a PR (to indicate that
a version-bump is now required), would it be useful for VersionBot to
produce a GitHub link that could be clicked on to view all the differences
(or perhaps just the differences in the Changelog) since the last
version-bump?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLUCOHuOVr4i24spdtJIrCfgyClXBkxks5rjqrSgaJpZM4L7CaJ>
.
|
|
Hmm, ok, I think being able to batch patch & minor updates as well makes for a nicer model. We should talk about this in the next process call. My main issue is that doing this piecemeal makes the development flow really strange:
You could have some kind of 'version frozen' state you flip into on a non-bumped major change, which you have some process to get out of later, and which disables all versioning in the meantime. I think there is a route through there, but it all gets a bit complicated. On the background too though: I can't talk for versionist, but semver doesn't have much to say, in spirit or otherwise, about doing more small rapid-fire bumps, and quite a bit of the spec actively encourages thoughtful & carefully managed bumping instead. For example, the downsides of doing major releases in quick succession is explicitly in there. That isn't a concession against semver, it's a more or less a recommendation from the spec itself. I definitely do agree we should avoid creating lots of versions for purely aesthetic reasons. Npm modules are different to live components though, as there's much more practical effect from a version bump. Repeated major version bumps clearly make real pains, but even for non-major versions it's inconvenient to downstream users if we create many many minor and patch versions. Their builds and dev environments become less reproducible (the output changes each time we do a release - frequent irrelevant releases make this messy), our changelogs gain noise and get harder to follow, and as a person it's harder to understand what the contents of a release is (rather than a series of related features/bug fixes arriving together as a coherent whole, they come out in a series of separate releases). Anyway, we should have a call to cover this properly. I'll ping in flowdock. |
|
Not sure how much of it applies to the discussion here, but I stumbled across this the other day: |
Yes, that's the plan, and if that's the case, I think this works great. If you send many PRs to a branch, instead of master, and then merge that branch into master later, they should get treated as if it was just a single PR to master with all those changes included (separate changelog lines for each change, but a single version bump). I haven't tested this yet, but I will shortly. Is there some reason that doesn't solve this issue?
Yeah, that bit's tricky, because you need to start merging created changelog files, not just parsed entries from the commits. Probably doable, but I'm pretty sure it doesn't work quite yet. You'd also need to be able to add changelog entries without bumping the version anyway (as in the original plan on this ticket), or to have some kind of version squashing or something... More complicated either way. I definitely agree though that this info (what's about to be added to the changelog) would be helpful, but that's true on normal PRs too. I've filed a separate ticket for that part: #67 |
If it works, sounds to me like it's a good workflow.
Exactly, that's why I suggested that changes made to changelog on a branch, should be ignored when merging that branch back into master. IYSWIM! So the master changelog would only ever be updated by versionist parsing the commit history. |
Ah, ok, I can see how you could do that kind of thing. You don't need to do ignore it file by file though, you could do it by commit author. If versionbot filters out its own commits before merging, you'll get the expected result. I'd rather this was surfaced in the github ui though, simply because doing magic to add and then automatically remove things from the git history sounds fiddly and error-prone. Anyway - this should be on #67, not here. |
|
@pimterry SGTM |
|
@pimterry @lurch I just reread the thread, I'm strongly against committing a changelog file in the release branch that will be ignored when the release branch gets merged onto
So I'd rather we move forward with the plan Tim nicely summarised here and update VersionBot to
@hedss how does this sound to you? |
Fair enough. I still think it might be nice to be able to have some kind of 'preview' of what additions a release branch would add to the |
|
I dunno if it's something that the github interface already enforces, but presumably we shouldn't allow a release branch to be PRed back to |
hedss
changed the title
|
@hedss are we closing this? |
|
Yes. Yes we are. |
|
@hedss what was the final resolution of this? Do we need to update any process docs? |
|
@lekkas I don't believe so, I don't think we ever said you'd be able to group PRs in the Commit/PR doc. (having scanned it, can't find mention of such). |
|
OK , just to clarify because this is a recurring question - We have mentioned this alternative path in the past: a. Create a temporary Is this a workflow supported by VersionBot? In any case, do we abandon the concept of grouping PRs to a single version at all cc @alexandrosm ? Asking because I'd love VersionBot in |
|
@lekkas Yes, that would work because only |
|
But does a branch being non-protected (i.e. not |
|
VersionBot would still operate on the branch, yes. The events are per-repo, not per-branch. And yes, if a maintainer added the label to the branch that wasn't meant to be doing the bump, then you'd get a bump. I guess one way round this would be to have a new property in the Thoughts? |
|
Apart from the name, SGTM ;-) It's a bit early in the morning for me to think about it too hard, but in which circumstances would you want |
|
Yes, we could warn (though presumably the maintainer would have set it up in the first place). You wouldn't neccessarily end up with two branches of the same version. Imagine a policy where all external contributions get merged into a holding branch with no version bumping, and all internal contributions get merged into another holding branch which is bumped. Finally you want to merge both branches together onto master, you merge them into another branch which is then merged to master. You get a final version bump which essentially versions all of the externals in one lump, thus creating your now publishable version. Maybe it's a bit far fetched. :) |
Totally off-topic, but could that also be done by cherry-picking commits from both branches into a single PR against master? (I've never gone that complex with git!) |
|
Yeah, it def. could, but it'd be a bit more work. :) |
In the first procbot iteration we will basically cut a new version for every merged PR. For PRs that are related, we could use a scheme like:
Proceed with the review/merging process as usual, but only run versionist (i.e. cut a new version and create changelog) after the last milestone PR has been merged.Actually, we'll have to merge all grouped/milestone PRs just before running versionist. I'm not sure how we can signal process bots to start the process, the first idea that comes to my mind is simply closing the milestone, but we can find something better I guess
The text was updated successfully, but these errors were encountered: