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 reflection to grpc api #1512

Merged
merged 2 commits into from
Aug 7, 2019
Merged

Add reflection to grpc api #1512

merged 2 commits into from
Aug 7, 2019

Conversation

venezia
Copy link
Contributor

@venezia venezia commented Aug 6, 2019

This will add reflection as an option for the grpc api.
It also makes a small correction in the documentation - dex will not create self signed certificates if certs are not provided, rather it runs in plaintext mode.

Does not fix, but this work was discussed in #1499

@venezia
Copy link
Contributor Author

venezia commented Aug 6, 2019

travis wasn't able to connect to github during the test - unsure how to get travis to try again

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this! 😃 Would you mind squashing this into two commits -- one for the code, one for the revendor? That would also trigger travis again, I hope 🤞

cmd/dex/serve.go Outdated
@@ -6,6 +6,7 @@ import (
"crypto/x509"
"errors"
"fmt"
"google.golang.org/grpc/reflection"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put that into a section with the other 3rd-party imports :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I IDE did that automatically for me and I wasn't paying attention - sure

@venezia
Copy link
Contributor Author

venezia commented Aug 7, 2019

LGTM, thanks for this! 😃 Would you mind squashing this into two commits -- one for the code, one for the revendor? That would also trigger travis again, I hope 🤞

Sure, but just know I was only following dex's guidelines

That being said do you want, in the future, for commits involving dependencies to be in the same commit as regular code changes?

@srenatus
Copy link
Contributor

srenatus commented Aug 7, 2019

@venezia thanks for pointing out those guidelines. I had forgotten about that. They're quite agreeable -- at any rate, could you amend your commits to satisfy them? 😉
Like

  1. cmd/dex: make reflection configurable
  2. vendor: revendor

and the third commit can be squashed into the first one?

@srenatus
Copy link
Contributor

srenatus commented Aug 7, 2019

If you want to split commits more (while adhering to the guidelines), please do so -- I'm sorry for misleading you here 😉

@venezia
Copy link
Contributor Author

venezia commented Aug 7, 2019

If you want to split commits more (while adhering to the guidelines), please do so -- I'm sorry for misleading you here 😉

no, this is what happens when I misread a comment while still waking up. I think here one commit is just fine, sometimes if it is bigger maybe more than one commit is better (if they're doing separate things, etc.) but that's for another day.

But specifically, the small correction to the documentation regarding plaintext mode - its part of this "add reflection to grpc api" commit, but it really has nothing to do with adding reflection to grpc - it should be there regardless because it fixes something else that is incorrect.

Regardless, things should be squashed and commented better now.

@srenatus srenatus merged commit d9f6ab4 into dexidp:master Aug 7, 2019
@venezia venezia deleted the add_reflection branch August 7, 2019 11:59
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
Add reflection to gRPC API (configurable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants