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

Add a plus_system_scans table to the database #1554

Merged
merged 7 commits into from
Oct 27, 2022
Merged

Add a plus_system_scans table to the database #1554

merged 7 commits into from
Oct 27, 2022

Conversation

PSalant726
Copy link
Contributor

@PSalant726 PSalant726 commented Oct 25, 2022 •
edited by mfbrown
Loading

Closes #1396

Code Changes

  • Add a new plus_system_scans table to the database
  • Add a corresponding SystemScans SQL model
  • Annotate the new database table

Steps to Confirm

  • Start fides
  • List the relations in the databse
  • Observe that plus_system_scans is included

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

Description Of Changes

Adds the plus_system_scans relation to the fides database, and a corresponding SystemScans model. Any associated APIs will be added in fidesctl-plus, as a part of ethyca/fidesctl-plus#195.

@PSalant726 PSalant726 added enhancement python Pull requests that update Python code hacktoberfest-accepted labels Oct 25, 2022
@PSalant726 PSalant726 added this to the Zero to DataMap milestone Oct 25, 2022
@PSalant726 PSalant726 self-assigned this Oct 25, 2022
ThomasLaPiana
ThomasLaPiana approved these changes Oct 25, 2022
View reviewed changes
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.

The existence of said tables has been observed 😂

@ThomasLaPiana ThomasLaPiana self-requested a review October 25, 2022 10:07
ThomasLaPiana
ThomasLaPiana suggested changes Oct 25, 2022
View reviewed changes
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.

But I can't approve until tests are passing...try merging main?

allisonking
allisonking reviewed Oct 25, 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.

code looks good, just throwing in some existential questions 😬

src/fides/api/ctl/migrations/versions/6b9885e68cbb_add_plus_system_scans_table.py Outdated Show resolved Hide resolved
.fides/db_dataset.yml Outdated Show resolved Hide resolved
ThomasLaPiana
ThomasLaPiana suggested changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Need a test to confirm that this works as expected, even if it's something as simple as creating the object object in the database and then deleting it. Don't want to ship this and then realize it doesn't actually work when we try to use it in fidesctl-plus, unless there is some other way to prove it works?

@PSalant726
Copy link
Contributor Author

Need a test to confirm that this works as expected

I don't see any similar tests right now, and I'm not sure how it could be tested. It's just a model definition, and any APIs to interact with the model will exist in fidesctl-plus, so there's no logic to test. I would expect tests against the DB interactions to come with the code changes on a PR to address https://github.com/ethyca/fidesctl-plus/issues/195.

@ThomasLaPiana
Copy link
Contributor

Need a test to confirm that this works as expected

I don't see any similar tests right now, and I'm not sure how it could be tested. It's just a model definition, and any APIs to interact with the model will exist in fidesctl-plus, so there's no logic to test. I would expect tests against the DB interactions to come with the code changes on a PR to address ethyca/fidesctl-plus#195.

Understood, still feels off not being able to test it at all, but I suppose the repo relationship is a bit wonky as it is

allisonking
allisonking approved these changes Oct 27, 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.

🎉

@PSalant726 PSalant726 changed the base branch from main to deploy-1.9.5 October 27, 2022 15:54
@PSalant726 PSalant726 requested review from a team October 27, 2022 15:54
@PSalant726 PSalant726 changed the base branch from deploy-1.9.5 to main October 27, 2022 15:54
@PSalant726 PSalant726 changed the base branch from main to deploy-1.9.5 October 27, 2022 16:45
@PSalant726 PSalant726 mentioned this pull request Oct 27, 2022
Merged
6 tasks
@PSalant726 PSalant726 merged commit b5689a9 into deploy-1.9.5 Oct 27, 2022
@PSalant726 PSalant726 deleted the persist-system-scans branch October 27, 2022 17:21
PSalant726 added a commit that referenced this pull request Oct 27, 2022
@PSalant726
Add a plus_system_scans table to the database (#1554)
ec923a8 
* 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
PSalant726 added a commit that referenced this pull request Oct 27, 2022
@PSalant726
Update CHANGELOG.md for patch release 1.9.5 (#1582)
c31ceec 
* 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
@PSalant726 PSalant726 mentioned this pull request Oct 27, 2022
Closed
PSalant726 added a commit that referenced this pull request Oct 27, 2022
@PSalant726 @ThomasLaPiana @ssangervasi
Merge deploy-1.9.5 (#1583)
8b4e493 
* 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

* Remove extra added lines

Co-authored-by: Thomas <[email protected]>
Co-authored-by: Sebastian Sangervasi <[email protected]>
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 <[email protected]>

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

Co-authored-by: Allison King <[email protected]>

Co-authored-by: Sebastian Sangervasi <[email protected]>
Co-authored-by: Phil Salant <[email protected]>
Co-authored-by: Steve Murphy <[email protected]>
Co-authored-by: Allison King <[email protected]>
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

@ThomasLaPiana ThomasLaPiana ThomasLaPiana approved these changes

Assignees

@PSalant726 PSalant726

Labels
hacktoberfest-accepted python Pull requests that update Python code
Projects
None yet
Milestone
Zero to DataMap
Development

Successfully merging this pull request may close these issues.

Add table and models to support system scan history
3 participants
@PSalant726 @ThomasLaPiana @allisonking