-
Notifications
You must be signed in to change notification settings - Fork 915
Added example with oauth/oidc example with CCloud logicalCluster and identityPoolId #1593
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
base: master
Are you sure you want to change the base?
Conversation
This example is more specific to Confluent Cloud. Can we move it to a new file? |
Just make sure that this example is tested. |
@@ -131,5 +124,8 @@ def main(args): | |||
help="Token URL.") | |||
parser.add_argument('--scopes', dest="scopes", required=True, nargs='+', | |||
help="Scopes requested from OAuth server.") | |||
|
|||
parser.add_argument('--logical-cluster', dest="logical_cluster", required=False, help="Logical Cluster.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move this to CC specific example, does it make sense to mark these are required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original example is using an outdated callback based method so this example applies there too. Also, since the only difference between CC and AK/CP is this one param, it doesn't make sense to split it into another file. There's a wider task of updating all the examples to be using the recommended approach.
The purpose of examples is to educate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can improve old example as well and create a new example to cater mainly CC usecase. The idea is to make it easy for non CC user to use SASL as well. With everything specific to CC, user might get confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just specific to CC though. If you don't include the logicalClusterId and identityPoolId in the cli params, it's just just works against any AK or CP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't want code that'll be copy pasted for a 2 line difference. The cli works with both CP and CC.
the example was tested against confluent cloud |
@pranavrth there seems to be two issues here:
|
|
@homestealth the ccloud example is here in my PR with two additional parameters that does the trick and since this is a cli example I've added optional parameters to the cli which make it work. It seems overkill to have a separate file for the exact same code to be copied. |
I think it is fine to keep only one file but we should add a comment stating the two new configs are for CC case only. Please also sign the CLA. |
@emasab - Any comment from your side before we can merge this? |
This is a valuable example, since it is config-based and removes the callback. How can we close this out? |
Hi everyone, any plans to have this uploaded in some capacity? Ran into this issue today and this example would have been very helpful. |
@sarwarbhuiyan - Can you please address my last comment to add some documentation? Please sign the CLA as well. We should merge this PR. |
Hi team, two more customers have reported issues with our current example. GTS has been finding this pull and giving the customer the example here, any way we can move this to live? |
The following example shows how to configure the producer with OAUTH. For Confluent Cloud where we need to pass extensions for "logicalCluster" and "identityPoolId" we need to pass some additional paramters.