Skip to content

DOC-4990 AMR connection page for Go #1605

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

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

Conversation

andy-stark-redis
Copy link
Contributor

@andy-stark-redis andy-stark-redis commented May 22, 2025

DOC-4990

This (hopefully) gives Go about the same level of detail as the Jedis AMR docs.

All feedback is welcome, but a few things to note:

  • The other clients' docs/API refer to service principals explicitly, but the Go README doesn't. I'm assuming this is covered by client secret authentication, but let me know if this is not the case.
  • I've given the values for some of the constants (eg, AuthorityTypeMultiTenant = "multi-tenant") but let me know if it's better not to do this.
  • The Go examples roughly match the equivalent examples for Jedis, but let me know if there is anything extra that is specifically worth mentioning for Go.

@andy-stark-redis andy-stark-redis requested a review from a team May 22, 2025 10:52
@andy-stark-redis andy-stark-redis self-assigned this May 22, 2025
@andy-stark-redis andy-stark-redis added the clients Client library docs label May 22, 2025
Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

LGTM.

@andy-stark-redis
Copy link
Contributor Author

Thanks @dwdougherty !

Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

@andy-stark-redis I am leaving some suggestions as a first review of this page. Ping me to do a final one when you review the suggestions.

Comment on lines 208 to 214
clientID := os.Getenv("AZURE_CLIENT_ID")
redisEndpoint := os.Getenv("REDIS_ENDPOINT")
if clientID == "" || redisEndpoint == "" {
log.Fatal(
"AZURE_CLIENT_ID and REDIS_ENDPOINT env variables are required"
)
}
Copy link
Member

Choose a reason for hiding this comment

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

client id is not needed for system assigned identity provider

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 and others added 2 commits May 30, 2025 09:58
Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
@andy-stark-redis
Copy link
Contributor Author

@ndyakov Thanks for the review. I've implemented your suggestions - please let me know if there's anything else that needs to be changed.

Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, one examples should be fixed and I do think the options example can benefit from additional code to show where the options should be plugged.

.
.
provider, err := entraid.NewConfidentialCredentialsProvider(
entraid.ConfidentialIdentityProviderOptions{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entraid.ConfidentialIdentityProviderOptions{
entraid.ConfidentialCredentialsProviderOptions{

Copy link
Member

Choose a reason for hiding this comment

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

My bad, probably mistyped this in the last review.

Comment on lines +151 to +154
},
}
```

Copy link
Member

Choose a reason for hiding this comment

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

@andy-stark-redis do you think it will be beneficial to have a complete example on where those options are supposed to be passed?

For example, just after initiating this options variable, have something like :

provider, err := entraid.NewManagedIdentityCredentialsProvider(
	entraid.ManagedIdentityCredentialsProviderOptions{
	    CredentialsProviderOptions: options,
		ManagedIdentityProviderOptions: identity.ManagedIdentityProviderOptions{
			ManagedIdentityType:  identity.UserAssignedObjectID,
			UserAssignedObjectID: "<your-user-assigned-client-id>",
		},
	},
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients Client library docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants