When collaborating with contributors using a forking workflow, which is typical for most open source projects, being able to rebase and resolve conflicts is extremely useful. GitHub offers an option to Allow edits from maintainers when opening a Pull Request. GitLab should offer this as an option too.
Proposal
Add a checkbox to Allow edits from maintainers of the upstream project when opening a Merge Request from a fork
Allow users with Git write permissions to the upstream project to push to the branch on the fork
Allow users with Git write permissions to the upstream project to restart CI jobs on commits in the branch on the fork
The permission should:
only be available when both projects are Public or Internal
Given that, are we sure this doesn't need product work? I have a few questions, just from an initial look 🙂
What other read permissions are granted? Can you view the repository if the fork is private, or is that hidden apart from the branch you can push to?
Wouldn't we also want to let someone update a GitLab CI job, so that they can retry if something fails?
If the author of the MR can't force push to the branch that they have created (because the branch is protected), should upstream developers be able to? (In EE, we have branches that can be protected for a specific user, too, but I'm not sure we'd ask an OSS contributor to make that change too.)
Do the permissions persist if the MR is closed or merged? (Sometimes people create MRs from the master branch in their fork, for instance.)
@sarrahvesselov could we get someone to look at this ahead of 10.2 please?
Interested where to put the option in our various flows.
You can obviously include it as a setting, and perhaps in the MR flow if you're submitting an MR back to the canonical repository it could almost be enabled by default.
@jramsay : You shouldn't need it the widget itself. (Since it's a setting associated with the merge request itself). So it doesn't make sense to have it in the widget. The checkbox options in the widget are when you make a decision prior to clicking merge. So this doesn't apply. The edit form will be sufficient.
On the widget itself it might be nice to indicate the selection though. @cperessini should be able to determine that. See this similar design:
James Ramsay (ex-GitLab)changed title from Allow modification of branch by project maintainer to Allow modification of branch by project maintainers when opening an merge request from a fork
changed title from Allow modification of branch by project maintainer to Allow modification of branch by project maintainers when opening an merge request from a fork
@jramsay GNOME would also like this, it's their number 2 priority at the moment since it makes it easier to rebase contributor MRs :) If we could make sure it's in 10.6 that'd be awesome.
What other read permissions are granted? Can you view the repository if the fork is private, or is that hidden apart from the branch you can push to?
The description says:
The permissions should only apply to the source branch and should only apply while the merge request is open.
Does that mean that the maintainer can only fetch the code using git, and push to the single branch that is the source of the merge request? No other project pages are visible if the project permissions do not allow it?
@reprazent@jramsay Hmm. With open source projects, private forks are an edge case we could decide to ignore (by only exposing this feature on project that are internal/public and have the "Repository" feature set to be internal/public), but with private projects, all forks will be private too, and of these we likely have many.
Could we support pulling from/pushing to a magic MR ref in the target project, and automatically updating the backing source branch accordingly?
If the project is private, I think it would be unexpected if we give the maintainer any kind of access to any data other than the source branch, including other branches, even if read-only. And of course, Git doesn't support read permissions per branch.
@DouweM@reprazent Thanks for drawing my attention to those questions. I had mostly considered this issue from the perspective of Git access, not interface access.
I agree with @DouweM that accessing anything but the source branch of the MR is unexpected, and given that there is no way to control read permissions within a repository, I can't see how this feature would work for private projects.
If we exclude private projects from this feature, and limit the scope of changes in the feature to only only Git write permissions to the source branch, what edge cases or problems remain?
I can think of one: the interface wouldn't permit the maintainers to edit the source branch through the GitLab web interface. Are there any others?
@jramsay Why is that an edge case if we exclude private projects? On public/internal projects, we could allow the maintainer to edit files in the source branch through the web interface. If they want to use a different branch, we'd need to create the new branch in the maintainer's fork.
Note that by excluding private projects, we are excluding a large part of the potential users for this feature: companies that use the fork-workflow rather than giving every developer access to the main repo.
@DouweM I didn't mean excluding private projects was an edge case, but was meaning considering only public/internal projects are there any other difficulties. We could definitely make the GitLab interface work for public and internal projects, but it isn't necessarily a requirement of the first iteration of this feature.
Back to the private project scenario, we can't restrict read access to the Git repository, but in the simple case that may not be a problem: I fork the project, create a branch with changes and open an MR upstream. There isn't any private data in this situation.
The read problem becomes problematic if the fork contains multiple branches that differ from the upstream repository, particularly if there isn't any intention to contribute them upstream.
Maybe this is what you were getting at in https://gitlab.com/gitlab-org/gitlab-ce/issues/22292#note_58198207, I wonder if we could somehow make the downstream branch available on the upstream repository as a special ref they can checkout? (e.g. `git checkout -b feature-x origin/mr/1234/head) This seems like it would be quite complex to implement.
GitHub doesn't have to worry about this complexity because you can't make a fork of a public project private.
Private forks inherit the permissions structure of the upstream or parent repository. For example, if the upstream repository is private and gives read/write access to a team, then the same team will have read/write access to any forks of the private upstream repository. This helps owners of private repositories maintain control over their code.
@jramsay Do you think we could do the same thing GitHub does, as in, automatically give any private project member access to any forks of that private project?
@DouweM that was where my thinking was going and definitely seems like the cleanest solution, but I wonder if there is a reason this wasn't done previously?
I just tested forking a project into a private group, and I can't open a MR back onto the public project. Are we overcomplicating this? The MR access controls may already solve this.
@jramsay If I fork a private project, the fork will be private right? That seems like a use case we should at least cover. I'm not sure what happens if you create a fork of a public project, make the fork private, and try to create an MR.
As discussed with @DouweM, we could use the fork network to determine rights to certain forks: We allow members of the root project to access all the forks of the projects (direct or indirect).
We would be able to restrict this access to be read only (similar to anonymous users to public projects), and allow pushing by Developers & up when the MR-creator checks Allow edits from maintainers.
This does mean we are changing the policy somewhat: Before members of the source of a fork would not have access to the forked projects, now they would. We would also need to display this in the Members section of the project, mentioning that this access can be revoked by unlinking the fork.
@jramsay Another requirement should be that I can restart jobs of a pipeline. I'd be unpleasant if I can push, and trigger a new pipeline, but can not restart an individual job.
@reprazent@DouweM as we discussed, this issue will only address allowing modification by maintainers for projects where neither the source or target project is private.
For forks of private projects, inheriting permissions from the parent project would seem to be a good solution but one that would impact existing forks. For this reason, let's defer to a future release https://gitlab.com/gitlab-org/gitlab-ce/issues/43272
@zj good point - this has been added to the description.
@reprazent I don't want to be annoying, just want to make sure In dev means it's being worked on for 10.6? :D GNOME would love to have this in time for Google Summer of Code.
This feature seems to be exactly what we need to ease handling of our forks on our githost.io EE install - but having upgraded & tried it, it seems the checkbox defaults to off and there's no way to make it default to 'on'?
This still means in the general case our project approvers still can't rebase merge requests from forks (we use a ff-only workflow) - they can only do it if the user remember to set the checkbox.
Is this correct or did I miss something? To be honest, "allow edits from maintainers" is overkill and a confusing term compared to what we actually want, ie. "allow maintainers to merge this pull request". If you word the setting like that, it in no way makes sense to default it to "off"!