[Zope-CMF] GenericSetup: Apply profile dependencies only once

[Maurits van Rees]

Hi yuppie,

Op 25-09-15 om 10:31 schreef yuppie:
> Hi Maurits,
>
> Maurits van Rees wrote:
>> Op 24-09-15 om 13:54 schreef yuppie:
>>> if you run a base profile in purge mode, you usually want to undo all
>>> previous configuration and start from scratch. In theory you could do
>>> that just with some setup handlers and keep the rest of the
>>> configuration. But I doubt someone uses it that way.
>>>
>>> If you start from scratch, old profile versions data becomes incorrect.
>>> So I think GenericSetup should delete that data automatically.
>>
>> I have updated my pull request to add that purgeProfileVersions method
>> and to run this before running the import steps of a base profile.
>>
>> See https://github.com/zopefoundation/Products.GenericSetup/pull/18
>
> it looks a bit strange to have that new code inside the loop because
> versions should only be purged once before applying the first profile in
> the chain.
>
> But I hope these assertions are true:
>
> - a profile that depends on more than one base profile is broken anyway

Agreed.

> - if there is a base profile in the chain, it is always the first in the
> chain

Not necessarily, though I do hope so.

I am expecting that the base profile is the first and only profile in 
the chain.

But in the tests I explicitly test what would happen if the base profile 
itself has a dependency, although this makes no sense to me.  In this 
case the base profile would be the second in the chain.  Then the purge 
of all versions should happen right before applying the base profile, so 
after its dependency profile.

> So it might be ok to purge versions inside the loop. But I don't think
> it makes sense to purge versions if we don't reapply the base profile in
> purge mode. I would make the change after the BeforeProfileImportEvent.

Problem may then be that this part of the code is never reached. 
Between those two spots are the checks to see whether the profile that 
is about to be imported has already been applied previously.  And we use 
the profile upgrade versions for this.  When a base profile is in this 
way applied a second time, the checks would conclude it has already been 
applied and will continue with the next profile in the for loop, without 
purging the versions.

> In that place it should be possible to use the shouldPurge method
> instead of checking the profile type. Or is anyone running extension
> profiles in purge mode? In that case we have to check for both.

Let me think.

We have this method:

     def _getImportContext(self, context_id, should_purge=None, 
archive=None)

In all cases, if should_purge is explicitly set to True or False, that 
value wins.  In case it is None, you get this:

- tarballs/archives: should_purge = False
- snapshots: should_purge = True
- base/extension profiles: should_purge = (info.get('type') != EXTENSION)

For tarballs and snapshots I don't think we should purge the profile 
upgrade versions, but those kinds of profiles do not enter in the loop 
we are currently discussing.

For a BASE profile, the pull request currently always purges the profile 
upgrade versions.  It is probably good to not do this when someone 
explicitly calls the method with purge_old=False.

For an EXTENSION profile, the pull request currently does not purge the 
profile upgrade versions.  We could purge it when someone explicitly 
calls the method with purge_old=True.  But I am guessing this is not 
what people expect.  The use case for explicitly applying an extension 
profile with purge_old=True, would presumably be something like this 
example:
- The profile starts out with only a properties.xml, which adds a list 
property to the site root with three items in it.
- One of those was a mistake, so it is removed from properties.xml
- Now someone might apply the profile with purge_old=True, which should 
result in the list property ending up with only the two wanted 
properties.  (Better would of course be to write a proper upgrade step.)
In that case, I don't think the user can expect that the profile upgrade 
versions are purged.

In the case of a BASE profile, the UI already says something like: "this 
is dangerous, you probably do not want this."

So I would say: we purge the profile upgrade versions only if a base 
profile is imported, and should_purge is None or True.  I have updated 
the pull request with that.

Does that sound reasonable?

-- 
Maurits van Rees: http://maurits.vanrees.org/
Zest Software: http://zestsoftware.nl