HJ-23 Add connection_type to NamespaceMeta #5387
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides
|
||||||||||||||||||||||||||
| Project |
fides
|
| Run status |
|
| Run duration | 00m 39s |
| Commit |
|
| Committer | erosselli |
| 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. | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5387 +/- ##
=======================================
Coverage 85.58% 85.58%
=======================================
Files 379 379
Lines 23984 23989 +5
Branches 2623 2623
=======================================
+ Hits 20526 20531 +5
Misses 2906 2906
Partials 552 552 ☔ View full report in Codecov by Sentry. |
| """ | ||
| update ctl_datasets | ||
| set fides_meta = jsonb_set(fides_meta::jsonb, '{namespace, connection_type}', '"bigquery"', true) | ||
| where fides_meta::jsonb ? 'namespace'; |
There was a problem hiding this comment.
I just want to call out that I am a bit concerned about the performance of this migration -- opening the json to check whether it has a namespace key seems like it might not scale well.
There was a problem hiding this comment.
okay tested on 500,000 rows and it took like 7s and on 1,120,000 rows it took 20s. we should be okay
There was a problem hiding this comment.
Thanks for doing that, makes for much higher confidence
erosselli
requested review from
thingscouldbeworse and
andres-torres-marroquin
| ) | ||
|
|
||
| # If the namespace dict has a database_instance_id, we know it's an RDS MySQL dataset. | ||
| op.execute( |
There was a problem hiding this comment.
I'm missing context from the ticket, sorry, but do we care about any other dialects? We'll fill those in later?
There was a problem hiding this comment.
My fidesplus PR https://github.com/ethyca/fidesplus/pull/1673 (not ready for review yet) makes the necessary changes to start storing it for all datasets created through D&D . This migration just takes advantage of the fact that BQ and RDS MySQL were both already using the namespace key so we have a reliable way to identify existing datasets from these integrations. for other integrations we'll start storing the connection_type from now on but we don't have a way to populate their values for existing datasets
@thingscouldbeworse yes, fides_meta is a JSON column so it gets parsed as a dictionary. |
fides
|
||||||||||||||||||||||||||
| Project |
fides
|
| Run status |
|
| Run duration | 00m 40s |
| Commit |
|
| Committer | erosselli |
| 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. | |
Part of HJ-23
Description Of Changes
Migration to add the
connection_typefield to all existingnamespacedicts in thefides_metafield of theDatasetmodel.Code Changes
connection_typefor existing datasets with a namespaceSteps to Confirm
namespacekey in itsfides_metafieldnamespacekey and value{ "project_id": "some-id" }(simulates a BigQuery dataset created through D&D)namespacekey and value{ "database_instance_id": "some-id" }(simulates an RDS MySQL dataset created through D&D)namespacekey and value{ "other": "test" }connection_typefield with valuebigqueryas part of itsnamespaceconnection_typefield with valuerds_mysqlas part of itsnamespacePre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works