Skip to content
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

Allow multiple Plone instances in one Zope app #191

Merged
merged 1 commit into from
Sep 20, 2018
Merged

Conversation

tomgross
Copy link
Member

@tisto tisto self-requested a review June 27, 2018 05:23
@tisto
Copy link
Member

tisto commented Jun 27, 2018

@tomgross LGTM. Travis is failing though.

.travis.yml Outdated
- virtualenv .
- bin/pip install --upgrade pip setuptools zc.buildout
- virtualenv -p `which python` .
- bin/pip install setuptools==33.1.1 zc.buildout==2.9.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomgross pinning is a good thing. I prefer to have it in requirements.txt, to make sure it is used everywhere (local dev, travis).

@@ -63,6 +64,10 @@ def prepareData(data):
path = data.get('path')
if isinstance(path, dict) and not path.get('query'):
data.pop('path')
if not path in data:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomgross I'd suggest adding a comment here why this block is necessary.

@tomgross
Copy link
Member Author

@tisto The tests are always right 😉 The prepareData method was used in two different contexts on index and search. This is now fixed. Can you please review and merge.

This also pins zc.buildout and setuptools to fix travis, which is broken on master.

@tisto tisto merged commit 96a732a into 5.x Sep 20, 2018
@tisto tisto deleted the 5.x-multiple_instances branch September 20, 2018 12:37
@tisto
Copy link
Member

tisto commented Sep 20, 2018

@tomgross merged. Sorry, it took so long. I plan to do a new release soon and I will push things further to have a final c.solr release in the next weeks...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants