Skip to content

Conversation

@ChristoGrab
Copy link
Contributor

@ChristoGrab ChristoGrab commented May 20, 2024

What

Migrates the authenticators for sources Jira and Github from the deprecated auth package to the current requests_native_auth package. Batching these as the three connectors that did not complain after the update.

User Impact

No impact

Can this PR be safely reverted and rolled back?

  • [✓ ] YES 💚
  • NO ❌
@vercel
Copy link

vercel bot commented May 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 22, 2024 2:57pm
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 20, 2024
@ChristoGrab ChristoGrab marked this pull request as ready for review May 20, 2024 13:45
@aldogonzalez8
Copy link
Contributor

This looks good to me, how did you test authentication changes for these 3 sources?

@ChristoGrab
Copy link
Contributor Author

ChristoGrab commented May 20, 2024

This looks good to me, how did you test authentication changes for these 3 sources?

@aldogonzalez8 Glad you asked! I was going to say that for a small change like this it felt like successfully running the read interface on each connector locally and having a green test pipeline is enough. However, I'm realizing that for at least Amazon Ads there are streams we cannot populate in our sandbox account which include additional authentication steps that could cause issues that our pipeline won't catch.

I'll remove Amazon Ads from this PR for now, but I think Jira and Github should still be safe since they use the same authentication logic consistently across all streams and we are able to fetch all the expected_records in our sandbox accounts.

@ChristoGrab ChristoGrab changed the title Sources Amazon Ads, Jira, Github: update CDK authenticator Sources Jira, Github: update CDK authenticator May 21, 2024
Copy link
Contributor

@aldogonzalez8 aldogonzalez8 left a comment

Choose a reason for hiding this comment

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

APPROVED

@aldogonzalez8
Copy link
Contributor

Not sure if this is important but it shows two failing jobs and "Squash and merge" button looks enabled

image

@ChristoGrab ChristoGrab merged commit 77e42a7 into master May 22, 2024
@ChristoGrab ChristoGrab deleted the christo/update-certified-authenticators-1 branch May 22, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment