Summary

Add support for Elasticsearch

Review Request #8209 — Created June 2, 2016 and submitted June 20, 2016, 12:27 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

2a24611...

Reviewers

Groups

reviewboard

People

Description

The RB admin panel now includes settings for choosing between Whoosh
(the default) and Elasticsearch as search engines.

Testing Done

Set up Review Board to use Elasticsearch and was able to perform
searches after building the index.

Files

Issues

Description	From	Last Updated
Let's swap these fields. The index depends on the URL. We should also increase the width of both fields. The …	chipx86	June 17, 2016, 11:08 a.m.
Can we make a constant on the class and then use that both here and in the SEARCH_ENGINE_CHOICES tuple?	david	June 7, 2016, 4 p.m.
'elasticsearch' imported but unused	reviewbot	June 2, 2016, 12:04 p.m.
Since there's only one variable being formatted in, we can just use %s and positional-based formatting, which should shorten this …	david	June 7, 2016, 4 p.m.
How about "reviewboard"?	david	June 7, 2016, 4 p.m.
You can use the new constant(s) here, too.	david	June 7, 2016, 4:02 p.m.
You can use the new constant(s) here, too.	david	June 7, 2016, 4:02 p.m.
Add a blank line between these?	david	June 7, 2016, 4:02 p.m.
Hmm, this is a little weird. I think I'd prefer just attaching the change handler and then calling updateCacheFormDisplay() at …	david	June 7, 2016, 4:03 p.m.
This should fit on one line.	david	June 7, 2016, 4:03 p.m.
Same deal here.	david	June 7, 2016, 4:03 p.m.
'elasticsearch' imported but unused	reviewbot	June 8, 2016, 2:06 p.m.
'elasticsearch' imported but unused	reviewbot	June 8, 2016, 2:06 p.m.
Since these are POD constants, the variable names should be all caps. As it is where you used it I …	david	June 13, 2016, 4:45 p.m.
You could just make the jquery inline here rather than defining a variable and using add(): $('#row-search_results_per_page, #row-search_engine') .toggle(searchEnabled);	david	June 13, 2016, 4:46 p.m.
You could just make the jquery inline here rather than defining a variable.	david	June 13, 2016, 4:47 p.m.
You could just make the jquery inline here rather than defining a variable and using add(): $('#row-elasticsearch_index_name, #row-elasticsearch_url') .toggle(searchEnabled && …	david	June 13, 2016, 4:47 p.m.
There's an extra line here.	david	June 13, 2016, 4:48 p.m.
Something terrible has happened here with your editor. Please reindent. You can also combine the selectors into a single string …	david	June 13, 2016, 4:48 p.m.
Let's rename during import to search_engines, so it's clear and won't conflict in the future.	chipx86	June 17, 2016, 11:05 a.m.
I think something went horribly wrong with search/replace. We're now shouting WOOSH and ELASTICSEARCH in a lot of places that …	david	June 17, 2016, 11:08 a.m.
Missing docstring.	chipx86	June 17, 2016, 11:06 a.m.
'elasticsearch' imported but unused	reviewbot	June 13, 2016, 4:50 p.m.
Swap these.	chipx86	June 17, 2016, 11:06 a.m.
Let's rename here as well.	chipx86	June 17, 2016, 11:06 a.m.
Missing a file docstring.	chipx86	June 17, 2016, 11:04 a.m.
Can we document both of these?	chipx86	June 17, 2016, 11:04 a.m.
Why the "WHOOSH"?	chipx86	June 17, 2016, 11:04 a.m.
Can be: $('#id_search_engine, #id_search_enable') .change(updateSearchFormDisplay);	david	June 17, 2016, 11:09 a.m.
? This isn't how it's listed in pypi.	chipx86	June 17, 2016, 11:03 a.m.
'elasticsearch' imported but unused	reviewbot	June 17, 2016, 11:17 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html

reviewboard/admin/forms.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'elasticsearch' imported but unused
```

Added Files:

Branch:: master
release-3.0.x

reviewboard/admin/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we make a constant on the class and then use that both here and in the SEARCH_ENGINE_CHOICES tuple?

reviewboard/admin/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

Since there's only one variable being formatted in, we can just use %s and positional-based formatting, which should shorten this code a bit.

reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
How about "reviewboard"?
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You can use the new constant(s) here, too.
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You can use the new constant(s) here, too.
```
reviewboard/templates/admin/general_settings.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a blank line between these?
```

reviewboard/templates/admin/general_settings.html (Diff revision 1)

The issue has been resolved. Show all issues

Hmm, this is a little weird. I think I'd prefer just attaching the change handler and then calling updateCacheFormDisplay() at the end of the function.

reviewboard/templates/admin/general_settings.html (Diff revision 1)
The issue has been resolved. Show all issues
```
This should fit on one line.
```
reviewboard/templates/admin/general_settings.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Same deal here.
```

Change Summary:

Addressed David's issues.

Diff:

Revision 2 (+102 -9)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/templates/admin/general_settings.html

Change Summary:

Oops, wrong patch

Diff:

Revision 3 (+106 -9)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/search/engines.py
	reviewboard/templates/admin/general_settings.html

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html



Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/search/engines.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/search/engines.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html

reviewboard/admin/forms.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'elasticsearch' imported but unused
```
reviewboard/admin/forms.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'elasticsearch' imported but unused
```

reviewboard/search/engines.py (Diff revision 3)

The issue has been resolved. Show all issues

Since these are POD constants, the variable names should be all caps. As it is where you used it I mistook it for a class name.
Probably also don't need the blank line between them.

reviewboard/templates/admin/general_settings.html (Diff revision 3)

The issue has been resolved. Show all issues

You could just make the jquery inline here rather than defining a variable and using add():

$('#row-search_results_per_page, #row-search_engine')
    .toggle(searchEnabled);

reviewboard/templates/admin/general_settings.html (Diff revision 3)
The issue has been resolved. Show all issues
```
You could just make the jquery inline here rather than defining a variable.
```

reviewboard/templates/admin/general_settings.html (Diff revision 3)

The issue has been resolved. Show all issues

You could just make the jquery inline here rather than defining a variable and using add():

$('#row-elasticsearch_index_name, #row-elasticsearch_url')
    .toggle(searchEnabled && searchEngine === 'elasticsearch');

reviewboard/templates/admin/general_settings.html (Diff revision 3)
The issue has been resolved. Show all issues
```
There's an extra line here.
```

reviewboard/templates/admin/general_settings.html (Diff revision 3)

The issue has been resolved. Show all issues

Something terrible has happened here with your editor. Please reindent.

You can also combine the selectors into a single string with a comma.

Change Summary:

Address David's issues

Commit:

b45d758a8a2418a9f76629e61c9213451fbc8eaa

35eaaeded7060099b50331bf8a2630e5f904f4cd

Diff:

Revision 4 (+97 -10)

Show changes

	setup.py
	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/search/engines.py
	reviewboard/templates/admin/general_settings.html

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    setup.py
    reviewboard/search/engines.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    setup.py
    reviewboard/search/engines.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html

reviewboard/admin/forms.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'elasticsearch' imported but unused
```

reviewboard/admin/forms.py (Diff revision 4)

The issue has been resolved. Show all issues

I think something went horribly wrong with search/replace. We're now shouting WOOSH and ELASTICSEARCH in a lot of places that we shouldn't be. Only the variable names should be all-caps.

reviewboard/templates/admin/general_settings.html (Diff revision 4)

The issue has been resolved. Show all issues

Can be:

$('#id_search_engine, #id_search_enable')
    .change(updateSearchFormDisplay);

I think rather than hard-coding everything for Whoosh and Elasticsearch, we should do what we do for auth, hosting services, etc. and have a concept of backends that are registered as entrypoints, with config forms. It's a little more work up-front, but it makes it a lot easier down the road for us or third-parties to support additional search engines (say, AWS's search services).

We'd still go through Haystack for all this, so any extension providing such things would need to deal with that, but we'd have our side covered when it comes to settings and configuration.

elasticsearch_settings.png (Revision 1)

The issue has been resolved. Show all issues

Let's swap these fields. The index depends on the URL.

We should also increase the width of both fields. The URL especially.

reviewboard/admin/forms.py (Diff revision 4)

The issue has been resolved. Show all issues

Let's rename during import to search_engines, so it's clear and won't conflict in the future.

reviewboard/admin/forms.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing docstring.
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Let's rename here as well.
```
reviewboard/search/engines.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a file docstring.
```
reviewboard/search/engines.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we document both of these?
```
reviewboard/search/engines.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Why the "WHOOSH"?
```
setup.py (Diff revision 4)
The issue has been resolved. Show all issues
```
? This isn't how it's listed in pypi.
```
1. brennie June 16, 2016, 2:59 p.m.
  Find and replace gone wrong.

Change Summary:

Addressed all issues

Commit:

35eaaeded7060099b50331bf8a2630e5f904f4cd

2a246112bee6fc3747cd52d98f032e60387e836f

Diff:

Revision 5 (+116 -9)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/search/search_engines.py
	reviewboard/templates/admin/general_settings.html

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/search/search_engines.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/search/search_engines.py

Ignored Files:
    reviewboard/templates/admin/general_settings.html

reviewboard/admin/forms.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'elasticsearch' imported but unused
```

Ship it!

This looks good to me as-is, but I agree with Christian's comment about making this more pluggable. Perhaps in a follow-up change?

Status:: Completed
Change Summary:: Pushed to release-3.0.x (feb91a6)