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

Convert Library to use integration settings (PP-11) #1281

Merged
merged 14 commits into from
Jul 24, 2023

Conversation

jonathangreen
Copy link
Member

Description

Convert libraries to store their settings in a json column on the libraries table, rather then using configuration settings. See jira ticket PP-11.

Note: This PR is based on #1276 so that one should go in first.

Motivation and Context

This is part of the larger CM Config Settings epic: PP-4

How Has This Been Tested?

  • Running unit tests

Checklist

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

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 99.31% and project coverage change: -0.04 ⚠️

Comparison is base (3f936ce) 89.95% compared to head (54bed22) 89.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1281      +/-   ##
==========================================
- Coverage   89.95%   89.92%   -0.04%     
==========================================
  Files         197      197              
  Lines       29149    28982     -167     
  Branches     6695     6631      -64     
==========================================
- Hits        26220    26061     -159     
+ Misses       1903     1900       -3     
+ Partials     1026     1021       -5     
Impacted Files Coverage Δ
api/admin/dashboard_stats.py 100.00% <ø> (ø)
api/admin/problem_details.py 100.00% <ø> (ø)
api/admin/validator.py 89.61% <ø> (-8.37%) ⬇️
core/entrypoint.py 100.00% <ø> (ø)
core/model/constants.py 100.00% <ø> (ø)
core/model/licensing.py 87.48% <ø> (ø)
core/model/listeners.py 94.31% <ø> (-2.35%) ⬇️
scripts.py 84.97% <50.00%> (-0.06%) ⬇️
core/configuration/library.py 99.11% <99.11%> (ø)
api/admin/controller/library_settings.py 96.85% <100.00%> (+4.48%) ⬆️
... and 9 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jonathangreen jonathangreen added the DB migration This PR contains a DB migration label Jul 19, 2023
@jonathangreen jonathangreen requested a review from a team July 19, 2023 19:46
@jonathangreen jonathangreen marked this pull request as ready for review July 19, 2023 19:46
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.

I can't really find anything at fault here. Just wondering why are the settings added to the Library model as opposed to using the IntegrationLibraryConfiguration Model without a parent.

@jonathangreen
Copy link
Member Author

I decided to add the settings directly to the library table because I didn't want to overload the IntegrationLibraryConfiguration table too much. Since these settings are not really configurations for an integration, they didn't seem like they fit in the IntegrationLibraryConfiguration table.

Base automatically changed from feature/remove-shared-odl to main July 24, 2023 14:01
@jonathangreen jonathangreen force-pushed the feature/convert-library-settings branch from 7c5644b to 54bed22 Compare July 24, 2023 14:05
@jonathangreen jonathangreen merged commit fc59df1 into main Jul 24, 2023
@jonathangreen jonathangreen deleted the feature/convert-library-settings branch July 24, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB migration This PR contains a DB migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants