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.

Sign up for GitHub

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

Jump to bottom

Show systems without privacy declarations on data map #1603

Merged
merged 5 commits into from
Nov 1, 2022
Merged

Show systems without privacy declarations on data map #1603

merged 5 commits into from
Nov 1, 2022

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented Oct 29, 2022 •
edited
Loading

Closes #1414

Code Changes

  • Add a failing test for systems without privacy declarations
  • Add a path to handle systems without a privacy declaration

Steps to Confirm

  • list any manual steps taken to confirm the changes

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

Originally discovered as part of a plus feature where systems are generated without privacy declarations.

@SteveDMurphy SteveDMurphy self-assigned this Oct 29, 2022
@SteveDMurphy
Copy link
Contributor Author

Heyo @allisonking 👋🏽 I am going to try and test this once more with the updated scanner before officially putting into review but figured it made sense to get you a heads up at least :)

@SteveDMurphy
Copy link
Contributor Author

Heyo @allisonking 👋🏽 I am going to try and test this once more with the updated scanner before officially putting into review but figured it made sense to get you a heads up at least :)

Successfully installed and tested by copying the changes over to a branch for a new tagged version (release-1.9.6) - It probably makes sense to have this set up on a new minor version to keep moving, would you agree?

@SteveDMurphy SteveDMurphy marked this pull request as ready for review October 31, 2022 18:07
allisonking
allisonking approved these changes Oct 31, 2022
View reviewed changes
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.

looks great! 👏 just some small questions, nothing blocking.

src/fides/ctl/core/export.py Outdated Show resolved Hide resolved
src/fides/ctl/core/export.py Show resolved Hide resolved
tests/ctl/core/test_export.py Outdated Show resolved Hide resolved
allisonking
allisonking approved these changes Nov 1, 2022
View reviewed changes
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.

thanks for the changes!!

It probably makes sense to have this set up on a new minor version to keep moving, would you agree?

yes, agreed! would love to get this into fidesctl+ before the end of the sprint. so maybe another patch release (depending on the status of 2.0)

@SteveDMurphy
Copy link
Contributor Author

thanks for the changes!!

It probably makes sense to have this set up on a new minor version to keep moving, would you agree?

yes, agreed! would love to get this into fidesctl+ before the end of the sprint. so maybe another patch release (depending on the status of 2.0)

Sounds good! I'll wait until the morning tomorrow to straighten this out 🙌🏽 thanks @allisonking !!

@ThomasLaPiana I don't want to make anything more difficult with what you are doing this week so just want to double check - does it make sense to merge this into main and pull the changes into a completely separate release for 1.9.6 or something else?

@ThomasLaPiana
Copy link
Contributor

@SteveDMurphy if its for plus, go ahead and send it! I won't start work on unified plus until next sprint (a few days from now) so if you want to get a fix in to plus asap cutting another fidesctl release is the way to go.

That being said, starting next sprint we'll no longer be cutting ops/ctl releases, as full focus will be on unified plus

@SteveDMurphy SteveDMurphy merged commit cafff39 into main Nov 1, 2022
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-1414-systems-sans-privacy-declaration branch November 1, 2022 13:19
SteveDMurphy added a commit that referenced this pull request Nov 1, 2022
@SteveDMurphy @allisonking
Show systems without privacy declarations on data map (#1603)
5657039 
* add failing test for system sans privacy declaration

* handle systems without privacy declarations

* changelog

* [skip ci] Update tests/ctl/core/test_export.py

Co-authored-by: Allison King <allisonjuliaking@gmail.com>

* improve naming, add comment for final column n/a

Co-authored-by: Allison King <allisonjuliaking@gmail.com>
ThomasLaPiana added a commit that referenced this pull request Nov 1, 2022
@ThomasLaPiana @ssangervasi
@PSalant726 @SteveDMurphy @allisonking
Merge ctl 1.9.6 -> Unified Fides (#1646)
25ed18b 
* fix parsing when the manifest dir is empty

* allow the CLI to run without git

* ui/dataset: Only reset collection index on actual dataset update

* Add a `plus_system_scans` table to the database (#1554)

* Add a migration to create a `plus_system_scans` table

* Annotate the `plus_system_scans` table

* Update `CHANGELOG.md`

* Add the `SystemScans` SQL model

* Trigger CI after rebase

* Fix broken link

* Fix incorrectly sorted imports

* Update `CHANGELOG.md` for patch release 1.9.5 (#1582)

* Add a migration to create a `plus_system_scans` table

* Annotate the `plus_system_scans` table

* Update `CHANGELOG.md`

* Add the `SystemScans` SQL model

* Trigger CI after rebase

* Update `CHANGELOG.md` for patch release 1.9.5

* Add a `plus_system_scans` table to the database (#1554)

* Add a migration to create a `plus_system_scans` table

* Annotate the `plus_system_scans` table

* Update `CHANGELOG.md`

* Add the `SystemScans` SQL model

* Trigger CI after rebase

* Fix broken link

* Fix incorrectly sorted imports

* Show systems without privacy declarations on data map (#1603)

* add failing test for system sans privacy declaration

* handle systems without privacy declarations

* changelog

* [skip ci] Update tests/ctl/core/test_export.py

Co-authored-by: Allison King <allisonjuliaking@gmail.com>

* improve naming, add comment for final column n/a

Co-authored-by: Allison King <allisonjuliaking@gmail.com>

Co-authored-by: Sebastian Sangervasi <sebastian@ethyca.com>
Co-authored-by: Phil Salant <PSalant726@users.noreply.github.com>
Co-authored-by: Steve Murphy <steven.d.murphy@gmail.com>
Co-authored-by: Allison King <allisonjuliaking@gmail.com>
@allisonking allisonking mentioned this pull request Nov 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allisonking allisonking allisonking approved these changes

Assignees

@SteveDMurphy SteveDMurphy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Data Map Excluding Systems Without Privacy Declarations
3 participants
@SteveDMurphy @ThomasLaPiana @allisonking