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
_______________________________________________
Zope-CMF maillist - Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf
See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests
