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

Remove locally hosted LCP Collections 🔥 (PP-393) #1344

Merged
merged 6 commits into from
Sep 12, 2023

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Aug 29, 2023

Description

Remove locally hosted LCP collections from the CM, since we are not using it anywhere.

NOTE: This does NOT impact LCP content hosted by other vendors, but borrowed through the CM. Just CM hosted LCP collections, which are not being used anywhere currently.

Motivation and Context

As part of PP-95, the LCP mirror integration needs some updating. Since we are not using this anywhere, and it does not appear to be working currently, we decided to remove it rather then fix it.

How Has This Been Tested?

  • CI tests

Checklist

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

@jonathangreen jonathangreen changed the title Remove local LCP Collections (PP-393) Remove local LCP Collections 🔥 (PP-393) Aug 29, 2023
@jonathangreen jonathangreen changed the title Remove local LCP Collections 🔥 (PP-393) Remove locally hosted LCP Collections 🔥 (PP-393) Aug 29, 2023
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09% 🎉

Comparison is base (3a15ac6) 89.92% compared to head (1978586) 90.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1344      +/-   ##
==========================================
+ Coverage   89.92%   90.01%   +0.09%     
==========================================
  Files         211      204       -7     
  Lines       28475    28051     -424     
  Branches     6499     6458      -41     
==========================================
- Hits        25605    25250     -355     
+ Misses       1880     1822      -58     
+ Partials      990      979      -11     
Files Changed Coverage Δ
api/admin/controller/admin_search.py 100.00% <ø> (ø)
api/integration/registry/license_providers.py 100.00% <ø> (ø)
api/s3_analytics_provider.py 75.32% <ø> (ø)
core/lane.py 97.25% <ø> (ø)
core/model/collection.py 96.00% <ø> (-0.29%) ⬇️
core/model/configuration.py 91.76% <ø> (-0.21%) ⬇️
core/model/constants.py 100.00% <ø> (ø)
core/model/licensing.py 87.26% <ø> (-0.02%) ⬇️
core/model/listeners.py 94.25% <ø> (-0.07%) ⬇️
api/admin/dashboard_stats.py 100.00% <100.00%> (ø)
... and 2 more

... and 1 file with indirect coverage changes

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

@jonathangreen jonathangreen requested a review from a team August 30, 2023 00:22
@jonathangreen jonathangreen force-pushed the feature/remove-local-lcp branch from 3f37a2a to a0cac0d Compare August 30, 2023 09:52
@jonathangreen jonathangreen changed the base branch from main to feature/remove-directory-importer August 30, 2023 09:52
@jonathangreen jonathangreen force-pushed the feature/remove-local-lcp branch from a0cac0d to 85525ff Compare August 31, 2023 16:35
Base automatically changed from feature/remove-directory-importer to main August 31, 2023 21:49
@jonathangreen jonathangreen force-pushed the feature/remove-local-lcp branch from 85525ff to 662ad2b Compare August 31, 2023 22:34
@jonathangreen
Copy link
Member Author

🚨 The migration on this one has gotten stale and will need to be synced up with main before merging. Just leaving this as a note to myself so I make sure that happens before merging. This still should be fine for code review.

Copy link
Contributor

@RishiDiwanTT RishiDiwanTT left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Does this removal of code not affect the lcp passphrase that gets added to the OPDS feeds in api/opds.py ?

@jonathangreen
Copy link
Member Author

Does this removal of code not affect the lcp passphrase that gets added to the OPDS feeds in api/opds.py ?

It should not change the addition of that lcp passphrase, since that is used for LCP content that is hosted by other providers, as well as locally hosted LCP content. So we will still need that element in the feed to support other LCP providers like Palace Marketplace.

@jonathangreen jonathangreen merged commit f079d2d into main Sep 12, 2023
@jonathangreen jonathangreen deleted the feature/remove-local-lcp branch September 12, 2023 13:24
@tdilauro tdilauro added the incompatible changes Changes that require a new major version label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible changes Changes that require a new major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants