Merge `lib` submodule into `ops` #3134

ThomasLaPiana · 2023-04-24T12:27:19Z

Closes #2032

Code Changes

move source files from lib into ops, mostly maintaining the structure
remove the lib submodule completely once the functionality has been copied over
clean up ops/lib oauth utils/functions

Steps to Confirm

automated checks pass
logical code conversion/synthesis is done

Pre-Merge Checklist

All CI Pipelines Succeeded
~~Documentation~~:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created (Fidesplus follow-up)
Update CHANGELOG.md
~~For API changes, the Postman collection has been updated~~

Description Of Changes

Long-awaited change to merge the previously-separate lib code with the ops api code. This is an iteration on the path towards a total unification of ctl and ops server code as well

Given the size of the changes here as well as being a pure refactor change, I'm not expecting much in terms of a manual review here

cypress · 2023-04-24T12:44:00Z

Passing run #1795 ↗︎

0	4	0	0	0

Details:

Merge `c597ec7` into `b5b1649`...
Project: fides	Commit: `fdd87cc4f1 ℹ️`
Status: Passed	Duration: 00:43 💡
Started: May 6, 2023 5:22 PM	Ended: May 6, 2023 5:23 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

codecov · 2023-04-24T12:55:36Z

Codecov Report

Patch coverage: 94.94% and project coverage change: -0.05 ⚠️

Comparison is base (b5b1649) 87.43% compared to head (c597ec7) 87.39%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3134      +/-   ##
==========================================
- Coverage   87.43%   87.39%   -0.05%     
==========================================
  Files         314      305       -9     
  Lines       18341    18265      -76     
  Branches     2384     2384              
==========================================
- Hits        16036    15962      -74     
+ Misses       1868     1866       -2     
  Partials      437      437

Impacted Files	Coverage Δ
...c/fides/api/ops/cryptography/cryptographic_util.py	`100.00% <ø> (ø)`
src/fides/api/ops/cryptography/schemas/jwt.py	`100.00% <ø> (ø)`
src/fides/api/ops/oauth/jwt.py	`100.00% <ø> (ø)`
src/fides/api/ops/oauth/roles.py	`100.00% <ø> (ø)`
src/fides/api/ops/oauth/system_manager.py	`100.00% <ø> (ø)`
src/fides/api/ops/util/text.py	`100.00% <ø> (ø)`
src/fides/api/ops/common_exceptions.py	`91.01% <78.57%> (-5.77%)`	⬇️
.../ops/api/v1/endpoints/privacy_request_endpoints.py	`90.24% <87.50%> (-0.12%)`	⬇️
src/fides/api/ops/db/base_class.py	`87.33% <89.05%> (+15.90%)`	⬆️
...c/fides/api/ops/api/v1/endpoints/user_endpoints.py	`95.00% <95.06%> (-0.39%)`	⬇️
... and 93 more

... and 1 file 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.

src/fides/api/ops/db/base_class.py

ThomasLaPiana · 2023-04-25T15:34:36Z

@pattisdr I'm nearing the point of being ready for a review, but before I get there I wanted to make sure there wasn't anything super specific you had in mind for cleanup here? At the very least this will combine the code and do some minimal de-duplication

src/fides/api/ops/db/base_class.py

+from fides.api.ops.util.text import to_snake_case
+
+T = TypeVar("T", bound="OrmWrappedFidesBase")
+ALLOWED_CHARS = re.compile(r"[A-z0-9\-_]")


pattisdr · 2023-05-01T16:04:34Z

@ThomasLaPiana taking a look at this now, apologize for missing this notification last week

ThomasLaPiana · 2023-05-01T16:07:57Z

@ThomasLaPiana taking a look at this now, apologize for missing this notification last week

No worries! Release takes precedence and I'm fixing a few bugs real quick since merging in main 😄 they'll be in shortly

ThomasLaPiana · 2023-05-01T16:42:10Z

ok...seems like it should be good to go now!

pattisdr

nice work @ThomasLaPiana really appreciate the consolidation here. Main things that stood out were the ConnectionConfig/MessagingConfig type issues and some of endpoints/URL's/schemas still in separate files from there rest of the ops endpoint details.

src/fides/api/ops/oauth/api/routes/user_endpoints.py

CHANGELOG.md

src/fides/api/ops/schemas/base_class.py

src/fides/api/ops/util/text.py

tests/conftest.py

src/fides/api/ops/oauth/schemas/user.py

pattisdr · 2023-05-01T21:50:56Z

src/fides/api/ops/service/connectors/fides_connector.py

@@ -182,6 +182,6 @@ def filter_fides_connector_datasets(
    return {
        dataset.fides_key
        for connector_config in connector_configs
-        for dataset in connector_config.datasets
+        for dataset in connector_config.datasets  # type: ignore[attr-defined]


ahhh why are these connection config attributes suddenly causing problems

idk 😢 but it was a lot

pattisdr · 2023-05-01T21:51:21Z

src/fides/api/ops/service/messaging/message_dispatch_service.py

@@ -138,7 +138,7 @@ def dispatch_message(
        raise MessageDispatchException("No notification service type configured.")

    logger.info("Retrieving message config")
-    messaging_config: MessagingConfig = MessagingConfig.get_configuration(
+    messaging_config = MessagingConfig.get_configuration(


Noting a type hint being removed likely to get around linting issues

yep....I also have no idea why, but I had to add a lot of mypy workarounds and that makes me concerned, but not concerned enough to spend hours figuring out why 😅

src/fides/api/ops/service/messaging/messaging_crud_service.py

src/fides/api/ops/util/oauth_util.py

ThomasLaPiana · 2023-05-02T05:32:15Z

@pattisdr thank you for the thorough review! Will go through and make changes accordingly 😄

ThomasLaPiana · 2023-05-03T08:36:07Z

@pattisdr I've gone through and made all of the consolidation/refactor changes you mentioned!

The only ones left are the mypy failure ones and I'm honestly not sure what to do there...I could revert some of those changes and then add ignores instead?

pattisdr · 2023-05-04T02:12:45Z

thanks very much @ThomasLaPiana I plan to review first thing tomorrow. I know this touches a lot of files and you're anxious to merge -

pattisdr

@ThomasLaPiana Thanks very much especially for consolidating the user endpoints, that's probably what I hoped to get most out of this.

The cause of the type issues were bothering me so I dug into it and it looks like we were previously importing the MessagingConfig and ConnectionConfig models from a different directory in lib than the other models. I've tried to continue to clean up type hints in this corresponding PR that is pointed at your branch plus some other minor items I saw:

#3221

pattisdr · 2023-05-04T15:13:13Z

src/fides/api/ctl/routes/generate.py

-from fides.api.ops.util.oauth_util import verify_oauth_client_prod
+from fides.api.ops.oauth.utils import verify_oauth_client_prod


Something that just occurred to me is that this is going to need a corresponding plus PR that needs to go out in the same release as plus imports a lot of these oauth methods related to scopes and roles that should probably take priority then over other reorganization tickets this sprint

makes sense, I'll get on that as soon as this gets merged!

pattisdr · 2023-05-04T15:25:27Z

src/fides/api/ops/api/v1/endpoints/oauth_endpoints.py

-    connection_config: ConnectionConfig = ConnectionConfig.get_by(
+    connection_config = ConnectionConfig.get_by(
        db, field="key", value=authentication_request.connection_key
    )
+    assert connection_config


Either way your assert needs to be moved beneath the verify_oauth_connection_config as it is expected this could potentially return None. I'll take care of this in my corresponding PR to clean up some of this type stuff

src/fides/api/ops/util/text.py

Co-authored-by: Thomas <thomas.lapiana+github@pm.me>

ThomasLaPiana · 2023-05-05T09:54:16Z

@pattisdr This is ready for a final review, thanks again so much for the work you did in the type hints! I got those merged in and everything is looking good. I've also opened and cross-linked a Fidesplus issue for fixing things over there

pattisdr

Looks good to me @ThomasLaPiana you'll need another changelog update (group "changed" together) after getting this up to date one more time. Thanks again for all your efforts here.

ThomasLaPiana · 2023-05-06T16:48:41Z

Looks good to me @ThomasLaPiana you'll need another changelog update (group "changed" together) after getting this up to date one more time. Thanks again for all your efforts here.

Thank you so much for your contributions here! This is only the beginning of the decisive tech-debt payoff 🙂

ThomasLaPiana · 2023-05-06T17:49:01Z

failed tests are expected, merging

Update __init__.py

caa59bd

ThomasLaPiana self-assigned this Apr 24, 2023

feat: move the models into ops

94f2f7a

ThomasLaPiana added 3 commits April 24, 2023 21:02

feat: move the lib cryptography module

6f327c9

feat: move lib db module

c1d7dcb

fix: linting

2da1e83

github-advanced-security bot found potential problems Apr 24, 2023

View reviewed changes

src/fides/api/ops/db/base_class.py Fixed Show fixed Hide fixed

ThomasLaPiana added 10 commits April 24, 2023 21:49

fix: circular import

20f5765

feat: merge lib schemas

3759026

feat: move the oauth submodule into ops

a99fd59

feat: move utils into ops

a8b9642

feat: copy over the exceptions and fully remove the lib module

422bcde

fix: isort

0a8713d

feat: fix fides.lib patch

22186ea

fix: pylint

93793db

Merge branch 'main' into ThomasLaPiana-merge-lib-into-ops

945740a

fix: more mypy errors

32ac8f1

ThomasLaPiana added 2 commits April 25, 2023 23:37

Merge branch 'main' into ThomasLaPiana-merge-lib-into-ops

e87107b

fix: post-merge cleanup

f20186f

github-advanced-security bot found potential problems Apr 25, 2023

View reviewed changes

ThomasLaPiana added 4 commits April 26, 2023 16:37

fix: mypy errors, mostly through ignores!

c73c817

feat: merge the ops and lib oauth_utils

807b51d

docs: changelog

71ccc58

Merge branch 'main' into ThomasLaPiana-merge-lib-into-ops

0c1cff2

ThomasLaPiana requested review from pattisdr, seanpreston and SteveDMurphy April 26, 2023 09:07

fix: lib import in tests

1a5e515

galvana added the run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials label May 1, 2023

pattisdr suggested changes May 1, 2023

View reviewed changes

ThomasLaPiana added 7 commits May 3, 2023 14:31

feat: remove more lib stuff, large checkin

d86f2f2

fix: broken imports

aab61ae

Merge branch 'main' into ThomasLaPiana-merge-lib-into-ops

8f150c8

feat: consolidate the FidesSchemas

92f04e4

fix: pylint

6cb3778

docs: clarify to_snake_case examples

897fa72

fix: test collection

e5a4730

fix: small nit fixes

217f7a4

pattisdr reviewed May 4, 2023

View reviewed changes

pattisdr and others added 2 commits May 5, 2023 17:03

Lib -> Ops Type Hinting Fixes (#3221)

a9c9a94

Co-authored-by: Thomas <thomas.lapiana+github@pm.me>

Merge branch 'main' into ThomasLaPiana-merge-lib-into-ops

12928c1

ThomasLaPiana requested a review from pattisdr May 5, 2023 09:53

pattisdr approved these changes May 5, 2023

View reviewed changes

ThomasLaPiana added 3 commits May 7, 2023 00:52

Merge branch 'main' into ThomasLaPiana-merge-lib-into-ops

187354c

docs: CHANGELOG update

f4c777b

fix: static checks

c597ec7

ThomasLaPiana merged commit 8ecc3c7 into main May 6, 2023

ThomasLaPiana deleted the ThomasLaPiana-merge-lib-into-ops branch May 6, 2023 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `lib` submodule into `ops` #3134

Merge `lib` submodule into `ops` #3134

ThomasLaPiana commented Apr 24, 2023 •

edited

Loading

cypress bot commented Apr 24, 2023 •

edited

Loading

codecov bot commented Apr 24, 2023 •

edited

Loading

ThomasLaPiana commented Apr 25, 2023

pattisdr commented May 1, 2023

ThomasLaPiana commented May 1, 2023

ThomasLaPiana commented May 1, 2023

pattisdr left a comment

pattisdr May 1, 2023

ThomasLaPiana May 3, 2023

pattisdr May 1, 2023

ThomasLaPiana May 2, 2023

ThomasLaPiana commented May 2, 2023

ThomasLaPiana commented May 3, 2023

pattisdr commented May 4, 2023

pattisdr left a comment

pattisdr May 4, 2023

ThomasLaPiana May 5, 2023

pattisdr May 4, 2023

ThomasLaPiana commented May 5, 2023

pattisdr left a comment

ThomasLaPiana commented May 6, 2023

ThomasLaPiana commented May 6, 2023

		from fides.api.ops.util.oauth_util import verify_oauth_client_prod
		from fides.api.ops.oauth.utils import verify_oauth_client_prod

Merge lib submodule into ops #3134

Merge lib submodule into ops #3134

Conversation

ThomasLaPiana commented Apr 24, 2023 • edited Loading

Code Changes

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

cypress bot commented Apr 24, 2023 • edited Loading

Passing run #1795 ↗︎

codecov bot commented Apr 24, 2023 • edited Loading

Codecov Report

ThomasLaPiana commented Apr 25, 2023

pattisdr commented May 1, 2023

ThomasLaPiana commented May 1, 2023

ThomasLaPiana commented May 1, 2023

pattisdr left a comment

Choose a reason for hiding this comment

pattisdr May 1, 2023

Choose a reason for hiding this comment

ThomasLaPiana May 3, 2023

Choose a reason for hiding this comment

pattisdr May 1, 2023

Choose a reason for hiding this comment

ThomasLaPiana May 2, 2023

Choose a reason for hiding this comment

ThomasLaPiana commented May 2, 2023

ThomasLaPiana commented May 3, 2023

pattisdr commented May 4, 2023

pattisdr left a comment

Choose a reason for hiding this comment

pattisdr May 4, 2023

Choose a reason for hiding this comment

ThomasLaPiana May 5, 2023

Choose a reason for hiding this comment

pattisdr May 4, 2023

Choose a reason for hiding this comment

ThomasLaPiana commented May 5, 2023

pattisdr left a comment

Choose a reason for hiding this comment

ThomasLaPiana commented May 6, 2023

ThomasLaPiana commented May 6, 2023

Merge `lib` submodule into `ops` #3134

Merge `lib` submodule into `ops` #3134

ThomasLaPiana commented Apr 24, 2023 •

edited

Loading

cypress bot commented Apr 24, 2023 •

edited

Loading

codecov bot commented Apr 24, 2023 •

edited

Loading