-
Notifications
You must be signed in to change notification settings - Fork 505
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
Add documentation for optional param for get workflow step API #6736
Add documentation for optional param for get workflow step API #6736
Conversation
Signed-off-by: Owais Kazi <[email protected]>
6f3fc8c
to
4dc2a6d
Compare
@@ -30,18 +30,21 @@ OpenSearch validates workflows by using the validation template that lists the r | |||
} | |||
``` | |||
|
|||
The Get Workflow Steps API retrieves this file. | |||
The Get Workflow Steps API retrieves [this](https://github.com/opensearch-project/flow-framework/blob/2.x/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java#L120-L229) enum. |
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.
The line numbers on this link may change in the future. It's also not very explanatory. Should be something like, "Returns a list of Workflow Steps including their required inputs, outputs, default timeout value, ...." etc.
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.
Changed the description. Also, added link of 2.13
release branch to avoid changes later.
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Fanit Kolchina <[email protected]>
@owaiskazi19 Thank you for providing the PR! I reviewed and pushed my edits. We don't usually direct users to GitHub code because this is user documentation. I removed the link for now because I think the steps are self-explanatory but if you think we should list the response fields, then we have to create a response fields table on this page and port all the fields with descriptions here. Let me know what you think. |
Looks good |
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.
@owaiskazi19 @kolchfa-aws Please see my comments and changes and let me know if you have any questions. Thanks!
{: .warning} | ||
|
||
OpenSearch validates workflows by using the validation template that lists the required inputs, generated outputs, and required plugins for all steps. For example, for the `register_remote_model` step, the validation template appears as follows: | ||
This API returns a list of workflow steps, including their required inputs, outputs, default timeout value, and required plugins For example, for the `register_remote_model` step, the Get Workflow Steps API returns the following information: |
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.
Should "timeout value" be "timeout values" (plural)?
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.
@kolchfa-aws @natebower we have just one timeout value.
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.
@owaiskazi19 I think it's fine to keep it plural since "steps" is plural. So there are timeouts for a number of steps (even though each has only one timeout), if that makes sense.
|
||
| Parameter | Data type | Description | | ||
| :--- | :--- | :--- | | ||
| `workflow_step` | String | The step name of the step to retrieve. Specify multiple step names as a comma-separated list. For example, `create_connector,delete_model,deploy_model`. | |
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.
Can the first instance of "step" be removed? Ignore if essential to the meaning.
``` | ||
{% include copy-curl.html %} | ||
|
||
|
||
#### Example response | ||
|
||
OpenSearch responds with the validation template containing the steps. The order of fields in the returned steps may not exactly match the original JSON but will function identically. | ||
OpenSearch responds with the workflow steps. The order of fields in the returned steps may not exactly match the original JSON but will function identically. |
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.
Should a noun follow JSON?
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 think so, I think it's understood that JSON means the "document in JSON format".
Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: kolchfa-aws <[email protected]>
Signed-off-by: Fanit Kolchina <[email protected]>
…-website into optional-param
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
…earch-project#6736) * Added documentaion for optional param for get workflow step API Signed-off-by: Owais Kazi <[email protected]> * Addressed PR comment Signed-off-by: Owais Kazi <[email protected]> * Doc review Signed-off-by: Fanit Kolchina <[email protected]> * Apply suggestions from code review Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: kolchfa-aws <[email protected]> --------- Signed-off-by: Owais Kazi <[email protected]> Signed-off-by: Fanit Kolchina <[email protected]> Signed-off-by: kolchfa-aws <[email protected]> Co-authored-by: Fanit Kolchina <[email protected]> Co-authored-by: kolchfa-aws <[email protected]> Co-authored-by: Nathan Bower <[email protected]>
Description
opensearch-project/flow-framework#538 added an optional
workflow_step
param to the fetch specific workflow steps in the workflow steps APIIssues Resolved
Part of opensearch-project/flow-framework#541
Checklist
For more information on following Developer Certificate of Origin and signing off your commits, please check here.