Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Flow_framework spec refactor: Added Tests, Param, and Error Types #574

Merged
merged 5 commits into from
Sep 17, 2024
Merged

Flow_framework spec refactor: Added Tests, Param, and Error Types #574

merged 5 commits into from
Sep 17, 2024

Conversation

junweid62
Copy link
Contributor

Description

  1. Added missing param for update workflow request body
  2. Added more 4xx Errors:
    • TemplateNameRequiredError
    • InvalidTemplateVersionError
    • UnsupportedFieldUpdateError
    • WorkflowParsingError
  3. Added test for workflow sample template

Issues Resolved

Task two of #833

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.

refactor:
e8898dc 
1. Added missing param for update workflow request body
2. Added test for create workflow with sample test

Signed-off-by: Junwei Dai <[email protected]>
@junweid62
Copy link
Contributor Author

Hey @dbwiddis just created a new pr, please take a look when you get a chance :)

dblock
dblock previously approved these changes Sep 14, 2024
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have some small asks to consider, please?

tests/default/flow_framework/sample_templates_test/deploy_openai_model.yaml Outdated Show resolved Hide resolved
tests/default/flow_framework/sample_templates_test/deploy_openai_model.yaml Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

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

Changes Analysis

Commit SHA: 66a92a2
Comparing To SHA: 811a5c9

API Changes

Summary

└─┬Components
  ├──[➕] schemas (45982:7)
  ├──[➕] schemas (46001:7)
  ├──[➕] schemas (45966:7)
  ├──[➕] schemas (45907:7)
  └─┬flow_framework.common:FlowFrameworkUpdate
    └──[➕] properties (45542:9)
Document Element Total Changes Breaking Changes
components 5 0
  • Total Changes: 5
  • Additions: 5

Report

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

API Coverage

Before After Δ
Covered (%) 560 (54.85 %) 560 (54.85 %) 0 (0 %)
Uncovered (%) 461 (45.15 %) 461 (45.15 %) 0 (0 %)
Unknown 25 25 0

@github-actions GitHub Actions
Copy link

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

Spec Test Coverage Analysis

Total Tested
484 290 (59.92 %)

@dblock
Copy link
Member

dblock commented Sep 14, 2024

Appreciate if you could suppress these warnings too.

PASSED  flow_framework/deprovision.yaml (tests/default/flow_framework/deprovision.yaml)
WARNING Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues.
  /_plugins/_flow_framework/workflow/{workflow_id}/_status
  /_plugins/_flow_framework/workflow/{workflow_id}/_deprovision

PASSED  flow_framework/provision.yaml (tests/default/flow_framework/provision.yaml)
WARNING Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues.
  /_plugins/_flow_framework/workflow/{workflow_id}/_status
  /_plugins/_flow_framework/workflow/{workflow_id}/_provision

I believe _status can be moved to a prologue, or may not actually be necessary at all. Generally try to keep these tests to the minimum that exercises the API being tested. This is because we're going to be using them in documentation examples.

@junweid62
Copy link
Contributor Author

@dblock Hi, just pushed a new version addressed all the comment :)

@junweid62
Copy link
Contributor Author

@dblock fixed the link check and pushed a new version. :)

dblock
dblock reviewed Sep 17, 2024
View reviewed changes
tests/default/flow_framework/template.yaml
name: test_update_work_flow
response:
status: 404
- synopsis: Search workflow state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general you want to break this one up too into tests/default/flow_framework/state/search.yaml with just enough prerequisites. It does feel like repetition, but we will be generating doc pages for each API and we'll want an example that only shows that API and not anything else.

@dblock dblock merged commit d4d4fa9 into opensearch-project:main Sep 17, 2024
17 checks passed
dblock pushed a commit to dblock/opensearch-api-specification that referenced this pull request Sep 23, 2024
@junweid62 @dblock
Flow_framework spec refactor: Added Tests, Param, and Error Types (op…
cb1883a 
…ensearch-project#574)

* refactor:
1. Added missing param for update workflow request body
2. Added test for create workflow with sample test

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

* Update schema to use application/json content type

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

* Update schema to use application/json content type

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

* Update the template link to pass the link check

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

---------

Signed-off-by: Junwei Dai <[email protected]>
Co-authored-by: Junwei Dai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@sachetalva sachetalva Awaiting requested review from sachetalva sachetalva is a code owner

@nhtruong nhtruong Awaiting requested review from nhtruong nhtruong is a code owner

@Xtansia Xtansia Awaiting requested review from Xtansia Xtansia is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@Tokesh Tokesh Awaiting requested review from Tokesh Tokesh is a code owner

@aabeshov aabeshov Awaiting requested review from aabeshov aabeshov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@junweid62 @dblock