-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
gRPC Connectors API #3245
gRPC Connectors API #3245
Conversation
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
hi @nabokihms, this time it should pass. I tried running that workflow in a separate branch. |
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.
My only concern regarding this PR is that we extend capabilities for existing API servers. So users who can control oauth2 clients can now control connectors also.
I think that it won't affect a standard usage of Dex (at least from what I know about it), but I totally understand your concern. Wouldn't increasing the minor version be sufficient as a way to alert developers? After all, it's a backward-compatible change, although it depends on what is meant by "backward compatibility." |
Could adding an 'allowEditConnectors' (false if missing) in the grpc section of the config file be a solution? Connectors API functions would still be there, but would return an error when called when that config entry is false. |
Yeah, something like that will work. I assume it is better to make it the list of features instead of a boolean flag, e.g., In the future, we will be able to extend the API server with more features. |
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
6cde7e4
to
3ceb3b5
Compare
hi @nabokihms , is there any way I could help move the PR forward? I am available to correct any inaccuracies or to add any missing elements |
Signed-off-by: Maksim Nabokikh <[email protected]>
Signed-off-by: Giovanni Campeol <[email protected]>
@twoojoo, everything looks ok. I'm ready to merge this as soon as the test/lint problem is fixed. |
Signed-off-by: Giovanni Campeol <[email protected]>
a499fa1
to
abc5bd8
Compare
Overview
I added gRPC API support for connectors (CRUD operations)
What this PR does / why we need it
I think it could be useful to have the possibility of handling connectors dynamically without the need to restart Dex.
Special notes for your reviewer
Does this PR introduce a user-facing change?
It adds connectors functions and types to the gRPC client, but it shouldn't break anything.