Skip to content
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

update durabletask protos, set custom status #31

Merged
merged 5 commits into from
Oct 28, 2024

Conversation

famarting
Copy link
Contributor

initial work for dapr/dapr#8008 (comment)

here I updated the durabletask protobuf definitions and regenerated the client code.

Also added support for dapr/python-sdk#739 with a new test included

Also started dapr/python-sdk#655 but have not added a test yet

@famarting
Copy link
Contributor Author

I wanted to create this PR first and check with @cgillum if everything looks ok... the grpc code that got generated seems a bit weird to me

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Left a few requests for changes.

I haven't looked at the auto-generated protobuf changes closely, but I appreciate that we're updating the submodule reference. Will run this through the CI to see if it causes any issues.

durabletask/task.py Outdated Show resolved Hide resolved
durabletask/task.py Outdated Show resolved Hide resolved
tests/test_orchestration_e2e.py Outdated Show resolved Hide resolved
Signed-off-by: Fabian Martinez <[email protected]>
@famarting
Copy link
Contributor Author

@cgillum I just noticed this comment https://github.com/microsoft/durabletask-python/blob/main/Makefile#L14 which could explain the unexpected build errors I'm getting on CI...

do you know what changes are missing to the proto generated files?

Signed-off-by: Fabian Martinez <[email protected]>
@famarting
Copy link
Contributor Author

found this https://github.com/microsoft/durabletask-python/blob/main/durabletask/internal/orchestrator_service_pb2_grpc.py#L7 , but I think I still have test failures somehow due to the generated code

@cgillum
Copy link
Member

cgillum commented Oct 28, 2024

This seems like a different type of error than what I’d expect in the case where I needed to update an import statement. I can try to help take a look but I may not be able to get to this right away due to some other urgent priorities at the moment.

Signed-off-by: Fabian Martinez <[email protected]>
@famarting
Copy link
Contributor Author

This seems like a different type of error than what I’d expect in the case where I needed to update an import statement. I can try to help take a look but I may not be able to get to this right away due to some other urgent priorities at the moment.

ok, it was much simpler than I anticipated, actually after fixing the protobuf generation it was just a matter of fixing the build on the tests

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Looks like tests are passing now. Thanks for making these changes!

@cgillum cgillum merged commit 8598f6b into microsoft:main Oct 28, 2024
5 checks passed
@famarting famarting deleted the update-proto branch October 29, 2024 08:39
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