-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adding a get status API #148
Adding a get status API #148
Conversation
9539ae0
to
da29f84
Compare
10475ff
to
c5b7d72
Compare
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.
Initial pass, looks good so far, a couple of comments mostly on naming.
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowResponse.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.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.
Initial pass. Need tests
src/main/java/org/opensearch/flowframework/model/ResourcesCreated.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateConnectorStep.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateConnectorStep.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateConnectorStep.java
Show resolved
Hide resolved
Typically params don't have the |
I see this is covered - thanks! |
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
0d77699
to
5583855
Compare
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.
Another pass
src/main/java/org/opensearch/flowframework/workflow/WorkflowData.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateConnectorStep.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateConnectorStep.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Show resolved
Hide resolved
@amitgalitz resource created should have the exact entry we have for WorkflowData createConnectorFuture.complete(
new WorkflowData(Map.ofEntries(Map.entry("connector_id", mlCreateConnectorResponse.getConnectorId())))
); "resources_created": [
{
"workflow_step_name": "create_connector",
"resource_id": "fZTmz4sBVvQ80zEwkC2z",
"connector_id": "XYZ"
}
] |
resource_id and connector_id are the same here |
then let's call it connector_id |
I want it to be static field though so its cleaner on searching and mapping, the keys of resource_id and workflow_step_name wont change between resources. If you still think otherwise thats fair, more info here: #136. This also might have some small changes as Dan implements the delete api. |
src/main/java/org/opensearch/flowframework/model/WorkflowState.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/GetWorkflowRequest.java
Show resolved
Hide resolved
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
e6c9ab4
to
3ee4d72
Compare
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.
Generally looks good, but need to understand more about how the id String tells me what sort of resource I will need to remove later.
Are we assuming a one-to-one correspondence between workflow step and resource? I'm not sure how certain that assumption is.
src/main/java/org/opensearch/flowframework/model/ResourcesCreated.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/model/ResourcesCreated.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/model/WorkflowState.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
…hanges Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
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.
LGTM with pending TODO handled in another PR
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.
Read latest updates and comments, no concerns. Giving a soft approval :)
* adding workflow state and create connector resources created Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * add plugin enabled setting check for get status api Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * addressing comments Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * addressing additional conflicts Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * addressed comments and added more tests Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * removed additional listener in provision action Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * changed resourcescreated to resourceCreated and other minor comment changes Signed-off-by: Amit Galitzky <amgalitz@amazon.com> --------- Signed-off-by: Amit Galitzky <amgalitz@amazon.com> (cherry picked from commit e3bbfcb) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Adding a get status API (#148) * adding workflow state and create connector resources created * add plugin enabled setting check for get status api * addressing comments * addressing additional conflicts * addressed comments and added more tests * removed additional listener in provision action * changed resourcescreated to resourceCreated and other minor comment changes --------- (cherry picked from commit e3bbfcb) Signed-off-by: Amit Galitzky <amgalitz@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* adding workflow state and create connector resources created Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * add plugin enabled setting check for get status api Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * addressing comments Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * addressing additional conflicts Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * addressed comments and added more tests Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * removed additional listener in provision action Signed-off-by: Amit Galitzky <amgalitz@amazon.com> * changed resourcescreated to resourceCreated and other minor comment changes Signed-off-by: Amit Galitzky <amgalitz@amazon.com> --------- Signed-off-by: Amit Galitzky <amgalitz@amazon.com> (cherry picked from commit e3bbfcb) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Adding a get status API (#148) * adding workflow state and create connector resources created * add plugin enabled setting check for get status api * addressing comments * addressing additional conflicts * addressed comments and added more tests * removed additional listener in provision action * changed resourcescreated to resourceCreated and other minor comment changes --------- (cherry picked from commit e3bbfcb) Signed-off-by: Amit Galitzky <amgalitz@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Joshua Palis <jpalis@amazon.com>
Description
Currently implemented so when not given _all=true the response is:
when:
/_plugins/_flow_framework/workflow/yfRsposBMbAFAhZEJ1qE/_status?all=true
then response includes all of state index like this:
Currently GC content (template) is not embedded within response, added some of the prerequisites for that but not fully implemented, can be done when GET workflow API is complete.
Additional Issues highlighting further work:#136 -> This PR adds the resources created addition logic to create_connector, following PR will add the work for each step.
#171 -> adding the full template in the status response as well to help with faster frontend fetching
Issues Resolved
Closes #22
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.