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

DOC-4815 added candidate AMR connection page #1136

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andy-stark-redis
Copy link
Contributor

@andy-stark-redis andy-stark-redis commented Feb 6, 2025

DOC-4815

Note: this isn't anywhere near final yet - it's just a suggestion for a possible format for the AMR client docs. A few things to consider:

  • AMR seems significant enough to deserve its own page, but we could put the info in the existing Connect page instead if that works better.
  • I assume we need code examples for both service principals and managed identities. I'm using the example from the Github repo for service principals, but I've just guessed some placeholder content for the managed identity example.
  • I'm thinking I should also add a credential providers section to the main Connect page, just to give a general overview of them. If it would be useful to supply other cred provider examples (eg, like the ones in the external redis-py docs site) then maybe we could separate this out into a doc subfolder in the future.
  • Do we need more detail about how the user actually gets the client/tenant ID etc from Azure? Maybe just point them to a Microsoft page if that's enough?
  • I figured that for the doc page, it would be better to have a realistic example of TLS connection rather than suggest an insecure test connection. Again, is it obvious to a user how to set up the certificates with Azure or should we explain it or point them somewhere?

Any other feedback, suggestion, etc, are very welcome :-)

Copy link
Contributor

github-actions bot commented Feb 6, 2025

@andy-stark-redis andy-stark-redis requested review from uglide and a team February 6, 2025 11:36
from redis_entraid.cred_provider import *

credential_provider = create_from_managed_identity(
identity_type=ManagedIdentityType.USER_ASSIGNED,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use ManagedIdentityType.SYSTEM_ASSIGNED as an example. System-assigned method requires no additional inputs, however User-assigned requires at least id_type and id_type argument, so System-assigned is better for the most basic use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@andy-stark-redis andy-stark-redis marked this pull request as ready for review February 6, 2025 13:08
Copy link
Contributor

@cmilesb cmilesb left a comment

Choose a reason for hiding this comment

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

Looks good to me so far.

@andy-stark-redis
Copy link
Contributor Author

Thanks @cmilesb and @vladvildanov !

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.

3 participants