PROD-2316: database exclude list #5080
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Passing run #8976 ↗︎
Details:
Review all test suite changes for PR #5080 ↗︎ |
||||||||||||||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5080 +/- ##
==========================================
+ Coverage 86.47% 86.48% +0.01%
==========================================
Files 357 357
Lines 22271 22280 +9
Branches 2944 2945 +1
==========================================
+ Hits 19258 19269 +11
+ Misses 2498 2496 -2
Partials 515 515 ☔ View full report in Codecov by Sentry. |
| """Check that the list of databases included and excluded has no intersection""" | ||
| include = data.get("databases", []) | ||
| exclude = data.get("excluded_databases", []) |
There was a problem hiding this comment.
Does it make sense to have configured both databases AND excluded_databases? I'd think this would be an either you explicitly monitor specific DBs, or you explicitly "skip" specific DBs, but not both?
There was a problem hiding this comment.
no, and I misread that slack thread. Here is the fix: ca17da8
| raise ValueError( | ||
| "The lists of included and excluded databases must have no overlap." | ||
| ) | ||
| return True |
There was a problem hiding this comment.
why do we return a boolean here if we don't use the return value? also I think either we raise the exception in the invalid configuration case and not return anything, or we return True when OK and False when not. I think I prefer the exception tbh
There was a problem hiding this comment.
Yeah I buy that, fixed here: 3cffff6
| assert mc.databases == ["db1", "db2"] | ||
| assert mc.excluded_databases == ["db3"] | ||
|
|
||
| def test_update_monitor_config_fails_with_conflicting_dbs( |
There was a problem hiding this comment.
nice work on the tests 😎
src/fides/api/alembic/migrations/versions/f712aa9429f4_add_field_for_excluding_databases_on_.py
Show resolved
Hide resolved
| """ | ||
| Override the base class `create` to validate database include/exclude | ||
| and derive the `execution_trigger` dict field | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Override the base class `create` to validate database include/exclude | |
| and derive the `execution_trigger` dict field | |
| """ | |
| """ | |
| Override the base class `update` to validate database include/exclude | |
| and derive the `execution_trigger` dict field | |
| """ |
There was a problem hiding this comment.
nice catch; e5d32d7
| self, db: Session, create_monitor_config, connection_config: ConnectionConfig | ||
| ) -> None: | ||
| """ """ | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
nit: can we check specifically for ValueError here rather than exception?
There was a problem hiding this comment.
thank you, nice catch: e5d32d7
thingscouldbeworse
deleted the
PROD-2316_support-excluded-databases-field-on-monitor-config
branch
Passing run #8979 ↗︎
Details:
Review all test suite changes for PR #5080 ↗︎ |
||||||||||||||||||||
DB prereq for #PROD-2316
Description Of Changes
BE adjustments to allow excluding databases in a monitor along with including them
Code Changes
excluded_databasesproperty to theMonitorConfigmodelSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works