Page MenuHomePhabricator

Default to feature branch in Vector
Closed, DeclinedPublic

Description

Given the current pandemic crisis we would like to continue work on desktop refresh without adding risk to production wikis.

As a result we decided to default Vector code to a feature branch for the forseeable future and only apply essential patches to Vector's master branch as and when needed.

Event Timeline

Change 581108 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Set default branch to DIP

https://gerrit.wikimedia.org/r/581108

@JTannerWMF @ovasileva following up here as well as Slack as got some unexpected push back here about temporarily changing the default branch. @Jdforrester-WMF has raised concerns on the ticket about doing this because of "potential breakage for everyone else". I'd like to understand these concerns better as freezing master or stopping trains seems like the most sensible thing we can do for site stability right now.

Apparently we have the option of using a non-default branch but I don't see how that's better want the team to have the additional anxiety of seeing big architectural changes happening and having to worry about whether the right code is in the right branch. If that really is the only option I'd like to suggest we completely freeze desktop refresh work. I don't feel comfortable with us proceeding with the current changes in flight in this form with the limited and distracted resources we have.

I suggest reaching out to RelEng to discuss our options if any here if that's a problem.

Security / UBN patches still need to get applied, even if we're in feature freeze. Breaking critical workflows just to save a handful of people from needing to manually write " DIP" is not a good idea.

Apparently we have the option of using a non-default branch but I don't see how that's better want the team to have the additional anxiety of seeing big architectural changes happening and having to worry about whether the right code is in the right branch.

Gerrit has a "Move Branch" feature/button. It makes it *very* easy to move a patch onto another branch if you put it on the wrong one...

Slack

It may be worth reconsidering the use of such exclusionary mediums, as I understand there is a Slack instance that is restricted to @wikimedia.org G-Suite account holders only (i.e., basically foundation staff and contractors).

freezing master or stopping trains seems like the most sensible thing we can do for site stability right now.

If that happens I think it should be decided for all repositories what the policy should be and it should come from RelEng or TechCom, possibly involving an RfC. This is not up to us as individual developers or other groups of developers.

I don't see how that's better want the team to have the additional anxiety of seeing big architectural changes happening and having to worry about whether the right code is in the right branch.

People +2ing changes in Gerrit should not be ignoring what branch they're merging to - if they can't be relied upon to review this then I probably would not vote for them to be granted +2 rights. This is something that should happen at review time, approvers cannot rely on contributors or default config (plus assuming contributors use the git-review tool at all) to get it right.

If that really is the only option I'd like to suggest we completely freeze desktop refresh work. I don't feel comfortable with us proceeding with the current changes in flight in this form with the limited and distracted resources we have.

That sounds unnecessary. Risky changes can simply be merged to a separate branch (in fact with a separate branch it may be possible to skip review entirely, as long as they get reviewed before going to master or a deployment branch) until developers are comfortable with them going through master and the deployment train.

I recommend not to change the default branch to a branch that will not be deployed. We have our guidelines in place ( https://wikitech.wikimedia.org/wiki/Deployments/Covid-19 ) and expect the train to continue and code to still be merged for the foreseeable future. The guidelines above and the email from Grant and Toby encourage WMF teams to instead focus their time on other activities (eg documentation) *instead of* big architectural work. We (Engineering Productivity and Release Engineering) do NOT want teams to just being building up a large amount of big changes that are bottled up for when these guidelines are removed. That will only cause immense pain later for us, SRE, and others.

tl;dr: focus your time on other activities, save your big architectural work until later, keep working on the master branch so that your changes go out quickly/normally to reduce the build up of issues in the future.

Thanks @greg for the follow up here. If the concern is we don't want a big future release to build up that's totally fine and if that's the case it sounds like a feature branch of any kind is not a good idea and I will feed that back to my product team.

I do think the replies prior to your message objecting to this change have been a little flippant and to be honest confusing and I'd be keen to understand where they are coming from for future reference. They give an ambiguous message.

Gerrit has a "Move Branch" feature/button. It makes it *very* easy to move a patch onto another branch if you put it on the wrong one...

As @Reedy puts it this is easy, but this works both ways regardless of what the default is. If the default is DIP I can move patches or cherry pick them to master is accordingly.

Security / UBN patches still need to get applied, even if we're in feature freeze. Breaking critical workflows just to save a handful of people from needing to manually write " DIP" is not a good idea.

This would still be possible. As @Reedy says you can just move/cherry-pick the branch. On the other hand it pushes the onus to keep on top of that to the maintainers of Vector (us). Right now we have to monitor everything going into master, with this model we make sure the right things go to master.

People +2ing changes in Gerrit should not be ignoring what branch they're merging to -

However what seems to be the basis of your argument is that people will accidentally merge into a branch that's expected to be master but is not master which sounds a lot like ignoring the branch being merged to.

So what exactly what the objection here? Was it:

  1. Hey there, I don't think you should change the default branch as this will cause unnecessary confusion to people outside your team who will merge code under the assumption it's going to master and we don't want to have to rely on readers web to act as gatekeepers where that happens.

Or is it:

  1. Hey there, using a default branch that's not master will make X stop working in the release engineering pipeline. We don't support this because Y.

(if it's this what is X and Y? Is there an open bug?)

Or is it both?
This would be helpful context for next time.

Thanks @greg for the follow up here. If the concern is we don't want a big future release to build up that's totally fine and if that's the case it sounds like a feature branch of any kind is not a good idea and I will feed that back to my product team.

Correct on both counts. Thanks!

Another thing is that if we change the default branch, the patches won't be available on beta for testing, unless we change the branch on beta (can we do that, is it extra work/change to a script?)... Which may result in more confusion "why isn't this change I just merged to master appearing?"

Change 581108 abandoned by Jdlrobson:
Set default branch to DIP

Reason:
We are continuing discussion on Phab.

https://gerrit.wikimedia.org/r/581108

Declined based on conversation above. For now, we will proceed without a feature branch, with extra care for minimizing risk such as additional review and QA.