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

Add new categories and uses to the Default Taxonomy #144

Merged
merged 23 commits into from
Aug 16, 2023

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Aug 7, 2023

Closes #142
Closes #141

Description Of Changes

Link to Data Categories Sheet
Link to Data Uses Sheet

Follow-Up Issue for Docs: https://github.com/ethyca/fideslang/issues/150

NOTE: CI fixes happening here: https://github.com/ethyca/fideslang/pull/145

All of the changes we make here will need to get listed out for the corresponding migration PR over in Fides

Code Changes

  • add new data categories
  • add new data uses
  • add additional assertions to the tests for additional data integrity checks

Steps to Confirm

  • list any manual steps taken to confirm the changes

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated (Authors note: New files generated for the docs site)
  • Issue Requirements are Met
  • Update CHANGELOG.md

@ThomasLaPiana ThomasLaPiana self-assigned this Aug 7, 2023
@ThomasLaPiana ThomasLaPiana requested review from a team, pattisdr, rsilvery and NevilleS August 9, 2023 07:57
@ThomasLaPiana
Copy link
Contributor Author

@NevilleS I'm thinking this needs a 2.0 bump? These are pretty large changes, and will definitely break things as we don't have proper versioning in yet

@ThomasLaPiana
Copy link
Contributor Author

I'm investigating CI failures here due to our friend PyYaml and Cython: yaml/pyyaml#736

@ThomasLaPiana
Copy link
Contributor Author

@pattisdr will merging this hinder your in-flight work in any way?

@pattisdr
Copy link
Contributor

pattisdr commented Aug 9, 2023

Not sure, you seem to be working in different files though. Either way we both have items that need to get in this sprint so I'd just go for it when you're ready -

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

@ThomasLaPiana Large effort here!

Reviewed by comparing taxonomy files to this confluence doc. I understand there have been other conversations since this doc and it is outdated in places so I'll leave the details for you to sort -

Nice testing additions as well. Another nice test to add might be one to make sure all the parents (and their parents, etc) exist.

src/fideslang/default_taxonomy/data_uses.py Outdated Show resolved Hide resolved
src/fideslang/default_taxonomy/data_uses.py Outdated Show resolved Hide resolved
data_files/data_uses.json Outdated Show resolved Hide resolved
data_files/data_uses.yml Show resolved Hide resolved
src/fideslang/default_taxonomy/data_uses.py Outdated Show resolved Hide resolved
src/fideslang/default_taxonomy/data_categories.py Outdated Show resolved Hide resolved
src/fideslang/default_taxonomy/data_categories.py Outdated Show resolved Hide resolved
src/fideslang/default_taxonomy/data_categories.py Outdated Show resolved Hide resolved
data_files/data_categories.csv Show resolved Hide resolved
data_files/data_uses.json Show resolved Hide resolved
@ThomasLaPiana ThomasLaPiana changed the title Add new categories and uses to the Default Taxonomy [Do Not Merge] Add new categories and uses to the Default Taxonomy Aug 10, 2023
@ThomasLaPiana ThomasLaPiana changed the title [Do Not Merge] Add new categories and uses to the Default Taxonomy Add new categories and uses to the Default Taxonomy Aug 14, 2023
@ThomasLaPiana
Copy link
Contributor Author

@pattisdr the review here is kind of confusing...

I was given a CSV file and told that it is the intended end-state of the taxonomy (one CSV for each type actually, categories and uses)

I converted those CSV files into google sheets and marked them up/had comments as per the linked sheets at the top of the PR

Anything that isn't in that file, I took as an explicit flag to remove it from the current taxonomy. There was additional discussion in the comments around what to do with certain fields

Hugely appreciate the thorough review here! I know it's hard to follow, I want @rsilvery to chime in as well and confirm the CSV in this PR matches the requirements. This is very detail-oriented work and deserves a lot of eyes on to verify my changes!

@ThomasLaPiana ThomasLaPiana requested a review from pattisdr August 16, 2023 06:11
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

thanks for your changes here - approved with the addition of the missing parent fides key item

CHANGELOG.md Outdated Show resolved Hide resolved
src/fideslang/default_taxonomy/data_uses.py Show resolved Hide resolved
@ThomasLaPiana ThomasLaPiana merged commit fb87dea into main Aug 16, 2023
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-add-data-categories branch August 16, 2023 17:33
@rsilvery
Copy link
Contributor

Is there still something I need to review here? I don't get GH notifications in a timely way so please ping me on Slack if so.

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.

Update fideslang data categories to support GVL Update fideslang data uses to support GVL
3 participants