search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

RFC: Amendment to RFC-9344 - keep branch aliases for the master branch #9560

Open robbieaverill opened 4 years ago

robbieaverill commented 4 years ago

Abstract

RFC-9344 has one sentence in it which reads "Remove all extra.branch-alias directives from every repository." A combination of this sentence and not actually implementing it has caused confusion for me, as a maintainer and member of the SilverStripe core team.

Problem

RFC-9344 suggests that we should not have any branch aliases on any branches. I disagree when it comes to the master branches, which should always represent the next major version branch, until a time where one is created. At this point, the branch alias should be updated to represent the next major release again.

RFC-9344 highlights many problems with maintainer efficiency in regards to branch aliases in our repositories, and all of them relate to major version branches. There is no mention of master branches at all, other than describing the purpose of the branch alias in a master branch:

master branch contains the next major version (unreleased)

Proposal

Alternative

cc @silverstripe/core-team for feedback please.

maxime-rainville commented 4 years ago

What about we rename the master branches to match their expected major release, instead? For example, the master branch of framework could be rename 5. That way you could install it with 5.x-dev. It would also alleviate some occasional confusion for people who think master is default branch where you should target your PRs.

robbieaverill commented 4 years ago

Following your logic, what do you do with 6.x then?

maxime-rainville commented 4 years ago

When you release 5.0.0, you would just create a new 6 branch from the HEAD of the 5 branch.

robbieaverill commented 4 years ago

Alright so you're suggesting we create 5 branches for SilverStripe 5 now, which would presumably then be merged up to master too, and that master branch would continue to not have branch aliases (note: they mostly still have them, since RFC-9344 wasn't completely implemented).

I'm against that. I was against RFC-9344 in the first place (I did vote in favour in the end). Its intent was to reduce maintenance burden. By creating 5 branches now you'd be increasing the maintenance burden dramatically, since merging up between major versions is already difficult to do. I say that as the person who still continues to do a large portion of those merge ups, as normally they're either not done at all or merged up from say 4.4 to 4 and left there.

Edit:

When you release 5.0.0, you would just create a new 6 branch from the HEAD of the 5 branch.

The point of having a branch alias on master is that I can use 5.x-dev all the way up until it is released. What happens after that is less important, because then I can use 5.0.x-dev naturally from the 5.0 branch, or just use stable tags.

maxime-rainville commented 4 years ago

I would rename the master branch to 5. So we wouldn't have a master branch at all.

dnsl48 commented 4 years ago

I agree with Maxime. I don't think branch-aliases in this case will bring any more benefits to what we already have. A simple renaming of the master branch into 5 would be a better alternative (IMHO) in case we have a problem working with the next release and would like to call it by its digital representation (which is currently number 5).

On the other hand, I think having a branch-alias in configs introduces an extra entity that does not solve any real problems for us, but potentially increases maintenance burden and the config complexity.

I disagree when it comes to the master branches, which should always represent the next major version branch

The master branch already represents the next version, which is our current convention. A benefit of having an alias to it is questionable, unless we plan to have several next releases simultaneously.

We re-introduce the master branch aliases in any repositories that they have been removed from.

I'm trying to understand the problem we're trying to solve here. IMHO, having fewer entities to maintain is better, thus NOT having branch aliases is better than having them.

Cheddam commented 4 years ago

I've been working on an RFC to rename our master branches for a different reason, but in my proposal I recommended 5 / 2 as the new naming convention and outlined this exact situation as a benefit. I'd be really keen to see the branch name change implemented - it's a much better use of our time than maintaining branch aliases.

robbieaverill commented 4 years ago

One of the justifications for removing branch aliases was that they're pointless/unnecessary/have questionable benefit. While I disagree, it's worth noting that if we rename all of our master branches, then people will once again not be able to install the next major versions, because dev-master (the currently accepted status quo) will no longer work.

ScopeyNZ commented 4 years ago

Some interesting background on branch aliases from @tractorcow: https://github.com/composer/composer/pull/3480

ScopeyNZ commented 4 years ago

I think that the removal of master branch aliases is something beyond removal of branch aliases on minor release or patch release branches. The issue I linked even implies that right at the beginning, where the alias feature was intended to be used specifically for master with it's original implementation. It's interesting to read the background around why we introduced the aliases in the first place.

Honestly, I don't know why the original RFC was ever actually implemented. Updating the composer.json branch aliases when releasing was completely negligible, considering you're updating constraints anyway. Even if there was an impact, it gives consumers more flexibility, and might help some contributors understand the branching patterns, so it's not without benefits.

I don't want dev-master constraints in our dependencies, and I don't think we should force that constraint on people who might be developing sites utilising SS5 (which will hopefully pick up soon with more plans/features). The master branch alias has a lot of value here.

emteknetnz commented 4 years ago

In terms of major branch-alias i.e. "dev-master": "5.x-dev", it seems there are 3 options: a) remove branch-alias, keep master branch as is. Means that developing for SS5 is very annoying, have the have a composer.json file with a bunch of dev-master requirements, need to suddenly switch them over at the same time when SS5 is actually released. b) major branch-alias, keep master branch as is (status-quo). Mean that developers for SS5 can have 5.x-dev requirements, makes release SS5 much nicer c) remove branch-alias, rename master branch to 5. Seems good for SS5 development as well? Downside is people will once again not be able to install the next major versions, because dev-master (the currently accepted status quo) will no longer work.

I don't think a) is viable

It seems like we're currently at b), and going to c) which some people are keen on might work?

The downside of c), I'm not sure how many people this would affect? Only people doing SS5 development? It seems that simply installing 5-x.dev would be the same as installing master? So only a minor difference? Or Is there other benefits to the major branch alias that I've missed?

ScopeyNZ commented 4 years ago

I think that option C is a discussion that can be considered separately in it's own RFC, so we shouldn't conflate this. This is "make sure we keep master branch aliases, and issue an amendment to RFC-9344 to be completely clear about that".

So this being accepted is the option B you listed, and this being declined is option A.

emteknetnz commented 4 years ago

Sure option C is scope creep for sure =) It's probably a lot of work to change all to the master branches to 5 on how ever many repos

I don't want people working on Silverstripe 5 to feel as though they're being unnecessarily hindered on what they're working on

A major branch-alias in a composer.json has little to no affect on doing minor branch management so it does not personally affect me.

So if we're talking strictly about major branch-aliases ("dev-master": "5.x-dev") and it's between a) and b) at this stage then it would be a b) from me

dnsl48 commented 4 years ago

Honestly, I don't know why the original RFC was ever actually implemented

To summarize, it appears that not having branch aliases has some real value, while having them does not.

Updating the composer.json branch aliases when releasing was completely negligible

Sorry, but I can't agree with you on that one. I reckon it is not negligible, it is toil and it violates the KISS principle.

I don't want dev-master constraints in our dependencies

Several workarounds for that were discussed in the RFC.

I don't think we should force that constraint on people

Lack of an unnecessary feature !== forcing constraints on people

robbieaverill commented 4 years ago

@emteknetnz:

a) remove branch-alias, keep master branch as is. Means that developing for SS5 is very annoying

Reminder that we're not just talking about core here, this is a policy that affects the entire ecosystem. If somebody contributes a breaking change to a module then wants to use it in their project, they currently must use dev-master as the module's constraint, which is going to be more maintenance for them in future (when dev-master is changed to a different major release line and composer update no longer works). This is one example of why you use branch aliases, and it also applies to us in both our inter-module and recipe dependencies as well as CI config.

I don't want people working on Silverstripe 5 to feel as though they're being unnecessarily hindered on what they're working on

See above, this is not specific to SS5 although that's a good and relevant example at the moment.

b)major branch-alias, keep master branch as is (status-quo). Mean that developers for SS5 can have 5.x-dev requirements, makes release SS5 much nicer

This is unfortunately NOT the status quo, which is why I opened this issue. The status quo is that the master branch should NOT have a branch alias. The fact that most of the repos I quickly checked earlier today do is simply because they were not removed during RFC 9344's work.

c) remove branch-alias, rename master branch to 5.

You can do this if you want to, but note that having 5.x-dev as an existing branch alias would mean that doing this makes the transition seamless for developers, instead of having to change from 5.x-dev to dev-master then back to 5.x-dev again (where composer update breaks each time). You can do C as well if you want, I'm less concerned about what the branch aliases point to (dev-master is still a branch alias, just a default one).

A major branch-alias in a composer.json has little to no affect on doing minor branch management so it does not personally affect me.

I'm not sure what you mean here by major and minor, but if doesn't affect you then why did your team vote to delete them?

So if we're talking strictly about major branch-aliases ("dev-master": "5.x-dev") and it's between a) and b) at this stage then it would be a b) from me

(Your) B is what this RFC is for, so I'm glad you're on board. Please vote on my issue though, and I'll keep your vote in mind when the core team has cast their votes.

@dnsl48:

Your RFC seems sensible enough for removing branch aliases in existing release line branches, but not for the master branch - you make everything much more difficult for developers AND maintainers by removing it from the master branch, hence this amendment RFC.

Sorry, but I can't agree with you on that one. I reckon it is not negligible, it is toil and it violates the KISS principle.

KISS is actually a great example of why you would use branch aliases. I, as a contributor, make a breaking change PR. It gets merged, I update my elemental (or whatever) constraint to 5.x-dev instead of ^4.0. When 5.x is released as stable I stay on that release line. If I want to I can change to ^5.0 for a stable release, but I have confidence there won't be any more breaking changes I'm unaware of. By not aliasing the master branch you throw away any of those guarantees and make things substantially less simple for developers (and maintainers, as I've mentioned already).

[I don't want dev-master constraints in our dependencies] Several workarounds for that were discussed in the RFC.

What is a better workaround than putting a branch alias on the master branch so I can specify the major release line I want to install? We're not the first ones to do this by the way, it's basically standard practice in the PHP ecosystem.

Cheddam commented 4 years ago

I support retaining / reintroducing the branch aliases on master branches as a sensible approach for the time being. We should make sure any relevant documentation recommends 5.x-dev as a version constraint over dev-master. I'll follow this RFC up with one focussed on renaming the master branches in the near future.

dnsl48 commented 4 years ago

I don't believe we're fighting a real problem here. I still can't understand what is the issue with referencing dev-master as 5.0.0. Tossing in branch aliases across a hundred of repositories sounds like a waste of time to me, although I cannot bring any valuable arguments against it either (except what I already said that it's less consistent and unnecessarily overcomplicates the configs). In reality I don't think we'll be releasing major versions too often, so we won't have to touch those aliases even if we have them. That means, it doesn't contradict the main purpose of the RFC, so I think that's acceptable. Still, I believe renaming the master branch would a better option IMHO.

P.S.

We should make sure any relevant documentation recommends 5.x-dev as a version constraint over dev-master

why?

When 5.x is released as stable I stay on that release line

yes, but I don't think that's a valuable enough outcome to support hundreds of branch aliases across the modules; the other option, to support aliases for some modules and not the others, which makes it less consistent

robbieaverill commented 4 years ago

We're talking about one branch alias per module here, not hundreds. I can vouch for what @ScopeyNZ said that maintaining them is negligible effort given you already need to adjust dependency constraints anyway.

The reasons to use branch aliases have been outlined many times already, so I'm not going to go over them again, please re-read the comments in this thread.

dnsl48 commented 4 years ago

We're talking about one branch alias per module here, not hundreds.

I meant that we have a hundred of modules we'd like to keep in a consistent state with each other.

Cheddam commented 4 years ago

The impression I got was that for the most part these aliases still exist in the master branches of our modules, and that we'd simply be reinstating a few to get them back into a consistent state. I'm hopeful that @robbieaverill would be happy taking on at least some of this work 😉

The maintenance cost of these aliases in master is a lot lower than it was in 4 / 1, because 4 / 1 generally needs updating every 3 months, whilst master (in core) has remained aliased to 5 / 2 for 2-3 years (and if we don't proceed with branch renaming, will likely have at least another year in this state.)

As for why we should recommend 5.x-dev over dev-master - it means that whether we have branch aliases or real branches, the constraint will remain the same, and when we eventually branch to 6.x this will make it an explicit opt-in.

maxime-rainville commented 4 years ago

From what I can tell, SS5 is still pretty far on the horizon and no one is actively working with it. In the short term, my vote goes to whichever option minimise our regular workload and keeps our set up in a consistent state.

If most of the modules still have their branch alias on the master branch and we just needs to revert the changes to a few modules, than I'm fine with accepting this RFC.

We can revisit this decision once we start having active development on the Silverstripe CMS 5.x line.

dhensby commented 4 years ago

My view is that we should be keeping the branch aliases in master because the only other option is to force people to use dev-master.

Renaming master to 5 is going to add more confusion than it's going to solve and we then need to remember that 5 is the "unstable" branch where no promises are made but it actually implies that there is some commitment to keeping things non-breaking. master is clear to most people that it's WIP not stable and should not be relied upon, that's how it's always been used in SS projects anyway.

Branch aliases specifically have their place and their maintenance is really not a burden when they are in the master branch; when was the last time someone did a release from master and needed to bump the branch alias? I've done a lot of releases of SS and I can tell you, renaming branch aliases was not the area I thought would really save me time!

@robbieaverill also touches on an important point. By removing our aliases here we actually push the burden of maintenance onto the entire community. They now need to keep active track of what "dev-master" means (is it 5, 6, 7??), hooking onto dev-* (or -dev) which are just implicit branch aliases will cause big on going pain for the community AND* us when we need to change all our branches from relying on dev-master to 5.x-dev

dnsl48 commented 4 years ago

Just gave it another thought and I agree with the proposal. It makes sense to reduce maintenance burden by allowing branch aliases for master branches. I think removing them is just a waste of time. I'd only like to suggest we make it optional, so we don't have to create aliases for a hundred of repos.

P.S. I'm still not convinced there is any actual value in having them though. IMHO, having aliases doesn't make things better, but violates KISS principle and increases the configs complexity. I disagree that lack of aliases would bring any burden on the community. We have clear conventions declaring that master is the next major and yet unreleased version. Renaming master into number could make things cleaner and more consistent. An example of how it can work is drupal.

ScopeyNZ commented 4 years ago

but violates KISS principle

This principle is very very relative. Who are you aiming to keep things simple for? Us as maintainers? I think this is where you need to be very clear. I'm not saying removing them isn't making it simpler for maintainers (although I have stated it has negligible impact to maintenance). I think it definitely makes things less simple to developers who are familiar with composer.

robbieaverill commented 4 years ago

@silverstripe/core-team can we get some +1/-1 reactions on the PR description here please? I'm keen for a resolution

sminnee commented 4 years ago

Are you proposing both (A) and (B) on this? I.e. (B) covers #9614?

If we do (B), is (A) redundant?

ScopeyNZ commented 4 years ago

I think this shouldn't cover B, as we've got #9614. There's a question about whether we alias dev-master if we do #9614, to make it easier for those who might be using dev-master already.

robbieaverill commented 4 years ago

I think I'm proposing (A) here. I'll update the description to make that clearer.

sminnee commented 4 years ago

OK so if we're going ahead with #9614 this seems redundant... It feels like either/or.

If we do go with #9614 we may wish to alias 5.x-dev back to dev-master, but it feels like we'd regret that in the long-term even if it would make a composer update easier for a small number of people.

EDIT: Okay, I see now, we need to:

I'm on board with that :+1: