search

silverstripe / cwp-recipe-kitchen-sink

The CWP kitchen sink! Includes all optional and suggested modules. Used for internal testing.
BSD 3-Clause "New" or "Revised" License
1 stars 5 forks source link

All CWP modules should have a branch targeting next minor release #46

Closed brynwhyman closed 4 years ago

brynwhyman commented 4 years ago

Particularly in the CWP modules, the master is targeting the latest minor release (4.4.x), which means that we don't have automated builds running CWP against the next minor branch (4.5.x). This is in contrast to the core modules which use master to target SS5.

We need to do some immediate work to fix this for the upcoming release.

AC

PRs

dnsl48 commented 4 years ago

Affected recipes

Affected modules

dnsl48 commented 4 years ago

The plan (for every repository):

ScopeyNZ commented 4 years ago

I feel like this is creating a lot of work for little reason? I'm interested to know the motivation behind this. All repos have a "next minor" branch - this is either the single digit branch name or master if that doesn't exist.

Creating major branches without any (existing or planned) major changes is just creating pointless maintenance burden?

dnsl48 commented 4 years ago

With the number of modules we have to deal with, inconsistencies in branch naming cause troubles and make things more complicated than necessary. If every module had the same conventions about master and major branches, it would be easier for maintenance and should simplify automation of some toil later.

ScopeyNZ commented 4 years ago

it would be easier for maintenance

I very much disagree with this statement until we actually have automation in place. Even then I think it would be trivial to include the logic in a script to identify whether master is the next major or minor. The absence of a major branch is quite normal (I'd say) if a major release is not planned (like in the case of CWP)

Perhaps we should update the docs contributed by silverstripe/silverstripe-framework#9310 to indicate more specifically that this applies to core repos, and "satellite" modules may use master as the next minor release.

dnsl48 commented 4 years ago

"satellite" modules may use master as the next minor release.

Then we would have to give definition for "satellite" modules and maintain different conventions for different modules. I don't think that's a good idea. The effort is to make things more consistent around all the modules. Your suggestion feels more like to make conventions around the current inconsistencies and keep them as is.

dnsl48 commented 4 years ago

Creating major branches without any (existing or planned) major changes is just creating pointless maintenance burden?

Yeah, @robbieaverill just suggested we can delete master branches altogether. Though, I'm not currently sure this would make things easier, since that also introduces inconsistencies between modules that "have some planned major changes" and the ones that don't.

ScopeyNZ commented 4 years ago

Then we would have to give definition for "satellite" modules and maintain different conventions for different modules. I don't think that's a good idea.

But we already have that definition because none of our other modules (except those arbitrarily defined as "core") have the idea of lockstep minor versions. We had an argument about this a few months back that resulted in us allowing ourselves to release patch versions of core modules out of "lockstep" but I would still prefer we remove the idea of lockstep from minor and major releases too.

The effort is to make things more consistent around all the modules. Your suggestion feels more like to make conventions around the current inconsistencies and keep them as is.

Yep - it has been a convention for as long as I've worked with SilverStripe - we're talking about documenting them now. Sure it's inconsistent from the documentation that was just contributed to the docs, but it's definitely easier to maintain and is easier to manage right now because we can just leave it as is.

We can delete master branches altogether. Though, I'm not currently sure this would make things easier, since that also introduces inconsistencies between modules that "have some planned major changes" and the ones that don't.

I don't think

"have some planned major changes" and the ones that don't

is actually an inconsistency. It's a fact of the module. Each module is different and has a different roadmap, a different current version, a different number of patch releases, a different maintainer, etc.

I think not having master branches and then re-adding, and then removing them again is not a nice workflow either, but if we're adamant on having a "first part of version" (ie. single digit) branch for the minor then I'd rather delete the master if there's no planned major rather than keep it around.

dnsl48 commented 4 years ago

Thank you for your thoughts! It was a bit lonely to do all these PRs without hearing what people think about it :) I can see there are several different opinions about what's the best approach here and I really don't think there is the single best way.

idea of lockstep minor versions

However, it doesn't contradict in any ways with modules following same exact conventions about "master" and "major" branches?

I would still prefer we remove the idea of lockstep from minor and major releases too

I really love this. I'd prefer this as well. However, this is not really feasible until we improve our release processes and tooling, which this current issue is a part of.

it has been a convention for as long as I've worked with SilverStripe - we're talking about documenting them now

Why if we have an opportunity to "fix" it now? Does it actually solve any real problems?

it's definitely easier to maintain and is easier to manage right now because we can just leave it as is.

Agree, it's always easier to leave stuff as is. However, I reckon it would be more consistent (and as such better) if every module followed the same conventions. IMHO it will also be easier to maintain and automate stuff when it's the case.

It's a fact of the module. Each module is different and has a different roadmap, a different current version

True. And I'm thinking it may be good, but I really don't want to do this as a "bulk" change, which would make me to decide for every module right now if it has "future" or not. I don't really want to bring it into the scope of this issue. However, do you agree that unneeded master branches can be removed later on the case by case basis?

dnsl48 commented 4 years ago

There are some inconsistencies for branch aliases too, by the way. E.g. https://github.com/symbiote/silverstripe-advancedworkflow/blob/master/composer.json has dev-master aliasing 5.3.x-dev and not 5.x-dev

robbieaverill commented 4 years ago

I think that's fine, it's not our module. We can suggest policies for things like branching strategies, however. To me it sounds like the maintainers may not want to accept breaking change contributions (or it may just be an oversight).

brynwhyman commented 4 years ago

+1 to proceeding with this work.

We're carving out time internally to focus on reducing toil and improving some of our development and release processes. @dnsl48 has already spent the better part of a day to bring consistency to how modules are branched; work on automatic merge-ups is coming up. Let's keep this moving.

brynwhyman commented 4 years ago

One question though, in https://github.com/silverstripe/silverstripe-framework/pull/9310 the docs state:

"choose the "default branch" of the repository where you want to contribute to. That would usually target the next minor release of the module."

That seems to contradict with your plan to: "switch repository default branch to the new major (currently it's master)

Or am I missing something?

dnsl48 commented 4 years ago

No, that's correct. This is not a requirement, but rather a guess about the usual case. We assume there that usually people make non-breaking changes, which should end up in a minor release, so the major branch is the target for it. In that case master becomes the target for breaking changes (next major release).

ScopeyNZ commented 4 years ago

Ok, in the end I'm probably not realistically going to be the one having to keep branches merged/up to date with travis/etc so you do what you want. I just wanted to indicate that I don't actually see where the "toil" is here. I genuinely believe this is a bunch of work for no obvious benefit? I think you're still better off cutting any losses unless someone can point out what processes are being improved here.

Just before you go deleting master branches remember to go through the sources for userhelp (@sachajudd should be able to help you here) and ensure that any PRs based on the branches you're going to delete are closed.

sachajudd commented 4 years ago

+1 for checking in with @silverstripeux before deleting branches. You can find the branches userhelp relies on here: https://github.com/silverstripe/userhelp.silverstripe.org/blob/master/app/_config/docs-repositories.yml

dnsl48 commented 4 years ago

Thank you for pointing that out. I'm not thinking deleting any branches in bulk mode would be a good approach - there could be some side effects we may not anticipate at the moment. On the other hand, we should be able to delete them later one by one where it's necessary - I'm not gonna do that in scope of this issue.

dnsl48 commented 4 years ago

I don't actually see where the "toil" is here.

More consistency allows safer "bulk" operations, simpler manual processes and easier scripting for some automation of those. I think that's quite enough to begin with. We don't have to solve all the problems at once, but doing small steps like this one eventually will allow us to reduce toil and do better about automation.

ScopeyNZ commented 4 years ago

Okay - if you aren't deleting master branches then I still think that removing branch aliases is introducing toil.

ScopeyNZ commented 4 years ago

I am probably starting to sound a bit annoying right now but I don't accept the statements:

Again, perhaps I've latched onto this a little too much but it pains me to watch you have to spend days on something that might save us a few hours over the next few years - and I'm not joking here - I've had to deal with the existing system for a while now.

I guess the other thing that gets me here is that the issue is titled "All CWP modules should have a branch targeting next minor release" and if I saw it then I could've said "they do".

dnsl48 commented 4 years ago

Again, perhaps I've latched onto this a little too much

No worries, it's always good to have a discussion, especially for things that affect that many modules!

spend days on something that might save us a few hours

Well, to be fair, 95% of the time goes into updating travis configs and making stuff green, it has not much to do with updating our branching strategies and removing branch aliases.

Simpler conventions

They are simpler because we don't have to describe different types of modules and explain what's what. As such, there is no different workflows for core, satellite, cwp, supported, unsupported etc modules.

I don't think that having a master with a branch alias is any more confusing than having master and a minor branch.

Yes, that's been confusing for me as well at first. However, that's how community chose to work with the core modules. I really believe having different rules for different modules would increase complexity. Currently the conventions are really simple and fit into a single paragraph with 3 rules. If we start introducing satellite modules and different rules for them, then it becomes more complicated than necessary. That's what I was meaning rather than the workflow itself.

robbieaverill commented 4 years ago

Hi team, thought I'd jump in to provide my two cents.

(CWP) modules included kitchen-sink are tracking builds of the next minor release of core modules to ensure that we're prepared in time for the upcoming release.

Setting this up involves creating a dev recipe for the next release, e.g. CWP 2.5.x. Once all the branches are created for these minor branches, and everything is merged up, you then have your next release branch. It's a good idea to do this immediately after a new CWP minor release so you can ensure you get the maximum coverage in the lead up to the next release.

In the past, we've bumped dependencies of CWP by minor versions as they're needed (or desired).

recipes should require dev-master branches

Here are some quick fire problems with this:

In my experience, most of the CWP modules never have branch aliases changed on master, because most of them never have new major releases created. It's usually only when you create a minor version branch that you'd remove the branch alias, which is only because Composer treats 4.0 as 4.0.x-dev by default, so it provides no value there.

Then we would have to give definition for "satellite" modules and maintain different conventions for different modules. I don't think that's a good idea.

Not saying we should have different conventions for core and non-core modules, but we already have that distinction. "Core" is everything that you get when you install SilverStripe. Non-core is everything you get when you install CWP (via the kitchen sink at least). If the code is under our namespace on GitHub then it's a "supported module", and if not then it's a "supported dependency". There are some exceptions to this rule, e.g. fluent is a supported module.

Particularly in the CWP modules, the master is targeting the latest minor release (4.4.x), which means that we don't have automated builds running CWP against the next minor branch (4.5.x).

To be honest, I think this issue can be resolved in Travis configuration and maybe some documentation or release process note updates. We've previously discussed the idea of a "require latest" and "require lowest" kind of switch in this config to automate this for us, by installing the highest and lowest possible set of dependencies that our Composer constraints support.

work on automatic merge-ups is coming up

I'd love to see this happen! Be careful with automated merge ups in the composer.json file, things like branch aliases change independently in minor release branches sometimes and conflicts need to be resolved by hand. If there are no conflicts you should always check what Git has decided to merge in.

Lastly, I'd say that most of the things in our Travis configuration files are there for a reason. This includes installing dist/source depending on each repository's needs, the fact that each module has its own phpcs config, etc. Be careful when you remove something to ensure that you're adding or improving value. The last PR I saw removed the "version matrix" which allows us to test our code across multiple SilverStripe versions - it's basically the number 2 most important part of the config other than running automated tests.

I love the idea of making releasing smoother and catching issues sooner. Good luck getting this over the line!

dnsl48 commented 4 years ago

Thank you all participating in the discussion and reviewing the PRs. There's a lot of really good and valuable feedback! :)

Trying to keep in mind everything discussed so far here's the refined description of the plan.

  1. Update the master branch, fix Travis configs and remove the branch aliases (one of the following steps is to create a new major branch that substitutes the alias). Particularly in the travis config:
    • update PHP version matrix so that it reflects 7.1, 7.2 and 7.3
    • update requirements: recipe-cms:^4, postgres:^2 (to reduce chances for false-positives)
    • do not use framework code style configs (always use module own configs for phpcs)
  2. Create a new major branch for every module using the current master (using pre-merged version)
    • make the new branch default for each repository
    • pull some of the travis updates for the modules with behat tests (update to xenial)
    • update Travis configs for modules where builds are broken (pulling some changes from master)
  3. Create major branches for recipes
    • major branches of recipes should require major branches of modules
    • master branches of recipes should require master branches of modules (except framework, which is ^4)

Please, let me know what you think.

robbieaverill commented 4 years ago

1b. update requirements: recipe-cms:^4, postgres:^2 (to reduce chances for false-positives)

There's a lot of value in testing against the latest dev head. For example, we realise during development that MFA and subsites have a conflict (this happened) - the builds would remain gred until release time if you use stable constraints. You may also not be able to use framework in some older versions with a stable constraint - it used to exclude important files like TestAssetStore from dist packages. I think that's fixed now so it might be fine - either way, you'll need to constrain to a minor release line (or work out how to use --prefer-lowest etc flags to control this) otherwise you're still losing the "version matrix" feature.

Otherwise LGTM 👍

dnsl48 commented 4 years ago

you're still losing the "version matrix" feature

Right. I think that makes sense for major branches and has less value in master. I'll keep the version matrix and 4.x-dev requirements for the modules that have it configured now in the current pre-patched master (which is gonna become next minor). For the master branches though, I think it makes sense to reduce amount of moving parts to the minimum, since we ideally shouldn't touch master branches at all until we start working on the next major release.

ScopeyNZ commented 4 years ago

shouldn't touch master branches at all

Aside from the usual merge-ups 🙂

Cheddam commented 4 years ago

Config updates have been completed, and new major branches have been created. Closing this out.