HJ-24 Add DataHub integration config #5401
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5401/merge
|
| 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. | |
| View all changes introduced in this branch ↗︎ | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5401 +/- ##
==========================================
+ Coverage 83.86% 83.89% +0.03%
==========================================
Files 382 384 +2
Lines 24073 24112 +39
Branches 2624 2624
==========================================
+ Hits 20188 20228 +40
+ Misses 3303 3302 -1
Partials 582 582 ☔ View full report in Codecov by Sentry. |
erosselli
force-pushed
the
HJ-24
branch
2 times, most recently
from
9fe7d7c to
87b8c83
Compare
erosselli
changed the title
| # TODO: use token for authentication | ||
| self.datahub_client = DataHubGraph( | ||
| DataHubGraphConfig(server=str(self.config.datahub_server_url)) | ||
| ) |
There was a problem hiding this comment.
right now the datahub we're running doesn't require any token, I'll address this in a separate ticket
|
|
||
| def upgrade(): | ||
| # Add 'datahub' to ConnectionType enum | ||
| op.execute("ALTER TYPE connectiontype RENAME TO connectiontype_old") |
There was a problem hiding this comment.
Did you get any answer from other people about why we do the connectiontype_old instead of altering the existing one? Or just better to keep with how it was done before?
There was a problem hiding this comment.
tbh I never asked 🙈 I've just seen it done this way always. I'll ask around in the eng channel
There was a problem hiding this comment.
I think "it's just been done this way before" is a 100% valid argument for doing it in the PR that way, for all we know it's catastrophic with the way we do Alembic and we realized in the past or something. But we should also just get an answer if anyone knows whether that's the case or not. I think good to merge as is and ask.
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.add_column( | ||
| "connectionconfig", | ||
| sa.Column("last_ran_at", sa.DateTime(timezone=True), nullable=True), |
There was a problem hiding this comment.
Sorry to be annoying, how do we feel about last_run_time? Non blocking.
There was a problem hiding this comment.
sure. I'll change it to last_run_timestamp so it's consistent with last_test_timestamp
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 36s |
| 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. | |
| View all changes introduced in this branch ↗︎ | |
Closes HJ-24
Description Of Changes
Adds support for a DataHub integregation.
Code Changes
DatahubSchemaclass that includes all necessary arguments for a Datahub integrationConnectionType.datahublast_ran_atnullable column to theConnectionConfigmodelDatahubConnectorclass. For now only implementstest_connectionmethod.Steps to Confirm
source your/venv/path/bin/activate) or create one if you haven't done so already and then activate it.pip install -r requirements.txt(with the venv activated)DATAHUB_MAPPED_GMS_PORT=8082 datahub docker quickstart(theDATAHUB_MAPPED_GMS_PORTvariable is so that datahub doesn't try to run on port 8080, which is already in used by fides)Datahub server URLfield, puthttp://host.docker.internal:8082testdaily)Pre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works