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.

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 support for multiple test verbs and fix /_render/template. #723

Closed
wants to merge 4 commits into from

Conversation

dblock
Copy link
Member

@dblock dblock commented Dec 10, 2024

Description

You can now do this:

- synopsis: Use POST and PUT interchangeably.
  path: /{index}
  method:
    - POST
    - PUT

Avoids duplicating the test.

Added tests and fixed /_render/template.

Issues Resolved

Part of #663.

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.

Copy link
Contributor

github-actions bot commented Dec 10, 2024

Changes Analysis

Commit SHA: 958c971
Comparing To SHA: c0237fb

API Changes

Summary

└─┬Paths
  ├─┬/_render/template/{id}
  │ ├─┬GET
  │ │ └─┬Requestbody
  │ │   └─┬application/json
  │ │     └─┬Schema
  │ │       ├──[➕] properties (27313:15)
  │ │       ├──[➖] properties (27313:15)❌ 
  │ │       └─┬params
  │ │         └──[🔀] additionalProperties (27321:39)❌ 
  │ └─┬POST
  │   └─┬Requestbody
  │     └─┬application/json
  │       └─┬Schema
  │         ├──[➕] properties (27313:15)
  │         ├──[➖] properties (27313:15)❌ 
  │         └─┬params
  │           └──[🔀] additionalProperties (27321:39)❌ 
  └─┬/_render/template
    ├─┬GET
    │ └─┬Requestbody
    │   └─┬application/json
    │     └─┬Schema
    │       ├──[➕] properties (27313:15)
    │       ├──[➖] properties (27313:15)❌ 
    │       └─┬params
    │         └──[🔀] additionalProperties (27321:39)❌ 
    └─┬POST
      └─┬Requestbody
        └─┬application/json
          └─┬Schema
            ├──[➕] properties (27313:15)
            ├──[➖] properties (27313:15)❌ 
            └─┬params
              └──[🔀] additionalProperties (27321:39)❌ 

Document Element Total Changes Breaking Changes
paths 12 8
  • BREAKING Changes: 8 out of 12
  • Modifications: 4
  • Removals: 4
  • Additions: 4
  • Breaking Removals: 4
  • Breaking Modifications: 4

Report

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

API Coverage

Before After Δ
Covered (%) 606 (59.35 %) 606 (59.35 %) 0 (0 %)
Uncovered (%) 415 (40.65 %) 415 (40.65 %) 0 (0 %)
Unknown 43 43 0

Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
536 463 (86.38 %)

Signed-off-by: dblock <[email protected]>
- POST
- PUT
```

Copy link
Collaborator

@nhtruong nhtruong Dec 10, 2024

Choose a reason for hiding this comment

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

This is a great shorthand for writing the same chapter multiple times with different methods. I think we should treat it as such: Multiple chapters that virtually identical but the method. And then when the story is read, we will turn that 1 chapter of Create index "books" into two chapter:

  • Create Index "book" [With POST]
  • Create Index "book" [With PUT]

Then you will only have to modify the story parser to turn that 1 chapter into 2 and the rest of the framework can stay intact. We can add a line in this guide saying that the chapter will be split into 2 so they know what to expect in the Result page.

Arguably, a case like this should be treat as 2 different stories because rarely are our non-PUT requests idempotent (even when you delete a resource on OS twice, you get 404 the second time). So you could not actually test this on PUT/POST /books because executing PUT /books after POST /books will result in index named books already exists error. This will open a can of worm of matrix argument when we have multiple of such chapters in an original story.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay let me see if I can do a parser-based implementation instead ...

@dblock
Copy link
Member Author

dblock commented Dec 11, 2024

Closing in favor of #724

@dblock dblock closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants