Skip to content

Merge stable to develop#2482

Merged
joernu76 merged 11 commits intodevelopfrom
merge_stable_to_develop
Aug 28, 2024
Merged

Merge stable to develop#2482
joernu76 merged 11 commits intodevelopfrom
merge_stable_to_develop

Conversation

@ReimarBauer
Copy link
Member

Purpose of PR?:

get recent fixes into develop

@ReimarBauer ReimarBauer requested review from gisi90 and joernu76 August 22, 2024 15:08
@joernu76 joernu76 merged commit 2c89c79 into develop Aug 28, 2024
@matrss
Copy link
Collaborator

matrss commented Aug 28, 2024

My understanding was that these kinds of PRs should be merged using a real merge, not by squashing them.

@joernu76
Copy link
Member

dammit, my bad. shall I revert this?

@joernu76
Copy link
Member

We can revert by MR or by force push. probably should use the revert feature?

@matrss
Copy link
Collaborator

matrss commented Aug 28, 2024

I think force pushing would be a bad idea since it is the public develop branch, not some feature branch. At least one of the GSoC students has already pulled the squash commit, so force pushing would necessitate some cleanup. Revert seems better, but ultimately I would defer that decision to @ReimarBauer.

@ReimarBauer
Copy link
Member Author

I think also revert is better. Please prepare that. We learn also.

joernu76 added a commit that referenced this pull request Aug 29, 2024
@joernu76
Copy link
Member

Yes, we should rediscuss this. This squashing/non-squashing is always an issue. gitlab handles this much better IMO.

@matrss
Copy link
Collaborator

matrss commented Aug 29, 2024

FWIW, if we used a backports-based workflow as suggested in #2206 then this kind of accident would become impossible. Every PR merge would always be a squash. Also, there would be no confusion as to which branch to target with PRs, as (almost) all changes would go to develop first.

I don't see how GitLab would handle this differently, as long as there are different kinds of PRs that need to be treated differently there will be a potential for these mistakes.

matrss pushed a commit that referenced this pull request Aug 29, 2024
@ReimarBauer
Copy link
Member Author

ReimarBauer commented Aug 29, 2024

The downside of the approach is that I think it fits good to EffVer and I have doubts that we will do the right changes for a released version having SemVer.
We need to have a different discussion than over all kinds of PRs to get to a decission.

@joernu76
Copy link
Member

Gitlab allows to determine the kind of merge upon creating the Merge Request via checkboxes. I.e. the creator can set them properly and the reviewer can check. 4-eye-principle. Whereas github forces the sole responsibility on the button-presser.
Another way to reduce the problem would be to immediately merge stable into develop after each-and-every fix, which would probably be a good idea anyway.
We really should schedule a day to discuss things. I doubt that there is an obvious solution to our problems, but would like to be positively surprised.

@matrss
Copy link
Collaborator

matrss commented Aug 29, 2024

Gitlab allows to determine the kind of merge upon creating the Merge Request via checkboxes.

OK, I didn't know that. That indeed would make the situation somewhat better, but the result would be the equivalent of what we have with targeting stable vs develop for each PR: the creator of the PR must know what to choose, and the reviewer must check it. We have seen that this review is also easy to forget with the target branch.

A backports-based workflow would completely eliminate these potential pitfalls: essentially every new change goes to develop, every PR is squashed. Every merged or un-merged PR can be labeled as to-be-backported at any point in time, even long after it was merged, and as long as there are no conflicts an automated process can create a backport PR targeting stable. If there are conflicts the backport must be done manually. But then again, this would be a situation in which, right now, we would get semantic conflicts in the "merge stable to develop" PRs, which can be hard to spot when these PRs combine a bunch of unrelated changes into one and aren't even reviewed for semantic correctness of the changes.

I think there is a reason that many major open source projects supporting more than their develop version use this kind of workflow.

We really should schedule a day to discuss things.

Yes please.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants