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: Maintain max 1 minor branch per major #9674

Open sminnee opened 4 years ago

sminnee commented 4 years ago

9077 Problem

Right now, framework has (among others) the following branches:

This means that PRs can be made to any of these branches, having the following downsides:

In short, our code management is unnecessarily complex. I'd like to limit us to the necessary complexity. ;-)

Recommendation

My recommendation is that we retain only a single minor release line per major branch:

Other considerations

Security releases

l am not suggesting that we necessarily drop security support for earlier minor release lines as part of this change: just because the branch is deleted doesn't mean we're unable to produce a security patch.

If we need to create a security release on an older version (this happens about 3 times / year based on the last 2 years' data), then a release branch could be created using the latest stable tag as the starting point. This will mean that the security release will only contain the security fix, plus anything in the previous stable release, which seems superior.

I'm not sure how hard this it to do in Cow, but hopefully it's relatively straightforward.

Broken composer checkouts

If people are referencing 4.2.x-dev, I believe this would fail on a composer update. I'd have to test whether it breaks on composer install.

Cleaning up dangling commits

If a branch due for deletion has any commits on it that haven't been tagged in a release, then they should be tagged in a beta release and may be left that way without an RC or stable release ever happening.

This ensures that people who have tied their project to the dev version of that minor release don't lose any changes in their next composer update.

Prior work

A couple of years ago I had raised #8189, which was effectively suggesting that we minimise the use of the minor release branches altogether. That was accepted, on the understanding that there's some professional judgement about what goes into minor releases.

The fact is that work still makes its way into older branches, so it's not effective in simplifying our branching strategy on its own.

sminnee commented 4 years ago

CC @emteknetnz as you asked in a comment "why would we ever delete the 4.6 branch?" ;) my thinking was lengthy enough for an RFC.

emteknetnz commented 4 years ago

Yeah I think I'm generally on board with this because it moves in the direction of reducing the maintenance burden. It also aligns with our support timeline where we only fully support the latest minor and only touch the previous minors for critical/security bugfixes.

For pull-requests that target really old minors such as 4.3, I think this is usually just a really old pull-request and 4.3 was actually the latest minor at the time. However it may also be "My project still uses an old minor version" so I'll just target that. Either way I would prefer these pull-request to go straight into 4.6 rather than 4.3 for the reduced toil.

Also merging something into 4.5 for someone doesn't always feel great because old minors hardly gets a stable release and we are kind of setting a bad expectation. For instance in the last quarterly release we did not include the 4.5 recipes and dependent modules because there was nothing security related to release.

"Merge-ups" are more time-consuming and complex

Agree there, particularly with something like asset-admin javascript where you need to checkout the same admin module version in tandem and yarn install both and yarn build asset-admin at each merge-up step

If you just wanted to use the latest version of 4.2, but also stay on 4.2 after 4.3 has been a released, then what are you doing?

It is pretty normal for end-users to stay on fairly out of date versions because they don't have a robust minor upgrade process in place, or perhaps they are highly risk adverse and see upgrading minors as too risky since there may be regressions. Or maybe they just setup their composer.json with ~ instead of ^ back in the day, don't know any better and never updated it.

That said, a composer requirement that doesn't upgrade minors such as ~4.2.0 will still work even if the 4.2 branch is deleted because we're won't be removing the 4.2.n tags, only the 4.2 branch. People really should not be using 4.2.x-dev on production sites so I'm not fussed about loosing that functionality.

I'm not sure how hard this it to do in Cow, but hopefully it's relatively straightforward.

If we just created say the 4.5 branch manually when we needed it from the latest 4.5.n tag as you suggest, then we shouldn't need any changes in cow. You have provided data showing that it's an infrequent event so I don't think this fairly simple step is worth investing time to automate.

sminnee commented 4 years ago

It is pretty normal for end-users to stay on fairly out of date versions because they don't have a robust minor upgrade process in place, or perhaps they are highly risk adverse and see upgrading minors as too risky since there may be regressions.

I think that's fine, but to be using an old version and be using the dev version of that minor release seem to be in conflict: I don't think a project can simultaneously be risk-averse and bleeding edge.

For the risk-averse folks, use ~4.2.0 rather than 4.2.x-dev

EDIT: I think we are using different words to agree 😆

michalkleiner commented 4 years ago

@emteknetnz I think this

Or maybe they just setup their composer.json with ~ instead of ^ back in the day, don't know any better and never updated it.

might be sending a wrong message, suggesting the right way to do composer constraints is using ^ instead of ~, allowing minor version updates (instead of just patches). It might be fine cross-requiring dependencies between SS modules, but from my experience so far I'd say it's reasonably fine to let patches in from SS modules to a project, but not always the case with new minor versions as they may require project code updates/changes to function properly.
Remember all the hassle going from 4.0 to 4.1 to 4.2 to .... and protected assets and file hash issues etc.?

It's probably offtopic in case of this RFC, just wanted to voice my concern if that ("people should use ^") became a reason/supporting argument for some changes here.

sminnee commented 4 years ago

The point was more that people shouldn't use 4.n.x-dev. Either ~ or ^ are better. You can pin to @dev, or reference a not-yet-released number (eg ^4.8 will check out 4.x-dev until 4.8.0 is released, which is great if you just got a PR merged into 4 and you're happy to use the dev branch until your PR makes its way into a stable tag)

sminnee commented 3 years ago

What do we do if we want to delete minor-release branches but there is untagged content in them? Clearly, we should do a final 'merge-up' to future branches if one is needed.

However, is that sufficient? Imagine the scenario:

If someone is currently using 4.4.x-dev, ~4.4.0@dev, or ~4.4.0 with min-stability: dev, then there project will include BUGFIX-123.

If we just merge 4.4 into 4 or 4.5 and delete the branch, then they will lose this.

Now it's an unreleased fix, but someone might have switched to dev stability precisely because they needed that fix.

My suggestion is that in such a case we tag 4.4.2-beta1 but made no attempt for a release beyond that. This will keep people's projects working but don't force us to go through the testing process required for an additional stable release. A dangling beta release is a little odd but seems like a fair reflection of the real-world situation.

cc @dnsl48 @chillu @maxime-rainville

sminnee commented 3 years ago

In terms of approving this do we want to PR some branch management guidelines into the ope-source contributor docs?

maxime-rainville commented 3 years ago

If you're running a dev branch of an out-of-support release on something important, I'm inclined to be unsympathetic if deleting the branch ends up breaking your project.

I would try to merge up the fix on a "best effort" basis, but not much beyond that.

emteknetnz commented 3 years ago

I'm inclined to be unsympathetic if deleting the branch ends up breaking your project.

Keep in mind that a lot of websites out in the wild were was once well resourced development projects several months or years ago, but when that project was finished the developers then moved on to the next thing, and the website barely have any sort of ongoing development resource / maintenance budget, so it has a prehistoric composer.json file that made perfect sense at the time.

If the site needs to be redeployed for whatever reason by their managed hosting provider, such as an infrastructure upgrade, then all of a sudden their code gets reverted to something pre-bugfix :'(

I think Sam's solution of tagging the very last thing on on the branch as an untested beta seems like a good idea.

maxime-rainville commented 3 years ago

Just went through a couple modules to get a better idea of how many repos have old branches with unreleased code. Most of the older modules have a couples. So most of them would need some of those fake beta tags.

My gut feeling at this stage is that adding beta tags to those branches before deleting them would transform this job from something that could be done in a couple hours to something that would take a couple days. Are we really willing to spend a week of dev time on this?

sminnee commented 3 years ago

What work are you assuming is necessary beyond "git tag x && git push --tags"?

Can you avoid that work and still get a better result than throwing the code away?

emteknetnz commented 3 years ago

If toil is a concern then this be a good candidate for automation?

A simple php cli script along these lines may be useful?

<?php

$branches = shell_exec('git fetch && git branch'); // just pretend this returns a php array

foreach ($branches as $branch) {
    if (!preg_match('^[0-9]\.[0-9]$', $branch)) {
        continue;
    }
    shell_exec("git checkout $branch");
    $commitInGitTag = shell_exec("some git command that works out if the latest commit exists it at least 1 tag");
    if (!$commitInGitTag) {
        $tag = '1.2.3-beta'; // work out by incrementing the patch number on the latest tag that matches $branch
        shell_exec("git tag $tag && git push --tags");
    }
}
Cheddam commented 3 years ago

One issue we've hit when pruning old core branches in the past (and part of the reason we stopped doing this in the first place) is that all of our satellite modules have CI matrixes that cover a range of old minors, and dropping the branches therefore indirectly busts those builds. This could be resolved in a similar manner to what's being recommended for projects - using ~4.4.0@dev instead of 4.4.x-dev - but just worth noting that there's a bundle of toil waiting to be unleashed if/when we proceed with this.

(We've explored options for reducing this toil in #9174.)

emteknetnz commented 3 years ago

Presumably we'd also want to drop a bunch of the older entries from the satellite modules matrix too? For instance just have a 4.6 entry and a --prefer-lowest and call it a day?

We'd may also want to convert them to ~4.6.0@dev notation at the same time to make them resilient for when 4.6 silverstripe/recipe-cms gets pruned sometime in the distant future.

Another option may be to have a --prefer-highest and a --prefer-lowest and not denote minor versions at all to remove the need to update them going forward

Seems like at least one round of travis toil is pretty unavoidable here

chillu commented 3 years ago

Another option may be to have a --prefer-highest and a --prefer-lowest and not denote minor versions at all to remove the need to update them going forward

Yes I think that is the best way to have a general purpose build matrix, and covered in https://github.com/silverstripe/silverstripe-framework/issues/9174

dnsl48 commented 3 years ago

We'd may also want to convert them to ~4.6.0@dev

Hit a problem with that approach. Most of our modules have prefer-stable: true, which makes it install only stable tags on CI (even when dev branches exists).

A potential workaround is to turn it off in CI (e.g. like that: composer config prefer-stable false). However, that would make it install unstable 3rd party libraries too, not merely our own recipes (e.g. we don't want to test with dev-master branch of symfony components).

So far it looks like we may need to go with --prefer-lowest and --prefer-highest if we want to avoid the toil.

sminnee commented 3 years ago

Yes fair enough. The use of travis templates referencing a .x-dev release (Steve's recent work) may also help.

chillu commented 3 years ago

There's four core committers involved in this discussion, and it sounds like there's agreement on the high level RFC, just some discussion on the technicalities - so I'm marking this as rfc/accepted.

This does smell a bit like increased toil though, especially if we want to roll this out to all "supported" modules. Both as a one-off (review branches, dangling commits, tag beta releases), and as an ongoing effort when we create new minor branches. We could automate this on cow release? Every time you make a stable release, it checks existing branches and offers to delete ones which would become unsupported. So when releasing 4.8.0, it'd detect the presence of a 4.6 branch and delete it.

Alternatively we could do this via Github Actions, but an automated process deleting branches seems quite difficult to get right. Especially since you'd need to supply fixes to each individual module in the way that Github Actions is structured.

How often do we actually hit cases where PRs target the wrong branch, and a maintainer misses that and merges? Is it worth the upfront and ongoing effort?

emteknetnz commented 3 years ago

We could automate this on cow release

I'd strongly prefer not to automate this step via cow, because as the recent issues with cow translate have shown, while computers may not mistakes, sometimes the humans programming them do

It would be worthwhile getting cow or a similar tool that pings the github api to list old branches to make it easier for a human to manually delete them

How often do we actually hit cases where PRs target the wrong branch, and a maintainer misses that and merges? Is it worth the upfront and ongoing effort?

Very infrequently. Probably more of an issue when merging very old PR's that originally targeted the correct branch. Generally branch targeting issues people have a targeting the master branch or default 4 for simple bugfix that could be in the next patch release.

Regardless though this is still worth doing IMO because it makes it much clearer to everyone that critical fixes or even general fixes don't need to "target the oldest feasible branch and then merge it up 5 times"

chillu commented 3 years ago

I'd strongly prefer not to automate this step via cow,

There's different levels to this:

maxime-rainville commented 3 years ago

FYI We now have a view on our dashboard that list which branches have unrelease commits for all our module. This should hopefully make it easier to visualise which branches might need one last unstable release before we can delete the branch.

https://maxime-rainville.github.io/travis-dashboard/unreleases