feat: Support for the `DAPR_HTTP_ENDPOINT` and `DAPR_GRPC_ENDPOINT` environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

elena-kolevska · 2023-09-12T23:02:02Z

Description

Adds support for DAPR_API_TOKEN to gRPC client
Adds support for the DAPR_GRPC_ENDPOINT environment variable
Adds support for the DAPR_HTTP_ENDPOINT environment variable

Issue reference

Issue this PR will close: #502 and #520

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

Signed-off-by: Elena Kolevska <[email protected]>

…ls-support

Signed-off-by: Elena Kolevska <[email protected]>

codecov · 2023-09-19T06:08:04Z

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (553affa) 35.23% compared to head (3b27c23) 35.34%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
+ Coverage   35.23%   35.34%   +0.11%     
==========================================
  Files          90       90              
  Lines       10143    10187      +44     
  Branches      415      429      +14     
==========================================
+ Hits         3574     3601      +27     
- Misses       6503     6516      +13     
- Partials       66       70       +4

Files	Coverage Δ
src/utils/Settings.util.ts	`70.58% <100.00%> (+17.25%)`	⬆️
...mplementation/Client/GRPCClient/GRPCClientProxy.ts	`15.00% <0.00%> (-0.79%)`	⬇️
src/implementation/Client/GRPCClient/GRPCClient.ts	`76.78% <80.00%> (+0.04%)`	⬆️
src/utils/Client.util.ts	`43.26% <26.47%> (-2.89%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Elena Kolevska <[email protected]>

src/utils/Client.util.ts

test/unit/grpc/GRPCClient.test.ts

shubham1172 · 2023-09-20T08:10:37Z

tsconfig.json

@@ -12,7 +12,7 @@
    // "jsx": "preserve",                           /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
    "declaration": true /* Generates corresponding '.d.ts' file. */,
    // "declarationMap": true,                      /* Generates a sourcemap for each corresponding '.d.ts' file. */
-    // "sourceMap": true,                           /* Generates corresponding '.map' file. */
+    "sourceMap": true /* Generates corresponding '.map' file. */,


Do we need this?

@shubham1172 I enabled this so I could debug the code. I'm not a typescript person though, so if you think it's better not to have it in a release, that's totally ok for me, I'll just enable it for my local development.

Since we use the same tsconfig for dev and prod, the production build size will also increase. We could add support to conditionally generate sourceMap for development only, but for now we should turn this off.

Has this been resolved meanwhile?

Co-authored-by: Shubham Sharma <[email protected]> Signed-off-by: Elena Kolevska <[email protected]>

Signed-off-by: Elena Kolevska <[email protected]>

test/unit/utils/Client.util.test.ts

shubham1172

@elena-kolevska LGTM overall! I wish this was two PRs instead of one but it's ok for now since we are very close to release and both of them are affecting the DaprClient.

Great work with adding the test to check for Dapr API token and fixing the proxy to include interceptors from gRPC client!

src/implementation/Client/GRPCClient/GRPCClient.ts

src/utils/Client.util.ts

src/utils/Settings.util.ts

XavierGeerinck · 2023-09-25T06:11:18Z

tsconfig.json

@@ -12,7 +12,7 @@
    // "jsx": "preserve",                           /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
    "declaration": true /* Generates corresponding '.d.ts' file. */,
    // "declarationMap": true,                      /* Generates a sourcemap for each corresponding '.d.ts' file. */
-    // "sourceMap": true,                           /* Generates corresponding '.map' file. */
+    "sourceMap": true /* Generates corresponding '.map' file. */,


Has this been resolved meanwhile?

Signed-off-by: Elena Kolevska <[email protected]>

elena-kolevska · 2023-09-25T09:42:41Z

@XavierGeerinck and @shubham1172 Thank you both for the review! I updated the PR and commented on some of your questions.

…he docs Signed-off-by: Elena Kolevska <[email protected]>

Signed-off-by: Elena Kolevska <[email protected]>

shubham1172 · 2023-09-27T05:09:58Z

daprdocs/content/en/js-sdk-docs/js-client/_index.md

+// Using HTTP, when DAPR_HTTP_ENDPOINT is set
+const client = new DaprClient({ daprHost, daprPort });
+
+// Using gRPC, when DAPR_GRPC_ENDPOINT is set
+const client = new DaprClient({ communicationProtocol: CommunicationProtocol.GRPC });


Are we showing here that this is the equivalent of setting the environment variable?

Yes, this is how we can instantiate the client when we have DAPR_GRPC_ENDPOINT set. I noticed I had a copy/paste error for the HTTP example above though, so I fixed that. Basically, we don't need to pass constructor arguments for host and port when we have the env variables set.

I wonder if DAPR_GRPC_ENDPOINT is set, should we also by-default use communication protocol GRPC. Also while instantiating the DaprClient, should we clearly log if we used env var to figure out host/port/protocol.

Good point! I can add a log.
Regarding the automatic setting of the protocol, I think we can't do it, because, as per the proposal we can have both DAPR_GRPC_ENDPOINT and DAPR_HTTP_ENDPOINT set at the same time.

DAPR_GRPC_ENDPOINT and DAPR_HTTP_ENDPOINT can be set at the same time since some SDKs (Java, as of now) supports both protocols at the same time and app can pick which one to use.

Right (and maybe this can be added to the proposal to unify SDK implementations), so if

DAPR_GRPC_ENDPOINT is set, set protocol to gRPC

DAPR_HTTP_ENDPOINT is set, set protocol to HTTP

Both are set, use HTTP

Finally, if function param is specified, that overrides everything

No function param or env var also means HTTP

PS, we can track this separately as well.

shubham1172

LGTM! Thanks for updating the parseEndpoint implementation, much better to delegate the parsing!

Signed-off-by: Elena Kolevska <[email protected]>

XavierGeerinck · 2023-09-27T12:54:10Z

Thank you so much! Running the tests now and if they pass we can merge :) thanks again for sticking with us and fixing the URL lib usage

elena-kolevska added 5 commits September 7, 2023 11:37

feat: Adds endpoint parsing

aaf074c

Signed-off-by: Elena Kolevska <[email protected]>

Adds support for the DAPR_HTTP_ENDPOINT environment variable

49ab722

Signed-off-by: Elena Kolevska <[email protected]>

Adds tests for endpoint environment variables (HTTP only)

0aaa913

Signed-off-by: Elena Kolevska <[email protected]>

test: Adds tests for endpoint environment variables (HTTP only)

6fd90c5

Signed-off-by: Elena Kolevska <[email protected]>

Merge branch 'tls-support' of github.com:elena-kolevska/js-sdk into t…

8f26003

…ls-support

elena-kolevska changed the title ~~Support for the DAPR_HTTP_ENDPOINT environment variable~~ feat: Support for the DAPR_HTTP_ENDPOINT environment variable Sep 12, 2023

elena-kolevska marked this pull request as ready for review September 13, 2023 09:52

elena-kolevska requested review from a team as code owners September 13, 2023 09:52

elena-kolevska added 2 commits September 13, 2023 17:44

fix(style) Linter fixes

a29bc66

Signed-off-by: Elena Kolevska <[email protected]>

Adds support for dapr-api-token metadata

6d175a2

Signed-off-by: Elena Kolevska <[email protected]>

elena-kolevska marked this pull request as draft September 18, 2023 15:15

elena-kolevska changed the title ~~feat: Support for the DAPR_HTTP_ENDPOINT environment variable~~ feat: Support for the DAPR_HTTP_ENDPOINT and DAPR_GRPC_ENDPOINT environment variable Sep 18, 2023

elena-kolevska changed the title ~~feat: Support for the DAPR_HTTP_ENDPOINT and DAPR_GRPC_ENDPOINT environment variable~~ feat: Support for the DAPR_HTTP_ENDPOINT and DAPR_GRPC_ENDPOINT environment variables. Adds support for DAPR_API_TOKEN to gRPC client Sep 18, 2023

elena-kolevska added 2 commits September 18, 2023 16:58

Adds support for the DAPR_GRPC_ENDPOINT environment variable

d009132

Signed-off-by: Elena Kolevska <[email protected]>

Fixes linter issues

e63f270

Signed-off-by: Elena Kolevska <[email protected]>

elena-kolevska marked this pull request as ready for review September 18, 2023 16:03

elena-kolevska mentioned this pull request Sep 18, 2023

Add DAPR_API_TOKEN to gRPC client #520

Closed

elena-kolevska added 4 commits September 18, 2023 17:27

Fixes linter issues

0e5672d

Signed-off-by: Elena Kolevska <[email protected]>

Only add api token interceptor if it’s specified

dc482b9

Signed-off-by: Elena Kolevska <[email protected]>

Reorganises the code a bit

e0fd4a3

Signed-off-by: Elena Kolevska <[email protected]>

Runs pretty-fix

08407db

Signed-off-by: Elena Kolevska <[email protected]>

elena-kolevska mentioned this pull request Sep 19, 2023

REQUEST: New membership for elena-kolevska dapr/community#353

Closed

4 tasks

Adds test for scheme prefix removal for grpc

6e16458

Signed-off-by: Elena Kolevska <[email protected]>

shubham1172 reviewed Sep 20, 2023

View reviewed changes

elena-kolevska and others added 4 commits September 20, 2023 11:43

Apply suggestions from code review

2f1a277

Co-authored-by: Shubham Sharma <[email protected]> Signed-off-by: Elena Kolevska <[email protected]>

Apply suggestions from code review

959ea7b

Co-authored-by: Shubham Sharma <[email protected]> Signed-off-by: Elena Kolevska <[email protected]>

Adds examples for the parseEndpoint function

afb2eaf

Signed-off-by: Elena Kolevska <[email protected]>

Adds tests for the dapr-api-token metadata in gRPC calls

1d731d1

Signed-off-by: Elena Kolevska <[email protected]>

shubham1172 reviewed Sep 25, 2023

View reviewed changes

test/unit/utils/Client.util.test.ts Outdated Show resolved Hide resolved

shubham1172 reviewed Sep 25, 2023

View reviewed changes

XavierGeerinck suggested changes Sep 25, 2023

View reviewed changes

Updates after review

342a5a8

Signed-off-by: Elena Kolevska <[email protected]>

elena-kolevska added 3 commits September 25, 2023 10:57

docs: Adds info and examples about the new environment variables to t…

9226112

…he docs Signed-off-by: Elena Kolevska <[email protected]>

Addresses review comments

605c587

Signed-off-by: Elena Kolevska <[email protected]>

Small formatting fix

94f271e

Signed-off-by: Elena Kolevska <[email protected]>

shubham1172 reviewed Sep 27, 2023

View reviewed changes

shubham1172 previously approved these changes Sep 27, 2023

View reviewed changes

Fixes docs

3b27c23

Signed-off-by: Elena Kolevska <[email protected]>

elena-kolevska dismissed shubham1172’s stale review via 3b27c23 September 27, 2023 09:08

XavierGeerinck approved these changes Sep 27, 2023

View reviewed changes

shubham1172 approved these changes Sep 27, 2023

View reviewed changes

XavierGeerinck added this pull request to the merge queue Sep 27, 2023

Merged via the queue into dapr:main with commit 7464735 Sep 27, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support for the `DAPR_HTTP_ENDPOINT` and `DAPR_GRPC_ENDPOINT` environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

feat: Support for the `DAPR_HTTP_ENDPOINT` and `DAPR_GRPC_ENDPOINT` environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

elena-kolevska commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

shubham1172 Sep 20, 2023

elena-kolevska Sep 20, 2023 •

edited

Loading

shubham1172 Sep 25, 2023

XavierGeerinck Sep 25, 2023

shubham1172 left a comment

XavierGeerinck Sep 25, 2023

elena-kolevska commented Sep 25, 2023

shubham1172 Sep 27, 2023

elena-kolevska Sep 27, 2023

shubham1172 Sep 27, 2023

elena-kolevska Sep 27, 2023

shubham1172 Sep 27, 2023 •

edited

Loading

shubham1172 left a comment

XavierGeerinck commented Sep 27, 2023

feat: Support for the DAPR_HTTP_ENDPOINT and DAPR_GRPC_ENDPOINT environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

feat: Support for the DAPR_HTTP_ENDPOINT and DAPR_GRPC_ENDPOINT environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

Conversation

elena-kolevska commented Sep 12, 2023 • edited Loading

Description

Issue reference

Checklist

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

elena-kolevska Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1172 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elena-kolevska commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1172 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

shubham1172 left a comment

Choose a reason for hiding this comment

XavierGeerinck commented Sep 27, 2023

feat: Support for the `DAPR_HTTP_ENDPOINT` and `DAPR_GRPC_ENDPOINT` environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

feat: Support for the `DAPR_HTTP_ENDPOINT` and `DAPR_GRPC_ENDPOINT` environment variables. Adds support for DAPR_API_TOKEN to gRPC client #519

elena-kolevska commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

elena-kolevska Sep 20, 2023 •

edited

Loading

shubham1172 Sep 27, 2023 •

edited

Loading