Skip to content

Conversation

@filintod
Copy link
Contributor

@filintod filintod commented Oct 23, 2025

Description

Allow to create clients with different dapr-api-tokens in the same process

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

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

…iginal env var DAPR_API_TOKEN if found

Signed-off-by: Filinto Duran <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
@filintod filintod requested review from a team as code owners October 23, 2025 22:27
Copy link
Member

@acroca acroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add more information in the PR description 🙏

falls back to DAPR_API_TOKEN environment variable.
"""
DaprHealth.wait_until_ready()
self._api_token = api_token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't set api_token to default to settings.DAPR_API_TOKEN, and use it throughout this init function?

Suggested change
self._api_token = api_token
api_token = api_token if api_token is not None else settings.DAPR_API_TOKEN

Copy link
Contributor Author

@filintod filintod Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, makes sense, I'll make changes in a commit as it covers more than just this line

raise DaprInternalError(f'{error}') from error

self._logger = Logger('DaprWorkflowClient', logger_options)
self._api_token = api_token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to go to _api_token? Can't we just use api_token directly? I don't think it's used outside of this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the underscore prefix usually hints to tell people that the variable is internal (kind of private and tell people to not used outside, not part of the external api, even though anything in python is usually accessible if you want to) more than anything. like _logger there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the _logger is used in other places, the api_token doesn't leave this function.

Copy link
Contributor Author

@filintod filintod Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, like why make is it part of the instance of the object. true, let me check

Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
@filintod filintod changed the title multi app token workflow multi app token workflow (started from allow to create clients with different dapr-api-token ) Oct 27, 2025
filintod and others added 3 commits October 27, 2025 00:57
Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
@filintod filintod requested a review from acroca October 27, 2025 14:22
@filintod
Copy link
Contributor Author

@acroca please take another look

filintod and others added 6 commits October 27, 2025 09:58
Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
Co-authored-by: Albert Callarisa <[email protected]>
Signed-off-by: Filinto Duran <[email protected]>
Copy link
Member

@acroca acroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validations have failed, please fix them 🙏

@filintod filintod requested a review from acroca October 27, 2025 16:52
@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

❌ Patch coverage is 98.19277% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.61%. Comparing base (bffb749) to head (ebcdf1b).
⚠️ Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
...pr-ext-workflow/tests/test_multi_token_workflow.py 96.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #856      +/-   ##
==========================================
+ Coverage   86.63%   87.61%   +0.98%     
==========================================
  Files          84       94      +10     
  Lines        4473     6289    +1816     
==========================================
+ Hits         3875     5510    +1635     
- Misses        598      779     +181     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants