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

Add credential provider support #253

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

trentbitterman
Copy link
Contributor

Adds an abstract credential provider class that can be used to programmatically supply credentials at
authentication time, instead of just a static username and password.

Adds an abstract credential provider class that can
be used to programmatically supply credentials at
authentication time, instead of just a static username
and password.
@trentbitterman
Copy link
Contributor Author

Closes #246

@trentbitterman trentbitterman marked this pull request as ready for review December 2, 2024 21:07
coredis/credentials.py Outdated Show resolved Hide resolved
Copy link
Owner

@alisaifee alisaifee left a comment

Choose a reason for hiding this comment

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

Generally this looks great! Thank you for the comprehensive and very clean contribution!

I've left a few minor suggestions for your consideration. Additionally if you have an implementation for an IAM provider for example - perhaps this could be added to coredis.recipes and referenced in the documentation?

coredis/client/basic.py Outdated Show resolved Hide resolved
coredis/connection.py Outdated Show resolved Hide resolved
coredis/credentials.py Outdated Show resolved Hide resolved
coredis/credentials.py Outdated Show resolved Hide resolved
* Added an IAM Elasticache credential provider example
* Simplified some of the Credential provider implementation and usage
@trentbitterman
Copy link
Contributor Author

@alisaifee Thanks for the review! Let me know if there are any other changes you'd like me to make.

@alisaifee
Copy link
Owner

@alisaifee Thanks for the review! Let me know if there are any other changes you'd like me to make.

Wow, amazing turnaround to address the suggestion! Thank you! Looks great to me. I wasn't able to verify the rendering of the docs since I've broken my local sphinx environment but that's a minor detail we can iron out in master.

Regarding the typing issues in CI with respect to boto - feel free to add it to the ignore list.

@trentbitterman
Copy link
Contributor Author

@alisaifee Hopefully that's the correct place to ignore them. Let me know if it's not.

pyproject.toml Outdated Show resolved Hide resolved
@alisaifee
Copy link
Owner

@alisaifee Hopefully that's the correct place to ignore them. Let me know if it's not.

It is indeed. Needed a few tweaks to get past mypy. Since it's a recipe it's fine to be a bit relaxed on the type strictness IMO.

Co-authored-by: Ali-Akber Saifee <ali@indydevs.org>
@alisaifee alisaifee self-assigned this Dec 3, 2024
@alisaifee alisaifee added the enhancement New feature or request label Dec 3, 2024
@alisaifee
Copy link
Owner

@trentbitterman unless you have anything else to add, I'll probably merge this once CI is done.

@trentbitterman
Copy link
Contributor Author

That works for me, I didn’t have any other changes planned. Thanks!

@alisaifee alisaifee merged commit 313764b into alisaifee:master Dec 4, 2024
55 checks passed
alisaifee pushed a commit that referenced this pull request Dec 6, 2024
Adds an abstract credential provider class that can
be used to programmatically supply credentials at
authentication time, instead of just a static username
and password.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants