-
Notifications
You must be signed in to change notification settings - Fork 171
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
workflow examples: remove use of deprecated functions #640
workflow examples: remove use of deprecated functions #640
Conversation
Signed-off-by: Fabian Martinez <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
+ Coverage 58.04% 62.46% +4.42%
==========================================
Files 55 56 +1
Lines 3568 4143 +575
==========================================
+ Hits 2071 2588 +517
- Misses 1375 1429 +54
- Partials 122 126 +4 ☔ View full report in Codecov by Sentry. |
@@ -211,3 +215,7 @@ func (c *Client) PurgeWorkflow(ctx context.Context, id string) error { | |||
} | |||
return c.taskHubClient.PurgeOrchestrationState(ctx, api.InstanceID(id)) | |||
} | |||
|
|||
func (c *Client) Close() { | |||
_ = c.conn.Close() |
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 think logging at the debug level here would be helpful rather than discarding any underlying errors from closing the grpc connection
Signed-off-by: Fabian Martinez <[email protected]>
its not clear to me if the current test failures are due to the changes in this PR |
I'm taking a look at the test failures now, the grpc-service example is flaky due to the sidecar init times. I'll look at why the service example is failing with your changes. |
examples/service/Makefile
Outdated
@@ -31,7 +31,7 @@ client: ## Runs the uncompiled example client code | |||
custom-grpc-client: ## Runs the uncompiled example custom grpc client code | |||
dapr run --app-id custom-grpc-client \ | |||
-d ./config \ | |||
--dapr-http-max-request-size 41 \ | |||
--max-body-size 4Mi \ |
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 the sidecar was terminating following the updated arg --max-body-size
. This change should be fine... the validation workflow was pinned to a previous dapr runtime version (1.14.1) but this shouldn't have made a difference, it looks like the CLI hasn't implemented the required changes to accommodate the deprecation of --dapr-http-max-request-size
My recommendation is to revert this change until the CLI has implemented this.
examples/service/README.md
Outdated
@@ -91,7 +91,7 @@ expected_stdout_lines: | |||
```bash | |||
dapr run --app-id custom-grpc-client \ | |||
-d ./config \ | |||
--dapr-http-max-request-size 41 \ | |||
--max-body-size 4Mi \ |
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.
As above
Signed-off-by: Fabian Martinez <[email protected]>
Signed-off-by: Fabian Martinez <[email protected]>
should be all good now |
Indeed, lgtm 👍 |
Description
implementation for #634
Issue reference
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: