Pass dapr api token #598

DeepanshuA · 2023-09-13T14:57:44Z

Description

microsoft/durabletask-python#16 is merged. So, updating this PR.

This change enables user to pass an API token to WorkflowRuntime and DaprWorkflowClient.
If user doesn't pass explicitly and API Token and environment variable is set for DAPR_API_TOKEN, it will be picked.

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: #594

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: Deepanshu Agarwal <[email protected]>

RyanLettieri · 2023-09-13T16:08:49Z

Great changes! The only question I have is if/how these changes were tested and verified and if it is possible to create a test (or modify an existing one) in order to ensure that this new behavior doesn't break in the future.

berndverst · 2023-09-13T21:09:59Z

ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_client.py

        address = getAddress(host, port)
-        self.__obj = client.TaskHubGrpcClient(host_address=address)
+        metadata = []
+        if apiToken is not None:


[not blocking] Are you sure you even need the API token as parameter? I'd rather refer people to using the settings package to set the token:

from dapr.conf import settings settings.DAPR_API_TOKEN='mytokenhere'

And as you know that is only necessary if the token itself isn't already set as an environment variable.

You can keep the parameter if you'd like to have it for consistency with other SDKs - but if other SDKs do not use this, then I recommend also removing it here.

I have removed api_token option. It will pick only if corresponding env. setting is there.
python DaprClient also doing the same thing - so to maintain consistency.

berndverst · 2023-09-13T21:11:06Z

ext/dapr-ext-workflow/dapr/ext/workflow/util.py

@@ -17,6 +17,7 @@

 from dapr.conf import settings

+DAPR_API_TOKEN_HEADER = 'dapr-api-token'


Please import this instead so we can change everything in one place if it ever must change.

from dapr.ext.workflow.util import DAPR_API_TOKEN_HEADER

yeah, used from original place only now.

berndverst

Left some feedback - 2 small questions, but you do need to use snake case instead of camelcase :)

berndverst · 2023-09-13T21:12:19Z

ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_client.py

@@ -37,9 +38,14 @@ class DaprWorkflowClient:
       This client is intended to be used by workflow application, not by general purpose
       application.
    """
-    def __init__(self, host: Optional[str] = None, port: Optional[str] = None):
+    def __init__(self, host: Optional[str] = None, port: Optional[str] = None, apiToken: Optional[str] = None):


Suggested change

def __init__(self, host: Optional[str] = None, port: Optional[str] = None, apiToken: Optional[str] = None):

def __init__(self, host: Optional[str] = None, port: Optional[str] = None, api_token: Optional[str] = None):

Python requires snake case instead of camelcase per style guide

removed the option.

berndverst · 2023-09-13T21:12:45Z

ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py

@@ -31,10 +32,15 @@ class WorkflowRuntime:
    """WorkflowRuntime is the entry point for registering workflows and activities.
    """

-    def __init__(self, host: Optional[str] = None, port: Optional[str] = None):
+    def __init__(self, host: Optional[str] = None, port: Optional[str] = None, apiToken: Optional[str] = None):


Make this snake case. api_token

removed the option.

berndverst · 2023-09-13T21:13:59Z

ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py

-    def __init__(self, host: Optional[str] = None, port: Optional[str] = None):
+    def __init__(self, host: Optional[str] = None, port: Optional[str] = None, apiToken: Optional[str] = None):
+        metadata = []
+        if apiToken is not None:


Same feedback [not blocking] - do you really need this in the method signature?

In Python we really want folks to use our settings package to centrally set these things (if they are not already provided via environment variables)

yeah, removed the option

berndverst · 2023-09-13T21:15:40Z

Please run the linter and typecheck locally before updating this PR and fix issues.

tox -e flake8
tox -e type

Current issues for lint:

./ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_client.py:41:101: E501 line too long (111 > 100 characters)
./ext/dapr-ext-workflow/dapr/ext/workflow/util.py:22:1: E302 expected 2 blank lines, found 1
./ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py:16:1: F401 'typing.Any' imported but unused
./ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py:16:1: F401 'typing.Dict' imported but unused
./ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py:35:101: E501 line too long (111 > 100 characters)
./ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py:44:5: E301 expected 1 blank line, found 0

berndverst · 2023-09-13T21:17:18Z

Great changes! The only question I have is if/how these changes were tested and verified and if it is possible to create a test (or modify an existing one) in order to ensure that this new behavior doesn't break in the future.

@RyanLettieri I do not think we should modify the examples - however, if you'd like a unit test / mock could be added here.

To me this is not a blocking issue because this code is very straightforward and in line with what we have already in the SDK.

berndverst

The workflow example also currently appears broken

		== APP == Traceback (most recent call last):
		== APP ==   File "/home/runner/work/python-sdk/python-sdk/examples/demo_workflow/app.py", line 113, in <module>
		== APP ==     main()
		== APP ==   File "/home/runner/work/python-sdk/python-sdk/examples/demo_workflow/app.py", line 49, in main
		== APP ==     workflowRuntime = WorkflowRuntime(host, port)
		== APP ==   File "/home/runner/work/python-sdk/python-sdk/ext/dapr-ext-workflow/dapr/ext/workflow/workflow_runtime.py", line 43, in __init__
		== APP ==     self.__worker = worker.TaskHubGrpcWorker(host_address=address, metadata=metadata)
		== APP == TypeError: __init__() got an unexpected keyword argument 'metadata'

This is likely caused by the fact you made metadata a list now instead of a tuple. See comment above.

berndverst · 2023-09-13T21:24:32Z

@DeepanshuA was there no update to durable-task-python required to make this work? Otherwise there should be a change to the requirements.txt / setup.cfg.

berndverst · 2023-09-13T21:29:08Z

@DeepanshuA @RyanLettieri this PR cannot work right now. This needs to be done:

This has to be merged: https://github.com/microsoft/durabletask-python/pull/16/files
You have to cut a new release of durabletask-python
Then update the version here: https://github.com/dapr/python-sdk/blob/master/ext/dapr-ext-workflow/setup.cfg

ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_client.py

DeepanshuA · 2023-09-15T05:43:13Z

@DeepanshuA @RyanLettieri this PR cannot work right now. This needs to be done:

This has to be merged: https://github.com/microsoft/durabletask-python/pull/16/files

You have to cut a new release of durabletask-python

Then update the version here: https://github.com/dapr/python-sdk/blob/master/ext/dapr-ext-workflow/setup.cfg

Yeah @berndverst.
I forgot to update about this in the description of this PR.

Signed-off-by: Deepanshu Agarwal <[email protected]>

berndverst · 2023-09-20T19:23:40Z

@DeepanshuA now that microsoft/durabletask-python#16 is merged, please cut a release and complete this PR so that this will not delay the Python SDK release. Thanks!

Signed-off-by: Deepanshu Agarwal <[email protected]>

codecov · 2023-09-21T07:10:16Z

Codecov Report

Patch coverage: 83.33% and project coverage change: -0.02% ⚠️

Comparison is base (f9eedff) 86.42% compared to head (7fbaec7) 86.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #598      +/-   ##
==========================================
- Coverage   86.42%   86.40%   -0.02%     
==========================================
  Files          74       74              
  Lines        3595     3605      +10     
==========================================
+ Hits         3107     3115       +8     
- Misses        488      490       +2

Files Changed	Coverage Δ
dapr/clients/grpc/_helpers.py	`89.85% <ø> (ø)`
...workflow/dapr/ext/workflow/dapr_workflow_client.py	`97.43% <83.33%> (-2.57%)`	⬇️
...ext-workflow/dapr/ext/workflow/workflow_runtime.py	`70.27% <83.33%> (+1.52%)`	⬆️

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

Pass dapr api token

ce9870b

Signed-off-by: Deepanshu Agarwal <[email protected]>

DeepanshuA requested review from a team as code owners September 13, 2023 14:57

berndverst reviewed Sep 13, 2023

View reviewed changes

berndverst requested changes Sep 13, 2023

View reviewed changes

berndverst reviewed Sep 13, 2023

View reviewed changes

ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_client.py Outdated Show resolved Hide resolved

DeepanshuA changed the title ~~Pass dapr api token~~ [DEPENDENCY_AWAITED] Pass dapr api token Sep 19, 2023

Incorporate comments

84878f3

Signed-off-by: Deepanshu Agarwal <[email protected]>

Use latest dtf python

1bca7c7

Signed-off-by: Deepanshu Agarwal <[email protected]>

Merge branch 'master' of github.com:dapr/python-sdk into dapr-api-token

7fbaec7

DeepanshuA changed the title ~~[DEPENDENCY_AWAITED] Pass dapr api token~~ Pass dapr api token Sep 21, 2023

DeepanshuA requested a review from berndverst September 21, 2023 08:08

berndverst approved these changes Sep 25, 2023

View reviewed changes

Merge branch 'master' into dapr-api-token

dabf651

berndverst merged commit 010a5b3 into dapr:master Sep 25, 2023
6 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass dapr api token #598

Pass dapr api token #598

DeepanshuA commented Sep 13, 2023 •

edited

Loading

RyanLettieri commented Sep 13, 2023

berndverst Sep 13, 2023 •

edited

Loading

berndverst Sep 13, 2023

DeepanshuA Sep 21, 2023

berndverst Sep 13, 2023

DeepanshuA Sep 21, 2023

berndverst left a comment

berndverst Sep 13, 2023

DeepanshuA Sep 21, 2023

berndverst Sep 13, 2023

DeepanshuA Sep 21, 2023

berndverst Sep 13, 2023

DeepanshuA Sep 21, 2023

berndverst commented Sep 13, 2023

berndverst commented Sep 13, 2023

berndverst left a comment •

edited

Loading

berndverst commented Sep 13, 2023

berndverst commented Sep 13, 2023

DeepanshuA commented Sep 15, 2023

berndverst commented Sep 20, 2023

codecov bot commented Sep 21, 2023 •

edited

Loading

		@@ -17,6 +17,7 @@

		from dapr.conf import settings

		DAPR_API_TOKEN_HEADER = 'dapr-api-token'

	def __init__(self, host: Optional[str] = None, port: Optional[str] = None, apiToken: Optional[str] = None):
	def __init__(self, host: Optional[str] = None, port: Optional[str] = None, api_token: Optional[str] = None):

Pass dapr api token #598

Pass dapr api token #598

Conversation

DeepanshuA commented Sep 13, 2023 • edited Loading

Description

Issue reference

Checklist

RyanLettieri commented Sep 13, 2023

berndverst Sep 13, 2023 • edited Loading

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

Choose a reason for hiding this comment

berndverst left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berndverst commented Sep 13, 2023

berndverst commented Sep 13, 2023

berndverst left a comment • edited Loading

Choose a reason for hiding this comment

berndverst commented Sep 13, 2023

berndverst commented Sep 13, 2023

DeepanshuA commented Sep 15, 2023

berndverst commented Sep 20, 2023

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

DeepanshuA commented Sep 13, 2023 •

edited

Loading

berndverst Sep 13, 2023 •

edited

Loading

berndverst left a comment •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading