-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
PLIP: Ship Plone with plone.restapi #2177
Comments
Review by Eric Bréhault (ebrehault@gmail.com, barbichu@irc.freenode.net) The PLIP was reviewed on Ubuntu 16.04 using python 2.7.8 (https://github.com/collective/buildout.python) and Chromium 49.0.2623.108. December 10, 2017 Review steps
and for plone.test:
Notes and observationsAutomated testing
Manual testingTested directly with Postman, and tested from various Javascript frontend apps. Tested in anonymous mode. Tested in authenticated mode. Code reviewImplementation is clean. plone.api is only used in tests. Archetypes support is provided through Very good documentation. Conclusionplone.rest and plone.restapi are ready to be shipped with Plone. We need a pull request on Products.CMFPlone to add plone.restapi as a dependency. |
plone.restapi 1.0.0 released: https://pypi.python.org/pypi/plone.restapi/1.0.0 |
Pull Request: #2263 |
It seems Plone 5.2 removed Products.Five.metaclass: http://jenkins.plone.org/job/pull-request-5.2/160/console
It there an upgrade guide for Plone 5.2 with a list of removed imports? |
@jensens five.pt has been removed in Plone 5.2? Is there an upgrade guide for Plone 5.2? |
Ok. Found the change: |
I fixed the import failure with plone/plone.rest#71. Though, there are quite a few regressions in both plone.rest and plone.restapi. @plone/framework-team Is there any upgrade guide for Zope 4 / Plone 5.2? |
@pbauer thanks for the link! Though, this is still pretty far away from an integrators-ready upgrade guide. :) @plone/framework-team When I wrote the PLIP, I really expected this to be a 30 minutes job. If this becomes more a days/week-like challenge, because of the Zope 4 changes, I might have to drop the PLIP. I really want to focus on pushing things on the front end (plone-react, pastanaga, etc) and I think it makes more sense for the Plone community if I spend my time there. If anybody wants to help out or take over this PLIP, I am happy to do what I can to help... |
@tisto whats the stopper exactly? May we help you getting this done, like adding it as a sprint topic? |
@jensens I will set up a PLIP jenkins job to show the current test failures. It seems something in plone.rest is broken that leads to additional test failures in plone.restapi. I fixed the obvious import problems. Though, as said, I have no idea about the changes in Zope 4, so I am lost. Would be great if someone at the sprint could look into this! |
The jenkins job that @tisto created is here http://jenkins.plone.org/view/PLIPs/job/plip-plone-restapi/ for everyone's pleasure to run it 🙂 |
As a starting point, I would recommend looking into why plone.rest returns a 404 instead of a 401: |
It doesn't surprise me if it takes some work to make plone.rest work on Zope 4, since plone.rest added adding a feature to the Zope publisher, and Zope 4 involves a pretty big change to use the WSGI publisher. |
I've added some fixes: And updated the PLIP buildout: plone.rest goes green. i don't know about plone.restapi yet. |
@thet there is a current bug plone.rest and the plone theme editor. Believe its still current. We are trying to add a test for it now to show the fix works. |
We've now added a test for the plone.rest issue that caused a regression in the theme editor in plone. plone/plone.rest#61. If someone could review it, we can get it merged |
We had a look at the TUS implementation. It seems it stores the uploads in tmp-dirs. How is it supposed to scale up over more than one machine? |
@jensens there is a TUS implementation in plone 5 and this issue was got around by turning it off by default. Not ideal but I don't see any choice but to do the same with restapi. |
I would store the parts as blobs in ZODB and do a manual merge after the next part arrived and is committed. Or do i miss something? |
I think one could set up a nfs share and point TUS_TMP_FILE_DIR to it, alike sharing blobs between instances? |
I'd love to see an analysis and roadmap of how current Plone core components (like folder contents and relation widget) will use the REST API together with this PLIP. This will make it easier for new developers to understand. Otherwise this is yat (Yet another thing 😉 ) in Plone. |
I personally hope with restapi all those distributed wild endpoints will die as the next step. But not together with this PLIP, that would broaden the scope too much. |
@tomgross +1, we need to write down some best practices for add-on developers how to implement a proper plone.rest-based and plone.restapi compliant API as well as start to move core components that do not need to be server-side rendered. Though, I fully agree with @jensens that this is beyond the scope of this PLIP. The first step is to ship with plone.restapi. Then we can start with migrating. Not sure if we even need a PLIP, if both React and plone.restapi have been approved in the core. Ideally, we can just make components from plone-react re-usable and integrate them into standard Plone. Personally, I plan to fully focus on plone-react in the future. Though, the plone-react team is more than happy to assist if anybody is interested in using components outside of plone-react. :) cc @robgietema |
This is a blocker for the PLIP! |
@plone/framework-team thanks to @davisagli and @sunew we are very close to a green build. Just one test isolation issue is left. Can we merge the PLIP right away on a green build? I see the risk that we have to spend way more work on this if we don't merge it as soon as it is green since the Plone 5.2 branch is under heavy development right now. :) |
Huge +1 from me |
Yes, it is approved and reviewed. Merge it after green build. |
Thanks for the swift reply! We are very close. Just one flickering robot test prevents the merge. :) |
@tisto The flickering test is apparently more or less normal:
|
as seen in this build: https://jenkins.plone.org/job/pull-request-5.2/909/consoleFull |
@tisto @sunew @gforcada in such cases, re-playing failing tests and merging the results is a pretty good strategy, see http://laurent.bristiel.com/re-executing-failed-test-cases-and-merging-outputs-with-robot-framework/ |
Merged: plone/buildout.coredev#494 |
Plone 5.2 build is green: https://jenkins.plone.org/job/plone-5.2-python-2.7/1254/ |
@gforcada it seems plone.rest and plone.restapi do not have the post-push hooks on Jenkins to trigger a 5.2 coredev build on changes. Could you please run our script to update that? |
@tisto sure |
@tisto ... and done! 🎉 let me know if it does not work |
Thank you Gil! |
I am pretty sure the Plip is not finished integration wise.
|
This was resolved by closing #2265 and merging plone/Plone#13 |
PLIP: Ship Plone with plone.restapi
Responsible Persons
Proposer: Timo Stollenwerk tisto@plone.org
Seconder: @hvelarde
Abstract
plone.restapi is a RESTful hypermedia API for Plone. Companies such as 4teamwork, code syntax, kitconcept, VNC and others use plone.restapi in production since many years already. Adding it to Plone core will allow us to further enhance our headless CMS efforts.
Motivation
Allow developers and integrators to build modern JavaScript front-ends on top of Plone.
Assumptions
The Plone community wants to go ahead with adopting modern JavaScript solutions.
Proposal & Implementation
plone.restapi 1.0 will be released this year and is considered to be a stable and mature piece of software.
Deliverables
Risks
There is a small risk that plone.rest could break existing functionality of browser views that use application/json as HTTP accept header. We encountered a single issue since plone.restapi has been created. We do not really expect any problems, but they might occur.
Participants
The text was updated successfully, but these errors were encountered: