RFC: Streamlining pytest's git workflow

[bluetech]

This RFC proposes changes to pytest's git workflow, with the aim of streamlining development.

Current situation

There are two branches, master and features.

CONTRIBUTING.md and the pull request template instruct contributors to

• Target master for bug fixes/doc changes/trivial changes.
• Target features for new features/improvements/functionality changes/removals/deprecations.

Patch releases are made by branching from master, making the release and merging back to master in the release PR.

Minor releases are made by branching from features, merging master, making the release and merging back to master in the release PR.

Occasionally, changes that pile up in master are merged back into features with a "Merge master into features" PR.

Problems with current situation

Contributors need to make a choice to target either master or features. This adds some friction.

Sometimes, they don't read CONTRIBUTING.md or the pull request template, and they target master when they should have targeted features. The maintainers need to ask them to rebase.

Sometimes, the maintainers disagree with the contributor's choice, and the contributor is asked to rebase.

Sometimes, maintainers themselves disagree if some change should go to master or just features.

To need for "Merge master into features" PRs adds work and overhead.

Sometimes, some feature development discovers bugs or minor fixes which should go to master. They then need to be split and submitted in a separate PR. The feature PR is then blocked until a "Merge master into features" happens.

Sometimes, changes that don't really need to go to patch releases are merged to master, and cause regressions in patch releases, which end users expect to be safe to upgrade to.

Proposed solution

The features branch is removed. Both feature and bugfix development happens on master.

Releases are made as follows:

Create a branch MAJOR.MINOR.x (e.g. 5.3.x), if one doesn't already exist from a previous minor/major release, branched from the latest MAJOR.MINOR.0 tag (e.g. 5.3.0), and push it to the main repository.

Branch from the MAJOR.MINOR.x branch, cherry pick commits for the release from master, make the release and merge back to MAJOR.MINOR.x in the release PR.

After the release, cherry-pick the changelog/announce from MAJOR.MINOR.x to master.

Rationale

Developing on just the master branch is the most common and "default" git workflow for contributors, so should not cause any confusion, and reduce the "bureaucracy".

Avoiding the two branches removes the synchronization overhead and friction.

The slight overhead of cherry-picking commits helps with ensuring that patch releases are minimal, in that only fixes that really affect some users and need to be in a patch release get picked. This reduces the chances of regressions.

If a release needs time to "bake", it is possible to create the MAJOR.MINOR.x branch earlier, when creating the 0 release, without blocking master.

pytest already sort-of works this way for major releases, with the 4.6-maintenance branch.

Prior art

The proposed workflow is quite common, but to stay close to home I'll mention two examples:

• CPython (ref): All development is done on master. Each release (e.g. 3.7, 3.8) gets a branch names MAJOR.MINOR. Changes from master can be backported to release branches. There seems to be a bot miss-islington which can be ordered to automatically create cherry-pick PRs. We can try something like that if needed.

• Django (ref): All development is done on master. Release branches are called stable/MAJOR.MINOR.x. Backports for patch releases happen regularly as needed.

• Pillow (ref) - see comment by hugovk.

Transition plan

1. Create 5.3.x from existing 5.3.4 tag, release 5.3.5 according to new workflow.
2. Freeze master.
3. Merge features into master.
4. Delete features.
5. Merge this PR.
6. Rename 4.6-maintenance branch to 4.6.x.
7. Ask current open PRs filed against features to rebase against master (one last time ☺).

Contingency

If the new workflow turns out to have unfixable issues, than worst case we can just revert back to change.

[bluetech]

As promised, this is my proposal. This is really more intended for discussion. Two things I didn't consider are the CI setup and the Read The Docs setup. I can look into that, but would appreciate any help/comments.

[blueyed]

Thanks, I agree with your suggestion.

(As for the transition plan we might want to not include Node.from_parent when merging features into master though, keeping it in a separate feature branch. That's what I would hope for though only (#6387 (comment) etc), and should not hold the transition back. So this should be discussed only further after general agreement has been achieved.)

[nicoddemus]

Thanks @bluetech, looks good!

[RonnyPfannschmidt]

lovely expansion on what i proposed a while back on the ml,
i like the bugfix branch workflow

[bluetech]

I added a "Contingency" section to the RFC, just for completeness (not very interesting).

I added a commit to update CI branch filters -- I don't think there is anything more needed for that?

Regarding RTD, I don't see a configuration file for it in the repo, but maybe I missed it. If it's updated from master, it means that it will start showing unreleased documentation. Maybe that's fine? Ideally it will render from latest tag or such.

[hugovk]

👍

Problems with current situation

Contributors need to make a choice to target either master or features. This adds some friction.

Sometimes, they don't read CONTRIBUTING.md or the pull request template, and they target master when they should have targeted features. The maintainers need to ask them to rebase.

Also, as a contributor, sometimes I don't read those until after making changes, and seeing the link in the PR editor, and then have to rebase/redo the work on the other base branch.

So from a contributor point of view, definitely +1 to a single master branch.

---

We have a similar process with Pillow (ref) and I think it works well.

Essentially:

• All development is done on master
• Major and minor releases (aka "main releases") are made from master
• During major and minor release, we branch master to, say, 5.2.x and tag 5.2.0
• For patch releases (less common, aka "point releases"), changes are first made to master, and then cherry-picked to the 5.2.x branch, then tagged as 5.2.1

---

Suggestion:

Create a branch MAJOR.MINOR.x (e.g. 5.3.x), if one doesn't already exist from a previous patch release, branched from the latest vMAJOR.MINOR.0 tag (e.g. v5.3.0), and push it to the main repository.

Either use or don't use v for both branches and tag names. It'll just get confusing to have to remember which scheme is used for which.

Existing tags don't use v, so I suggest not using for branches either. For example:

• Branch: 5.3.x
  • Tag: 5.3.4
  • Tag: 5.3.5
• Branch: 5.4.x
  • Tag: 5.4.0
  • Tag: 5.4.1
• Branch: 6.0.x
  • Tag: 6.0.1
  • Tag: 6.0.2

---

Very minor suggestion:

Great you have a HOWTORELEASE.rst to document this. Maybe call it RELEASING.rst?

RELEASING.rst|md is about 10x more common than HOWTORELEASE.rst|md

[bluetech]

Thanks for taking a look @hugovk.

We have a similar process with Pillow and I think it works well.

Thanks, that's a good reference, I'll add it to the text.

Either use or don't use v for both branches and tag names. It'll just get confusing to have to remember which scheme is used for which.

For some reason I thought the tags use v, but they don't. I'll remove the v reference from the text.

Great you have a HOWTORELEASE.rst to document this. Maybe call it RELEASING.rst?

👍 from me.

[bluetech]

@blueyed

(As for the transition plan we might want to not include Node.from_parent when merging features into master though, keeping it in a separate feature branch. That's what I would hope for though only (#6387 (comment) etc), and should not hold the transition back. So this should be discussed only further after general agreement has been achieved.)

In order to unblock this, I tried looking at the from_parent changes but wasn't able to move it forward for now.

I think the best course of action is to slightly change the transition plan so that we don't need to block on it. I'll describe the changes in a separate comment.

[bluetech]

I'd like to slightly amend the workflow. I'd leave it for a couple of days in case anyone would like to comment, and then apply it to the PR. After that, we could hopefully set a date for the transition.

---

Previously, I wrote that the MAJOR.MINOR.x branch is only created "as needed" for the first patch release. The idea was to avoid the branch if it's not needed. But I realize now that's not a great idea. For example, if some contributor wants to propose (or is asked to create) a backport PR, they need to have a branch to target, so it better exists!

So I'd like to change it so the MAJOR.MINOR.x branch is always created when MAJOR.MINOR.0 is released.

[bluetech]

I think the best course of action is to slightly change the transition plan so that we don't need to block on it. I'll describe the changes in a separate comment.

(Sorry, I got slightly confused, there are no changes needed. The transition plan as laid out branches 5.3.x before merging features, so the situation after the transition will be the same in that regard as it is now - from_parent is queued for the next minor release, but not for the next patch releases).

[blueyed]

I've pushed a 5.3.x branch (based on the 5.3.4 tag), looking into releasing 5.3.5 with the fix for #6517

[bluetech]

Thanks @blueyed. I applied your change (squashed).

I also updated the text and HOWTORELEASE according to my comment above.

[nicoddemus]

We might rename the branch too, I think.

Just need to update the name in py27-py34-deprecation.rst too.

[bluetech]

Good idea, this will be more consistent. I'll go ahead and add the doc changes to this PR, and add the rename to the transition steps.

[nicoddemus]

I think we might rename this to RELEASING.rst as @hugovk suggests. 👍

[bluetech]

Thanks for reminding me, I'll add it to this PR.

[nicoddemus]

One thing that is not clear for me from the docs: do we keep MAJOR.MINOR.x branches around forever, or do we delete them when they go unsupported?

Currently a version goes unsupported when the next minor/major is released: so for example the 5.3 series gets unsupported after 5.4 gets its first release.

I think we should delete 5.3.x when 5.4.0 is released for the first time. This avoids generating confusion that a bunch of 5.3.x, 5.4.x, 5.5.x, etc, on the repository means that those versions are still supported.

[bluetech]

My inclination is to keep the branches around forever, for the following reasons, however I don't feel strongly about this. If you prefer to delete, I can update the text - let me know.

1. It makes it easier to switch over to old reference points in git and in GitHub UI. Tags are less convenient because you need to check which is the latest first, while with the branch you just have .x. And in GitHub UI switching to old tags is takes more steps than switching to a branch. I do it often in django at least.

2. You never know when you might need that branch anyway, even if it's unsupported.

3. I don't like deleting things :)

Also, I don't really think that people will construe the existence of a branch to mean it's still supported, though maybe I'll be proven wrong about it... Confusion/fat finger might happen though.

[nicoddemus]

If you prefer to delete, I can update the text - let me know.

I don't feel strongly about it either, let's see if others chime in. If nobody does, let's go with keeping the branches around given you are the author of the PR. 👍

[bluetech]

If it becomes a problem we can always start deleting then!

[blueyed]

1. We need to streamline/clarify how tagging should be done.

With the 5.3.5 I've created it before merging like we did before, but it is inconsistent with the 5.3.4 tag on the 5.3.x branch (which I've created based on the tag).

I think it would make sense to create the tag on the 5.3.x branch directly, i.e. after merging release-5.3.5 into it.

2020-01-29 … Daniel  M─┐ [5.3.x] {origin/5.3.x} Merge pull request #6614 from blueyed/release-5.3.5
2020-01-29 … Daniel  │ o [release-5.3.5] {blueyed/release-5.3.5} <5.3.5> Preparing release version 5.3.5
2020-01-20 … Ran Be  │ o Revert "Fix type errors after adding types to the `py` dependency"
2020-01-25 … Daniel  o─┘ ci: codecov: only use "comment: off"
2020-01-25 … Ran Be  o Update CI branch filters for new workflow
2020-01-20 … Bruno   o [release-5.3.4] {nicoddemus/release-5.3.4} <5.3.4> Preparing release version 5.3.4
2020-01-20 … Bruno   M─┐ Revert "fixtures register finalizers with all fixtures before t… (#6496)

2. to get the docs from the release merged I've used the following, to be run on the 5.3.x branch. This could really get passed in the version/tag already, of course.

#!/bin/sh

set -ex

tag=$(git describe --abbrev=0 --tags)
branch=${tag%.*}.x
relcommit=$(git rev-parse "origin/$branch^{/^Preparing release version}")

git checkout --no-track origin/master -b cp-release
git show "$relcommit" -- doc/en/announce doc/en/changelog.rst | git apply --index -
git commit -m "$(printf 'doc: release-%s\n\n(cherry picked from commit %s)' "$tag" "$relcommit")"

This is similar to what was done for the 4.6-maintenance branch (point 6 in https://github.com/pytest-dev/pytest/blob/8b123446fd53e771c9894efbee6b987c0fee17ec/HOWTORELEASE.rst#release-procedure), and should be generalized, since our release branches are similar now.

[nicoddemus]

With the 5.3.5 I've created it before merging like we did before, but it is inconsistent with the 5.3.4 tag on the 5.3.x branch (which I've created based on the tag).

I think it would make sense to create the tag on the 5.3.x branch directly, i.e. after merging release-5.3.5 into it.

Indeed. We used to push the tag to release-MAJOR.MINOR.PATCH because nothing prevented other commits to get into the master/features in the meantime between creating the release-MAJOR.MINOR.PATCH branch and the actual release.

In the new workflow we don't have this risk, because the branches are isolated, so 👍 from me.

Also 👍 regarding cherry-picking the docs.

[bluetech]

I think it would make sense to create the tag on the 5.3.x branch directly, i.e. after merging release-5.3.5 into it.

Agreed. The current text does imply the tag should be created on the release branch, but I'll clarify it.

to get the docs from the release merged I've used the following, to be run on the 5.3.x branch

Yes, in the latest update I generalized this step of cherry-picking the doc updates to master from only running for the maintenance branch to be run on all releases. I kept the commands from the previous version, if they need improvement please push to this PR, I'll probably get it wrong myself :)

[bluetech]

Updated according to comments.

Thanks @blueyed and @nicoddemus for kicking it off with the 5.3.5 release.

Unless there are further comments, I'd like to set the transition to this Friday (2 days from now). I think I can do all of the steps myself.

[nicoddemus]

Awesome, thanks @bluetech for taking charge of this. 👍

[blueyed]

@bluetech
I still hope for master to not get "tainted" also by merging features as-is into it (#6571 (comment)), since you seem to want to do this in #6640.

I know that @RonnyPfannschmidt said he cannot be bothered to do this on a separate branch in #6387 (comment), but at least would like to "warn" about this "one last time". (I think there should be a refactor-nodes branch still, where this could evolve / get done - there is no reason to break plugins unnecessarily because of this, especially since currently it is only about the redirection, and no "real changes" in general).

I have not checked, but probably just running "mypy" with coverage reports before and after "node-from-parent" should indicate that we're not helping ourselves with this indirection, and from what I understood there is no plan / way to add typing to it at least.
(on top I find it harder to read item = X.from_parent(…) than just item = X(parent=…)).

I'm sorry to be annoying in this regard and will shut up after this eventually gets merged, but since my previous comment about it appears to have been ignored I wanted to state this again ("one last time").

[bluetech]

@blueyed I understand (and actually share...) your concern, but I think it shouldn't affect this transition, because the status quo regarding releases remains the same:

• Patch release branch (old master, new 5.3.x) doesn't have it
• Minor/major release branch (old features, new master) does have it

[blueyed]

@bluetech

@blueyed I understand (and actually share...) your concern, but I think it shouldn't affect this transition, because the status quo regarding releases remains the same:

• Patch release branch (old master, new 5.3.x) doesn't have it
• Minor/major release branch (old features, new master) does have it

Sure, but merging features into master now makes it more complicated to back out, just like e.g. merging #6387 "just because it is tainted already" (#6387 (comment)).
I.e. when it gets merged into master it cannot be simply removed from the history, but only actually reverted, while we currently could still have a features-without-node-from-parent branch (rebased) that could be merged.

[bluetech]

@blueyed,

So while I still do not approve it myself, I'd rather have this unblocked, and then can try/do the reverts myself.

Do you mean you're OK with merging features to master?

If so, I'd be really great to have assistance from an admin of the repo, because there are some things that need permissions (I think). To recap the transition steps, we should do the following, preferably in quick succession:

• Merge master into features (to make the next step clean).
• Merge features into master.
• Configure out any references to features branch in the repo settings.
• Delete the features branch from the repo.
• Rename 4.6-maintenenace branch to 4.6.x.
• Allow to target 4.6.x and 5.3.x (or maybe a regex?) in PRs in the repo settings (if it's configured there?).
• Merge this PR.

[nicoddemus]

If so, I'd be really great to have assistance from an admin of the repo

Just to clarify, you have the required permissions for all you listed. 😁

I would love to help, but I'm leaving for the day so wanted to leave that comment in case it helps.

[bluetech]

Just to clarify, you have the required permissions for all you listed.

Maybe I'm missing something but I don't think I have permissions to do these things:

• Change repo settings (I don't see a "Settings" tab that I see in my own repos).
• Merge/approve my own PRs (so can do the merges quickly).
• Delete/rename (protected?) branches.

At least I hope I don't :)

[nicoddemus]

Hi @bluetech,

I'm back, sorry for the delay.

Indeed some of the things will need to be done by an admin, my bad. Could you please update this PR after #6712 gets merged? I will then take it from there.

Thanks for the patience!

[blueyed]

@bluetech

• Delete the features branch from the repo.

I would wait with that until all PRs have been changed to use master, otherwise PRs might be broken. Should be no problem after features is merged, but better to let PR owners do it manually I guess. PR list: https://github.com/pytest-dev/pytest/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+base%3Afeatures
This does not have to block the other steps though, and might not be necessary after all, but GitHub might just not handle a missing base for PRs gracefully.

[bluetech]

My plan was to leave comments on these PRs. Excluding PRs from us who participated in this PR, it leaves 6 open PRs.

but GitHub might just not handle a missing base for PRs gracefully.

Let's see... If it doesn't handle it, we can defer actually deleting features until all of the PRs are moved over.

[hugovk]

In a test repo, I branched from features and created a PR against features. When I deleted the features branch, it automatically closed the PR:

• Test PR against features branch hugovk/test#23

[bluetech]

Aw, so we shouldn't delete it immediately. Thanks for checking @hugovk.

@nicoddemus, let's skip the "delete features branch" for now and I'll open an issue to track this afterwards.

[hugovk]

It's possible to change the base branch for PRs:

• https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-base-branch-of-a-pull-request

[bluetech]

@nicoddemus Updated.

[nicoddemus]

I will open a PR updating 4.6-maintenance and related docs, and also doing another features -> master merge. 👍

Duh the PR already contained the docs changed, so I just pushed 4.6.x, made it protected, and removed the old 4.6-maintenance.

For the next release we need to make sure to always merge features into master, at least until we remove features completely.

[nicoddemus]

@bluetech wanna do the honors of merging features into master now?

[bluetech]

🎉 @nicoddemus I sent a PR #6716.

[bluetech]

Okay, @nicoddemus did the technical part. All that's left now is to handle the open PRs against features. I'll open a tracking issue and comment on the PRs later today.

[nicoddemus]

All that's left now is to handle the open PRs against features. I'll open a tracking issue and comment on the PRs later today.

Thanks, appreciate it! 🙇