Page MenuHomeSoftware Heritage

enable landing patches via the web UI for all repos
Open, HighPublic

Description

As shown in D1430 our processes for landing patches coming from external contributors aren't great (as in: not terribly user friendly).

I think historically asking contributors to just "git push" once their diffs are accepted used to work, but even if it is the case that isn't a great workflow, so I don't think we should try to fix it that way. Rather, we should make it trivial for us, core developers, to land accepted patches.

Again, I think some repos allow landing via the Phabricator web UI, e.g., swh-web, but not all of them to.

We should fix this, making sure all repos allows via-the-web landing and make sure all repos created in the future allow that too.

cc: @olasd as I think he is the person who made the magic possible for swh-web.

Event Timeline

zack created this task.Wed, Apr 24, 2:23 PM
zack triaged this task as High priority.
olasd added a comment.Tue, May 14, 2:37 PM

So, according to the fine manual (https://secure.phabricator.com/book/phabricator/article/differential_land/) the "Land Revision" button on the Phabricator user interface depends on setting up:

I did all these steps by hand way back when, then the worker I had that setup on was reinstalled and the setup broke (obviously).

All these Phabricator components have big scary red banners telling you not to use them and that they're broken and that they might change without warning. However, since I last touched them in 2016, they don't seem to have changed an inch, so they're probably more """""stable""""" (i.e. dead) than upstream claims.

I feel kinda awful setting up such a Rube-Goldberg machine just to be able to click a button to merge a f...abulous patch on a f...riendly web interface. I'd rather invest some time on letting a comment trigger a jenkins job that would push the patch once the tests pass.

olasd added a comment.Tue, May 14, 2:55 PM

(looks like Herald can't easily trigger on a comment, because that'd be too easy)

zack added a comment.Tue, May 14, 4:36 PM

Thanks for the investigation @olasd.

I think we need uniformity more than anything else. So if, for any reason, the web-based landing isn't an option, we should remove it from where it exists (I think that's just the Web UI repo, but I haven't checked.)

I'm not a fan of automated merging when the test suite passes. I think merging code from third parties should always be a manual step by someone. If it can be made a Web UI button, even better, if not we will just standardize to the local arc workflow, which I've recently documented in the wiki.

And of course it's not OK if we need to configure the thing manually for all new repos we create in the future, that would be too much of a burden (hence we'll surely forget about it).

olasd added a comment.Tue, May 14, 4:39 PM
In T1689#31539, @zack wrote:

I'm not a fan of automated merging when the test suite passes. I think merging code from third parties should always be a manual step by someone. If it can be made a Web UI button, even better, if not we will just standardize to the local arc workflow, which I've recently documented in the wiki.

That's not what I suggested. My suggestion was having *someone* trigger the merge with a comment on the diff, once the tests pass.

And of course it's not OK if we need to configure the thing manually for all new repos we create in the future, that would be too much of a burden (hence we'll surely forget about it).

Yeah, well, with Phabricator I don't think we'll avoid that.

zack added a comment.Tue, May 14, 5:01 PM
In T1689#31540, @olasd wrote:

IMy suggestion was having *someone* trigger the merge with a comment on the diff, once the tests pass.

Oh. Then yes, that would be cool !