-
Notifications
You must be signed in to change notification settings - Fork 89
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
If an api token is not passed in the constructor, check the env variable #548
Conversation
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.
Thanks for the PR @elena-kolevska. Small nits and the lint is failing.
src/utils/Client.util.ts
Outdated
@@ -292,7 +292,7 @@ export function getClientOptions( | |||
isKeepAlive: clientOptions?.isKeepAlive, | |||
logger: clientOptions?.logger ?? defaultLoggerOptions, | |||
actor: clientOptions?.actor, | |||
daprApiToken: clientOptions?.daprApiToken, | |||
daprApiToken: clientOptions?.daprApiToken ?? process.env.DAPR_API_TOKEN, |
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 move this to Settings.util.ts for consistency.
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 thought about the same thing, but it seemed like we only use it when there is a default value. Let me try it out and see how it looks.
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.
Yeah, I noticed that in some cases we even have ""
as the default, which essentially is the same as not having one.
Co-authored-by: Shubham Sharma <[email protected]> Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
============================================
- Coverage 100.00% 35.30% -64.70%
============================================
Files 1 91 +90
Lines 6 10447 +10441
Branches 1 411 +410
============================================
+ Hits 6 3688 +3682
- Misses 0 6700 +6700
- Partials 0 59 +59
☔ View full report in Codecov by Sentry. |
Signed-off-by: Elena Kolevska <[email protected]>
Description
Adds the possibility to get an api token from the standard environment variable, as described in this proposal https://github.com/dapr/proposals/blob/main/0010-S-unified-api-token-env-variable.md
Issue reference
#547
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: