Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Maurits, Maurits van Rees wrote: > Op 25-09-15 om 10:31 schreef yuppie: >> 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. why are you testing features that don't make sense to you? Are the names BASE and EXTENSION not clear enough? Why would someone expect that you can use an EXTENSION profile as the base and extend it with a BASE profile? > Then the purge > of all versions should happen right before applying the base profile, so > after its dependency profile. Why would someone want to import the first profile in the chain (version is set automatically) and remove the version data again in the next step? >> 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. Why would an upgrade step (re-)apply a complete base profile in purge mode? Why should we purge the versions if the code that purges the old configuration is never reached? >> 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. [...] > 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? Well, I would say: There are several ways to use the import machinery. Only a few ways should normally be used. All other options are some kind of expert mode that allows to (re-)apply selected parts of all kinds of profiles. You should use those options only if you know exactly what you do. I started the discussion about purging versions automatically. And I think we should do that only in the normal use case where someone obviously wants to start from scratch. And that's the case if we are in purge mode and (re-)apply a "complete" profile. Base profiles, snapshots and tarballs are usually complete profiles. But snapshots and tarballs don't care about versions and upgrades, so it might be ok to ignore them. Your updated pull request still purges versions if we depend on a base profile, but don't (re-)apply it. That's not what I would expect. Cheers, Yuppie ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Maurits, Maurits van Rees wrote: > Op 23-09-15 om 16:53 schreef yuppie: >> if you run a base profile in purge mode, shouldn't that purge profile >> versions automatically? > > GenericSetup itself is not doing this currently. > It might be good to do that, but I guess it is not always needed. > Then again, I have never written a base profile myself, only extension > profiles, so I'm not sure what creators of base profiles expect here. 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. >> Sorry, but I'm still not convinced. >> >> I agree the negative effect is smaller in the tests. I would not object >> if you make automated cleanups in tests before you modify them. > > If you mean you want me to only cleanup an individual test file or even > an individual test method when I touch it, then: no way. Then pep8 > fixes and real fixes will get thrown together making it harder to judge > a pull request. Let's just get it over with for the tests in one go. > Otherwise: never mind, let's not worry about pep8 for this package ever > at all. But maybe I misinterpret your words. I meant: Clean up the files you plan to touch, commit that change directly without PR, create your PR. > Anyway, let's not lose too much sleep arguing over this. > I have created a new pull request, superseding the other pep8 one. > https://github.com/zopefoundation/Products.GenericSetup/pull/21 > > This cleans up pep8 in the tests. The first commit is only white space. > The second commit is more aggressive, but far, far smaller. > > Plus in the rest of the code this fixes only pep8 issues in comments or > in empty lines: removing or adding a line or removing white space on an > otherwise blank line. This would be a compromise I can live with. But I was not the only one who voted against your first PR. Cheers, Yuppie ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
On 2015-09-22 12:30, yuppie wrote: [...] >> - pep8. This fixes over 6000 pep8 errors... Most of them fixed with the >> autopep8 command line tool. Small in scope yes, but due to all those >> errors a *very* large pull request. All tests pass. > > -1 > > I agree with the goal to try to respect pep8 rules and to use tools that > help doing this. But this is a massive reformatting that adds a lot of > noise if you use blame or similar techniques. And I use often diffs > between different versions to understand the history of the code. > > There might be a subset of pep8 rules that is already respected in most > parts of the code and where fixing the rest wouldn't add much noise. I dont agree. The noise is one commit. Blame does not make sense without looking at the whole history anyway. So its one more diff in a whole series. My only point is to not make code pep8 is to not affect other peoples branches/ open pull requests, because rebase/merge after any massive change is indeed lot of work. Jens Klein ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Maurits,
Maurits van Rees wrote:
> Adding 'purgeProfileVersions' is also on my wish list now, which is
> really simple:
>
> portal_setup._profile_upgrade_versions = {}
>
> There were a few problems in Plone due to my change with the dependency
> strategies. I found that those were caused by importing the base Plone
> profile (so no extension profile), so this ran in purge mode, which
> meant several settings of add-ons were overwritten. Only a problem you
> really ever encounter in test code that tries to do to much.
>
> Purging the profile versions from portal setup helped solve this. And I
> would rather call an official method than accessing the private
> _profile_upgrade_versions from within Plone code.
if you run a base profile in purge mode, shouldn't that purge profile
versions automatically?
> But actually, it is not that bad I think. When I look at the top level
> files at how many lines of them are to 'blame' on my pep8 change, I get
> this table. Not sure if this gets across nice in email:
>
> FilenameBlameLinesPercentage
> __init__.py5559%
> components.py285595%
> content.py254176%
> context.py16272322%
> differ.py6419633%
> events.py0550%
> exceptions.py0210%
> interfaces.py638477%
> metadata.py207726%
> permissions.py0160%
> registry.py447466%
> rolemap.py4421920%
> testing.py01780%
> tool.py10514267%
> upgrade.py142745%
> utils.py499275%
> zcml.py203725%
>
> So the file with the biggest percentage of lines changed, is differ.py
> with 33 percent. We have context.py, metadata.py and rolemap.py between
> 20 and 26 percent. The rest is below 10 percent.
> The biggest and most central one, tool.py, has 7 percent of its lines
> changed.
>
> In the tests directory things are very different, given that there are
> about 5000 pep8 errors there. git itself says for three files that it
> completely rewrote them:
>
> rewrite Products/GenericSetup/tests/test_differ.py (66%)
> rewrite Products/GenericSetup/tests/test_registry.py (78%)
> rewrite Products/GenericSetup/tests/test_rolemap.py (64%)
>
> But I would say for the tests a 'git blame' is less needed.
Sorry, but I'm still not convinced.
I agree the negative effect is smaller in the tests. I would not object
if you make automated cleanups in tests before you modify them.
Cheers,
Yuppie
___
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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Jens, Jens W. Klein wrote: > On 2015-09-22 12:30, yuppie wrote: > [...] >>> - pep8. This fixes over 6000 pep8 errors... Most of them fixed with the >>> autopep8 command line tool. Small in scope yes, but due to all those >>> errors a *very* large pull request. All tests pass. >> >> -1 >> >> I agree with the goal to try to respect pep8 rules and to use tools that >> help doing this. But this is a massive reformatting that adds a lot of >> noise if you use blame or similar techniques. And I use often diffs >> between different versions to understand the history of the code. >> >> There might be a subset of pep8 rules that is already respected in most >> parts of the code and where fixing the rest wouldn't add much noise. > > I dont agree. The noise is one commit. Blame does not make sense without > looking at the whole history anyway. So its one more diff in a whole series. why would you look at the whole history? Blame tells you which revision modified the lines you are interested in. So you can jump directly to that revision. It's annoying if this revision is just a code cleanup. > My only point is to not make code pep8 is to not affect other peoples > branches/ open pull requests, because rebase/merge after any massive > change is indeed lot of work. This is not an issue with GenericSetup, but if you have several release branches, it also makes it hard to port changes from one branch to an other. I'm not completely against code cleanups, but I don't think touching thousands of lines just to do massive automated cleanups is the right way to do it. Cheers, Yuppie ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Op 22-09-15 om 12:30 schreef yuppie:
Hi Maurits,
Maurits van Rees wrote:
Meanwhile, I have added two more pull requests, far smaller in scope:
- Add 'unsetLastVersionForProfile' method to portal_setup. This removes
the profile id from the profile upgrade versions.
+1
Adding 'purgeProfileVersions' is also on my wish list now, which is
really simple:
portal_setup._profile_upgrade_versions = {}
There were a few problems in Plone due to my change with the dependency
strategies. I found that those were caused by importing the base Plone
profile (so no extension profile), so this ran in purge mode, which
meant several settings of add-ons were overwritten. Only a problem you
really ever encounter in test code that tries to do to much.
Purging the profile versions from portal setup helped solve this. And I
would rather call an official method than accessing the private
_profile_upgrade_versions from within Plone code.
- pep8. This fixes over 6000 pep8 errors... Most of them fixed with the
autopep8 command line tool. Small in scope yes, but due to all those
errors a *very* large pull request. All tests pass.
-1
I agree with the goal to try to respect pep8 rules and to use tools that
help doing this. But this is a massive reformatting that adds a lot of
noise if you use blame or similar techniques. And I use often diffs
between different versions to understand the history of the code.
There might be a subset of pep8 rules that is already respected in most
parts of the code and where fixing the rest wouldn't add much noise.
I can imagine your concern about reduced value for 'blame'. You can
still checkout a previous version and do the git blame there, but it is
more of a hassle, I agree. It is personal preference whether this
reduced blame value weighs more heavily than the reduced frustration of
working with ugly looking code.
But actually, it is not that bad I think. When I look at the top level
files at how many lines of them are to 'blame' on my pep8 change, I get
this table. Not sure if this gets across nice in email:
FilenameBlame Lines Percentage
__init__.py 5 55 9%
components.py 28 559 5%
content.py 25 417 6%
context.py 162 723 22%
differ.py 64 196 33%
events.py 0 55 0%
exceptions.py 0 21 0%
interfaces.py 63 847 7%
metadata.py 20 77 26%
permissions.py 0 16 0%
registry.py 44 746 6%
rolemap.py 44 219 20%
testing.py 0 178 0%
tool.py 105 14267%
upgrade.py 14 274 5%
utils.py49 927 5%
zcml.py 20 372 5%
So the file with the biggest percentage of lines changed, is differ.py
with 33 percent. We have context.py, metadata.py and rolemap.py between
20 and 26 percent. The rest is below 10 percent.
The biggest and most central one, tool.py, has 7 percent of its lines
changed.
In the tests directory things are very different, given that there are
about 5000 pep8 errors there. git itself says for three files that it
completely rewrote them:
rewrite Products/GenericSetup/tests/test_differ.py (66%)
rewrite Products/GenericSetup/tests/test_registry.py (78%)
rewrite Products/GenericSetup/tests/test_rolemap.py (64%)
But I would say for the tests a 'git blame' is less needed.
--
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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 09/22/2015 06:30 AM, yuppie wrote: > -1 > > I agree with the goal to try to respect pep8 rules and to use tools > that help doing this. But this is a massive reformatting that adds a > lot of noise if you use blame or similar techniques. And I use often > diffs between different versions to understand the history of the > code. I'm -1 on that one, too, for essentially the same reasons. Tres. - -- === Tres Seaver +1 540-429-0999 tsea...@palladion.com Palladion Software "Excellence by Design"http://palladion.com -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJWAb/mAAoJEPKpaDSJE9HYlN4QAIaUNifw3HY9DXKEwms+y3yg hMHDtK4Y6kXZgTKQU6BvTtz9jdLhZJoAqgTgFDpjn8LCdMCgLyYa/J7ABDEWdaDk 0pWVQJCg34lIPwHHxyGV6LuV2q7Eqzt6Ht2BzeXKmzcX5650cIAnKtfrVpHfCvqW 2flThW0dl6tTds8qIml6UrQY6f2yndTGOsnTHwPIcnFJ1jgP7hj8mhS9FR6j2wx1 9NkiXbXbJFDKKU0l5QQplEGhWrBiJdeZvJl7lCluKmr3PUhnc+PNW6Cn68izxZzC 9rz0KUUvqdkMcUwEzULDu847Ubo0iN2UcdS2o0dipfBdpQ8F6s4D9e/Bc2gcZHhh Khx7VbZfU8vQI2k2Xu2BProy4oJdANf6lvuLnHJFKJdFYCpdt9n3W8Kc/IRYnLPV TCseACQZ71vkAtbWH8OBtrLmNCfaEn5N6+ckIhlX7kEOMXiFRLalxKsdxpKbl32b Yin+0Nt+iX41D4zZ6rANLmhROqj+GSvP7lzt2yA4U03fuCOFp5jLL6T3mS89Ht99 9U1wZX0GJYqcQmt0evppQdlcutJkZ3cq6l9ebU1C+YnEuhzkZvwj0bCaQgqr/lNY x3ctaxDx+MQ0uX0udT8NSMws/bWEUugojCm3hPKHOzZEwUJtsK26C0KDhAsxwT8N SDC4yNcuvHKDHYw2oJx0 =PTvJ -END PGP SIGNATURE- ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Maurits, Maurits van Rees wrote: > Meanwhile, I have added two more pull requests, far smaller in scope: > > - Add 'unsetLastVersionForProfile' method to portal_setup. This removes > the profile id from the profile upgrade versions. +1 > - pep8. This fixes over 6000 pep8 errors... Most of them fixed with the > autopep8 command line tool. Small in scope yes, but due to all those > errors a *very* large pull request. All tests pass. -1 I agree with the goal to try to respect pep8 rules and to use tools that help doing this. But this is a massive reformatting that adds a lot of noise if you use blame or similar techniques. And I use often diffs between different versions to understand the history of the code. There might be a subset of pep8 rules that is already respected in most parts of the code and where fixing the rest wouldn't add much noise. Cheers, Yuppie ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Op 18-09-15 om 09:45 schreef yuppie: Hi Maurits, Maurits van Rees wrote: Op 14-09-15 om 09:02 schreef Charlie Clark: This sounds like a good idea. The ZMI has traditionally suffered from just having more and more knobs to twiddle with little thought of the actual UI. I don't think that should block this PR (if it's required to solve a common problem at short notice). I would like to look at the UI later. An extra tab and some cleanup on the current Import should not be that difficult. that would be nice, but I agree the UI issues should not block your PR. Meanwhile, is it okay to merge the current pull request and make a release? It seems that most people think it is okay, but yuppie is most on the fence. No objections. Thanks. This was merged. Since it *is* a change to earlier behavior, it may be better to increase the minor version and release this as 1.8.0. At least Plone needed a few changes in code used for creating a site, and in tests. I have updated the version accordingly. Meanwhile, I have added two more pull requests, far smaller in scope: - Add 'unsetLastVersionForProfile' method to portal_setup. This removes the profile id from the profile upgrade versions. - pep8. This fixes over 6000 pep8 errors... Most of them fixed with the autopep8 command line tool. Small in scope yes, but due to all those errors a *very* large pull request. All tests pass. -- 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Maurits, Maurits van Rees wrote: > Op 14-09-15 om 09:02 schreef Charlie Clark: >> This sounds like a good idea. The ZMI has traditionally suffered from >> just having more and more knobs to twiddle with little thought of the >> actual UI. I don't think that should block this PR (if it's required to >> solve a common problem at short notice). > > I would like to look at the UI later. An extra tab and some cleanup on > the current Import should not be that difficult. that would be nice, but I agree the UI issues should not block your PR. > Meanwhile, is it okay to merge the current pull request and make a > release? It seems that most people think it is okay, but yuppie is most > on the fence. No objections. Cheers, Yuppie ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Op 14-09-15 om 09:02 schreef Charlie Clark: I didn't have a look at the Plone 5 control panel, but as you describe it, something similar would be quite useful in the portal_setup UI. But the Import tab has already too many options for rare use cases. It might be better to add a new tab for importing add-ons. This sounds like a good idea. The ZMI has traditionally suffered from just having more and more knobs to twiddle with little thought of the actual UI. I don't think that should block this PR (if it's required to solve a common problem at short notice). I would like to look at the UI later. An extra tab and some cleanup on the current Import should not be that difficult. Meanwhile, is it okay to merge the current pull request and make a release? It seems that most people think it is okay, but yuppie is most on the fence. To reiterate for clarity, the most important change is the default behavior of runAllImportStepsFromProfile: Old situation: _all_ dependency profiles are applied New situation: _new_ dependency profiles are applied, and for _old_ (already applied) dependency profiles we run the upgrade steps Cheers, -- 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Your mail with the screen shot was delayed, so I didn't see it before I wrote my reply. X-Mailman-Approved-At: Fri, 11 Sep 2015 15:20:01 +0200 Did someone have to approve that manually? Yes, I think that there's a limit to 100 KB per e-mail, though I'm surprised that the screenshot is as big as it is. Anyway: Looks like a tab that tries to make everybody happy. It's too complex, but that's not your fault. This is definitely the case. I think anyone not familiar with the implementation would not be able to tell the difference between the new options. But, for me, this is not about how it works in the ZMI. I am sure with some back and forth like this we can work something out. It is mostly about: what happens when in code you do runAllImportStepsFromProfile with the default settings. BTW 2, Plone 5 is still also using CMFQuickInstaller, but that is going the way of the dodo. Slowly. I didn't have a look at the Plone 5 control panel, but as you describe it, something similar would be quite useful in the portal_setup UI. But the Import tab has already too many options for rare use cases. It might be better to add a new tab for importing add-ons. This sounds like a good idea. The ZMI has traditionally suffered from just having more and more knobs to twiddle with little thought of the actual UI. I don't think that should block this PR (if it's required to solve a common problem at short notice). Charlie -- Charlie Clark Managing Director Clark Consulting & Research German Office Kronenstr. 27a Düsseldorf D- 40217 Tel: +49-211-600-3657 Mobile: +49-178-782-6226 ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 09/10/2015 11:43 PM, Maurits van Rees wrote: > I thought I would add a link to my branch on Launchpad, but apparently > only the master and 1.6 branches are synced, so mine is not visible: > https://code.launchpad.net/zope-genericsetup The mirroring bits aren't (yet) automated in Launchpad's Git integration. I just updated manually via: > $ git remote -v launchpad > git+ssh://git.launchpad.net/zope-genericsetup (fetch) launchpad > git+ssh://git.launchpad.net/zope-genericsetup (push) origin > g...@github.com:zopefoundation/Products.GenericSetup (fetch) origin > g...@github.com:zopefoundation/Products.GenericSetup (push) $ git fetch > origin remote: Counting objects: 61, done. remote: Compressing > objects: 100% (31/31), done. remote: Total 61 (delta 34), reused 22 > (delta 22), pack-reused 8 Unpacking objects: 100% (61/61), done. From > github.com:zopefoundation/Products.GenericSetup * [new branch] > maurits-apply-dependency-only-once -> > origin/maurits-apply-dependency-only-once $ git checkout > maurits-apply-dependency-only-once Branch > maurits-apply-dependency-only-once set up to track remote branch > maurits-apply-dependency-only-once from origin. Switched to a new > branch 'maurits-apply-dependency-only-once' $ git push --all launchpad > Total 0 (delta 0), reused 0 (delta 0) To > git+ssh://git.launchpad.net/zope-genericsetup * [new branch] > maurits-apply-dependency-only-once -> > maurits-apply-dependency-only-once Tres. - -- === Tres Seaver +1 540-429-0999 tsea...@palladion.com Palladion Software "Excellence by Design"http://palladion.com -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJV8uiCAAoJEPKpaDSJE9HY04QP/jcMG0C8LUVl1f1M0UpGoFMJ VhZeWMmvLQteaJQw5vXahnmFrn1UaqjIyKkoZcpPoi00vUXcR3cvKgWsJ0Q7AmW+ 7dAn0Zt9CSjHNGOOos4ke6+JHKGP2ITL+pJiRSASQz70sCsF1WPU4xK7K2tNFnQd 7AYndav7t3z0YQnywMmwa3eb7h8fbcF7qydU5zqeylaxFwtG+rbnej2h4FSR1tON /S9UXxlJ64vkJRkS54Hh6ra2fX3PCwMo6lX3dHxS3E9qEMT3AGvihEq43icg8tMP 7xhc7ansg1frlt698l/m+OiVyVP66OHuniN4XwBPuGHV0O0njlY0VRAGw/XpDw8h cSxkXK135qmPkREzm0fo0cWI0Aswa5r8N11p84cETDuPY16MjaGDvRnGjkd65sJg 3c0B/0YxNRqssf7pil2FTQKVXf/Q8w0bBn+3lcBcyV2IdCiCey+Cb70Wv/Mh9NkZ PpTp+iEiQIStWfLod/8jFloUBbtEBqH4lEWm+QGBiJ+IvbfH1sQAO+1/5yk5Hh4I 9wSnGAmR6+GBT00xGeDOtucHUb39azkrNTUKEgsO3akayFunKnGq07mm7QVpjm+n bzg4vtufbi0atpMQRIfQtA720TjmVfWSFkXWAnUqt0t76nMfdaqSdZ9kXbKs0nGK dC4QlOeTt7ZXHTpvednr =RH2J -END PGP SIGNATURE- ___ 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi yuppie, Op 11-09-15 om 09:00 schreef yuppie: The Import tab wasn't built with extension profiles and upgrade steps in mind. It is not exactly the UI you expect for installing add-ons. One issue you want to fix is this: Someone installs an add-on on a site that has outdated configuration. Because nobody warns him, he messes up the configuration of his site. Your answer is: Don't bother him with this detail, I'm sure he wanted to upgrade his dependencies first. I'll do that for him silently. My answer is: His site is in a bad state if he didn't run the upgrades. He should fix that before he starts installing add-ons. Someone has a site with profile hoopyfrood up to date. He has no idea what this does, it was just applied as a dependency of another add-on. He adds an addon with profile towel to the buildout config. This requires a new version of the package that has profile hoopyfrood. Fine, he updates the version pins, runs buildout and restarts the site. He installs the towel add-on. Old answer: no matter what the state of the hoopyfrood profile, we apply it. This may be mostly harmless (I swear I did not plan this culture reference), but it does more than necessary and may be harmful in some cases. My answer: apply the upgrade step of hoopyfrood. Everybody happy. Your answer: give an error, pointing to the Upgrades tab, telling him to run the upgrades of hoopyfrood first? Then nothing happens. Your answer may be fine when applying a profile in the ZMI, but then again, I have updated the pull request to give the user more options in the ZMI so that may be fine already. So maybe you are happy with that already. From the point of view of existing code in add-ons that call runAllImportStepsFromProfile and expect it to maybe do too much but at least work, your answer would be a regression: either nothing is done or you get an exception. BTW, the add-ons control panel in Plone 5 lists the available upgrades first, and then the installable new add-ons. We could improve the ZMI similarly, like: - add a warning at the top of the Imports tab if there are upgrades - Show on the Upgrades tab which profiles actually have upgrades that need to be applied. Now I sometimes click through the entire list to check this. Maybe something for another pull request. But, for me, this is not about how it works in the ZMI. I am sure with some back and forth like this we can work something out. It is mostly about: what happens when in code you do runAllImportStepsFromProfile with the default settings. BTW 2, Plone 5 is still also using CMFQuickInstaller, but that is going the way of the dodo. Slowly. In other words: 'profile-' is the default prefix. All methods handle ids without prefix the same way as ids with the default prefix. Correct? Yes, exactly. Thanks for the feedback, -- 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi yuppie, Op 09-09-15 om 11:43 schreef yuppie: Hi Maurits, Maurits van Rees wrote: Dependency profiles from metadata.xml that are already applied, are not applied again. Instead, its upgrade steps, if any, are applied. In code you can choose the old behavior of always applying the dependencies, by calling runAllImportStepsFromProfile with always_apply_profiles=True. Or you can choose to be happy with any applied version and ignore any upgrade steps, by using upgrade_dependencies=False. Note that these arguments cannot both be True. assuming that profiles always depend on the latest versions of specified profiles is fine. But wouldn't it be better to make upgrading existing profiles an explicit extra step in the UI? Does it make sense to upgrade only the dependencies, not other applied profiles? Basically I am thinking: we used to reapply the entire profile, let's instead not do nothing at all, but meet halfway: run the upgrade steps. I guess we could add an extra option in the UI, making use of these new options. The user already has the option 'Include dependencies' there, default Yes. An extra option might be 'Apply upgrade steps of already applied profiles instead of reapplying them completely', with default Yes. We might then need to make it possible to select all possible combinations of what I now made possible. Danger is that it gets confusing for the end user (well, site admin). Unrelated profiles should be left alone. Possibly a method 'applyAllUpgradeStepsOfAllProfile' could be useful, with a big button on the Upgrades tab. But not in this pull request. I would just raise an error if the dependencies are not up to date and ignoring the problem or running upgrades or re-applying profiles is not explicitly specified. If only one option is allowed, why not using one argument? outdated_dependencies=None|ignore|upgrade|reapply Maybe 'upgrade_strategy=...' It *does* mean one keyword argument less, which can be nice. But I can also live with your solution. As long as the behavior doesn't change if the dependencies were not applied before, I have no objections. Good. Note that there is some discussion on github too, but not really about the above points. See https://github.com/zopefoundation/Products.GenericSetup/pull/16 Less tricky is the second change: Be more forgiving when dealing with profile ids with or without profile- at the start. All functions that accept a profile id argument and only work when the id does not have this string at the start, will now strip it off if it is there. For example, getLastVersionForProfile will give the same answer whether you ask it for the version of profile id foo or profile-foo. This doesn't make things clearer to me. IIRC the prefixes were added to have separate namespaces and an easy way to figure out which kind of profile we are dealing with. Do you propose to make these namespaces obsolete in the code? Or only in some places? How do I know if the profile_id argument requires the prefix in a specific method? Basically I want to avoid that GenericSetup says "No, there is no profile with id X" and silently thinks "You should have asked me for profile-X instead, I have got that one lying around here just fine." The 'profile-' and 'snapshot-' prefixes are still used. Most methods only cared about 'profile-', but some of those expected the stripped id and others the id with the prefix. I found that frustrating to deal with. Some methods handle both 'normal' profiles and snapshots. With my changes, these now have: - if profile starts with 'snapshot-': do A. - elif profile starts with 'profile-': do B. - else: same as 'profile-' Is that clearer? -- 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
Re: [Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi Maurits, Maurits van Rees wrote: > Dependency profiles from metadata.xml that are already applied, are not > applied again. Instead, its upgrade steps, if any, are applied. In code > you can choose the old behavior of always applying the dependencies, by > calling runAllImportStepsFromProfile with always_apply_profiles=True. Or > you can choose to be happy with any applied version and ignore any > upgrade steps, by using upgrade_dependencies=False. Note that these > arguments cannot both be True. assuming that profiles always depend on the latest versions of specified profiles is fine. But wouldn't it be better to make upgrading existing profiles an explicit extra step in the UI? Does it make sense to upgrade only the dependencies, not other applied profiles? I would just raise an error if the dependencies are not up to date and ignoring the problem or running upgrades or re-applying profiles is not explicitly specified. If only one option is allowed, why not using one argument? outdated_dependencies=None|ignore|upgrade|reapply But I can also live with your solution. As long as the behavior doesn't change if the dependencies were not applied before, I have no objections. > Less tricky is the second change: > > Be more forgiving when dealing with profile ids with or without profile- > at the start. All functions that accept a profile id argument and only > work when the id does not have this string at the start, will now strip > it off if it is there. For example, getLastVersionForProfile will give > the same answer whether you ask it for the version of profile id foo or > profile-foo. This doesn't make things clearer to me. IIRC the prefixes were added to have separate namespaces and an easy way to figure out which kind of profile we are dealing with. Do you propose to make these namespaces obsolete in the code? Or only in some places? How do I know if the profile_id argument requires the prefix in a specific method? Cheers, Yuppie ___ 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
[Zope-CMF] GenericSetup: Apply profile dependencies only once
Hi, Can someone have a look at this pull request I made? https://github.com/zopefoundation/Products.GenericSetup/pull/16 It does two changes. I think they are both sane, but the first one could be tricky, so feedback would be good. This is the change: Dependency profiles from metadata.xml that are already applied, are not applied again. Instead, its upgrade steps, if any, are applied. In code you can choose the old behavior of always applying the dependencies, by calling runAllImportStepsFromProfile with always_apply_profiles=True. Or you can choose to be happy with any applied version and ignore any upgrade steps, by using upgrade_dependencies=False. Note that these arguments cannot both be True. Less tricky is the second change: Be more forgiving when dealing with profile ids with or without profile- at the start. All functions that accept a profile id argument and only work when the id does not have this string at the start, will now strip it off if it is there. For example, getLastVersionForProfile will give the same answer whether you ask it for the version of profile id foo or profile-foo. For more background info see the pull request. I added tests and all tests pass. Thanks, -- 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
