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: Remove lock step versioning between core modules #8970

Closed robbieaverill closed 5 years ago

robbieaverill commented 5 years ago

Affected Version

4.x

Description

Currently we have a lock step between core modules, in that they are only ever tagged as new versions during a recipe release process, e.g. cms/framework/reports/siteconfig are all 4.3.1, admin/versioned/campaign-admin/asset-admin are all 1.3.1 etc.

Problems with removing lock step

Benefits of removing lock step

Proposed solution

  1. We remove the necessity for fringe-core modules to be in lock step with each other for patch versions. Recipes continue to be compiled with the latest minor/patch releases available at the time, and/or release new minor/patch versions during the process of creating them.
  2. We continue to keep silverstripe/installer, silverstripe/recipe-core, silverstripe/recipe-cms, silverstripe/framework, and silverstripe/cms in lock step with each other, since these are the most commonly used modules and recipes
  3. Add notes to the documentation that clarify this process for the core team

Side notes

cc @silverstripe/core-team

kinglozzer commented 5 years ago

It makes it a little harder to know which versions people are probably using with admin 1.3.1 (for example) if they report a bug in it

composer show is all bug reporters need to run though, right? And if they downloaded ye-olde-zip, we should be able to tell what package versions were released with that the recipe version (or ask them to upload the composer.lock that’s bundled in the ZIP)?

  • We can quickly release a fix for a small issue without doing a full recipe release
  • Less active core contributors will be happier to do module tags for new fixes

I’d be happy with this approach - I’m perfectly happy creating tags, but the full release process is daunting and probably too time consuming for me to want to volunteer for! Are there any practical concerns with adopting this approach (e.g. around security releases)? None spring to mind, but I’m not heavily involved with releases...

We continue to keep silverstripe/installer, silverstripe/recipe-core, silverstripe/recipe-cms, silverstripe/framework, and silverstripe/cms in lock step with each other, since these are the most commonly used modules and recipes

Recipes/installer I can understand, but why keep silverstripe/framework + silverstripe/cms in lock step? Their minor versions would still always be in lock step, so I don’t see why it would matter if the patch versions diverged (as long as we correctly bump cross-package dependencies if we need to).

robbieaverill commented 5 years ago

Yeah it was for an “ease into it” approach but I’m happy to make a blanket rule that recipes are locked and modules (incl framework) are not

stevie-mayhew commented 5 years ago

I'm all for this, it should make things easier which is always a good thing. It should also speed up the "speed to market" of peoples PR's making it out to their instances which has always been a slow, painful process for the person who has contributed. I'm all for it being across everything that the core team maintains.

ScopeyNZ commented 5 years ago

blanket rule that recipes are locked and modules (incl framework) are not

I'm still in favour of this. composer show is a common request anyway for raised issues. Any core contributor should feel comfortable releasing a patch version if they think there's reason.

dnsl48 commented 5 years ago

continue to keep silverstripe/installer, silverstripe/recipe-core, silverstripe/recipe-cms, silverstripe/framework, and silverstripe/cms in lock step with each other, since these are the most commonly used modules and recipes

What are the main reasons for that? Having "exceptions" for a rule usually makes it more complicated. People will have to remember what module is in-sync and what isn't. Why can't we apply the rule ultimately to everything and make things simpler?

chillu commented 5 years ago

Great discussion, I think the benfits outweigh the risks here. Faster release cycles on individual modules is a big one. That relies on either more non-staff core committers willing to tag, or staff core committers making it a better part of their team processes. Sounds reasonable.

It'll make it harder to tell if a certain module release line is still supported: You need to look up which release is packaged in a recipe, since that's what determines support. So if we package silverstripe/versioned:1.4.1 with silverstripe/recipe-cms:4.4.0, but then silverstripe/versioned:1.6.0 with silverstripe/recipe-cms:4.5.0, it effectively means silverstripe/versioned:1.4 and silverstripe/versioned:1.6 minor release lines are supported for the duration of the recipe, but silverstripe/versioned:1.5 isn't (was never part of a recipe). We'll need to document that well, and make it easy for people to make that connection.

We remove the necessity for fringe-core modules to be in lock step with each other for patch versions. Recipes continue to be compiled with the latest minor/patch releases available at the time, and/or release new minor/patch versions during the process of creating them.

As discussed in in https://github.com/silverstripe/silverstripe-versioned/issues/230, this does not imply that we're aiming to support modules across minor release lines. We should bump dependencies as required, I'm not willing to jump through a lot of hoops to have silverstripe/framework:4.6 support silverstripe/versioned:1.2 unless it comes "for free". And we won't actively test combinations other than the ones declared in recipes.

Does anyone have a view on how this affects the cow release tool? I guess we'll need to do more "recipe planning" than currently, but the mechanics for that should already exist because it's how the CWP release works, right?

As a sidenote, this doesn't change any discussions around requiring new major versions of modules alongside new minor versions of core. silverstripe/graphql is the (only?) offender there, we've bumped from 1.x to 3.x in silverstripe/admin:4.x releases. That's an exception rather than the rule. In my opinion, we should've never tagged silverstripe/graphql:1.x, and hence never declared any of those APIs as stable. That has some nasty downstream effects on modules like silverstripe/versioned-admin and dnadesign/silverstripe-elemental who need to pull in those new releases. As far as I'm concerned, none of this will change with this ticket: It's still an awkward exception, we should a) stick to semver incl. dependencies and b) be very careful what we declare as public/stable APIs.

chillu commented 5 years ago

continue to keep silverstripe/installer, silverstripe/recipe-core, silverstripe/recipe-cms, silverstripe/framework, and silverstripe/cms in lock step with each other, since these are the most commonly used modules and recipes

I think we could do a "soft lockstep" where it's more common to bump dependencies. If you're introducing a new utility class or interface in framework because you need it in versioned, we'd bump the versioned dependency on framework. They'll both need minor or patch releases.

That brings up a good point: We have to be super careful with these cross module dependencies on new behaviour/classes/interfaces. It'll be very easy to just tag versioned in the example above, and forget that you'll also need to tag framework. This is an argument for only implementing these features in a backwards compatible way, but it won't always be as straightfoward as a class_exists() check.

robbieaverill commented 5 years ago

I really still think that the recipes should continue to be in lock step with each other.

While I'm proposing we remove lock step for patch tags, I can also see that modules like reports or siteconfig that never have any new features added may get to a point where it doesn't make any sense to release a new minor version of them any more if they aren't in lock step. Then you get siteconfig + reports 4.4.0 being used in silverstripe-installer 4.9.0 or something, which goes back to making it tricky to achieve what you said @chillu about not supporting cross module versions.

Summary: I think we need to keep lock step on minor versions, but remove it for patch versions. And keep it for recipes.

chillu commented 5 years ago

Summary: I think we need to keep lock step on minor versions, but remove it for patch versions. And keep it for recipes.

So in terms of the benefits we're gaining from this: Bugfixes (mostly patch level) can be pushed out quicker, but new features (mostly minor level) need to wait until the next minor recipe release. I think that's a good compromise. It also allows us to test cross-interactions of features before a) releasing them and b) committing to their stable APIs. Otherwise you might release a new versioned API, and then figure out it's a bad idea because it doesn't work with fluent two months later in the CWP recipe pre-release testing. Some of this can be captured through automation tests and peer review, but there's a lot of combos to consider. If people are are eager to use new features prior to recipe releases, we could always work with per-module alpha releases?

dnsl48 commented 5 years ago

we could always work with per-module alpha releases?

I think using dev branches is good enough for that as long as we keep tests green.

dnsl48 commented 5 years ago

continue to keep silverstripe/installer, silverstripe/recipe-core, silverstripe/recipe-cms, silverstripe/framework, and silverstripe/cms in lock step with each other, since these are the most commonly used modules and recipes

It feels I may be misunderstanding this bit. Do you suggest recipe versions reflect module versions, or do you mean silverstripe/framework patch version should be kept in sync with silverstripe/cms?

robbieaverill commented 5 years ago

I think the consensus is that all modules should not be in lock step any more, but I think recipes still should be with other recipes (installer is also a recipe)

kinglozzer commented 5 years ago

Looks like everyone so far is in agreement, we could always trial this approach and if we do run into any unforeseen issues it wouldn’t be hard to go back to the status-quo for the next minor version. We could even go back in a minor release by just tagging everything with the highest patch version number and performing a release. Tldr; I don’t see any risks of trialing this.

One thing worth considering is what constitutes a release-worthy change? My instinct says that will probably just amount to “use your best judgement” so we avoid tagging every merged PR (or actually, maybe we could if we wanted? it’s just another number, right?)

dnsl48 commented 5 years ago

One thing worth considering is what constitutes a release-worthy change?

I'd consider anything higher than impact/low

michalkleiner commented 5 years ago

I'd consider anything worth being installable. So a docs change doesn't matter that much as a code change, even a small patch, which could be released. The most annoying thing for us are modules with code changes that are not "released". When people do PRs, they expect to be able to use them back in their projects when they are merged and referencing them via commit hash or worse via dev-branch wouldn't be preferable I'd say.

ScopeyNZ commented 5 years ago

One thing worth considering is what constitutes a release-worthy change? My instinct says that will probably just amount to “use your best judgement” so we avoid tagging every merged PR (or actually, maybe we could if we wanted? it’s just another number, right?)

I'm a big fan of "use your best judgement". "It's just another number" also hits the nail on the head. It's pretty easy to release with GitHub UI - I don't really conceive any downsides of tagging

I'd consider anything higher than impact/low

This is a good indicator - but probably doesn't have to be the rule.

So a docs change doesn't matter that much as a code change, even a small patch, which could be released.

When we release CWP - we don't tag docs changes or updates to tests/test configuration. We will release anything else though.

The most annoying thing for us are modules with code changes that are not "released". When people do PRs, they expect to be able to use them back in their projects when they are merged and referencing them via commit hash or worse via dev-branch wouldn't be preferable I'd say.

Currently, any module that's not part of the "core" definition (not included in silverstripe/installer) is not part of this "lock-step" versioning. You're welcome to open an issue on a repo (or pop a message in community Slack - perhaps the #get-involved channel) and ask for a tag - we're happy to do that. Given this RFC is accepted that will be possible for any other (non-recipe) module too!

robbieaverill commented 5 years ago

@tractorcow would you like to weigh in?

sminnee commented 5 years ago

I'm generally supportive of this approach and also keen that it is most enthusiastically supported by the people who do release work most frequently, which seems to be the case. :+1: from me

chillu commented 5 years ago

This all sounds good. @unclecheese As the main person performing core recipe releases at the moment, can you please weigh in here? Is anyone keen to draft the required release process docs and guidance for core committers?

robbieaverill commented 5 years ago

I’m happy to draft a write up for this

maxime-rainville commented 5 years ago

Not really involved in the actual release, so I don't have the most informed opinion about this. But it seems like a relatively easy way to gain more flexibility without downside.

Put me in the thumbs up column.

robbieaverill commented 5 years ago

Cool - accepted with a quorum of 7/13 core committers in favour (with @unclecheese confirming verbally). Noting that the remainder have not responded, rather than having voted against this idea.

robbieaverill commented 5 years ago

Versioned 1.3.4 now breaks the core module version lock step. This RFC has been accepted, so going to close it now. Thanks everyone 🎉

chillu commented 5 years ago

@robbieaverill Can you please document this somewhere for core maintainers? In particular, which modules continue in lockstep and which aren't?

@unclecheese Does this have any implications on the "making a core release" docs?

robbieaverill commented 5 years ago

Docs PR at https://github.com/silverstripe/silverstripe-framework/pull/9000

kinglozzer commented 5 years ago

Late thought: what do we do about changelogs? IIRC cow currently generates these, and they encompass the changes across all core modules for that given release. Lets say we find a high-impact bug in the CMS module tomorrow, and decide to release 4.4.2 of just CMS, do we create a 4.4.2.md, even though framework doesn’t have a 4.4.2? I think that would be a problem point for us, and confusing for devs reading the docs.

robbieaverill commented 5 years ago

The changelogs at the moment are really a list of what's included in a recipe release, rather than individual modules (just info, not saying it should remain this way)

chillu commented 5 years ago

Late thought: what do we do about changelogs?

Yeah, I guess one of the "amenities" we provide through a recipe release is a more curated changelog. For patch releases, I would say that Github commit messages are sufficient. This is an effort to simplify our release process, and we're not achieving that by introducing a repo/module-specific changelog generation and release process.

Github provides release drafts with Markdown notes. While that's a convenient shortcut, it also risks that the info contained in there is not duplicated in the highlevel recipe changelog. I'd rather keep it simple: If you want to stay up to date with bugfix releases, you need to read raw changelogs. Otherwise wait for recipe releases.

Here's a curly one: Do we allow module security releases without recipe releases? https://docs.silverstripe.org/en/4/contributing/making_a_silverstripe_core_release/#security-release-process says "make a core release", which implies a recipe release. If we don't follow the core release process, important steps will likely be skipped (e.g. comms). I'd suggest keeping security releases to recipe releases, unless they need to be actioned quickly (e.g. already publicly disclosed). And then aim to do at least one or two recipe releases per quarter. Put another way: Security releases are currently our main driver for perfoming those recipe releases

Also, I've gone through https://www.silverstripe.org/download/security-releases/ and made it clear which modules those versions relate to, that was a bit haphazard - they should be resolveable composer constraints now.

ScopeyNZ commented 5 years ago

Do we allow module security releases without recipe releases?

I think we can probably make a light ruling around this. If a patch is prepped for a minor security issue I don't see a problem with just releasing it for the masses when it's ready? Saves juggling more things during a recipe release.