Add flow_framework namespace specs #553

junweid62 · 2024-09-04T21:29:09Z

Description

Part of Adding missing API specs #168

API covered in this PR:
POST /_plugins/_flow_framework/workflow
GET /_plugins/_flow_framework/workflow/{workflow_id}
PUT /_plugins/_flow_framework/workflow/{workflow_id}
DELETE /_plugins/_flow_framework/workflow/{workflow_id}

The rest of API spec will be added in the next PR.

Issues Resolved

Part of #508

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.

Signed-off-by: Junwei Dai <[email protected]>

refactor: 1.add epilogues that deletes the workflow if it was created. 2.rewrite all the 'whether' 3. remove empty lines 4. add validation enum Signed-off-by: Junwei Dai <[email protected]>

add version to flow framework and test Signed-off-by: Junwei Dai <[email protected]>

add delete path param, add delete test, would also delete the workflow previously created Signed-off-by: Junwei Dai <[email protected]>

Add schema for 4xx response Signed-off-by: Junwei Dai <[email protected]>

Add eslint ignore 'reprovision' Signed-off-by: Junwei Dai <[email protected]>

junweid62 · 2024-09-04T21:31:17Z

@dbwiddis I have opened a new pull request and would appreciate it if you could take a moment to review it when you have the time. Thank you!

remove empty line Signed-off-by: Junwei Dai <[email protected]>

fix merge conflict error Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

github-actions · 2024-09-04T21:44:17Z

Changes Analysis

Commit SHA: 67e89be
Comparing To SHA: 0733e62

API Changes

Summary

├─┬Paths
│ ├──[➕] path (3445:3)
│ └──[➕] path (3472:3)
└─┬Components
  ├──[➕] requestBodies (23900:7)
  ├──[➕] requestBodies (23895:7)
  ├──[➕] responses (25967:7)
  ├──[➕] responses (25963:7)
  ├──[➕] responses (25986:7)
  ├──[➕] responses (25991:7)
  ├──[➕] responses (25948:7)
  ├──[➕] responses (26002:7)
  ├──[➕] responses (25977:7)
  ├──[➕] responses (25982:7)
  ├──[➕] responses (25959:7)
  ├──[➕] responses (25937:7)
  ├──[➕] responses (25972:7)
  ├──[➕] parameters (16916:7)
  ├──[➕] parameters (16910:7)
  ├──[➕] parameters (16897:7)
  ├──[➕] parameters (16890:7)
  ├──[➕] parameters (16882:7)
  ├──[➕] parameters (16868:7)
  ├──[➕] parameters (16903:7)
  ├──[➕] parameters (16922:7)
  ├──[➕] parameters (16875:7)
  ├──[➕] schemas (44660:7)
  ├──[➕] schemas (44541:7)
  ├──[➕] schemas (44588:7)
  ├──[➕] schemas (44498:7)
  ├──[➕] schemas (44568:7)
  ├──[➕] schemas (44610:7)
  ├──[➕] schemas (44620:7)
  ├──[➕] schemas (44670:7)
  ├──[➕] schemas (44650:7)
  ├──[➕] schemas (44600:7)
  ├──[➕] schemas (44524:7)
  ├──[➕] schemas (44479:7)
  ├──[➕] schemas (44556:7)
  ├──[➕] schemas (44630:7)
  ├──[➕] schemas (44640:7)
  └──[➕] schemas (44680:7)

Document Element	Total Changes	Breaking Changes
paths	2	0
components	38	0

Total Changes: 40
Additions: 40

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10728488330/artifacts/1901913020

API Coverage

	Before	After	Δ
Covered (%)	533 (52.2 %)	537 (52.6 %)	4 (0.4 %)
Uncovered (%)	488 (47.8 %)	484 (47.4 %)	-4 (-0.4 %)
Unknown	26	26	0

dblock

This looks great! I only have some nits, up to you if you want to address them.

The validator failed for good reason, check, https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10709911469/job/29695704593?pr=553.

spec/namespaces/flow_framework.yaml

spec/schemas/flow_framework._common.yaml

dblock · 2024-09-04T21:45:18Z

spec/schemas/flow_framework._common.yaml

+paths: {}
+components:
+  schemas:
+    FlowFrameworkException:


Does this one inherit from an OpenSearch error type?

yes , it inherit from OpenSearchException

FlowFrameworkException extends OpenSearchException

That makes it an Exception, though. "Error" in exception handling has a specific meaning (the program should be halted).

https://github.com/opensearch-project/flow-framework/blob/b3f9d6514d2c0530c8bffac654fe175452673169/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java#L113C5-L146C55
How about these, should i call it error or exception ?

It's interesting, the rest response includes the word "error" as an "error message" although it's an "exception". I'll defer to @dblock here on what the appropriate spec/api terminology is here.

🤷‍♂️ @nhtruong ?

I would map to actual class names in OpenSearch.

We've been calling them errors through out the Spec (e.g ReponseError). The clients also consider non 2XX responses as errors for error handling. As far as Client-Server communication is concerned (i.e. Web API), there's no distinction between Error and Exception, in my opinion. Any non-2XX response tells the user that something has gone wrong, and we usually call it a 500 Error Code etc instead of a 500 exception code.

Let's go with 'Error' instead. I'll go ahead and update the naming from 'Exception' to 'Error'.

tests/default/flow_framework/workflow.yaml

github-actions · 2024-09-04T21:47:11Z

Spec Test Coverage Analysis

Total	Tested
563	271 (48.13 %)

Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

1.change flow_framework._common to flow_framework._errors 2.change exception to Error for consistency Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

dbwiddis

Looks great! Just one missing param as mentioned in the earlier review (and I'm about to file a PR adding the missing docs)

dbwiddis · 2024-09-04T22:00:38Z

spec/namespaces/flow_framework.yaml

+        - $ref: '#/components/parameters/flow_framework.create::query.provision'
+        - $ref: '#/components/parameters/flow_framework.create::query.validation'
+        # eslint-disable-next-line @cspell/spellchecker
+        - $ref: '#/components/parameters/flow_framework.create::query.reprovision'


I still think you're missing use_case: https://opensearch.org/docs/latest/automating-configurations/workflow-templates/

dbwiddis · 2024-09-04T22:03:31Z

spec/schemas/flow_framework._common.yaml

+paths: {}
+components:
+  schemas:
+    FlowFrameworkException:


That makes it an Exception, though. "Error" in exception handling has a specific meaning (the program should be halted).

spec/schemas/flow_framework._common.yaml

Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

junweid62 · 2024-09-04T23:02:50Z

@dblock Hi, I have addressed all the comments and made the necessary changes. Regarding the suggestion about:

file: 'namespaces/flow_framework.yaml',
location: 'Operation: DELETE /_plugins/_flow_framework/workflow/{workflow_id}',
message: "The 400 response must be a reference object to '#/components/responses/flow_framework.create@400'."

I believe that using the error name directly like this:

          oneOf:
            - $ref: '../schemas/flow_framework._errors.yaml#/components/schemas/BadRequestError'
            - $ref: '../schemas/flow_framework._errors.yaml#/components/schemas/ConflictError'
            - $ref: '../schemas/flow_framework._errors.yaml#/components/schemas/MissingParameterError'
            - $ref: '../schemas/flow_framework._errors.yaml#/components/schemas/ParameterConflictError'
            - $ref: '../schemas/flow_framework._errors.yaml#/components/schemas/MaxWorkflowsLimitError'
            - $ref: '../schemas/flow_framework._errors.yaml#/components/schemas/WorkflowSaveError'

makes the it clearer in this particular case. Please let me know your thoughts on this approach.

dbwiddis

LGTM! Good for now, I've suggested some tests you can add, but those can also come in a later PR as you iterate.

tests/default/flow_framework/workflow.yaml

dbwiddis · 2024-09-04T23:04:10Z

tests/default/flow_framework/workflow.yaml

+    response:
+      status: 201
+    output:
+      test_workflow_id: payload.workflow_id


Could add some invalid JSON here that would fail parsing for another test.

Will add in the next pr

dbwiddis · 2024-09-04T23:05:26Z

tests/default/flow_framework/workflow.yaml

+  - synopsis: Delete workflow.
+    id: delete_flow_framework
+    path: /_plugins/_flow_framework/workflow/{workflow_id}
+    method: DELETE
+    parameters:
+      workflow_id: ${create_flow_framework.test_workflow_id}
+    response:
+      status: 200


Can test the wrong ID here as well.

Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

Signed-off-by: Junwei Dai <[email protected]>

dblock · 2024-09-05T10:55:11Z

Please let me know your thoughts on this approach.

Makes sense to me. I am still figuring the best way to do these things too! @nhtruong might have other ideas.

Iterate until https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10713232032/job/29722019367?pr=553 is green either way?

junweid62 · 2024-09-05T16:48:34Z

Please let me know your thoughts on this approach.

Makes sense to me. I am still figuring the best way to do these things too! @nhtruong might have other ideas.

Iterate until https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10713232032/job/29722019367?pr=553 is green either way?

sure, will do

nhtruong

LGTM
Feel free to merge unless you want to add more test cases.

Signed-off-by: Junwei Dai <[email protected]>

junweid62 · 2024-09-05T18:28:17Z

@dblock Hi, I just fix the lint spec errors, hope everything goes green this time !

Signed-off-by: Junwei Dai <[email protected]>

junweid62 · 2024-09-06T15:16:03Z

@dblock Hi, got all lint spec passed locally, hope everything goes well this time!

dblock · 2024-09-06T17:55:01Z

👏

Junwei Dai added 9 commits September 4, 2024 14:15

add flow framework create

e6a0c9e

Signed-off-by: Junwei Dai <[email protected]>

add flow framework delete

62e0fd9

Signed-off-by: Junwei Dai <[email protected]>

add flow framework put and get

a451624

Signed-off-by: Junwei Dai <[email protected]>

add changelog info

accea08

Signed-off-by: Junwei Dai <[email protected]>

fix mistype error

1c133f1

refactor: 1.add epilogues that deletes the workflow if it was created. 2.rewrite all the 'whether' 3. remove empty lines 4. add validation enum Signed-off-by: Junwei Dai <[email protected]>

refactor:

a5910b3

add version to flow framework and test Signed-off-by: Junwei Dai <[email protected]>

refactor:

10a88a6

add delete path param, add delete test, would also delete the workflow previously created Signed-off-by: Junwei Dai <[email protected]>

refactor:

d5446bb

Add schema for 4xx response Signed-off-by: Junwei Dai <[email protected]>

refactor:

bcdc900

Add eslint ignore 'reprovision' Signed-off-by: Junwei Dai <[email protected]>

junweid62 requested review from dblock, harshavamsi, sachetalva, nhtruong, Xtansia, VachaShah, Tokesh and aabeshov as code owners September 4, 2024 21:29

Junwei Dai added 3 commits September 4, 2024 14:33

refactor:

bc8e404

remove empty line Signed-off-by: Junwei Dai <[email protected]>

refactor:

d772954

fix merge conflict error Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

refactor:

b99a822

fix merge conflict error Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

dblock previously approved these changes Sep 4, 2024

View reviewed changes

junweid62 dismissed dblock’s stale review via a96e1ad September 4, 2024 21:47

junweid62 and others added 2 commits September 4, 2024 14:47

Update tests/default/flow_framework/workflow.yaml

a96e1ad

Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

refactor:

ceb229f

1.change flow_framework._common to flow_framework._errors 2.change exception to Error for consistency Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

dbwiddis reviewed Sep 4, 2024

View reviewed changes

add use_case parameter, add reprovision to .cspell

f84cb79

Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

dbwiddis approved these changes Sep 4, 2024

View reviewed changes

Junwei Dai added 2 commits September 4, 2024 19:50

refactor: fix eslint

4d7d28e

Signed-off-by: Junwei Dai <[email protected]> Signed-off-by: Junwei Dai <[email protected]>

refactor: remove comment

03ed38a

Signed-off-by: Junwei Dai <[email protected]>

nhtruong previously approved these changes Sep 5, 2024

View reviewed changes

refactor: fix lint spec error

fb4174d

Signed-off-by: Junwei Dai <[email protected]>

junweid62 dismissed nhtruong’s stale review via fb4174d September 5, 2024 18:25

Junwei Dai added 3 commits September 5, 2024 13:02

refactor: fix lint spec error

97860be

Signed-off-by: Junwei Dai <[email protected]>

refactor: fix lint spec error

488c35a

Signed-off-by: Junwei Dai <[email protected]>

refactor: fix lint spec error， add flow_framework.common file

67e89be

Signed-off-by: Junwei Dai <[email protected]>

nhtruong approved these changes Sep 6, 2024

View reviewed changes

dblock merged commit 9193d58 into opensearch-project:main Sep 6, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flow_framework namespace specs #553

Add flow_framework namespace specs #553

junweid62 commented Sep 4, 2024 •

edited

Loading

junweid62 commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 •

edited

Loading

dblock left a comment •

edited

Loading

dblock Sep 4, 2024

junweid62 Sep 4, 2024

dbwiddis Sep 4, 2024

junweid62 Sep 4, 2024

dbwiddis Sep 4, 2024

dblock Sep 5, 2024 •

edited

Loading

nhtruong Sep 5, 2024 •

edited

Loading

junweid62 Sep 5, 2024

github-actions bot commented Sep 4, 2024

dbwiddis left a comment

dbwiddis Sep 4, 2024

dbwiddis Sep 4, 2024

junweid62 commented Sep 4, 2024 •

edited

Loading

dbwiddis left a comment

dbwiddis Sep 4, 2024

junweid62 Sep 5, 2024

dbwiddis Sep 4, 2024

dblock commented Sep 5, 2024 •

edited

Loading

junweid62 commented Sep 5, 2024

nhtruong left a comment

junweid62 commented Sep 5, 2024

junweid62 commented Sep 6, 2024

dblock commented Sep 6, 2024

Add flow_framework namespace specs #553

Add flow_framework namespace specs #553

Conversation

junweid62 commented Sep 4, 2024 • edited Loading

Description

Issues Resolved

junweid62 commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 • edited Loading

Changes Analysis

API Changes

Summary

Report

API Coverage

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

nhtruong Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 4, 2024

Spec Test Coverage Analysis

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junweid62 commented Sep 4, 2024 • edited Loading

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Sep 5, 2024 • edited Loading

junweid62 commented Sep 5, 2024

nhtruong left a comment

Choose a reason for hiding this comment

junweid62 commented Sep 5, 2024

junweid62 commented Sep 6, 2024

dblock commented Sep 6, 2024

junweid62 commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024 •

edited

Loading

dblock left a comment •

edited

Loading

dblock Sep 5, 2024 •

edited

Loading

nhtruong Sep 5, 2024 •

edited

Loading

junweid62 commented Sep 4, 2024 •

edited

Loading

dblock commented Sep 5, 2024 •

edited

Loading