Move storage integrations to be configured as services (PP-95) #1377

jonathangreen · 2023-09-13T00:46:12Z

Description

Add a dependency injection container, that contains services used by our controllers.
Create framework for adding new services, and configuring them via environment variables, validated via pydantic.
Remove the existing mirror integration code, and replace it with a new storage service.
- Modify the marc exporter and s3 analytics provider to use the new storage service.
- Add the new environment variables for the storage service to README.

Notes 🚨: (these are now all resolved)

~~The PR this is based on Remove collection content mirroring (PP-415) #1358 should get merged before this one does.~~
~~This PR is dependant on an admin UI PR, that removes the admin UI side functionality for configuring storage integrations: Remove storage services component (PP-95) circulation-admin#87~~
Before this is merged we will need to update our hosting-playbook to deploy the relevant environment variables our to our hosting environments. PR for this up here: https://github.com/ThePalaceProject/hosting-playbook/pull/489

Motivation and Context

JIRA: PP-95
The motivation here is to allow services that don't need to be configured in the admin UI to be configured via environment variables, and instantiated via a dependency injection container, so we are able to easily mock the services for testing.
This uses the storage service as a test case for the new services architecture, and implements it as a s3 storage service.

How Has This Been Tested?

Notes: The docker-compose.yml included in the repo was updated with the new environment variables, so that environment can be used for integration testing.

Manually tested in a docker-compose environment.
- Imported the Palace Bookshelf collection.
- Configured s3 analytics and marc integrations.
- Checked out a book.
  - Made sure analytics json files ended up in minio bucket.
- Ran the cache_marc_files script.
  - Made sure that marc files ended up in the public access bucket.
  - Made sure links to the files worked at the /marc endpoint.
Ran unit tests locally and in CI.

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2023-09-13T14:55:30Z

Codecov Report

Patch coverage: 93.27% and project coverage change: +0.05% 🎉

Comparison is base (ac78b47) 90.17% compared to head (42586d2) 90.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1377      +/-   ##
==========================================
+ Coverage   90.17%   90.22%   +0.05%     
==========================================
  Files         226      229       +3     
  Lines       29871    29905      +34     
  Branches     6845     6831      -14     
==========================================
+ Hits        26936    26983      +47     
+ Misses       1893     1880      -13     
  Partials     1042     1042

Files Changed	Coverage Δ
api/admin/controller/__init__.py	`100.00% <ø> (ø)`
api/admin/routes.py	`94.70% <ø> (-0.13%)`	⬇️
core/model/collection.py	`95.14% <0.00%> (-0.86%)`	⬇️
core/model/configuration.py	`90.94% <ø> (-0.68%)`	⬇️
core/model/resource.py	`86.25% <ø> (-0.61%)`	⬇️
api/s3_analytics_provider.py	`68.11% <68.00%> (-7.21%)`	⬇️
core/service/storage/s3.py	`93.96% <93.96%> (ø)`
api/admin/config.py	`96.05% <100.00%> (ø)`
api/admin/controller/analytics_services.py	`96.10% <100.00%> (+3.33%)`	⬆️
api/admin/controller/catalog_services.py	`89.33% <100.00%> (-0.03%)`	⬇️
... and 11 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonathangreen · 2023-09-19T12:48:16Z

This should be ready to merge now, once it gets a code review. All the dependant PRs have gone in, and its been rebased against the current main branch.

tdilauro · 2023-09-19T16:26:24Z

I started looking at this yesterday and will continue today.

tdilauro

I'm still looking at the tests, but the app generally looks good. I do have a concern about relying on the Dependency Injector project because it's not clear that it is being maintained (they haven't merged a PR in 9 months).

api/s3_analytics_provider.py

bin/odl2_import_monitor

core/marc.py

core/mock_analytics_provider.py

core/model/collection.py

tdilauro · 2023-09-21T20:46:27Z

core/scripts.py

+        if services is None:
+            services = container_instance()
+        self._services = services


Minor (style) - I think this is more clear as a ternary.

Not sure I agree here, but I updated this one anyway.

That's why it's "minor" and "style". I def won't be offended if you disagree or otherwise decide not to change it. I'll try to use "style pref" in the future, to make that more clear.

I generally like the ternary in these cases because it does two things:

Makes it more clear to me that the main thing happening is that self._services is gonna get something assigned, either a specified value or a default.

Makes it impossible for what's happening in ll. 127-128 and l. 129 above to get separated in the code.

(2) here is also why I'm a fan of the walrus operator, though I never suggest that anyone change that one, since it seems more controversial.

tdilauro · 2023-09-21T20:51:19Z

core/service/storage/configuration.py

+
+
+class StorageConfiguration(ServiceConfiguration):
+    region: str = "us-east-1"


Should we allow a default here? It might lead to some non-obvious misconfiguration.

Okay. I removed the default here. This required a good number of code changes. I pushed a new commit that:

Updates documentation

Updates tests

Adds new tests to s3 class constructor to make sure the URL template is consistent with the now optional region parameter.

scripts.py

jonathangreen · 2023-09-22T13:41:42Z

I pushed a few commits resolving code review feedback.

I do have a concern about relying on the Dependency Injector project because it's not clear that it is being maintained (they haven't merged a PR in 9 months).

Its hard to find a dependency injection container for Python. In the past I've found that constructor injection with a dependency injection container to wire up the dependencies is a helpful structure for webapps, and my hope is that we can slowly decouple our services this way.

Despite not having any PRs merged in the last 9 months, the library does have 1 million downloads a month on Pypy making it by far the most popular dependency injection container I've found for Python. It also has well written documentation https://python-dependency-injector.ets-labs.org/ and has been around since 2015.

There is a issue here ets-labs/python-dependency-injector#688 where the maintainer mentions that he considers the library feature complete at this point, but intends to support it to update python versions, dependency versions etc.

If you have a suggestion of a different library, I'm willing to take a look at switching this code. Otherwise the library becoming unsupported is a risk, but IMO the immediate gain of decoupling our services is worth it.

tdilauro · 2023-09-22T14:41:40Z

Otherwise the library becoming unsupported is a risk, but IMO the immediate gain of decoupling our services is worth it.

I'm fine with that (and agree about the docs). Just wanted to ensure that you were aware of the support situation. Along with the time since last commit, it was that same issue 688 along with issue 742 in that project that gave me pause.

jonathangreen · 2023-09-22T15:24:47Z

I rebased this with main to resolve the conflicts with #1268. The only changes in the rebase other then imports and poetry are in tests/core/test_marc.py around creating the fake search index for the marc tests, since that changed in #1268.

…iner.

tdilauro

This looks good! 🚀

One question about whether a test might need a network connection.

tdilauro · 2023-09-22T17:44:46Z

tests/core/service/storage/test_configuration.py

+
+def test_region_validation_fail():
+    with pytest.raises(CannotLoadConfiguration) as exc_info:
+        StorageConfiguration(region="foo bar baz")


Does the StorageConfiguration's validator's region check need a network connection?

I don't think so, but I'll verify.

Doesn't look like it. It seems like the available regions get loaded from the huge amount of json data that is included with botocore.

jonathangreen changed the base branch from main to feature/remove-content-mirroring September 13, 2023 00:46

jonathangreen mentioned this pull request Sep 13, 2023

Remove storage services component (PP-95) ThePalaceProject/circulation-admin#87

Merged

2 tasks

jonathangreen changed the title ~~Move storage integrations to be configured as services~~ Move storage integrations to be configured as services (PP-95) Sep 13, 2023

jonathangreen marked this pull request as ready for review September 13, 2023 15:08

jonathangreen requested a review from a team September 13, 2023 15:09

jonathangreen mentioned this pull request Sep 13, 2023

Remove the externalintegrationslinks table #1379

Merged

2 tasks

jonathangreen added UI Update This PR requires an admin UI update cleanup migration PR that will need a cleanup migration once its been fully deployed incompatible changes Changes that require a new major version labels Sep 13, 2023

jonathangreen mentioned this pull request Sep 14, 2023

Remove the unused ConfigurationStorage classes (PP-313) #1382

Merged

2 tasks

jonathangreen force-pushed the feature/remove-content-mirroring branch from a13d3b1 to fe97e88 Compare September 15, 2023 12:03

Base automatically changed from feature/remove-content-mirroring to main September 15, 2023 12:40

jonathangreen force-pushed the feature/storage-integration branch 2 times, most recently from d739045 to 3d690c5 Compare September 19, 2023 12:44

tdilauro self-requested a review September 20, 2023 13:11

tdilauro reviewed Sep 21, 2023

View reviewed changes

jonathangreen requested a review from tdilauro September 22, 2023 13:42

jonathangreen force-pushed the feature/storage-integration branch from 9a49beb to 7371a07 Compare September 22, 2023 15:11

jonathangreen added 6 commits September 22, 2023 12:38

Move storage integrations to be configured as services

a63e510

Make sure PALACE_ environment variables are passed into scripts conta…

acaffa1

…iner.

Add environment variables for minio to docker-compose.yml.

820a5fb

Update README with new environment variables.

e3eaacb

Bump admin UI package version.

92bf3e6

Code review feedback

2ba0175

Code review feedback: Make region configuration optional.

42586d2

jonathangreen force-pushed the feature/storage-integration branch from 7371a07 to 42586d2 Compare September 22, 2023 15:38

tdilauro approved these changes Sep 22, 2023

View reviewed changes

jonathangreen merged commit 778ed79 into main Sep 22, 2023
31 checks passed

jonathangreen deleted the feature/storage-integration branch September 22, 2023 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move storage integrations to be configured as services (PP-95) #1377

Move storage integrations to be configured as services (PP-95) #1377

jonathangreen commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

jonathangreen commented Sep 19, 2023

tdilauro commented Sep 19, 2023

tdilauro left a comment

tdilauro Sep 21, 2023

jonathangreen Sep 22, 2023

tdilauro Sep 22, 2023

tdilauro Sep 21, 2023

jonathangreen Sep 22, 2023

jonathangreen commented Sep 22, 2023

tdilauro commented Sep 22, 2023

jonathangreen commented Sep 22, 2023

tdilauro left a comment •

edited

Loading

tdilauro Sep 22, 2023 •

edited

Loading

jonathangreen Sep 22, 2023

jonathangreen Sep 22, 2023



		class StorageConfiguration(ServiceConfiguration):
		region: str = "us-east-1"

Move storage integrations to be configured as services (PP-95) #1377

Move storage integrations to be configured as services (PP-95) #1377

Conversation

jonathangreen commented Sep 13, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

jonathangreen commented Sep 19, 2023

tdilauro commented Sep 19, 2023

tdilauro left a comment

Choose a reason for hiding this comment

tdilauro Sep 21, 2023

Choose a reason for hiding this comment

jonathangreen Sep 22, 2023

Choose a reason for hiding this comment

tdilauro Sep 22, 2023

Choose a reason for hiding this comment

tdilauro Sep 21, 2023

Choose a reason for hiding this comment

jonathangreen Sep 22, 2023

Choose a reason for hiding this comment

jonathangreen commented Sep 22, 2023

tdilauro commented Sep 22, 2023

jonathangreen commented Sep 22, 2023

tdilauro left a comment • edited Loading

Choose a reason for hiding this comment

tdilauro Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

jonathangreen Sep 22, 2023

Choose a reason for hiding this comment

jonathangreen Sep 22, 2023

Choose a reason for hiding this comment

jonathangreen commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

tdilauro left a comment •

edited

Loading

tdilauro Sep 22, 2023 •

edited

Loading