-
Notifications
You must be signed in to change notification settings - Fork 223
Feat Cross App CallActivity #1468
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Note, this PR should be cherrypicked into the release branch once merged. |
Signed-off-by: Cassandra Coyle <[email protected]>
This PR is also dependent on the dapr release for 1.16. Javi has a PR to update to the latest rc version here. We will need both merged in, then cherrypick both to the release branch |
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
I need to release durabletask-java with this PR merged in before this current PR will be green |
Signed-off-by: Cassandra Coyle <[email protected]>
@cicoyle I was looking at this PR, but it doesn't have anything related to cross app call activity. |
Did you by chance read the thorough readme updates I added? |
I just got a workflow IT test locally 🎉 Need to cleanup and push, will do that + fix the conflict in my morning. Please feel free to review everything else and know I will update the pom.xml in my morning and add an IT test. Ill remove the mechanical markdown since Ill be adding the IT test. |
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
.../test/java/io/dapr/it/testcontainers/workflows/crossapp/WorkflowsCrossAppCallActivityIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Cassandra Coyle <[email protected]>
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.
@cicoyle I've left a bunch of comments and suggestions. I am really against adding support for customHttp
and customGrpc
ports that is something that users shouldn't need to do as that goes against the idea of using containers that are started with random ports, but still allow the user to obtain the mapped ports to connect to them.
.withWorkingDirectory("/app") | ||
.withCommand("java", "-cp", "/app/classes:/app/libs/*", | ||
"-Ddapr.app.id=app2", | ||
"-Ddapr.grpc.endpoint=app2-sidecar:50002", |
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.
@cicoyle here you can use the Testcontainers mapped (random) port instead of relying on a custom port number.
.../test/java/io/dapr/it/testcontainers/workflows/crossapp/WorkflowsCrossAppCallActivityIT.java
Outdated
Show resolved
Hide resolved
.../test/java/io/dapr/it/testcontainers/workflows/crossapp/WorkflowsCrossAppCallActivityIT.java
Outdated
Show resolved
Hide resolved
.../test/java/io/dapr/it/testcontainers/workflows/crossapp/WorkflowsCrossAppCallActivityIT.java
Outdated
Show resolved
Hide resolved
.../test/java/io/dapr/it/testcontainers/workflows/crossapp/WorkflowsCrossAppCallActivityIT.java
Show resolved
Hide resolved
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
testcontainers-dapr/src/main/java/io/dapr/testcontainers/DaprContainer.java
Outdated
Show resolved
Hide resolved
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.
@cicoyle overall the example looks good and the new additions for Cross App Call are good, but I believe that all the changes in the DaprContainer class will cause issues. I am curious to see what are the issues that you are facing if you don't introduce any change in that class.
DaprContainer is a critical class being used by customers now, so I would change the behavior of it.
My recommendation would be to revert the changes on DaprContainer
and I can help you to figure out what is going wrong with the containers when the pipelines fail.
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1468 +/- ##
============================================
+ Coverage 76.91% 78.68% +1.77%
- Complexity 1592 1921 +329
============================================
Files 145 237 +92
Lines 4843 5940 +1097
Branches 562 610 +48
============================================
+ Hits 3725 4674 +949
- Misses 821 944 +123
- Partials 297 322 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add support for cross app call activity calls.