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

Re-think the develop/stable workflow #2206

Open
matrss opened this issue Feb 15, 2024 · 17 comments
Open

Re-think the develop/stable workflow #2206

matrss opened this issue Feb 15, 2024 · 17 comments

Comments

@matrss
Copy link
Collaborator

matrss commented Feb 15, 2024

I don't like the way it is now.

  • PRs go to two different branches depending on what they do. This means development is happening on two base branches which will always produce unnecessary conflicts.
  • CI failures on stable are basically invisible, since the repository landing page does not show them prominently.
  • PRs going to a non-default branch do not automatically close issues that are mentioned as "Fixes #" in the PR description. This means PRs to stable always have to remember to manually add this link in GitHub.

My intuition on how this should work and what I initially thought it was like is this:

  1. all PRs go to develop
  2. for each PR, one can decide to also backport this PR to stable (e.g. make a cherry-pick on stable to apply the change there too) (note: I think it would be better to just call this branch by its released version number, e.g. a v8.3 or v8 branch that gets backports for fixes that should go to that version as a minor/patch version)
  3. whenever a new release is cut from develop stable is set to point at the same commit as develop

This would also completely eliminate any merge commits in the git history for the future. I don't like merge commits either.

@matrss
Copy link
Collaborator Author

matrss commented Feb 15, 2024

Even when explicitly linking a PR to an issue the issue is not automatically closed, for some reason. This happened in #2205: I've linked the PR to #2199 manually, then it was merged, and then the issue was still not automatically closed.

@ReimarBauer
Copy link
Member

github has some limitations compared to bitbucket where we started.

We have for a good reason a stable branch. This is used to destinguish what features and bug fixes are for.

Bug fixes have to go to the stable branch and a minor or patch level release is the follow up. We have to regular merge from stable to develop. This is a pretty good constant where we also know what a release is based on and what only gets to a user.

The main problem is, that we need long to make new major releases. But when we have constantly more people this can speedup.

@matrss
Copy link
Collaborator Author

matrss commented Feb 16, 2024

We have for a good reason a stable branch. This is used to destinguish what features and bug fixes are for.

I am not suggesting to get rid of stable, and I am also not suggesting to no longer differentiate between bug fixes and features. I am just suggesting to re-think the workflow between the two, since right now we effectively have two development branches, which leads to unnecessary conflicts.

Changes essentially flow in two ways: they either go to develop and will become part of stable whenever the next major release is made, or they go to stable and will be merged back into develop at some undefined point in the future. This means that stable is not, at all times, a subset of the changes on develop. The merge from stable to develop can become quite a headache if there are multiple changes conflated in it that touch stuff that was changed on develop previously, since you no longer have the contexts, in which these changes were initially made, separated and the one who does the merge (usually you?) is not necessarily the same person who had implemented the changes in the first place, making it even harder to judge how conflicts need to be fixed.

As an example: I've tried to backport my changes to the tests to stable. In doing so I noticed that it touches quite a bit of stuff that had previously been changed on develop. This means that if these PRs had gone to stable, then it would be pretty hard to now merge them into develop, because develop is different in some places that were changed. Specifically, the changes to conftest.py would not apply cleanly at all because the python settings files as written there have changed. Also, some tests that I've changed on develop do not even exist on stable, an issue that wouldn't come up as a merge conflict but just as failing tests or not at all.

What I am suggesting instead is to design the workflow in a way so that changes only ever flow in one direction. One approach to this would be a backport based workflow:

  1. have a develop and a stable (or v7, v8, v9, ...) branch
  2. all PRs always go to develop
  3. if a change should also happen on stable (i.e. a bug fix), then do another PR for the same change targeting stable
    • this would essentially move the potential for merge conflicts that exists right now to this PR, the conflict would happen in a context in which the intention of the changes is clear and in which no other changes are conflated into, since they have their own PRs

Step 3 can be mostly automated (unless there are conflicts), e.g. nixpkgs uses the korthout/backport-action with a corresponding actions workflow. A PR that should also go to stable then just needs a backport stable (or backport v7, backport v8, etc. if the branches were named like that) label and the bot will auto-create another PR targeting that other branch.

This would mean that the stable branch will, at all times, by design, be a subset of the changes on develop and no merge is ever needed in any direction, because changes only ever flow in one.

Bug fixes have to go to the stable branch and a minor or patch level release is the follow up. We have to regular merge from stable to develop. This is a pretty good constant where we also know what a release is based on and what only gets to a user.

You make it sound like this is the only way to achieve getting bug fixes on a stable branch and new features on a develop branch. It is not and there are alternatives (as outlined above) to achieve effectively the same with less potential for conflicts (i.e. no merge commits ever). That is all I am saying.

The main problem is, that we need long to make new major releases. But when we have constantly more people this can speedup.

The amount of people is mostly irrelevant, the level of automation in releasing is important. If releases would be automated to the point of pressing a button if all checks on stable (for a minor release) or develop (for a major release) are green, then we could release more often (and even automated once per week or something like that, if there are changes to be released).

@ReimarBauer
Copy link
Member

ReimarBauer commented Feb 16, 2024

Well mostly it is a sequence problem. We wanted to focus on doing a next major release. At the time where we had released it would have been ideally to overhaul the whole test environment.

But things are developed as they are. And it is also good to have better testings.

The amount of people is much relevant, it is not a one man project for only one user. This means when there is a campaign the only way we can help users is to make a minor or patch release. At this time it is very difficult to do a major and get this to the members of the campaign. Usually at campaigns we get many issues.

I have nothing against changing how we perform, but I want first seeing that we can faster create major releases. Both branches are very different now and this shouldn't be like this for much longer. I get closer to your idea on the next major, not now. Currently I doubt that an automated backported merge request works.

I had already in a different project the version branches. This were mostly triggerd by dependencies to linux distributions.
As long as we are only few people and have no linux distribution adding us (no compulsion or necessity) I want to avoid this.

Can we postpone this discussion after we have 9.0.0 or 10.0.0 released?

@matrss
Copy link
Collaborator Author

matrss commented Feb 16, 2024

The amount of people is much relevant, it is not a one man project for only one user. This means when there is a campaign the only way we can help users is to make a minor or patch release. At this time it is very difficult to do a major and get this to the members of the campaign. Usually at campaigns we get many issues.

I still think automation is the key to faster releases. E.g. stable should be in a releasable state after each and every commit, if all of the checks pass. So why not do an automated release daily, if there are new commits on stable?

The same idea could be applied to develop as well, if we take care that it stays in a state in which it can be released; i.e. no half-finished stuff goes into there. I am not advocating for actually doing scheduled automated releases of major versions, but having the process automated to the point of being a single button press would help a lot in reducing the barrier to do a major release when wanted.

Both branches are very different now and this shouldn't be like this for much longer.

I agree.

Currently I doubt that an automated backported merge request works.

I don't see why it wouldn't work. It would essentially be the same as the "PR to stable, then merge stable into develop" workflow we have now, except that the equivalent of "merge stable into develop" would happen after each and every change and its direction would be turned around so that there is a potential conflict between the PR and stable to be fixed, instead of between stable and develop. The potential conflicts would mostly be the same, except that the context in which they happen is changed (one merge PR vs one backport PR per change) and there is less opportunity for this stuff to pile up.

I had already in a different project the version branches. This were mostly triggerd by dependencies to linux distributions.
As long as we are only few people and have no linux distribution adding us (no compulsion or necessity) I want to avoid this.

I am not suggesting that we should support older major releases, other than the latest one. There is nothing wrong with completely abandoning a v8 branch once a v9 branch and release is made. But it would preserve the development history better, in an easier to grasp way, I think. But this is just a small nitpick regarding naming conventions.

Also, if a distribution decides to ship an old version I don't see any reason for us to support said old version. We just have to make clear what is supported and what is not, and direct bug reports to the distributions maintainers if necessary.

Can we postpone this discussion after we have 9.0.0 or 10.0.0 released?

Yes, definitely, I just wanted express some thoughts I had with this issue.

@matrss
Copy link
Collaborator Author

matrss commented May 24, 2024

To collect some practical issues the current workflow caused:

  • In Merge stable to develop 2024 #2323 at first msidp was missing from the entrypoints. I only caught this because I knew I had to look for it. This kind of issue would have been trivial to detect if the change it came from was first made on develop and then backported to stable, because then msidp would have been included where it shouldn't and that will cause an issue at install time, I am pretty sure.
  • In Merge stable to develop #2332 copyright notices for multiple files that only existed on develop, not on stable, were not fixed, even though it could have been fixed. The subsequent merge of that PR brought develop into an inconsistent state. Again, this would have been easier to deal with if the initial change was made on develop and then backported to stable, because then the missing would have come up as merge conflicts.
  • In Merge stable to develop #2367 there initially were proposed changes that reverted parts of an entirely unrelated commit. This would have been even more obvious if this happened in a backport PR for the specific change instead of one of those generic merge PRs, because then the context of the changes to be backported would have been clear and the erroneously changed lines would have stood out from that even more.

@matrss
Copy link
Collaborator Author

matrss commented Aug 5, 2024

Another thing that would not be necessary anymore with the workflow suggested here: manually enabling merges for the merge PRs. With the suggested workflow every PR would always be a squash, so no special configuration change is necessary, and the branch history would always be linear.

@matrss
Copy link
Collaborator Author

matrss commented Aug 5, 2024

Another issue with the current workflow: it is often unclear if a PR should go to develop or stable. Currently, the decision what to target is on the one who creates the PR. As a reviewer it is easy to forget to check the target branch, as has almost just happened in #2117.

If we had one development branch and used a backports-based workflow to get changes to the stable branch this would not be an issue: every new change would always target develop, and the decision to backport a change, or not do it, can be made at any point in time, even long after a PR was merged.

@ReimarBauer
Copy link
Member

I think I have to not only to forward issues to the next major but also to look on blocked PR and their issues.

During the creation of the issue it was not correct to place a milestone. I did because I was sure it won't be solved before we release.

But at the moment the fix is avalaible the MS needs to be verified where we as early as possible can solve our part.

@ReimarBauer
Copy link
Member

Another issue with the current workflow: it is often unclear if a PR should go to develop or stable. Currently, the decision what to target is on the one who creates the PR. As a reviewer it is easy to forget to check the target branch, as has almost just happened in #2117.

We have to care on the issues. On issue creation it might be sometimes not clear where the problem can be how solved or can a feature implemented without a new major.
It can be wrong to target a future major release instead a minor level in the issue. Sometimes you know that after working on the problem. In that case it was the wrong milestone from the beginning.

With patch level releases we are more familiar then trying to get minor releases in any case. The main discussion point is the decission making process which gets changes into a patch level release or a minor release. At the moment the API of develop is different it is an effort to do the same bug fix differently again.

@matrss
Copy link
Collaborator Author

matrss commented Aug 6, 2024

It can be wrong to target a future major release instead a minor level in the issue. Sometimes you know that after working on the problem. In that case it was the wrong milestone from the beginning.

That is an argument for not assigning milestones to issues at all. With a backports based workflow we could have issues without milestones and assign the next major release milestone to the PR solving the issue instead. Then, if and when a backport is done, the backport PR can additionally be assigned the milestone for the version to which it was backported.

@matrss
Copy link
Collaborator Author

matrss commented Aug 6, 2024

In general, it is impossible to correctly assign a milestone to an issue ahead of time without having the fix already worked out, at least in your head.

@matrss
Copy link
Collaborator Author

matrss commented Aug 6, 2024

Another issue with the current workflow: it is impossible to make a change only on stable. This is an edge case, but in hindsight that is what should have been done in #2205, since the issue only affected stable, not develop.

@matrss
Copy link
Collaborator Author

matrss commented Aug 6, 2024

Just to share this here as well:

MSS stable_develop workflow

@ReimarBauer
Copy link
Member

ReimarBauer commented Sep 1, 2024

I assume that we still have then branches based on develop which needs be merged into develop. Don't we have there the same situation? In develop will be changes which are unrelated to the refactoring finished.

Using develop with feature branches I would whish me also a practical improvement.

  • What merge conflicts were there? (Currently I see only the result, there is not a hint where a conflict was)
  • What does a merge of the solution look like?
  • We maybe should have a plausibility check, e.g. code after a return on same level

@ReimarBauer
Copy link
Member

About the last section with the major version branches. I don't want an inflation of branches. Are there other options?

@matrss
Copy link
Collaborator Author

matrss commented Sep 2, 2024

I assume that we still have then branches based on develop which needs be merged into develop. Don't we have there the same situation?

Yes, we would still have feature branches forked off from develop, which are used to propose changes in PRs to develop. Nothing changes in that regard.

No, feature branches for new changes are short-lived and mutable, so they are fundamentally different from having two long-running development branches with immutable history, as is the case with stable and develop. Either a feature branch does not have a conflict with the target branch, in which case it can be squash merged without issue even if it is not based on the current HEAD of develop, or it does have a conflict, in which case it can simply be solved by either merging develop into that feature branch or (my preference) rebasing the feature branch onto develop. In any case, this detail will be irrelevant once the PR is squashed.

What merge conflicts were there? (Currently I see only the result, there is not a hint where a conflict was)

I don't think it is possible to see that, that is information that just isn't available to GitHub to show to you.

What does a merge of the solution look like?

It always looks exactly like the target branch with the changes from the PR branch (what's shown in the PR to review) applied on top.

We maybe should have a plausibility check, e.g. code after a return on same level

I think you are asking for a lint rule catching obviously-dead-code? That is entirely unrelated to this issue, but I more or less brought that up already in #2293.

About the last section with the major version branches. I don't want an inflation of branches. Are there other options?

The branches can be deleted once that version will no longer receive any update.

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

No branches or pull requests

2 participants