-
Notifications
You must be signed in to change notification settings - Fork 214
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
fix: avoid overriding user config if possible #236
Conversation
@wingyplus could you review this please? I still need to write tests, but just check if the idea makes sense |
I think it is good to go. |
{:dialyxir, "~> 1.1", only: [:dev, :test], runtime: false} | ||
{:ex_doc, "~> 0.28.0", only: :dev}, | ||
{:inch_ex, "~> 2.0.0", only: [:dev, :test]}, | ||
{:dialyxir, "~> 1.1.0", only: [:dev, :test], runtime: false} |
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.
Why we change the version constraint?
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.
These are only for the lib itself (only prod deps are propagated to the client libs).
Therefore, I made this more strict because I always prefer to be as strict as possible with dependencies
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.
👍
else | ||
Map.put(open_opts, :transport_opts, transport_opts) | ||
end | ||
tls_opts = Keyword.merge(@default_transport_opts ++ ssl, transport_opts) |
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.
Just quick check from mobile.
which ssl configuration (via cred vs via transport_opts) is higher priority?
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.
Via transport options
There's a test that shows that
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.
Do we need to document GRPC.Stub.connect about this behaviour? Because the library encourage user to use GRPC.Credential to set ssl credential but now user can set it via adapter option when calling GRPC.Stub.connect.
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 don't think so. This is a per-adapter specification
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 decide to not let some overrides take place, though.
Although I wouldn't invest too much energy in the Gun adapter since we're planning on moving on to Mint
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 prefer the way it's currently coded, in which the opts
values have precedence. Could you point to where in the docs it encourages the SSL values to be used? I don't think we'll have to change those other than adding a "opts can override this" warning
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.
Here it is https://github.com/elixir-grpc/grpc/blob/master/lib/grpc/credential.ex#L3. Maybe it is only my understandable. 🙇♂️
Agree that if we can warning the opts can be override.
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'll improve on that :)
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.
Done :) I'm merging this, but let me know if things are still unclear
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.
That’s totally clear :)
Overall, LGTM. :) |
closes #176