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

include fides_key in datamap api output #992

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented Aug 16, 2022

Closes #987

Code Changes

  • Include the fides_key for system and dataset rows returned by the /datamap enpoint

Steps to Confirm

  • Validate the fides_key of items in the data map
  • Work with frontend to ensure it works for them

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 is not pretty but wanted to get a start out for this quickly as it sounds like a potential blocking item

@LKCSmith
Copy link
Contributor

@SteveDMurphy I really appreciate you getting started on this, despite feeling under the weather, and I hope you are feeling better or at least taking the time to feel better!! 🍵 Let me know when you think this is good for the FE to test with.

@SteveDMurphy
Copy link
Contributor Author

@SteveDMurphy I really appreciate you getting started on this, despite feeling under the weather, and I hope you are feeling better or at least taking the time to feel better!! 🍵 Let me know when you think this is good for the FE to test with.

Thanks @LKCSmith !! This at least has the fides key(s) for system and dataset included, so that may be enough to start. I am going to experiment with pulling in the system dependencies as well this morning but hoping you can get an early start

@SteveDMurphy
Copy link
Contributor Author

@LKCSmith both fides_key's and system_dependencies are now part of the output. I think the output is still technically a comma separated string but I think that should be easily fixed (Assuming it's easier for the frontend to have a list instead of a string) - would love to hear what you get from any testing!

@LKCSmith
Copy link
Contributor

@SteveDMurphy Thank you! I'll try this out today 🚀

@LKCSmith
Copy link
Contributor

@SteveDMurphy - One last request (if it makes sense to you) - is it possible to also include the system.description? If I had this included, I wouldn't need to map through an additional query to link it in the spatial map node. If it doesn't make sense to return it here, then I can do that mapping, but thought this might be more straightforward. What do you think?

@SteveDMurphy
Copy link
Contributor Author

@SteveDMurphy - One last request (if it makes sense to you) - is it possible to also include the system.description? If I had this included, I wouldn't need to map through an additional query to link it in the spatial map node. If it doesn't make sense to return it here, then I can do that mapping, but thought this might be more straightforward. What do you think?

Thanks for the heads up @LKCSmith ! I don't see any issue with including it as well, anything to we can do to make rendering the datamap a bit easier 👍🏽. The only problem I foresee is if there are a large number of rows for a single system (thinking many data categories) we end up duplicating a bunch of data unnecessarily for each row. I think this is likely a problem we can address down the road however and just focus on driving ahead here.

Will get that added either later tonight or first thing in the morning so you can drop that additional query, thanks again!

@SteveDMurphy SteveDMurphy marked this pull request as ready for review August 18, 2022 02:06
@SteveDMurphy SteveDMurphy requested a review from a team August 18, 2022 02:06
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana 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 the speedy unblocking action here :)

@SteveDMurphy
Copy link
Contributor Author

Thanks for the lightning fast review!! ⚡ ⚡

@SteveDMurphy SteveDMurphy merged commit 48a948c into main Aug 18, 2022
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-987-datamap-system-keys branch August 18, 2022 11:47
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.

Include system dependencies in datamap endpoint
3 participants