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.

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

feat: Support optional headers for OAuth request #1815

Conversation

s7clarke10
Copy link
Contributor

@s7clarke10 s7clarke10 commented Jul 7, 2023

Adding support for adding a header to an OAuth request for tokens. In a few instances an API has a requirement to have headers added to the request for an OAuth Token. An example of the type of header is a subscription key.

This extends the current OAuth Authenticator to accept a new optional parameter oauth_headers.

The PR also explicitly converts the returned expires_in value to int rather than assuming it is an integer. I had an example where it was returned as an string from this API.


📚 Documentation preview 📚: https://meltano-sdk--1815.org.readthedocs.build/en/1815/

@s7clarke10 s7clarke10 changed the title Supporting optional headers for oauth. feat: Supporting optional headers for oauth. Jul 7, 2023
Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

Thanks @s7clarke10! A few suggestions to simplify things a bit, let me know what you think :)

singer_sdk/authenticators.py Outdated Show resolved Hide resolved
singer_sdk/authenticators.py Outdated Show resolved Hide resolved
singer_sdk/authenticators.py Outdated Show resolved Hide resolved
@edgarrmondragon edgarrmondragon self-assigned this Jul 7, 2023
@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Merging #1815 (067a7e0) into main (b323d45) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 067a7e0 differs from pull request most recent head 260d141. Consider uploading reports for the commit 260d141 to get more accurate results

@@           Coverage Diff           @@
##             main    #1815   +/-   ##
=======================================
  Coverage   86.46%   86.47%           
=======================================
  Files          59       59           
  Lines        4996     4998    +2     
  Branches      816      816           
=======================================
+ Hits         4320     4322    +2     
  Misses        479      479           
  Partials      197      197           
Impacted Files Coverage Δ
singer_sdk/authenticators.py 64.02% <100.00%> (+0.38%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@s7clarke10
Copy link
Contributor Author

This PR has been tested on my enhancements to tap-rest-api-msdk. This is the branch that I have used for my testing https://github.com/s7clarke10/tap-rest-api-msdk/commits/feature/sdk_supported_oauth_headers .

Thanks for your support for this change and great suggestions to resolve the failed tests. I believe all the comments can be resolved now.

@s7clarke10
Copy link
Contributor Author

s7clarke10 commented Jul 10, 2023

fix: #1822

@edgarrmondragon edgarrmondragon changed the title feat: Supporting optional headers for oauth. feat: Supporting optional headers for OAuth request Jul 10, 2023
Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

Thanks @s7clarke10!

@edgarrmondragon edgarrmondragon changed the title feat: Supporting optional headers for OAuth request feat: Support optional headers for OAuth request Jul 10, 2023
@edgarrmondragon edgarrmondragon merged commit 1637b48 into meltano:main Jul 10, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants