Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: inject oauth params when generating sync input #22635

Merged
merged 8 commits into from
Feb 9, 2023
Merged

fix: inject oauth params when generating sync input #22635

merged 8 commits into from
Feb 9, 2023

Conversation

pedroslopez
Copy link
Contributor

@pedroslopez pedroslopez commented Feb 9, 2023 •
edited
Loading

What

When moving the configuration fetching to happen at the attempt level in #21629 we stopped injecting OAuth parameters into the configuration. This led to errors due to not having the right params during syncs.

How

  • Properly inject oauth prams.

@octavia-squidington-iii octavia-squidington-iii added area/platform issues related to the platform area/worker Related to worker labels Feb 9, 2023
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 09:28 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 09:28 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 09:32 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 09:33 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023 •
edited
Loading

Airbyte Code Coverage

File Coverage [86.68%] 🍏
GenerateInputActivityImpl.java 86.68% 🍏
Total Project Coverage 24.67% ❌

@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 09:36 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 09:36 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 16:57 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 16:57 — with GitHub Actions Inactive
@mfsiega-airbyte
Copy link
Contributor

(Left a comment along these lines in Slack, but for visibility)

What testing have we done to make sure this is safe?

@pedroslopez pedroslopez changed the title fix: inject oauth params when generating sync inpt fix: inject oauth params when generating sync input Feb 9, 2023
@pedroslopez
Copy link
Contributor Author

(Left a comment along these lines in Slack, but for visibility)

What testing have we done to make sure this is safe?

@mfsiega-airbyte I've added tests here to verify that we're injecting oauth params, but currently deploying this to dev-2 to check it out in cloud. Will merge once I confirm there.

@mfsiega-airbyte
Copy link
Contributor

@mfsiega-airbyte I've added tests here to verify that we're injecting oauth params, but currently deploying this to dev-2 to check it out in cloud. Will merge once I confirm there.

Did we not do this for the original PR?

@pedroslopez
Copy link
Contributor Author

pedroslopez commented Feb 9, 2023 •
edited
Loading

@mfsiega-airbyte I've added tests here to verify that we're injecting oauth params, but currently deploying this to dev-2 to check it out in cloud. Will merge once I confirm there.

Did we not do this for the original PR?

I did not test in cloud with an OAuth connector for the original PR.

@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 17:28 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 17:28 — with GitHub Actions Inactive
evantahler
evantahler reviewed Feb 9, 2023
View reviewed changes
...c/test/java/io/airbyte/workers/temporal/scheduling/activities/GenerateInputActivityTest.java
@@ -225,6 +239,8 @@ void testGetResetSyncWorkflowInput() throws IOException, ApiException {
.withDestinationConfiguration(DESTINATION_CONFIGURATION)
.withState(STATE);

verify(oAuthConfigSupplier).injectDestinationOAuthParameters(DESTINATION_DEFINITION_ID, WORKSPACE_ID, DESTINATION_CONFIGURATION);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could this test ensure that a secret was decrypted?

Copy link
Contributor Author

@pedroslopez pedroslopez Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test to actually check that we're using the config returned by the injection method in 716fe1c. Internals of how the injection works left up to unit tests for the OAuthConfigSupplier.

@pedroslopez pedroslopez enabled auto-merge (squash) February 9, 2023 18:26
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 18:26 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 9, 2023 18:26 — with GitHub Actions Inactive
@pedroslopez pedroslopez merged commit 44a85a9 into master Feb 9, 2023
@pedroslopez pedroslopez deleted the pedroslopez/fix-oauth branch February 9, 2023 19:00
marcosmarxm pushed a commit that referenced this pull request Feb 9, 2023
@pedroslopez @marcosmarxm
fix: inject oauth params when generating sync input (#22635)
cc0013c 
* fix: inject oauth params when generating sync inpt

* make it final

* rm oauth injection from job creator factory

* keep it

* keep it 2

* test that we're actually using the oauth values

* fmt
@github-actions github-actions bot mentioned this pull request Feb 9, 2023
Closed
This was referenced Feb 10, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evantahler evantahler evantahler approved these changes

@alafanechere alafanechere alafanechere approved these changes

@benmoriceau benmoriceau Awaiting requested review from benmoriceau

@mfsiega-airbyte mfsiega-airbyte Awaiting requested review from mfsiega-airbyte

Assignees
No one assigned
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pedroslopez @mfsiega-airbyte @evantahler @alafanechere @octavia-squidington-iii