-
Notifications
You must be signed in to change notification settings - Fork 4
SignalAPI and Execute API refactoring + Introduced CB Enabled for Service Registry #21
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: main
Are you sure you want to change the base?
Conversation
30a7023 to
b8ac8f8
Compare
b8ac8f8 to
fc251c9
Compare
1b704fa to
9c6fdb5
Compare
01a5a17 to
41e0093
Compare
06b5a77 to
889141d
Compare
889141d to
164ae03
Compare
| public class ProtoRegistryEntry { | ||
| private final String serviceName; | ||
| private final String filename; | ||
| private final byte[] data; |
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.
keeping this change ?
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.
yes we need them to deserialize the output from response.
| .build(); | ||
| .body(req); | ||
|
|
||
| // Only add query parameters if they are not null - let server use defaults |
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.
aren't these ifs already handled in conductor-client module ?
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.
No This is specific to executeWorkflow API. /workflow/execute/{name}/{version}
3e0500a to
0b3498b
Compare
| @@ -0,0 +1,96 @@ | |||
| /* | |||
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.
Is it part of conductor or orkes API? I don't see these endpoints here https://github.com/conductor-oss/conductor/tree/main/rest/src/main/java/com/netflix/conductor/rest/controllers .
If it is part of orkes API it should go to orkes-client project, from my standpoint
| * @param returnStrategy Strategy for what data to return | ||
| * @return SignalResponse with data based on the return strategy | ||
| */ | ||
| public SignalResponse signal(String workflowId, Task.Status status, Map<String, Object> output, ReturnStrategy returnStrategy) { |
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 don't see this and other endpoints in conductor repository. Shouldn't it go to orkes client?
Pull Request type
NOTE: Please remember to run
./gradlew spotlessApplyto fix any format violations.Changes in this PR
Describe the new behavior from this PR, and why it's needed
Issue #
Alternatives considered
Describe alternative implementation you have considered