Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Bump fideslang to 1.1.0 #890

Merged
merged 38 commits into from
Aug 12, 2022
Merged

Bump fideslang to 1.1.0 #890

merged 38 commits into from
Aug 12, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Jul 18, 2022 •
edited by seanpreston
Loading

Purpose

Bump fideslang to version 1.1.0

Changes

  • update requirements.txt
  • remove identifiable, nonidentifiable, derived and provided sub levels (they all just become user
  • fix tests

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • 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)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #757

@ThomasLaPiana ThomasLaPiana mentioned this pull request Jul 18, 2022
Closed
@ThomasLaPiana
Copy link
Contributor Author

@conceptualshark would appreciate a look-see here as there might be some docs cleanup from the fideslang upgrade changes. Shouldn't be quite as much as on fidesctl but more than I trust myself to handle!

@ThomasLaPiana
Copy link
Contributor Author

@pattisdr it looks like you were the one who wrote the tests that are failing now, it looks like customer_id isn't getting pulled out and thats why the tests are failing. I'm having a hard time nailing down why my change broke this but I'm sure I'm just not seeing it, maybe something in the example dataset yaml file is no longer being denoted as dangerous?

conceptualshark
conceptualshark reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@conceptualshark conceptualshark left a comment

Choose a reason for hiding this comment

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

Docs look covered - thank you! 👍

@SteveDMurphy SteveDMurphy mentioned this pull request Jul 19, 2022
Closed
13 tasks
@ThomasLaPiana
Copy link
Contributor Author

side note: I'm kind of surprised that this doesn't require a database migration given the additional fields for datasets but 🤷

@ThomasLaPiana ThomasLaPiana added the run unsafe ci checks Triggers running of unsafe CI checks label Jul 20, 2022
pattisdr
pattisdr reviewed Jul 20, 2022
View reviewed changes
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.

Understand this is still in draft, just did a quick skim while I was in here looking at the side effects of removing derived/provided categories. Will test more thoroughly when it's ready for review.

README.md

### Step Three: Run a Privacy Request to Access Data

Finally, you can issue a Privacy Request using Policy `example_request_policy` across your test databases for `jane@example.com`.
The following response will be uploaded to a local folder (for demo purposes). We've collected identifiable user-provided
The following response will be uploaded to a local folder (for demo purposes). We've collected identifiable user
information for Jane across tables in both the postgres and mongo databases.

```json
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd run the quickstart make quickstart and make sure the response body in the readme is updated to match the quickstart response. I assume you'll have more id-like variables.

Makefile Outdated Show resolved Hide resolved
docs/fidesops/docs/guides/query_execution.md Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
src/fidesops/migrations/versions/55d61eb8ed12_add_default_policies.py Outdated Show resolved Hide resolved
tests/api/v1/endpoints/test_dataset_endpoints.py Outdated Show resolved Hide resolved
tests/api/v1/endpoints/test_dataset_endpoints.py Outdated Show resolved Hide resolved
tests/service/connectors/test_queryconfig.py Outdated Show resolved Hide resolved
tests/service/connectors/test_queryconfig.py Outdated Show resolved Hide resolved
tests/task/test_filter_results.py Outdated Show resolved Hide resolved
seanpreston
seanpreston reviewed Aug 3, 2022
View reviewed changes
src/fidesops/migrations/versions/7abe778b7082_update_fideslang_data_categories.py Outdated
sessionlocal = get_db_session(config)
with sessionlocal() as session:
try:
datasets = DatasetConfig.all(db=session)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something I've found really useful with (potentially) large data migrations in the past is some sense of a progress indicator via logging. Now we've fetched the entire dataset it'd be good to log the count here, and current iteration below (or maybe every x to avoid spamming the logs).

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the recursion it could be tricky to give an accurate count. The only length we will know is the top level, but we won't know how deep each level goes. Thoughts on how to handle that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great point. I don't have a good solution for that. As long as the user gets some input, anything is better than nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added some more logging. How helpful it will be is going to depend on how deep vs how wide things are. See if you think it will be good enough.

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review August 10, 2022 04:52
@ThomasLaPiana
Copy link
Contributor Author

marked all outdated conversations are resolved

@ThomasLaPiana
Copy link
Contributor Author

It looks like the migration order got messed up, will fix

@ThomasLaPiana ThomasLaPiana added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Aug 10, 2022
@ThomasLaPiana
Copy link
Contributor Author

seems like a legit failure @sanders41 but I can't tell for sure

@sanders41
Copy link
Contributor

The hubspot tests have been failing on and off. There is an open issue on this #992. Seeing if a restart passes.

@sanders41
Copy link
Contributor

@adamsachs or @galvana does this looks like a real issue or is it a random hubspot failure?

@galvana
Copy link
Collaborator

galvana commented Aug 11, 2022

The failing tests for Hubspot are not caused by this change. There is a second round of work being done right now for Hubspot where we'll address the intermittent test failures.

@ThomasLaPiana
Copy link
Contributor Author

going to fix the merge conflicts and send for a final (final final final) review :D

@sanders41 sanders41 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Aug 11, 2022
@sanders41
Copy link
Contributor

@seanpreston the rebase is done and the only failing test is the hubspot one so we are ready for final review now.

@ThomasLaPiana
Copy link
Contributor Author

@sanders41 ty ty 💯

seanpreston
seanpreston approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

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

Thanks folks!

@seanpreston seanpreston merged commit 91e5535 into main Aug 12, 2022
@seanpreston seanpreston deleted the ThomasLaPiana-bump-fideslang-110 branch August 12, 2022 17:51
@eastandwestwind eastandwestwind mentioned this pull request Aug 17, 2022
Merged
10 tasks
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
@ThomasLaPiana @pattisdr
Bump fideslang to 1.1.0 (#890)
cab9bc5 
* Bump fideslang to 1.1.0

* find/replace user.provided.identifiable -> user

* remove derived mentions

* don't remove volumes on teardown

* update address fields

* replace user.derived and user.provided -> user

* fix two more tests

* fix pylint errors

* fix integration tests

* fix failing mongo tasks

* update the changelog

* fix the failing mongo task test

* another mongo task fix

* more mongo task fixes

* Revert test back to two addresses being masked.

* Update mongo array access test to reflect that underlying dataset has changed, and policy has changed, so more fields are returned.

* add the noxfiles

* update the dockerfile and get the nox docker commands working

* Revert "update the dockerfile and get the nox docker commands working"

This reverts commit 4b98c62.

* remove noxfiles

* updates from comments

* Update test

* Add migration

* Update categories in test config files

* Fix data categories

* Fix more data categories

* Change user.provided.nonidentifiable to user

* Update migraiton with review suggestions

* Run black

* Add more logging to migration

* Increment counter

* fix migration conflict

Co-authored-by: Dawn Pattison <pattisdr@users.noreply.github.com>
Co-authored-by: Paul Sanders <pau@ethyca.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@conceptualshark conceptualshark conceptualshark left review comments

@sanders41 sanders41 sanders41 left review comments

@pattisdr pattisdr pattisdr left review comments

@seanpreston seanpreston seanpreston approved these changes

Assignees

@seanpreston seanpreston

@ThomasLaPiana ThomasLaPiana

@conceptualshark conceptualshark

Labels
run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to Fideslang 1.1.0
6 participants
@ThomasLaPiana @pattisdr @sanders41 @galvana @seanpreston @conceptualshark