-
Notifications
You must be signed in to change notification settings - Fork 128
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
Updated endpoint parsing #618
Updated endpoint parsing #618
Conversation
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
dapr/conf/helpers.py
Outdated
scheme = self.get_scheme() | ||
port = "" if len(self.get_port()) == 0 else f":{self.port}" | ||
|
||
if scheme == "unix": |
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.
Is there a warning if the user uses the deprecated "https" or "http" schemas?
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.
Signed-off-by: Elena Kolevska <[email protected]>
…gRPC endpoints Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #618 +/- ##
==========================================
- Coverage 86.40% 86.13% -0.28%
==========================================
Files 74 74
Lines 3605 3700 +95
==========================================
+ Hits 3115 3187 +72
- Misses 490 513 +23
☔ View full report in Codecov by Sentry. |
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
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.
Please create a follow-up PR to remove these orphaned comments.
self.assertEqual(testcase["scheme"], o[0]) | ||
self.assertEqual(testcase["host"], o[1]) | ||
self.assertEqual(testcase["port"], o[2]) | ||
# if testcase["error"]: |
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.
Can you remove the commented out lines please if these are not necessary?
Thanks @berndverst. Good catch, I was using those prints to generate the matrix in this proposal. |
DEFAULT_HOSTNAME = "localhost" | ||
DEFAULT_PORT = 443 | ||
DEFAULT_AUTHORITY = "" | ||
ACCEPTED_SCHEMES = ["dns", "unix", "unix-abstract", "vsock", "http", "https", "grpc", "grpcs"] |
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.
cc @elena-kolevska
something's off here, both grpc
and grpcs
are not present in the naming resolution doc and are also missing in the go-sdk: https://github.com/dapr/go-sdk/blob/main/client/internal/parse.go#L160
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.
Fixed in this PR: #700
Description
This PR updates the python-sdk so it satisfies the requirements for gRPC endpoint parsing added in dapr/proposals#40.
It also adds support for all URIs that the gRPC name resolution document defines, including dns links with authority, unix paths with absolute and relative sockets, unix-abstract and virtual sockets.
Backwards compatibility has been maintained for endpoints where tls was inferred based on the scheme
https
.Issue reference
#619
Checklist