Update the ADDON_LIST of core profiles with upgrades

[mauritsvanrees]

See also my comment in Products.PlonePAS.

In the MigrationTool we have a list of add-ons. At the end of running @@plone-upgrade, all profiles in this list get upgraded.

In my opinion, all core packages that provide their own upgrade steps, should be in this list. That is what I intended it for. The list is missing a few profiles now. At least Products.PlonePAS and plone.volto.

I can have a look at updating it. It is on my list to have a look at anyway, for this issue: https://github.com/plone/Products.CMFPlone/issues/3346.

[rpatterson]

Good catch! I started digging into this, and now I'm at the bottom of a rabbit hole. Oh wait, I still haven't hit bottom. Expect a missive from me tomorrow. :-/

[rpatterson]

Good catch!

Actually now I kinda resent you for motivating me to read this code. ;-) It seems like there may be several messes here.

When I first read the code, it seemed to me that ADDON_LIST is about optional add-ons that are explicitly supported as a part of core Plone and thus should be upgraded when Plone is upgraded if the add-on is enabled/installed. Then, however, I noticed the overlap between the list of required core Plone "add-ons" defined as base profile dependencies and ADDON_LIST. Then I noticed that those dependency profiles aren't installed using the appropriate Products.GenericSetup install API but are instead installed by a post_handler. Then I noticed that the upgrade view is using it's own upgrade step logic instead of delegating to Products.GenericSetup. Then I noticed, if I'm reading the code correctly, that Products.GenericSetup doesn't support recursively upgrading a profile's dependencies. Ugh to all of this.

Why aren't we using the appropriate Products.GenericSetup upgrade API? If there's no reason we can't, then that's what we should do. Cleaning up the profile dependency handling on both install and upgrade seems like it would be a worthy PLIP.

More to follow...

[rpatterson]

More to follow...

Writing everything up was taking longer than just making the changes, so I drafted #3454 to both capture my findings and my suggested approach. See the commit messages there for more detailed discussion.

[mauritsvanrees]

I started preparing an answer, but it is weekend. :-) Your PR won't work completely, but it is an interesting approach that we can work with. More on Monday.

[mauritsvanrees]

Should I react on your PR or on this issue? I want to explain the current situation first, so let me do that here. I don't just explain it for you, but also for me. :-)

Current installation method

The add-plone-site view calls factory.addPloneSite . This installs the _DEFAULT_PROFILE, with is Products.CMFPlone:plone . This is one of the few base profiles instead of extension profiles. This means its steps are run in purge mode. But it is long ago that I dived into the difference between base and extension profiles. The files of this profile are in profiles/default . Its metadata.xml is empty, so no dependency profiles. It does have 24 other xml files. It has post_handler="Products.CMFPlone.setuphandlers.importFinalSteps" . This resets any installed profile versions, except from our default profile. There is a comment about this which I don't fully understand, but should be okay. Then the post handler installs our Products.CMFPlone:dependencies profile. This installs 14 other profiles, and then some portlets and registry settings. factory.addPloneSite then continues with installing the content profile (plone.app.contenttypes) and any selected optional profiles.

Current upgrade method

The main upgrade steps from Plone are in the separate plone.app.upgrade packages. These upgrades are registered for the Products.CMFPlone:plone profile. They can easily include code that is needed to upgrade a part of for example plone.app.portlets , which could have been done as an upgrade step in that package. But we have chosen to do most upgrade steps in plone.app.upgrade. When another package has upgrade steps, the reasons are either practical or historical:

• CMFPlacefulWorkflow is an optional package that you need to deliberately choose to activate. It makes sense for this to have its own upgrades.
• In Plone 4,plone.app.contenttypes was not yet in the core, so it needed its own upgrades. Nowadays, we could choose to do all its upgrades in plone.app.upgrades , and some of it might already be done there, but since the package has upgrades already, we would usually do it there.
• One other reason can be that one package version might be used in both Plone 5.2 and 6.0. If it needs an upgrade step, we would need to duplicate code in plone.app.upgrade.

The @@plone-upgrade view lists the upgrade steps, so you know roughly what is going to happen. It uses the list from MigrationTool.listUpgrades for this. This could be portal_setup.listUpgrades but we have one difference: we do not show upgrades that would upgrade the profile to a too new version. This small change means that you could have a Plone 5.2.0 site and still use the latest plone.app.upgrade which includes upgrade steps to Plone 5.2.7 and even Plone 6. You normally don't want this, but it could contain a needed improvement to an older upgrade step that you do need. I tested it just now, and this indeed works as I describe. We could port this change to GenericSetup itself though. The MigrationTool iterates over the steps with code that has not changed since 2010, so maybe some of it is no longer needed. If we change GenericSetup.listUpgrades, we could probably let GenericSetup upgrade our profile.

Then we upgrade the add-ons. This is a misleading name, because it is a list of core Plone profiles. Before I started this list, when a package got an upgrade step, someone would need to remember to add code in plone.app.upgrades to apply this upgrade step. This was easily forgotten. This list should contain all profiles that have upgrade steps. So not only the profiles listed in our dependencies profile, but also optional profiles, like plone.volto . We can choose to not include really optional profiles in here, because these would be visible as available upgrades in the Modules control panel. But I think it makes sense to include all of them here.

Future

I wonder if we can combine the current methods and your method:

• Move our listUpgrades change to GenericSetup (or keep it here for now).
• Keep the current plone and dependencies profiles mostly the same.
• Instead of a new core profile, create a post profile and put the two xml files from dependencies in there (portlets, registry).
• In MigrationTool we run the upgrades for the plone profile.
• And we upgrade (or reapply if needed) the dependencies profile.
• Profiles that are in the dependencies profile no longer need to be in the ADDON_LIST.
• But the ADDON_LIST should still contain profiles that are not strictly in the core dependencies.

I have not completely thought it through, but this might work, without many changes. It takes us closer to pure GenericSetup, as your intention is.

[rpatterson]

Should I react on your PR or on this issue?

I can't come up with a reason to care. ;-) But seriously, that PR is for you (or whoever) to do with as you please. Build on it, close it, whatever.

Current installation method

... This installs 14 other profiles, and then some portlets and registry settings.

Just in case the reason for this isn't clear, the actual import steps run in the Products.CMFPlone:dependencies profile are there and not in the base profile because they build on state imported from those 14 other dependency profiles.

Current upgrade method

They can easily include code that is needed to upgrade a part of for example plone.app.portlets , which could have been done as an upgrade step in that package. But we have chosen to do most upgrade steps in plone.app.upgrade.

I strongly disagree with this approach. It adds a gotcha for new contributors even if they've done The RIght Thing (tm) and learned our system for data migrations, GenericSetup upgrade steps. It requires more administrative work from both contributors and maintainers in that it doubles the quantity of PRs to be shepherded through to merge. Most importantly it means that related changes can't be in the same repository, let alone in the same commits as they should be. This makes code archaeology much more difficult. This is my opinion, but not necessarily relevant to what we're discussing here, and not anything I'll be tackling. Just wanted to share my thoughts.

The @@plone-upgrade view lists the upgrade steps, so you know roughly what is going to happen. It uses the list from MigrationTool.listUpgrades for this. This could be portal_setup.listUpgrades but we have one difference: we do not show upgrades that would upgrade the profile to a too new version. This small change means that you could have a Plone 5.2.0 site and still use the latest plone.app.upgrade which includes upgrade steps to Plone 5.2.7 and even Plone 6. You normally don't want this, but it could contain a needed improvement to an older upgrade step that you do need. I tested it just now, and this indeed works as I describe.

I can't imagine the use case you describe is at all common. In fact it sounds like an error condition, probably in dependency resolution and/or version pins. As such, this seems like additional complexity and maintenance cost and risk for no good reason. Again, just my thoughts.

Future

...

• Instead of a new core profile, create a post profile and put the two xml files from dependencies in there (portlets, registry). ...
• And we upgrade (or reapply if needed) the dependencies profile.

This seems more complicated to me than what I proposed. It still requires a profile that is explicitly empty of it's own import steps and using that to re-import that profile in order to access GenericSetup's existing support for recursive upgrades. So it saves no complexity on that count. With my proposal, upgrading a Plone portal (excepting optional add-ons) is the same thing as re-applying the Products.CMFPlone:plone base profile with the upgrade dependency strategy, which is just what I would expect it to be and is as simple to understand as I could get to. With this proposal, one has to know that upgrading a Plone involves doing the same but not with the base profile, with this special Products.CMFPlone:dependencies profile instead, which seems like more complexity and more to understand to me. Moving the actual import step data files from the Products.CMFPlone:dependencies profile to a separate profile seems to me like more work, one more thing to keep in mind, and adds separation between related concerns. IOW, I like that the data files there are about modifying or extending stuff done in the dependency profiles.

• But the ADDON_LIST should still contain profiles that are not strictly in the core dependencies.

Do you like my definition of optional add-ons that are supported by/in Plone core?

[mauritsvanrees]

You move all files from profiles/default to profiles/core. In my proposal we keep them in profiles/default, so there are much less changes.

re-applying the Products.CMFPlone:plone base profile

Sorry, but this sounds scary to me, although this may be without good reason.

• Re-applying a complete profile is something that we tell add-on authors not to do: you should write a proper upgrade step instead. It should be okay in this case, and I am going along with it in my proposal. But it is not at all what I would expect.
• A BASE profile is really something that you only apply once, as it automatically runs in purge mode: it is meant for new sites. In this case it should probably work because you pass purge_old=False, but I can't help but wonder which code in GenericSetup I am overlooking that will bite us.

I have an idea: wouldn't it help if GenericSetup had a method upgradeDependencies? Or installOrUpgradeDependencies? Most code from _runImportStepsFromContext would move there. Then we call upgradeDependencies("Products.CMFPlone:plone") and all dependencies get installed or upgraded. Then I don't need to worry anymore about applying any full profile.

I just thought of a big difference between your proposal and the current state:

• In the current state, first the steps from plone.app.upgrade are run, and then the dependencies (addons) are upgraded.
• In your proposal, it is the other way around. For most upgrade steps this probably does not matter. But some may expect that they are run on an already mostly migrated Plone 6 site, and now they are run on a not yet migrated Plone 5.2 site.
• Actually, you are not running any of the steps from plone.app.upgrade: they are registered for Products.CMFPlone:plone, and we do not upgrade this profile but reapply it. Should be fixable by registering these steps for Products.CMFPlone:core. This should also fix the order.

Do you like my definition of optional add-ons that are supported by/in Plone core?

Yes, that works.

I would still want to keep this list and apply any upgrades. And this means you have some profiles that are upgraded in "your" code because they are true dependencies, and some profiles that are upgraded in "my" code. So you have two places where an upgrade can happen.

[rpatterson]

You move all files from profiles/default to profiles/core. In my proposal we keep them in profiles/default, so there are much less changes.

We can't change the name of the base profile, Products.CMFPlone:plone, because of existing portals. In my proposal we need the base profile to only have dependencies and not run any import steps. So we either break the GenericSetup convention that the main profile is named default or we move all the real import step data files to a different profile directory, core in my proposal. Consider a developer browsing the ./Products/CMFPlone/profiles/ directory. They would reasonably assume that ./Products/CMFPlone/profiles/default/ directory is for the Products.CMFPlone:plone profile but without moving the files, that assumption would be wrong. So I still prefer what I propose but I can see both arguments and I don't care that much.

re-applying the Products.CMFPlone:plone base profile

Sorry, but this sounds scary to me, although this may be without good reason.

This is the only way that I know of that GenericSetup offers to recursively upgrade dependency profiles. This is the whole reason for having the top-level base profile having only dependencies and not running any upgrade steps.

• Re-applying a complete profile is something that we tell add-on authors not to do

Yes, this is definitely a pragmatic exception to that rule to access the recursive dependency profile upgrading.

I can't help but wonder which code in GenericSetup I am overlooking that will bite us.

Definitely needs good test coverage and extensive testing. Of course I think anything that touches the profile upgrade story needs that anyways.

I have an idea: wouldn't it help if GenericSetup had a method upgradeDependencies? Or installOrUpgradeDependencies?

That would work. I was just avoiding touching anything under zopefoundation/.... It definitely feels wrong to me that importing a profile is the only way to upgrade dependencies and that portal_setup.upgradeProfile() does not recursively upgrade profiles. In fact, I'd lobby for a portal_setup.upgradeProfile(upgrade_dependencies=True) kwarg instead of a new method.

But some may expect that they are run on an already mostly migrated Plone 6 site, and now they are run on a not yet migrated Plone 5.2 site.

Hmm, good catch. This will probably be a pain point and be a gotcha for developers writing new upgrade steps. IMO, one time transition pain is worth getting rid of the duplicated code, a more "correct" implementation, and less surprising behavior in the long run.

• Actually, you are not running any of the steps from plone.app.upgrade: they are registered for Products.CMFPlone:plone, and we do not upgrade this profile but reapply it.

I'll take your word on that. My recollection is that the base profile is upgraded separately elsewhere, but I would not be surprised to be wrong about that. Also would be moot if the changes you propose are made to GenericSetup.

Should be fixable by registering these steps for Products.CMFPlone:core. This should also fix the order.

Exactly.

Do you like my definition of optional add-ons that are supported by/in Plone core?

Yes, that works.

I would still want to keep this list and apply any upgrades.

Yes, but that list would now be the list of "optional add-ons that are supported by/in Plone core"? Are we understanding each other?

And this means you have some profiles that are upgraded in "your" code because they are true dependencies, and some profiles that are upgraded in "my" code. So you have two places where an upgrade can happen.

Sorry, not following you on this one. Can you elaborate/clarify?

[mauritsvanrees]

In fact, I'd lobby for a portal_setup.upgradeProfile(upgrade_dependencies=True) kwarg instead of a new method.

That could work.

In general, I have the idea that we need a few small PRs in GenericSetup to make our upgrade story easier.

Here is the first minor one, which is only about not logging uninteresting things by passing quiet=True: https://github.com/zopefoundation/Products.GenericSetup/pull/119 We would use that when calling upgradeProfile.

Yes, but that list would now be the list of "optional add-ons that are supported by/in Plone core"? Are we understanding each other?

I think we do. In current code, this definition is not correct. With your PR, the definition fits.

And this means you have some profiles that are upgraded in "your" code because they are true dependencies, and some profiles that are upgraded in "my" code. So you have two places where an upgrade can happen.

Sorry, not following you on this one. Can you elaborate/clarify?

Currently, all profiles that are upgraded are in ADDON_LIST. With your PR, some profiles are upgraded when we reapply the main profile, and others are upgraded because they are in the ADDON_LIST. That may cause some confusion. It may be enough to add a comment in both ADDON_LIST and metadata.xml, pointing to the other list with a small explanation.

[rpatterson]

And this means you have some profiles that are upgraded in "your" code because they are true dependencies, and some profiles that are upgraded in "my" code. So you have two places where an upgrade can happen.

Sorry, not following you on this one. Can you elaborate/clarify?

Currently, all profiles that are upgraded are in ADDON_LIST. With your PR, some profiles are upgraded when we reapply the main profile, and others are upgraded because they are in the ADDON_LIST. That may cause some confusion. It may be enough to add a comment in both ADDON_LIST and metadata.xml, pointing to the other list with a small explanation.

Ah, gotcha, makes sense and I have no better solution off the top of my head.

Good luck with all this and thanks for making this discussion so pleasant!