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

Move fideslib files into fides #1859

Merged
merged 42 commits into from
Dec 14, 2022
Merged

Move fideslib files into fides #1859

merged 42 commits into from
Dec 14, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Nov 28, 2022

Closes #1572

Code Changes

  • copy pasta the source code into src/fides/lib
  • copy pasta the tests over
  • update code paths
  • add a nox session for the lib tests
  • remove redundant tests from fideslib (Note to reviewer that not all fideslib tests will make it over, specifically config tests, session tests, endpoint tests that all already exist in ops or ctl tests)
  • merge the configuration code
  • A ton of mypy fixes. Now that fideslib isn't being exempt from mypy checks, so there are lots of new # type: ignore[something] comments in the code but I made sure they include specific exemptions instead of general ones

Steps to Confirm

  • automated tests pass

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

This PR does the work of merging fideslib source code into fides. Now that we have unified projects this should greatly simplify things when it comes to hunting down bugs in fideslib or vice versa, as well as making it easier to iterate by keeping the code together.

There is a huge number of changed files and particularly added lines due to:

  1. Adding all of the fideslib source files
  2. Changing all of the import statements from from fideslib to from fides.lib
  3. There were multiple PRs written on top of this branch (to prevent this PR and its mypy issues from being a blocker) that have also bloated the number of changed lines.

Generally, the tests remain untouched, so this should be a good sign that existing behaviour has generally been maintained. I understand that with this number of changed files and lines of code, this is not really feasible to review 🙁 so we're trusting our tests here mostly

@ThomasLaPiana ThomasLaPiana self-assigned this Nov 28, 2022
@ThomasLaPiana
Copy link
Contributor Author

currently got everything working, but need to go in and fix some tests/checks that are now failing due to the code move

@ThomasLaPiana
Copy link
Contributor Author

tests should all be passing now, next step is to fix static checks and then should be good to go

@ThomasLaPiana
Copy link
Contributor Author

ThomasLaPiana commented Nov 30, 2022

@sanders41 any idea where these failures are coming from?

https://github.com/ethyca/fides/actions/runs/3586465322/jobs/6035688944

fideslib shouldn't exist at all, so how is it getting imported here? do you know what the fides_client is?

Weirdly, this is all passing for me locally 🙁

@sanders41
Copy link
Contributor

sanders41 commented Nov 30, 2022

This isn't helpful other than to let you know you aren't going crazy...the tests pass locally for me also and I can't find any reference to fideslib anywhere. I think the Docker image gets cached in order to share between tests. Is there any chance it's using an old image that still has references to fideslib and that is causing the error? Seems unlikely, but maybe?

@ThomasLaPiana
Copy link
Contributor Author

This isn't helpful other than to let you know you aren't going crazy...the tests pass locally for me also and I can't find any reference to fideslib anywhere. I think the Docker image gets cached in order to share between tests. Is there any chance it's using an old image that still has references to fideslib and that is causing the error? Seems unlikely, but maybe?

Every little bit of confirmation helps! haha glad to know I was right in that locally everything is working...at least I've narrowed the problem down to CI and potentially caching, I'll take a look there

@ThomasLaPiana ThomasLaPiana added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Dec 2, 2022
@ThomasLaPiana
Copy link
Contributor Author

so I fixed this problem by merging in main, but I'm still not sure why it was happening in the first place, this could be an issue

@ThomasLaPiana
Copy link
Contributor Author

Failing external tests are expected (note main is also failing)

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review December 10, 2022 13:53
@ThomasLaPiana ThomasLaPiana requested a review from a team December 10, 2022 13:53
.github/workflows/backend_checks.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
src/fides/ctl/core/config/helpers.py Outdated Show resolved Hide resolved
src/fides/ctl/core/config/helpers.py Show resolved Hide resolved
src/fides/ctl/core/config/helpers.py Outdated Show resolved Hide resolved
src/fides/lib/oauth/api/deps.py Show resolved Hide resolved
src/fides/lib/oauth/api/urn_registry.py Show resolved Hide resolved
src/fides/lib/oauth/scopes.py Show resolved Hide resolved
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

good type hinting improvements here

src/fides/lib/oauth/api/deps.py Show resolved Hide resolved
src/fides/lib/oauth/oauth_util.py Show resolved Hide resolved
.fides/fides.toml Show resolved Hide resolved
@ThomasLaPiana
Copy link
Contributor Author

@pattisdr given the already large amount of code changes here, I decided that it is probably better/safer to create a separate issue/PR for further consolidation so that it will be easier to review those changes and verify that they didn't cause any breaks

#2032

Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for this, it will make things easier since we merged repos.

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

:shipit:

@ThomasLaPiana ThomasLaPiana merged commit 89e4383 into main Dec 14, 2022
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-merge-lib branch December 14, 2022 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge fideslib into unified-fides
4 participants