-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
Signed-off-by: Fabian Martinez <[email protected]>
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 |
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.
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.
Signed-off-by: Fabian Martinez <[email protected]>
@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]>
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 |
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]>
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 |
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.
Looks like tests are passing now. Thanks for making these changes!
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