Complete API spec implementation for flow_framework #565

junweid62 · 2024-09-10T03:44:30Z

Description

Part of Adding missing API specs #168

API covered in this PR:

/_plugins/_flow_framework/workflow/_steps:
/_plugins/_flow_framework/workflow/_search:
/_plugins/_flow_framework/workflow/state/_search:
/_plugins/_flow_framework/workflow/{workflow_id}/_deprovision:
/_plugins/_flow_framework/workflow/{workflow_id}/_provision:
/_plugins/_flow_framework/workflow/{workflow_id}/_status:

At this point, all FlowFramework APIs are covered

Issues Resolved

Resolved #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]>

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

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

junweid62 · 2024-09-10T03:46:58Z

@dbwiddis Just finished up the remaining API specs. I'd really appreciate it if you could check it out. Thanks!

dbwiddis

LGTM from a Flow Framework API perspective! A few documentation nits/suggestions.

Did not review the technical details of the openapi spec.

CHANGELOG.md

dbwiddis · 2024-09-10T04:27:27Z

spec/schemas/flow_framework.errors.yaml

+      properties:
+        error:
+          type: string
+          description: Error message when a duplicate key is found in the request.


Not sure "duplicate key" is self-descriptive here. Is this a duplicate param, or a duplicate field in the body JSON?

https://github.com/opensearch-project/flow-framework/blob/dc20feb9d25b7c60bd76c8f5cc66a5a1eed860e2/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java#L125C2-L128C22

Because we can pass it as a query parameter or as a string value of a request body field

The error message is this. I naming DuplicateKeyError by the error message, should i change it ?

This looks correct to me, should match the code.

For now as @dblock says it's ok to match the code. The error message should probably be "duplicate param" but not a big deal to leave it as is. If you get the message you'll know what you did wrong. Whoever wrote that code should have thought better. ;)

spec/schemas/flow_framework.common.yaml

dblock

Looks good. Will leave it unmerged to address various comments.

Consider breaking up the workflow test into workflow/privision.yaml, deprovision.yaml, etc. We are going to use these tests to provide samples for the documentation and users will be mostly looking to copy-paste examples to "privision a workflow" - it gets messy when there's too much going on and often setup may become unrelated (to deprovision a workflow you should have one privisioned in a prologue).

tests/default/flow_framework/workflow.yaml

dblock · 2024-09-10T12:02:49Z

tests/default/flow_framework/workflow.yaml

+      status: 200
+  - synopsis: Delete workflow With Wrong ID.


Suggested change

- synopsis: Delete workflow With Wrong ID.

- synopsis: Delete workflow using an invalid ID.

You don't like my suggestion? :)

@dblock Sorry about that! I used GitHub's signoff and commit suggestion feature for that commit, so I did it directly on the page. The other changes are committed locally, but I haven't pushed them yet since I'm still working on the tests. I'll let you know once everything's done!

tests/default/flow_framework/workflow.yaml

github-actions · 2024-09-10T12:07:49Z

Changes Analysis

Commit SHA: 7c76787
Comparing To SHA: 3dcced9

API Changes

Summary

├─┬Paths
│ ├──[➕] path (3640:3)
│ ├──[➕] path (3691:3)
│ ├──[➕] path (3522:3)
│ ├──[➕] path (3473:3)
│ ├──[➕] path (3615:3)
│ ├──[➕] path (3668:3)
│ ├─┬/_plugins/_flow_framework/workflow/{workflow_id}
│ │ ├─┬PUT
│ │ │ ├──[➕] parameters (17160:13)❌ 
│ │ │ ├──[➕] parameters (17179:13)❌ 
│ │ │ ├──[➕] parameters (17184:13)❌ 
│ │ │ ├──[➕] parameters (17166:13)❌ 
│ │ │ └─┬Responses
│ │ │   └──[➕] codes (26367:7)
│ │ └─┬DELETE
│ │   └─┬Responses
│ │     └──[➖] codes (25977:7)❌ 
│ └─┬/_plugins/_flow_framework/workflow
│   └─┬POST
│     ├──[➕] parameters (17081:13)❌ 
│     └─┬Parameters
│       └─┬Schema
│         └──[🔀] $ref (45146:20)❌ 
└─┬Components
  ├──[➕] requestBodies (24159:7)
  ├──[➕] requestBodies (24171:7)
  ├──[➕] requestBodies (24165:7)
  ├──[➖] responses (25977:7)❌ 
  ├──[➕] responses (26290:7)
  ├──[➕] responses (26278:7)
  ├──[➕] responses (26254:7)
  ├──[➕] responses (26272:7)
  ├──[➕] responses (26282:7)
  ├──[➕] responses (26328:7)
  ├──[➕] responses (26336:7)
  ├──[➕] responses (26262:7)
  ├──[➕] responses (26312:7)
  ├──[➕] responses (26332:7)
  ├──[➕] responses (26268:7)
  ├──[➕] responses (26367:7)
  ├──[➕] responses (26294:7)
  ├──[➕] responses (26320:7)
  ├──[➕] responses (26308:7)
  ├──[➕] responses (26348:7)
  ├──[➕] responses (26340:7)
  ├──[➕] responses (26352:7)
  ├──[➕] responses (26344:7)
  ├──[➕] responses (26324:7)
  ├──[➕] responses (26258:7)
  ├──[➕] responses (26286:7)
  ├──[➕] parameters (17164:7)
  ├──[➕] parameters (17175:7)
  ├──[➕] parameters (17183:7)
  ├──[➕] parameters (17159:7)
  ├──[➕] parameters (17122:7)
  ├──[➕] parameters (17128:7)
  ├──[➕] parameters (17134:7)
  ├──[➕] parameters (17080:7)
  ├──[➕] parameters (17147:7)
  ├──[➕] parameters (17117:7)
  ├──[➕] parameters (17111:7)
  ├──[➕] schemas (45429:7)
  ├──[➕] schemas (45158:7)
  ├──[➕] schemas (45370:7)
  ├──[➕] schemas (45161:7)
  ├──[➕] schemas (45266:7)
  ├──[➕] schemas (45146:7)
  ├──[➕] schemas (45239:7)
  ├──[➕] schemas (45284:7)
  ├──[➕] schemas (45388:7)
  ├──[➕] schemas (45122:7)
  ├──[➕] schemas (45025:7)
  ├──[➕] schemas (45282:7)
  ├──[➕] schemas (45498:7)
  ├──[➕] schemas (45449:7)
  ├──[➕] schemas (45508:7)
  ├──[➕] schemas (45310:7)
  ├──[➕] schemas (45172:7)
  ├──[➕] schemas (45202:7)
  ├──[➕] schemas (45420:7)
  ├──[➕] schemas (45029:7)
  ├──[➕] schemas (45328:7)
  ├──[➕] schemas (45459:7)
  ├──[➕] schemas (45226:7)
  ├──[➕] schemas (45189:7)
  ├──[➕] schemas (45297:7)
  ├──[➕] schemas (45109:7)
  ├──[➕] schemas (45235:7)
  ├──[➕] schemas (45261:7)
  ├──[➕] schemas (45150:7)
  ├──[➕] schemas (45411:7)
  ├──[➕] schemas (45548:7)
  └─┬flow_framework.errors:BadRequestError
    ├──[➕] type (45390:13)❌ 
    ├──[➕] properties (45392:9)
    └──[➕] properties (45396:9)

Document Element	Total Changes	Breaking Changes
paths	14	7
components	71	2

❌ BREAKING Changes: 9 out of 85
Modifications: 1
Removals: 2
Additions: 82
Breaking Removals: 2
Breaking Modifications: 1
Breaking Additions: 6

Report

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

API Coverage

	Before	After	Δ
Covered (%)	537 (52.6 %)	545 (53.38 %)	8 (0.78 %)
Uncovered (%)	484 (47.4 %)	476 (46.62 %)	-8 (-0.78 %)
Unknown	26	26	0

dblock · 2024-09-10T12:12:04Z

Looks like some legit test failures, might need to update the build ref in the CI.

        FAILED  Create workflow With Reprovision set true. (Operation "PUT /_plugins/_flow_framework/workflow" not found in the spec.)
        FAILED  Create workflow With Reprovision set true and UseCase Not null. (Operation "PUT /_plugins/_flow_framework/workflow" not found in the spec.)
        FAILED  Update workflow With Reprovision set true and UseCase Not null. (Operation "POST /_plugins/_flow_framework/workflow/{workflow_id}" not found in the spec.)

dbwiddis · 2024-09-10T15:35:40Z

Looks like some legit test failures, might need to update the build ref in the CI.

        FAILED  Create workflow With Reprovision set true. (Operation "PUT /_plugins/_flow_framework/workflow" not found in the spec.)
        FAILED  Create workflow With Reprovision set true and UseCase Not null. (Operation "PUT /_plugins/_flow_framework/workflow" not found in the spec.)
        FAILED  Update workflow With Reprovision set true and UseCase Not null. (Operation "POST /_plugins/_flow_framework/workflow/{workflow_id}" not found in the spec.)

These seem reversed. POST ends at /workflow, PUT requires the workflow id.

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

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

junweid62 · 2024-09-11T20:10:30Z

@dblock Hey, I just pushed a new version. Please take a look when you get a chance!:)

github-actions · 2024-09-11T20:14:12Z

Spec Test Coverage Analysis

Total	Tested
571	282 (49.39 %)

junweid62 · 2024-09-11T20:14:50Z

I was also wondering if there's a command that can remove all the empty lines?

nhtruong · 2024-09-11T20:17:52Z

@junweid62 most IDEs can do that for you "On Save". You should look up that feature for the IDE you're using.

Junwei Dai added 8 commits September 9, 2024 20:35

Add some 4xx error test

b789b85

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

1. Adding API spec for GET /workflow/_steps

4a4503a

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

1. Adding API spec for GET /workflow/_status

b957fdb

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

1. Adding API spec for POST /workflow/_deprovision

0ea367e

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

1. Adding API spec for POST /workflow/_provision

2132794

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

1. Adding API spec for POST /workflow/_search

06e14c0

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

1. Adding API spec for POST/GET /workflow/state/_search

312b471

2. Adding Test for this API Signed-off-by: Junwei Dai <[email protected]>

add change log

14fe053

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

junweid62 requested review from dblock, harshavamsi, sachetalva, nhtruong, Xtansia, VachaShah, Tokesh and aabeshov as code owners September 10, 2024 03:44

dbwiddis approved these changes Sep 10, 2024

View reviewed changes

dblock previously approved these changes Sep 10, 2024

View reviewed changes

Update tests/default/flow_framework/workflow.yaml

62fcc2e

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

junweid62 dismissed dblock’s stale review via 62fcc2e September 10, 2024 21:57

Junwei Dai added 2 commits September 10, 2024 15:41

change searchStateResponse spec.

8543168

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

update workflow.yaml add new test file seperate test senario

7c76787

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

dblock approved these changes Sep 11, 2024

View reviewed changes

dblock merged commit d05ba73 into opensearch-project:main Sep 11, 2024
17 checks passed

dbwiddis mentioned this pull request Sep 11, 2024

[FEATURE] Add specs for flow framework APIs #508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete API spec implementation for flow_framework #565

Complete API spec implementation for flow_framework #565

junweid62 commented Sep 10, 2024 •

edited

Loading

junweid62 commented Sep 10, 2024

dbwiddis left a comment

dbwiddis Sep 10, 2024

junweid62 Sep 10, 2024

dblock Sep 11, 2024

dbwiddis Sep 11, 2024

dblock left a comment

dblock Sep 10, 2024

dblock Sep 11, 2024

junweid62 Sep 11, 2024

github-actions bot commented Sep 10, 2024 •

edited

Loading

dblock commented Sep 10, 2024

dbwiddis commented Sep 10, 2024

junweid62 commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

junweid62 commented Sep 11, 2024

nhtruong commented Sep 11, 2024

	- synopsis: Delete workflow With Wrong ID.
	- synopsis: Delete workflow using an invalid ID.

Complete API spec implementation for flow_framework #565

Complete API spec implementation for flow_framework #565

Conversation

junweid62 commented Sep 10, 2024 • edited Loading

Description

Issues Resolved

junweid62 commented Sep 10, 2024

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis Sep 10, 2024

Choose a reason for hiding this comment

junweid62 Sep 10, 2024

Choose a reason for hiding this comment

dblock Sep 11, 2024

Choose a reason for hiding this comment

dbwiddis Sep 11, 2024

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock Sep 10, 2024

Choose a reason for hiding this comment

dblock Sep 11, 2024

Choose a reason for hiding this comment

junweid62 Sep 11, 2024

Choose a reason for hiding this comment

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

Changes Analysis

API Changes

Summary

Report

API Coverage

dblock commented Sep 10, 2024

dbwiddis commented Sep 10, 2024

junweid62 commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

Spec Test Coverage Analysis

junweid62 commented Sep 11, 2024

nhtruong commented Sep 11, 2024

junweid62 commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024 •

edited

Loading