Search integration configured as a service (PP-93) #1554
Conversation
jonathangreen
force-pushed
the
feature/marc-by-collection
branch
from
0e47244 to
cc42d6f
Compare
jonathangreen
force-pushed
the
feature/opensearch-service
branch
from
70dac7b to
2e13d10
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1554 +/- ##
==========================================
- Coverage 90.05% 89.90% -0.16%
==========================================
Files 248 249 +1
Lines 29196 29021 -175
Branches 6635 6607 -28
==========================================
- Hits 26293 26091 -202
- Misses 1916 1934 +18
- Partials 987 996 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
jonathangreen
force-pushed
the
feature/opensearch-service
branch
3 times, most recently
from
0904efb to
9662c1d
Compare
jonathangreen
force-pushed
the
feature/opensearch-service
branch
from
7eed479 to
5d1f8d3
Compare
There was a problem hiding this comment.
Looks good to me!
A few comments related to parenthesizing of multiple context managers and a possible extraneous file.
I also note that self-test functionality and some testing for misconfigured search configuration has been removed; however, that appears to be intentional.
| with admin_librarian_fixture.request_context_with_library_and_admin( | ||
| "/" | ||
| ), admin_librarian_fixture.ctrl.wired_container(): |
There was a problem hiding this comment.
Minor: Now that Python 3.10 is our minimum, we should now be able to make this a little more readable with parenthesized context managers:
with (
admin_librarian_fixture.request_context_with_library_and_admin("/"),
admin_librarian_fixture.ctrl.wired_container(),
):| with admin_librarian_fixture.request_context_with_library_and_admin( | ||
| "/" | ||
| ), admin_librarian_fixture.ctrl.wired_container(): |
There was a problem hiding this comment.
Minor: parenthesize context managers.
| with circulation_fixture.request_context_with_library( | ||
| "/" | ||
| ), circulation_fixture.wired_container(): |
There was a problem hiding this comment.
Minor: parenthesize context managers.
| @@ -96,7 +89,7 @@ def test_feed( | |||
| # Make a real OPDS feed and poke at it. | |||
| with circulation_fixture.request_context_with_library( | |||
| "/?entrypoint=Book&size=10" | |||
| ): | |||
| ), circulation_fixture.wired_container(): | |||
There was a problem hiding this comment.
Minor: parenthesize context managers.
jonathangreen
force-pushed
the
feature/opensearch-service
branch
from
5d1f8d3 to
bdd3513
Compare
jonathangreen
added
Deployment Changes
Description
This PR updates Opensearch to be configured as a service via environment variables, rather then in the database. This means that Opensearch is not necessary to run the CM, it won't start without the necessary environment variables set. Opensearch is required for the CM to function currently, but the code will start without it, so that it can be configured. Now that it is configured via environment variables, it needs to be supplied.
The new environment variable is:
PALACE_SEARCH_URLand the environment variable to configure the URL used when running unit tests is updated toPALACE_TEST_SEARCH_URL.Now that we require Opensearch to be running to start the CM instance, the github actions workflow is updated to use the
docker-compose.ymlto start the CM for testing. This makes sure all the services are started and thatdocker-compose.ymlstays up to date.Motivation and Context
🚨 Note: This is ready for review, but it cannot be merged until:
Need update for admin UIRemove search service components (PP-848) circulation-admin#100Need update for playbook to deploy the new environment variablehttps://github.com/ThePalaceProject/hosting-playbook/pull/560This is part of PP-4, and will make it easier to configure the elasticsearch service without having to manually write settings to the database and removes the reliance on external integration settings.
How Has This Been Tested?
Checklist