Prod 2435: bq connection test #5138
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5138 +/- ##
==========================================
- Coverage 86.52% 86.48% -0.05%
==========================================
Files 359 359
Lines 22470 22485 +15
Branches 2978 2980 +2
==========================================
+ Hits 19443 19446 +3
- Misses 2494 2506 +12
Partials 533 533 ☔ View full report in Codecov by Sentry. |
| @@ -323,6 +324,26 @@ def connection_status( | |||
| ) -> TestStatusMessage: | |||
| """Connect, verify with a trivial query or API request, and report the status.""" | |||
|
|
|||
| # test if this is a Bigquery connection | |||
| if connection_config.connection_type == ConnectionType.bigquery: | |||
There was a problem hiding this comment.
hmm. this will probably work, but i think there may be a cleaner way to inject the "custom" bigquery logic.
what i'd lean toward is implementing an overridden test_connection method in the BigQueryConnector class that's a subclass of the SQLConnector (this is the bigquery "DSR connector"): https://github.com/ethyca/fides/blob/main/src/fides/api/service/connectors/sql_connector.py#L476
that allows us to keep all the generic wrapping logic in tact, and it puts in the "custom" bigquery logic into a class specifically scoped toward bigquery. this will pave the road nicely for when we move the BigQueryConnector to leverage the python bigquery client, as is currently used in the d&d workflow.
There was a problem hiding this comment.
@thingscouldbeworse this looks good to me! just a few things that i think we could polish up here to get it as clean as possible, but generally this is how i was imagining this would work - nice job!
also, i assume you've done some manual testing? i didn't get to do any yet. if you've done that, could you please list out the manual steps you've taken to confirm in the PR description? i think it would be good for at least one of us to manually test out some of the error cases locally and make sure this is working as we expect - if you've done that, then great
| if all_projects: | ||
| return ConnectionTestStatus.succeeded | ||
| else: | ||
| raise ConnectionException("No datasets found with the provided credentials.") |
There was a problem hiding this comment.
this is nice handling! just a tweak on the wording:
| raise ConnectionException("No datasets found with the provided credentials.") | |
| raise ConnectionException("No bigquery projects found with the provided credentials.") |
There was a problem hiding this comment.
| else: | ||
| raise ConnectionException("No datasets found with the provided credentials.") | ||
| except Exception as e: | ||
| raise ConnectionException(f"Connection error: {e}") |
There was a problem hiding this comment.
this is solid for the exception that's raised up (and hopefully gets to the UI?) in addition, it might be nice to log the exception here too, ideally with a full stack trace - since this endpoint is specifically for troubleshooting, giving as much error information as possible can be helpful.
you can use logger.exception - i think that may give you a stack trace, or you may need to still specify some args/options to get a full stack trace.
There was a problem hiding this comment.
logger exception here: 16d65db#diff-15895ed412c26a8f5c097a7eddfa11273da29a8d999e7c342bdeb475224fff8aR537 I do see the exception in logs if I induce one
| # Overrides SQLConnector.test_connection | ||
| def test_connection(self) -> Optional[ConnectionTestStatus]: |
There was a problem hiding this comment.
may be a question of preference, but i tend to put these sort of comments as proper method docstrings, and that plays nicely with many IDEs. i would note here that we're specifically testing with the native bigquery python client that's used for the d&d workflows, even though it is not yet used for the DSR workflows. something like this?
| # Overrides SQLConnector.test_connection | |
| def test_connection(self) -> Optional[ConnectionTestStatus]: | |
| def test_connection(self) -> Optional[ConnectionTestStatus]: | |
| """ | |
| Overrides SQLConnector.test_connection with a BigQuery-specific connection test. | |
| The connection is tested using the native python client for BigQuery, since that is what's used | |
| by the detection and discovery workflows/codepaths. We will eventually look to migrate the | |
| rest of this class, used for DSR execution, to also make use of the native bigquery client. | |
| """ |
There was a problem hiding this comment.
thingscouldbeworse
changed the title
There was a problem hiding this comment.
looks good @thingscouldbeworse, thanks for making the requested updates! 👍
|
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
Closes #PROD-2435
Description Of Changes
A more reliable connection test for bigquery
Code Changes
test_connectionfunction of Bigquery connectors to runlist_all_projectsand return failure on an empty return value.Steps to Confirm
raise,i =1/0, etc) inside the try/exceptall_projectsto[]and confirm the test failsPre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works