HJ-87 and HJ-88: model changes #5600
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5600/merge
|
| Run status |
|
| Run duration | 00m 48s |
| Commit |
|
| Committer | Kirk Hardy |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5600 +/- ##
==========================================
- Coverage 87.14% 87.13% -0.01%
==========================================
Files 388 388
Lines 24000 24016 +16
Branches 2594 2595 +1
==========================================
+ Hits 20914 20927 +13
- Misses 2525 2526 +1
- Partials 561 563 +2 ☔ View full report in Codecov by Sentry. |
src/fides/api/alembic/migrations/versions/d9237a0c0d5a_add_hidden_column_to_stagedresource.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
ok thanks for making the requested updates! this is looking good, just a few minor nits that'd be nice to get addressed if you can.
separately, there seems to be a test failure? it seems unlikely to me that it'd b caused by your changes, but i don't see it on main in the past few commits. maybe it'll go away with merging main in?
and last note is just to keep an eye on the downrev in your migration. i know there's at least one other in-flight PR that updates our migrations, so if it gets merged before yours, you'll just have to remember to bump your downrev to the new head
There was a problem hiding this comment.
nit: i'd probably update the filename here just to be consistent with the new functionality - it can be hard to track down migrations sometimes and relying on the filename can help with looking it up, so this could add a bit of confusion moving forward!
There was a problem hiding this comment.
changed in 61f38a4#diff-6ea847dd5dab7785c69f11369a07c894290e837501325a3b8eb5f003059c33ea and made sure column isn't pluralized in the comment
CHANGELOG.md
Outdated
| @@ -21,6 +21,7 @@ The types of changes are: | |||
| - New page in the Cookie House sample app to demonstrate the use of embedding the FidesJS SDK on the page [#5564](https://github.com/ethyca/fides/pull/5564) | |||
| - Added event based communication example to the Cookie House sample app [#5597](https://github.com/ethyca/fides/pull/5597) | |||
| - Added new erasure tests for BigQuery Enterprise [#5554](https://github.com/ethyca/fides/pull/5554) | |||
| - Migration to add the `hidden` and `data_use` columns to `stagedresource` table, prereqs for Data Catalog work in Fidesplus [#5600](https://github.com/ethyca/fides/pull/5600/) | |||
There was a problem hiding this comment.
nit
| - Migration to add the `hidden` and `data_use` columns to `stagedresource` table, prereqs for Data Catalog work in Fidesplus [#5600](https://github.com/ethyca/fides/pull/5600/) | |
| - Migration to add the and `data_use` column to `stagedresource` table, prereq for Data Catalog work in Fidesplus [#5600](https://github.com/ethyca/fides/pull/5600/) |
There was a problem hiding this comment.
| @@ -273,6 +274,26 @@ def test_patch_connection_secrets_removes_access_token_for_client_config( | |||
| assert resp.status_code == HTTP_200_OK | |||
| assert resp.json()["items"][0]["authorized"] is False | |||
|
|
|||
| def test_system_patch_hidden( | |||
There was a problem hiding this comment.
it'd be nice for this test to assert that the updates are actually reflected in the db! there have been many times where an endpoint gives a 200 and the correct response body but it doesn't actually make the intended change to the db :)
There was a problem hiding this comment.
…igs" This reverts commit e167770.
Prereq migration and model change for HJ-87 & HJ-88
Description Of Changes
Write some things here about the changes and any potential caveats
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works