-
-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use current ZODB #317
use current ZODB #317
Conversation
Please tell me you don't need a contributor's agreement to change some version pins. :) |
I don't think we need a CLA for some version pins. It is an auto-generated message, part of our standard checks. Thanks for the contribution. It is a big step forward for Plones effort to modernize its code base which includes to use most recent releases of its dependencies! |
@jimfulton jenkins job is green!!! Thank you @jimfulton! @plone/framework-team, @esteele do we need to discuss this first or can we merge right away? |
Woo hoo! :) |
It doesn't break anything, right? So we can merge it, IMO.
|
@jimfulton for trivial changes lines this, we didn't need a CLA :D (me, hiding away)
|
Personally I'm ok with merging. Though, since we are already in beta I guess @esteele should briefly confirm that merging is ok. |
Given that this doesn't affect Plone features, I wouldn't think it would be a beta violation. |
I am ok for merging too |
I'm fine with merging as well, specially if tests pass 😄 , but I do have a few questions:
|
On Fri, Mar 31, 2017 at 3:18 PM, Gil Forcada Codinachs < ***@***.***> wrote:
I'm fine with merging as well, specially if tests pass 😄 , but I do have
a few questions:
- do we want to keep testing on the old version as well?
If it's not to hard and you feel paranoid.
- are there no backwards compatible change?
Not in terms of applications. I recently discovered ZRS doesn't work with
ZODB 5. I plan to fix that soon.
- is there any migration needed? A migration guide is in place if
that's the case?
No.
- could theoretically one run on ZODB 3.x, copy the database on
another server that runs 4.x or 5.x and then move it back to 3.x? Should we
warn about that?
AFAIK, but someone could try it. :)
FWIW, we updated a long-lived Pyramid project from ZODB 3 to ZODB 5 without
any problems. But we didn't try to go back.
People using ZEO who want to avoid downtime should update their clients and
then update their server, at which point they can start taking advantage of
new ZEO features, most-notably SSH.
|
@jamadden are you aware of any RelStorage issues people might run into switching to ZODB 5? Schemas haven't changed IIRC. |
I'm also +1 on merging this, after fixing the issues with ZRS; I suggest making some noise with a news release on plone.org explaining what this big achievement means to Plone users. @tkimnguyen is my man! |
@jimfulton: I'll let you get away without a CLA. This time... I know we have some groups using ZRS heavily (@vangheem in particular), but if it looks like something we can get fixed in the near future, I'm +1 to merge this. |
After cloning: and running
Also the zeoserver is not starting:
This is the zeo.conf:
Probably some work is needed for |
a) the config file looks like a concatenation of a ZEO config file and a zdaemon config file. Is this using some unholy ZConfig combination of zdaemon and ZEO config? If so, you have my sympathies. :) The modern practice is to define a standard ZEO config, https://github.com/zopefoundation/ZEO#server-configuration, and then separately define a zdaemon config, as described here https://pypi.python.org/pypi/zdaemon, that has:
|
The config file comes from this recipe: I think we may want to fiddle with it. I also noticed we can upgrade the ZConfig version to 3.1.0. |
Sigh. The zeoctl script is completely untested and, from your report, I guess changes made sometime broke it. I'll have to see WTF. Grrrrrrr. |
OK, well, I tried to fix zeoctl, but I don't know how to run it. I built out ZODB 3.10.5 and tried running it with https://gist.github.com/jimfulton/ebe75021f869e4a7481ef30c345286fc:
I suspect the fix is simple, but IDK how to run the dang thing. I even tried building Plone (a trip) and it doesn't have one. I tried googling for zeoctl, but couldn't find documentation. I need some working example of zeoctl for some version of ZODB. |
You want a plone.org news item for some version pin changes. Sure ;) |
Given As far as I understand it, the recommended way is using either Without having had a look at the details: For BBB/convenience reasons the |
I just released ZEO 5.1.0, which might plausibly fix the zeoctl issue. IDK how to run zeoctl, o I can't say for sure. |
On Mon, Apr 3, 2017 at 4:13 AM, Jens W. Klein ***@***.***> wrote:
Given zeoctl is completely untested and it is not the "standard" way to
start ZEO:
IMO we should better move to the standard way to configure and start ZEO
instead of fighting with a broken script.
Yup. Unfortunately, I think it was a "standard" way a long time ago.
As far as I understand it, the recommended way
<https://pypi.python.org/pypi/ZEO/4.2.0b1#installing-software> is using
either runzeo or zdaemon together with runzeo. From my point of view
Supervisor with runzeo (or similar) would be an option too for production
deployments. For Systemd based Linux systems runzeo could be controlled
directly by it.
The right way to think about deploying ZEO is to decouple running ZEO from
daemonizing it.
- Write/generate a ZEO configuration, which is passed to runzeo using the
-C option.
- daemonize the runzeo process using a wrapper like zdaemon (better, IMO
for automated deployments) or supervisord (maybe better for manual
deployments, but what are those? ;-) ) or whatever else works (systemd?
IDK).
zeoctl should be considered walking (or perhaps not) dead.
I want Plone to start using current ZODB, so I'd like to require fewer
short-term changes to Plone. I just released ZEO 5.1. It has a change that
might plausibly fix the zeoctl issue. Given that I don't know and can't
find out how ti successfully run zeoctl even with older ZODB releases, I
can't really test the change.
|
Thanks @jimfulton, See complete buildout here: So I am fine for merging but I would say we should also add some more pins. Thanks again @jimfulton, your contribution (as you can see by the enthusiastic reaction of all these Plonistas) is really appreciated! |
On Mon, Apr 3, 2017 at 4:45 PM, Alessandro Pisa ***@***.***> wrote:
Thanks @jimfulton <https://github.com/jimfulton>, ZEO = 5.1.0 did the
trick!
Yay!
When y'all have some time later, please stop using zeoctl. It's still
untested. :)
See complete buildout here:
- https://github.com/plone/simple-plone-buildout/compare/
master...plone:zodb5?expand=1
So I am fine for merging but I would say we should also add some more pins.
I will try to run ploneintranet test suite (which runs against a ZEO
cluster and report back) .
Thanks again @jimfulton <https://github.com/jimfulton>, your contribution
(as you can see by the enthusiastic reaction of all these Plonistas) is
really appreciated!
You're more than welcome. I'm glad that Plone users will soon be using the
latest greatest ZODB.
|
Also, I'm putting finishing touches on a ZRS release that works with ZODB 5. |
I Love it! Things to do before merge:
|
I will do a manual local merge and add those two changes, GTD! |
manual merge of this PR with fb67f9e together with a second commit e4d9d1c with above mentioned changes. This was on our wishlist for along time. Thanks @jimfulton for making this possible! |
@jensens sorry, but although you want it for a long time that does not mean that merging it right now was a good idea. There is quite a lot of ZRS installations out there and until jim finishes the release and it can be tried in the wild I would have very much preferred to not have this merged. Is only on 5.1 and we are still in beta, so nothing much to panic about, but for such big changes, giving the chance to get some early feedback, like @ale-rt did has been crucial to fix a few things already. And anyway, as long as there is no release, the version pin bumps are only on git, so unless you extend buildout.coredev git it does not really matter if it's merged or not. I would have rather announced it on community.plone.org with the set of new pins to test, to get some more testing before getting it merged. |
Looks like the tests that were green here are red now, and I have no good idea why that is that way, so I'll revert. |
@gforcada I am sure the ZRS issue will get fixed. From our real world experience people do not test branches (excpet those readin the PRs). Poor enough, I know. |
Totally agree on @gforcada opinion. We are in beta for 5.1, if that
potentially breaks something (it actually does) or untested in the wild
(that's why we have -pending releases) I would add it to later (and better
tested) releases.
Can we just add an experimental cfg for people to fiddle with it in the
meanwhile?
…On Wed, 5 Apr 2017 at 11:32, Gil Forcada Codinachs ***@***.***> wrote:
@jensens <https://github.com/jensens> sorry, but although you want it for
a long time that does not mean that merging it right now was a good idea.
There is quite a lot of ZRS installations out there and until jim finishes
the release and it can be tried in the wild I would have very much
preferred to not have this merged.
Is only on 5.1 and we are still in beta, so nothing much to panic about,
but for such big changes, giving the chance to get some early feedback,
like @ale-rt <https://github.com/ale-rt> did has been crucial to fix a
few things already.
And anyway, as long as there is no release, the version pin bumps are only
on git, so unless you extend buildout.coredev git it does not really matter
if it's merged or not.
I would have rather announced it on community.plone.org with the set of
new pins to test, to get some more testing before getting it merged.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#317 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAduD6w6rjAhxnHBtD9DGbOO81RimLTMks5rs1vxgaJpZM4Mv1rr>
.
|
After all I would like to see this in 5.1. But only if it does not break anything. Otherwise we need to wait for 6.0 and that would be really sad. |
@jensens don't get me wrong, I do want to have that on 5.1 as well, but if we already know that there is something that does really not work (ZRS) it does not make sense to merge and wait for "it will get fixed". Sure it will, hopefully, but on the meantime all and everyone that uses ZRS will not be able to check 5.1 as they could have been doing because of that. Let's follow a process please, not just "let me click the green button because I feel like it". We could even argue that was supposed to be the responsibility of the @plone/release-team actually. Putting releases together is part of our (as I'm on that team) job, so bumping versions should be approved by us. |
Well, increasing version pins of our dependencies was never done explicitly by the release team. I would wish there was that role. But in fact, a small group of contributors tracked the versions and did that job. I agree ZRS is important. @jimfulton stated already "I recently discovered ZRS doesn't work with ZODB 5. I plan to fix that soon." So I have no doubt that this will turn into a show stopper. Also, ZRS is not an integral part of Plone core, nor is it part of/ pinned in Plone ecosystem. |
OK, the ZRS problem just vanished. Thanks again @jimfulton |
@jensens as for a role to update version pins, IMHO anyone should /be encouraged to suggest version pin upgrades, but it should be the release team the one merging those... that's the role of a release team, get releases out and compile the KGS. |
BTW I still had no luck to run ploneintranet test suite either:
I can fix this error adding a transaction.commit() before tearing down zope, but I do not know how clean is this solution. |
I would to an abort in the tearDown before calling close. |
@jensens I will do that probably this evening |
I'm not aware of any RelStorage-specific significant issues. There are no schema changes that require a migration. It's been a drop-in upgrade for us from 1.6.0bX all the way through to 2.1a1. That said, a few things of note:
(Apologies for the delay, I've had a bad case of the flu.) |
@jensens in order for the build to run correctly I had to add another commit, |
@ale-rt thanks. Yes for cherry picking. It would be great if we can fix this in one PR. |
did it, see plone/plone.testing#31 |
Branch: refs/heads/master Date: 2020-11-16T11:44:23+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/plone.app.dexterity@b11b514 For increased security, in the modeleditor do not resolve entities, and remove processing instructions. See plone/Products.CMFPlone#3209 Files changed: A news/3209.bugfix M plone/app/dexterity/browser/modeleditor.py Repository: plone.app.dexterity Branch: refs/heads/master Date: 2020-11-16T21:17:35+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/plone.app.dexterity@69b9c31 Merge pull request #317 from plone/maurits/cmfplone-issue-3209-lxml Modeleditor: do not resolve entities, to avoid xml vulnerabilities Files changed: A news/3209.bugfix M plone/app/dexterity/browser/modeleditor.py
Branch: refs/heads/master Date: 2020-11-16T11:44:23+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/plone.app.dexterity@b11b514 For increased security, in the modeleditor do not resolve entities, and remove processing instructions. See plone/Products.CMFPlone#3209 Files changed: A news/3209.bugfix M plone/app/dexterity/browser/modeleditor.py Repository: plone.app.dexterity Branch: refs/heads/master Date: 2020-11-16T21:17:35+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/plone.app.dexterity@69b9c31 Merge pull request #317 from plone/maurits/cmfplone-issue-3209-lxml Modeleditor: do not resolve entities, to avoid xml vulnerabilities Files changed: A news/3209.bugfix M plone/app/dexterity/browser/modeleditor.py
Branch: refs/heads/master Date: 2023-06-30T23:23:24+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@39552a7 add upgrade step Fix Registry Settings Files changed: M plone/app/upgrade/v60/configure.zcml M plone/app/upgrade/v60/final.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-06-30T23:23:36+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@1e68709 Add test for Upgrade Step Files changed: A plone/app/upgrade/tests/test_fix_registry_settings.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-06-30T23:23:43+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@4337d3b add News Files changed: A news/315.bugfix Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-06-30T23:53:32+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@39236d8 Update fix_syndication_settings - use the uid in tuple for site_rss_items Files changed: M plone/app/upgrade/v60/final.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-01T10:42:51+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@852d58b remove unused import Files changed: M plone/app/upgrade/v60/final.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-01T10:43:21+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@5c327da add more comments in test Files changed: M plone/app/upgrade/tests/test_fix_registry_settings.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-02T10:17:54+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@eab66f7 remove unused code Files changed: M plone/app/upgrade/tests/test_fix_registry_settings.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-03T07:34:44+02:00 Author: Gil Forcada Codinachs (gforcada) <[email protected]> Commit: plone/plone.app.upgrade@fd8ba30 Merge pull request #317 from plone/fix#315 Fix#315 Files changed: A news/315.bugfix A plone/app/upgrade/tests/test_fix_registry_settings.py M plone/app/upgrade/v60/configure.zcml M plone/app/upgrade/v60/final.py
Branch: refs/heads/master Date: 2023-06-30T23:23:24+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@39552a7 add upgrade step Fix Registry Settings Files changed: M plone/app/upgrade/v60/configure.zcml M plone/app/upgrade/v60/final.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-06-30T23:23:36+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@1e68709 Add test for Upgrade Step Files changed: A plone/app/upgrade/tests/test_fix_registry_settings.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-06-30T23:23:43+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@4337d3b add News Files changed: A news/315.bugfix Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-06-30T23:53:32+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@39236d8 Update fix_syndication_settings - use the uid in tuple for site_rss_items Files changed: M plone/app/upgrade/v60/final.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-01T10:42:51+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@852d58b remove unused import Files changed: M plone/app/upgrade/v60/final.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-01T10:43:21+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@5c327da add more comments in test Files changed: M plone/app/upgrade/tests/test_fix_registry_settings.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-02T10:17:54+02:00 Author: 1letter (1letter) <[email protected]> Commit: plone/plone.app.upgrade@eab66f7 remove unused code Files changed: M plone/app/upgrade/tests/test_fix_registry_settings.py Repository: plone.app.upgrade Branch: refs/heads/master Date: 2023-07-03T07:34:44+02:00 Author: Gil Forcada Codinachs (gforcada) <[email protected]> Commit: plone/plone.app.upgrade@fd8ba30 Merge pull request #317 from plone/fix#315 Fix#315 Files changed: A news/315.bugfix A plone/app/upgrade/tests/test_fix_registry_settings.py M plone/app/upgrade/v60/configure.zcml M plone/app/upgrade/v60/final.py
bin/alltests passes for me.
I'm not set up to run the selenium tests, but am told that they're be tun automatically on a PR.