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


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

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

Zope-CMF maillist  -  Zope-CMF@zope.org

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

Reply via email to