Move DAPR GRPC port inside client #54
Conversation
|
@yaron2 is there anything holding this back? |
|
or is @shubham1172 able to approve? |
|
@yaron2 I see you merged another PR recently. Are you able to take a look at this at the same time? |
| @@ -17,7 +17,11 @@ impl<T: DaprInterface> Client<T> { | |||
| /// | |||
| /// * `addr` - Address of gRPC server to connect to. | |||
| pub async fn connect(addr: String) -> Result<Self, Error> { | |||
There was a problem hiding this comment.
Can we also support port: Option<String> that overrides the GRPC port environment variable?
Also, if the environment variable does not exist, it can fallback to a standard default, like 50051.
| @@ -17,7 +17,11 @@ impl<T: DaprInterface> Client<T> { | |||
| /// | |||
| /// * `addr` - Address of gRPC server to connect to. | |||
| pub async fn connect(addr: String) -> Result<Self, Error> { | |||
| Ok(Client(T::connect(addr).await?)) | |||
| // Get the Dapr port to create a connection | |||
| let port: u16 = std::env::var("DAPR_GRPC_PORT")?.parse()?; | |||
There was a problem hiding this comment.
It's better to use match and handle the errors here explicitly (we can revert error.rs then). In case of error parsing the environment variable, we can log and fallback to a standard default.
|
@dwhiteddsoft are you still interested in making the suggested changes? |
Yes, those changes can be a follow up IMO. EDIT: Just realized there are no maintainers/approvers yet so maybe @yaron2/someone from STC can merge it. |
|
Just got back from being out. It was so long ago, I thought it wasn't maintained any longer. If I can help in any way, please let me know |
|
@dwhiteddsoft Sorry it's been so long since this has been picked up, would you mind if I resolve conflicts on your behalf and get this merged? I'm in agreement that the comments can be picked up at a later date |
Sure. If there is anything I can or should do please let me know |
Signed-off-by: Mike Nguyen <[email protected]>
|
@dwhiteddsoft sorry - just one thing I'll need your help with please, looks like the DCO is failing as the commit wasn't signed off: |
ack - will work on tonight |
I have done as the docs asked. I have never done this particular step before so I hope I did it correctly. |
|
I can't see a force push to the branch, could you please try again? |
Signed-off-by: David White <[email protected]>
dwhiteddsoft
force-pushed
the
dwhiteddsoft/port-hiding
branch
from
06eb70e to
982c467
Compare
I think I did it correctly. You can reach out to me directly if a call would be easier |
https://github.com/dapr/rust-sdk/pull/54/checks?check_run_id=23111766425 This run has a set of instructions (slightly different) to the contributor docs. Give this a shot as it should work too, if the next force push doesn't work feel free to shoot me an email to the one on my profile 👍 |
dwhiteddsoft
force-pushed
the
dwhiteddsoft/port-hiding
branch
from
1e9ac86 to
982c467
Compare
|
I'll set to pass manually 👍 |
sorry |
Signed-off-by: mikeee <[email protected]>
Signed-off-by: mikeee <[email protected]>
Signed-off-by: mikeee <[email protected]>
Signed-off-by: mikeee <[email protected]>
Signed-off-by: mikeee <[email protected]>
Other DAPR SDKs "hide" the obtaining of the GRPC port from the user of client. This change is simply made in that spirit. Changes were: