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 datamap export API #2999

Merged
merged 3 commits into from
Apr 11, 2023
Merged

remove datamap export API #2999

merged 3 commits into from
Apr 11, 2023

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Apr 6, 2023

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

Code Changes

  • remove datamap export API (redefined in fidesplus)
  • remove datamap:read scope from fides (will be redefined in fidesplus)
  • rewire datamap UI to look at the plus endpoint instead

Steps to Confirm

  • confirm that datamap export API is not exposed/available if just running fides from this repo (not fidesplus)

Pre-Merge Checklist

Description Of Changes

We're moving the datamap export API over to fidesplus

attempt to rewire datamap UI to look at the plus endpoint instead
@cypress
Copy link

cypress bot commented Apr 10, 2023

Passing run #1284 ↗︎

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

Details:

Merge b18eea3 into 2aa7858...
Project: fides Commit: 361ddaf4db ℹ️
Status: Passed Duration: 00:36 💡
Started: Apr 11, 2023 10:07 PM Ended: Apr 11, 2023 10:08 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 Apr 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (0316ab2) 86.92% compared to head (b18eea3) 86.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2999      +/-   ##
==========================================
+ Coverage   86.92%   86.99%   +0.07%     
==========================================
  Files         303      302       -1     
  Lines       17322    17263      -59     
  Branches     2230     2224       -6     
==========================================
- Hits        15057    15018      -39     
+ Misses       1849     1827      -22     
- Partials      416      418       +2     
Impacted Files Coverage Δ
src/fides/api/ops/api/v1/scope_registry.py 100.00% <ø> (ø)
src/fides/lib/oauth/roles.py 100.00% <ø> (ø)
src/fides/api/main.py 79.14% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adamsachs adamsachs marked this pull request as ready for review April 10, 2023 18:18
@adamsachs
Copy link
Contributor Author

@allisonking i think this is just about good to go, if you could take another look - specifically at the FE pieces - that would be much appreciated!

@SteveDMurphy going to tag you as a reviewer here too, this is just the cleanup on fides corresponding to the addition to fidesplus

@TheAndrewJackson just an FYI since i know you're going to be working on datamap API code soon

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

FE looks good to me! thanks @adamsachs 🙏

Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

Everything looks and works as expected! Great work @adamsachs - left one comment just for potential discussion

src/fides/api/ctl/routes/datamap.py Outdated Show resolved Hide resolved
@adamsachs
Copy link
Contributor Author

adamsachs commented Apr 11, 2023

i was just doing a final pass through myself, and realized that one thing i missed in the migration was moving the scope over to fidesplus - it's still defined here in core fides: https://github.com/ethyca/fides/blob/remove-datamap-export-api/src/fides/api/ops/api/v1/scope_registry.py#L125

the new route in fidesplus references this scope from core fides, and that all seems to work fine. it's certainly a bit against the paradigm that's been set up, i think - where fidesplus endpoints have their scopes (and roles) defined in that code base. i'm not really sure what the end-user implications are, if any.

i was thinking of creating a follow up ticket for this migration rather than taking care of it immediately, since i think it's more of a tech-debt issue rather than one that impacts users significantly. but i could be wrong about that. UPDATE - based on some conversation with @pattisdr, we think there may be some nontrivial UI implications to keeping the scope here without the endpoint being in OSS. so that's changed the calculus a bit, and now it feels probably simpler/better to just migrate the scope over to plus. the only thing that @pattisdr brought up as a potential concern if we do that is that it could maybe impact any users created in the OSS product who've had this scope assigned?

@pattisdr @allisonking - tagging you both since i think you are closest to some of the scopes/roles refactoring that was done, specifically with moving some of that to fidesplus? @SteveDMurphy also curious if you've got any thoughts off the top of your head.

@adamsachs adamsachs merged commit 341f3a9 into main Apr 11, 2023
@adamsachs adamsachs deleted the remove-datamap-export-api branch April 11, 2023 22:35
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.

4 participants