Hi,
sajolida (2020-06-03):
> 1. syster created a MR for #17227 on:
> 2. Cody amended their MR by creating a MR against syster's repo:
> 3. Cody then opened another MR with the work of both:
> 4. When reviewing Cody's MR I added some commits to the branch
Wow, indeed that seems convoluted and painful. It took me a while to
think about this, learn about some GitLab features, and sleep on it.
I don't think we can make it entirely painless in the exact situation
you 3 people were in, i.e.:
- 2 rounds of reviews done by 2 different people, including 1 person
who doesn't have the commit bit.
- 2 of the 3 involved people cannot push to branches in tails/tails:
they can only push to their own fork. So they can't collaborate on
shared branches in tails/tails (see below for a possible solution
to this problem).
Below I'll point to a few features and workflow hints that may help.
> 1. syster created a MR for #17227 on:
>
> https://gitlab.tails.boum.org/tails/tails/-/merge_requests/8
So far, so good :)
> 2. Cody amended their MR by creating a MR against syster's repo:
>
> https://gitlab.tails.boum.org/syster/tails/-/merge_requests/1
>
> That sounds pretty convoluted to me already. Is this the proper way?
> 3. Cody then opened another MR with the work of both:
>
> https://gitlab.tails.boum.org/tails/tails/-/merge_requests/32
I think doing both (2) and (3) is sending conflicting signals
and creating confusion:
- (2) suggests that syster is still the "owner" of the task,
i.e. responsible to lead it to a state that can be merged
- (3) suggests that Cody took over the task
IMO one should pick only 1. Which one is better in a given situation
is a social question rather than a technical one. Regarding how to
implement each of them:
- Either syster remains the "owner" of the task
Possible implementations:
- What Cody did with (2) could be OK'ish for obvious improvements,
but indeed there's a risk that the discussion gets split between
2 MRs.
- Cody could have suggested changes on syster's MR (!8), and then
syster could have accepted those suggestions into !8:
https://docs.gitlab.com/ee/user/discussions/index.html#suggest-changes
- Cody and syster could share a fork on which they both have write
access. So they could have collaborated on the original MR: Cody
could have pushed his changes directly on top of the initially
proposed ones.
- Or Cody takes over the task
Possible implementations:
- Fork shared by Cody and syster (see above)
- Cody creates his own MR (!32) that supersedes !8,
and !8 must be closed (i.e. rejected as obsolete),
either by syster or by someone allowed to close arbitrary MRs.
It could be OK as long as the original MR (!8) did not have much
discussion, that would be lost by migrating future discussion
to !32.
> 4. When reviewing Cody's MR I added some commits to the branch:
> 3d8f1a284f..f8a5cd8289.
>
> I pushed these to doc/17227-letterboxing-tor-browser. They get
> automatically referenced on #17227 because I mention the issue in the
> commit message but they are not referenced on !32.
>
> My intention was to amend Cody's MR !32 with my commits and reassign to
> him for review but it doesn't seem to be possible.
Indeed, when creating !32, Cody did not tick the "Allow commits from
members who can merge to the target branch" checkbox. I see that since
then, Cody learned about this feature, e.g. on !58 he did tick that
checkbox, so if you wanted to amend his branch, I believe you could
do that \o/
Here's the doc for this GitLab feature:
https://docs.gitlab.com/ee/user/project/merge_requests/allow_collaboration.html
> What am I supposed to do now? Shall I Close Cody's MR !32 anyway, hoping
> that it won't get merged by magic and open a 4th MR for a single issue?
I think you could have asked Cody to edit the MR and tick the "Allow
commits from members who can merge to the target branch" checkbox.
> Does this mean that when going back and forth with reviews and
> amendments we have to create a new MR every time? Probably loosing
> comments and context each time?
I'm confident that the toolbox I've described above will avoid
the need to do that :)
> Shall we instead comment on the ticket not to loose information
> in-between these multiple MR?
In my experience (that's what we did on Redmine), that much less
pleasurable and efficient than discussing changes on MRs,
so hopefully the ideas & hints above will be sufficient to avoid
having to regress to that.
