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

Remove export command and backing code #3256

Merged
merged 3 commits into from
May 24, 2023

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented May 9, 2023

Partially closes https://github.com/ethyca/fidesplus/issues/879

Code Changes

  • remove export CLI command and backing code in export.py and export_helpers.py (it will be moved over to fidesplus)
  • remove automated tests for that code (also will be moved over to fidesplus)
  • make some adjustments to existing tests that needed them

Steps to Confirm

  • confirm that fides export no longer works in core fides with this change (should be an unrecognized command)
  • confirm that on the fidesplus side (see open PR here), fides export command does work as expected
  • confirm on the fidesplus side that datamap UI still works as expected

Pre-Merge Checklist

Description Of Changes

See fidesplus issue for context/motivation behind this migration

@adamsachs adamsachs added the do not merge Please don't merge yet, bad things will happen if you do label May 9, 2023
@cypress
Copy link

cypress bot commented May 9, 2023

Passing run #2141 ↗︎

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 0271d27 into db00d84...
Project: fides Commit: 530b928a7f ℹ️
Status: Passed Duration: 00:47 💡
Started: May 24, 2023 12:58 PM Ended: May 24, 2023 12:59 PM

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 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.18 🎉

Comparison is base (db00d84) 87.03% compared to head (0271d27) 87.21%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3256      +/-   ##
==========================================
+ Coverage   87.03%   87.21%   +0.18%     
==========================================
  Files         314      311       -3     
  Lines       19014    18586     -428     
  Branches     2474     2368     -106     
==========================================
- Hits        16549    16210     -339     
+ Misses       2021     1960      -61     
+ Partials      444      416      -28     
Impacted Files Coverage Δ
src/fides/cli/__init__.py 92.15% <ø> (-0.16%) ⬇️

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

@adamsachs adamsachs changed the title ** do not merge ** remove export command and backing code Remove export command and backing code May 9, 2023
@adamsachs adamsachs removed the do not merge Please don't merge yet, bad things will happen if you do label May 9, 2023
@adamsachs adamsachs marked this pull request as ready for review May 9, 2023 18:24
@adamsachs adamsachs self-assigned this May 10, 2023
@ThomasLaPiana
Copy link
Contributor

discussion in slack for removal vs. deprecation: link

@adamsachs
Copy link
Contributor Author

created #3264 for deprecation instead of removal. created #3265 to track removal.

if we're good with this approach, i'll mark this PR as "do not merge" until we're ready to officially remove the command in the subsequent release.

@ThomasLaPiana ThomasLaPiana added the do not merge Please don't merge yet, bad things will happen if you do label May 11, 2023
@ThomasLaPiana ThomasLaPiana marked this pull request as draft May 11, 2023 03:46
@adamsachs adamsachs force-pushed the fidesplus-879-migrate-export-code branch from e7963dc to f0948c5 Compare May 22, 2023 12:45
@adamsachs adamsachs removed the do not merge Please don't merge yet, bad things will happen if you do label May 22, 2023
@adamsachs adamsachs marked this pull request as ready for review May 22, 2023 12:46
@adamsachs
Copy link
Contributor Author

@ThomasLaPiana @SteveDMurphy this should be a straightforward one, assuming we're good to remove this in 2.14.0 now that it was included with the deprecation warning in 2.13.0. if we'd prefer to hold off for another release that's OK with me, from my POV we're good to knock this out now.

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented May 22, 2023

@ThomasLaPiana @SteveDMurphy this should be a straightforward one, assuming we're good to remove this in 2.14.0 now that it was included with the deprecation warning in 2.13.0. if we'd prefer to hold off for another release that's OK with me, from my POV we're good to knock this out now.

@NevilleS you comfortable with removing this after official deprecation for a release?

My gut feeling is yes, but we also don't know who's potentially using this. Worst-case they can fork and re-implement?

@NevilleS
Copy link
Contributor

Yup, let's do it 👍

@adamsachs adamsachs force-pushed the fidesplus-879-migrate-export-code branch from cd0e23d to 44f150c Compare May 24, 2023 12:07
@adamsachs
Copy link
Contributor Author

@ThomasLaPiana do the code changes (removals) look alright to you? just need a ✅ before i can merge this in!

@ThomasLaPiana
Copy link
Contributor

@adamsachs oh gotcha, let me take a look!

@ThomasLaPiana
Copy link
Contributor

@adamsachs i got two more little ones hiding, I think this is good to go now! Approving but we can wait for the PR checks to be sure

@adamsachs adamsachs merged commit 3594d16 into main May 24, 2023
@adamsachs adamsachs deleted the fidesplus-879-migrate-export-code branch May 24, 2023 13:45
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.

3 participants