🎉 Destination Snowflake + Normalization Core: Added OAuth support #11093
Conversation
…rt-oauth # Conflicts: # airbyte-integrations/connectors/destination-snowflake/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDatabase.java # airbyte-integrations/connectors/destination-snowflake/src/test-integration/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationIntegrationTest.java
Co-authored-by: timroes <timroes@users.noreply.github.com>
* fix(shopify): wrong type for tax_exemptions abandoned_checkouts customer tax_exemptions had the wrong field type * fix(shopify): wrong type for tax_exemptions abandoned_checkouts customer tax_exemptions had the wrong field type * bump connector version Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
…an 50 (#10588) * fix execute_in_batch * add tests * fix pre-commit config Co-authored-by: Sherif A. Nada <snadalive@gmail.com> Co-authored-by: Eugene Kulak <kulak.eugene@gmail.com> Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
Move the feature flag checks to the handler instead of the configuration API. This could have avoid some bug related to the missing flag check in the cloud project.
This is changing the check to see if a connection exist in order to make it more performant and more accurate. It makes sure that the workflow is reachable by trying to query it.
* add timeout to init container command * add disk usage check into init command * fix up disk usage checking and logs from init entrypoint * run format
* test time ranges for cancellations * try with wait * fix cancellation on worker restart * revert for CI testing that the test fails without the retry policy * revert testing change * matrix test the different possible cases * re-enable new retry policy * switch to no_retry * switch back to new retry * paramaterize correctly * revert to no-retry * re-enable new retry policy * speed up test + fixees * significantly speed up test * fix ordering * use multiple task queues in connection manager test * use versioning for task queue change * remove sync workflow registration for the connection manager queue * use more specific example * respond to parker's comments
* cast timestamp to date * change test name * fix corner cases * fix corner cases 2 * format code * changed method name * add return typing * bump version * updated spec and def yaml Co-authored-by: auganbay <auganenu@gmail.com>
as helm templates integers as float64, when using %d, it renders the value of external airbyte.minio.endpoint to "S3_MINIO_ENDPOINT: "http://minio-service:%!d(float64=9000)", therefore needed to be changed to %g
* Add custom survey_ids * bump version * Update survey_question schema * Add changelog * Allow null objects * merge master and format * Make all types safe with NULL and add survey_ids to all streams * Make additional types safe with NULL * Make additional types safe with NULL * One last safe NULL type * small fixes * solve conflic * small fixes * revert fb wrong commit * small fb correction * bump connector version Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
…om PR 9027) (#10631) * Added associations to some CRM Object streams in Hubspot connector * Added associations in the relevant schemas * fix eof * bump connector version Co-authored-by: ksoenandar <kevin.soenandar@gmail.com>
* added transactions model * changes * fix * few changes * fix * added new stream in configured_catalog*.json * changes * removed new stream in configured_catalog*.json * solve small schema issues * add eof * bump connector version Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com>
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
|
/publish connector=bases/base-normalization |
2 similar comments
|
/publish connector=bases/base-normalization |
|
/publish connector=bases/base-normalization |
|
/publish connector=bases/base-normalization
|
|
/publish connector=bases/base-normalization |
|
/publish connector=bases/base-normalization |
|
/publish connector=bases/base-normalization
|
| "password": { | ||
| "description": "The password associated with the username.", | ||
| "type": "string", | ||
| "airbyte_secret": true, | ||
| "title": "Password", |
There was a problem hiding this comment.
Does removing this field make the new connector version non-backward compatible with older configurations without migrating the old configs? (or update them?)
Can we tolerate config with the old password field being at this location in the spec? (making sure with a test too?)
There was a problem hiding this comment.
Hi @ChristopheDuong.
Nope, we have AdditionalProperties is set to true.
https://github.com/airbytehq/airbyte/pull/11093/files#diff-e1acb96aaf759d23333bec115cba9c1d1af77282bdd7dcdc1346240f7f871d01L3858
So it's not explicitly described in spec, but we handle this in java code:
https://github.com/airbytehq/airbyte/pull/11093/files#diff-0fd1ff4a11af6edb89860081578c13598aa6cdb289c3e6ceae2a9d44eb708b80R95
` if (credentials != null && credentials.has("auth_type") && "OAuth2.0".equals(
credentials.get("auth_type").asText())) {
LOGGER.debug("OAuth login mode is used");
...
...
} else if (credentials != null && credentials.has("password")) {
LOGGER.debug("User/password login mode is used");
// Username and pass login option is selected on UI
dataSource.setUsername(username);
dataSource.setPassword(credentials.get("password").asText());
} else {
LOGGER.warn(
"Obsolete User/password login mode is used. Please re-create a connection to use the latest connector's version");
// case to keep the backward compatibility
dataSource.setUsername(username);
dataSource.setPassword(config.get("password").asText());
}`
and in normalization side:
https://github.com/airbytehq/airbyte/pull/11093/files#diff-76b45d3c5541ea23ab17d448c349c37f1a4d12bf1a604cfa7cceedb130588facR236
if credentials.get("auth_type") == "OAuth2.0":
dbt_config["authenticator"] = "oauth"
dbt_config["oauth_client_id"] = credentials["client_id"]
dbt_config["oauth_client_secret"] = credentials["client_secret"]
dbt_config["token"] = credentials["refresh_token"]
elif credentials.get("password"):
dbt_config["password"] = credentials["password"]
else:
dbt_config["password"] = config["password"]
return dbt_config
I also added a test to check it:
https://github.com/airbytehq/airbyte/pull/11093/files#diff-aa693e6356d71e05c008807f89cc10f5f2bfb495c14086472892cd08ca02436eR175
There was a problem hiding this comment.
Ok, this sounds good!
I just arrived on this PR because I couldn't figure out why the snowflake tests were failing on other branches... it might have been nice to keep the old password field in the secret of the CI, so other branches without your changes wouldn't start failing though
| @@ -169,6 +171,17 @@ protected void tearDown(final TestDestinationEnv testEnv) throws Exception { | |||
| database.close(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testBackwardCompatibilityAfterAddingOauth() { | |||
| // -------------------------------------------- | ||
|
|
||
| private String extractAuthorizeUrl(JsonNode inputOAuthConfiguration) { | ||
| // var url = inputOAuthConfiguration.get("authorize_url"); |
There was a problem hiding this comment.
is that needed?
| // var url = inputOAuthConfiguration.get("authorize_url"); |
| props.getProperty("refresh_token")); | ||
| props.setProperty("token", token); | ||
|
|
||
| LOGGER.info("New refresh token has been obtained"); |
There was a problem hiding this comment.
the new refresh token is never persisted back with the config though? does it need to be propagated all the way back to the airbyte DB?
|
/publish connector=connectors/destination-snowflake
|
etsybaev
deleted the
grubberr/11092-normalization-snowflake-support-oauth
branch
What
Describe what the change is solving
It helps to add screenshots if it affects the frontend.
TODO:
** Release Normalization (NormalizationRunnerFactory and where else?)**
How
Describe the solution
Tested with local deployment:
Recommended reading order
x.javay.python🚨 User Impact 🚨
No breaking changes expected for existing connections, but eventually users will have to re-create connection to use new approach for either user\pass or oauth mode
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/SUMMARY.mddocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampledocs/integrations/README.mdairbyte-integrations/builds.mdAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereUpdating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.