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

Migration for system_dependencies #3285

Merged
merged 19 commits into from
Jun 7, 2023

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented May 10, 2023

Closes fidesplus#822

Code Changes

  • Create a migration to move system_dependencies to egress and delete the column
  • remove replace usage with egress across backend
  • remove replace usage with egress across frontend

Steps to Confirm

The following scenarios have been tested successfully

  • system_dependency exists, no egress exists
  • multiple system_dependencies exist, no egress exists
  • system_dependency exists, single egress exists
  • multiple system_dependencies exist, multiple egress exists
  • the same system exists as both a system_dependency and an egress

Other?

Pre-Merge Checklist

Description Of Changes

A fideslang update will be required as well

@cypress
Copy link

cypress bot commented May 10, 2023

Passing run #2512 ↗︎

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 d9ccb34 into 5545384...
Project: fides Commit: c2f82648eb ℹ️
Status: Passed Duration: 00:56 💡
Started: Jun 7, 2023 3:11 PM Ended: Jun 7, 2023 3:12 PM

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

@SteveDMurphy SteveDMurphy self-assigned this May 15, 2023
@SteveDMurphy
Copy link
Contributor Author

SteveDMurphy commented May 15, 2023

@TheAndrewJackson I was curious if this was along the lines of what you were expecting as well. I think I need to pair a new fideslang release with this as well to finally remove the deprecated system_dependencies but just hoping for a quik temp check

@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (5545384) 87.13% compared to head (d9ccb34) 87.14%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3285   +/-   ##
=======================================
  Coverage   87.13%   87.14%           
=======================================
  Files         312      312           
  Lines       18761    18759    -2     
  Branches     2390     2390           
=======================================
  Hits        16348    16348           
+ Misses       1989     1988    -1     
+ Partials      424      423    -1     
Impacted Files Coverage Δ
src/fides/api/ctl/sql_models.py 97.70% <ø> (-0.02%) ⬇️

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

@SteveDMurphy
Copy link
Contributor Author

This is in an ok state from the backend, the only failure seems to stem from the temporary installation from fideslang. Will put that into review, cut a release and bring that over. I think I can also look at shoring up the frontend in here if we want to do that as well

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.

@SteveDMurphy This is looking good. It's exactly what I would have done

Dockerfile Outdated Show resolved Hide resolved
@SteveDMurphy
Copy link
Contributor Author

This may need to be combined with any other work related to #3334

@SteveDMurphy SteveDMurphy mentioned this pull request Jun 2, 2023
29 tasks
@SteveDMurphy SteveDMurphy marked this pull request as ready for review June 6, 2023 13:05
@SteveDMurphy SteveDMurphy marked this pull request as draft June 6, 2023 19:16
@SteveDMurphy
Copy link
Contributor Author

Moved back to draft while I attempt to wrangle the front end required changes 😞

@SteveDMurphy SteveDMurphy force-pushed the SteveDMurphy-822-migrate-system-deps branch from 85c19fa to 3d6c011 Compare June 7, 2023 10:59
@SteveDMurphy SteveDMurphy marked this pull request as ready for review June 7, 2023 14:49
@SteveDMurphy
Copy link
Contributor Author

All tests are passing! Quite a few more snags with migration collisions but back into review, going to catch up to main again as well and add the changelog but should be nearly there 😅

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:

@SteveDMurphy SteveDMurphy merged commit 0833511 into main Jun 7, 2023
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-822-migrate-system-deps branch June 7, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants