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

Unnest ops API code #3318

Merged
merged 13 commits into from
May 18, 2023
Merged

Unnest ops API code #3318

merged 13 commits into from
May 18, 2023

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented May 17, 2023

Code Changes

  • move the fides.api.ops dir up one into fides.api

Steps to Confirm

  • tests pass

Pre-Merge Checklist

  • All CI Pipelines Succeeded (failures are all expected failures)
  • Relevant Follow-Up Issues Created (follow-up plus issue]
  • Update CHANGELOG.md

Description Of Changes

In preparation for fully integrating ops/ctl code, this small (but with a large diff 😬 ) iteration unnests the ops code into fides.api

@ThomasLaPiana ThomasLaPiana self-assigned this May 17, 2023
@cypress
Copy link

cypress bot commented May 17, 2023

Passing run #2055 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 1a70c8f into 83bada6...
Project: fides Commit: 82d3b49f5c ℹ️
Status: Passed Duration: 00:43 💡
Started: May 18, 2023 5:53 AM Ended: May 18, 2023 5:54 AM

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

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Patch coverage: 97.87% and no project coverage change.

Comparison is base (83bada6) 87.07% compared to head (1a70c8f) 87.07%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3318   +/-   ##
=======================================
  Coverage   87.07%   87.07%           
=======================================
  Files         313      313           
  Lines       18968    18968           
  Branches     2473     2473           
=======================================
  Hits        16517    16517           
  Misses       2009     2009           
  Partials      442      442           
Impacted Files Coverage Δ
src/fides/api/api/v1/endpoints/utils.py 100.00% <ø> (ø)
src/fides/api/api/v1/scope_registry.py 100.00% <ø> (ø)
src/fides/api/api/v1/urn_registry.py 100.00% <ø> (ø)
src/fides/api/cryptography/cryptographic_util.py 100.00% <ø> (ø)
src/fides/api/cryptography/schemas/jwt.py 100.00% <ø> (ø)
src/fides/api/ctl/view.py 0.00% <0.00%> (ø)
src/fides/api/email_templates/__init__.py 100.00% <ø> (ø)
src/fides/api/email_templates/template_names.py 100.00% <ø> (ø)
src/fides/api/graph/data_type.py 88.81% <ø> (ø)
src/fides/api/oauth/jwt.py 100.00% <ø> (ø)
... and 236 more

... and 8 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.

@ThomasLaPiana ThomasLaPiana added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label May 17, 2023
@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review May 17, 2023 12:34
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

this looks good, did some smoke testing on my local server too and things generally looked fine.

only question is around the failures with external saas tests - i'm almost positive those are unrelated to the changes, just the fact that there are so many (and with some rather unusual suspects from my experience) gives me pause. there also aren't any logs showing the failures/errors - maybe this is normal in CI?

looping in @galvana who's got some more recent experience with the saas tests to make sure things look OK from his POV!

@ThomasLaPiana
Copy link
Contributor Author

this looks good, did some smoke testing on my local server too and things generally looked fine.

only question is around the failures with external saas tests - i'm almost positive those are unrelated to the changes, just the fact that there are so many (and with some rather unusual suspects from my experience) gives me pause. there also aren't any logs showing the failures/errors - maybe this is normal in CI?

looping in @galvana who's got some more recent experience with the saas tests to make sure things look OK from his POV!

That is correct, we don't show those test logs in CI for security reasons. To put it simply, I did a mass search find for any mention of fides.api.ops, fides/api/ops, and other combinations so I truly don't think this PR would break it but also can't verify due to the logs being obscured.

To avoid continually spending time on merge conflicts, I'd rather ship now and follow up with a smaller PR that might fix any particular bug that slipped through

@adamsachs
Copy link
Contributor

this looks good, did some smoke testing on my local server too and things generally looked fine.
only question is around the failures with external saas tests - i'm almost positive those are unrelated to the changes, just the fact that there are so many (and with some rather unusual suspects from my experience) gives me pause. there also aren't any logs showing the failures/errors - maybe this is normal in CI?
looping in @galvana who's got some more recent experience with the saas tests to make sure things look OK from his POV!

That is correct, we don't show those test logs in CI for security reasons. To put it simply, I did a mass search find for any mention of fides.api.ops, fides/api/ops, and other combinations so I truly don't think this PR would break it but also can't verify due to the logs being obscured.

To avoid continually spending time on merge conflicts, I'd rather ship now and follow up with a smaller PR that might fix any particular bug that slipped through

that's good with me! sorry, looks like you've already got some conflicts :/

@ThomasLaPiana
Copy link
Contributor Author

this looks good, did some smoke testing on my local server too and things generally looked fine.
only question is around the failures with external saas tests - i'm almost positive those are unrelated to the changes, just the fact that there are so many (and with some rather unusual suspects from my experience) gives me pause. there also aren't any logs showing the failures/errors - maybe this is normal in CI?
looping in @galvana who's got some more recent experience with the saas tests to make sure things look OK from his POV!

That is correct, we don't show those test logs in CI for security reasons. To put it simply, I did a mass search find for any mention of fides.api.ops, fides/api/ops, and other combinations so I truly don't think this PR would break it but also can't verify due to the logs being obscured.
To avoid continually spending time on merge conflicts, I'd rather ship now and follow up with a smaller PR that might fix any particular bug that slipped through

that's good with me! sorry, looks like you've already got some conflicts :/

haha no worries! I knew I wouldn't get away clean this time 😂 after going through the Fides 2.0 merge I think I can handle just about anything in terms of conflicts

@ThomasLaPiana ThomasLaPiana merged commit 9a26ca1 into main May 18, 2023
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-unnest-ops-api-code branch May 18, 2023 08:21
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.

2 participants