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

Completed ISM specs and tests. #578

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Conversation

dblock
Copy link
Member

@dblock dblock commented Sep 15, 2024

Description

Completed ISM specs and tests.

Issues Resolved

Closes #224.

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 Sep 15, 2024

Spec Test Coverage Analysis

Total Tested
496 302 (60.89 %)

Copy link
Contributor

github-actions bot commented Sep 15, 2024

Changes Analysis

Commit SHA: f746a39
Comparing To SHA: d4d4fa9

API Changes

Summary

├─┬Paths
│ ├──[➕] path (12229:3)
│ ├──[➕] path (3999:3)
│ ├──[➕] path (3818:3)
│ ├──[➕] path (12055:3)
│ ├──[➕] path (3740:3)
│ ├──[➕] path (11995:3)
│ ├──[➕] path (11874:3)
│ ├──[➕] path (12034:3)
│ ├──[➕] path (11794:3)
│ ├──[➕] path (4034:3)
│ ├──[➕] path (4053:3)
│ ├──[➕] path (3890:3)
│ ├──[➕] path (11708:3)
│ ├──[➕] path (11751:3)
│ ├──[➕] path (5035:3)
│ ├──[➕] path (3779:3)
│ ├─┬/_plugins/_ism/remove/{index}
│ │ └─┬POST
│ │   ├──[🔀] operationId (4018:20)❌ 
│ │   └─┬Parameters
│ │     ├──[➖] style (20486:14)
│ │     └──[🔀] in (21014:11)❌ 
│ ├─┬/_plugins/_ism/change_policy/{index}
│ │ └─┬POST
│ │   ├──[🔀] operationId (3800:20)❌ 
│ │   └─┬Parameters
│ │     ├──[➖] style (20434:14)
│ │     └──[🔀] in (20916:11)❌ 
│ ├─┬/_opendistro/_ism/add/{index}
│ │ └─┬POST
│ │   ├──[🔀] operationId (11731:20)❌ 
│ │   └─┬Parameters
│ │     ├──[➖] style (20424:14)
│ │     └──[🔀] in (20897:11)❌ 
│ ├─┬/_opendistro/_ism/policies/{policyID}
│ │ ├──[➕] head (11934:7)
│ │ └─┬GET
│ │   ├──[🔀] description (11918:20)
│ │   └─┬External Docs
│ │     └──[🔀] url (11920:14)
│ ├─┬/_plugins/_ism/add/{index}
│ │ └─┬POST
│ │   ├──[🔀] operationId (3761:20)❌ 
│ │   └─┬Parameters
│ │     ├──[➖] style (20424:14)
│ │     └──[🔀] in (20897:11)❌ 
│ ├─┬/_opendistro/_ism/change_policy/{index}
│ │ └─┬POST
│ │   ├──[🔀] operationId (11774:20)❌ 
│ │   └─┬Parameters
│ │     ├──[➖] style (20434:14)
│ │     └──[🔀] in (20916:11)❌ 
│ ├─┬/_opendistro/_ism/explain/{index}
│ │ ├──[➕] post (11855:7)
│ │ └─┬GET
│ │   ├──[➕] requestBody (25499:7)❌ 
│ │   ├──[🔀] description (11837:20)
│ │   ├──[🔀] operationId (11835:20)❌ 
│ │   └─┬Extensions
│ │     └──[🔀] x-operation-group (11836:26)
│ ├─┬/_plugins/_ism/explain/{index}
│ │ ├──[➕] post (3873:7)
│ │ └─┬GET
│ │   ├──[➕] requestBody (25499:7)❌ 
│ │   ├──[🔀] description (3857:20)
│ │   ├──[🔀] operationId (3855:20)❌ 
│ │   └─┬Extensions
│ │     └──[🔀] x-operation-group (3856:26)
│ ├─┬/_opendistro/_ism/remove/{index}
│ │ └─┬POST
│ │   ├──[🔀] operationId (12016:20)❌ 
│ │   └─┬Parameters
│ │     ├──[➖] style (20486:14)
│ │     └──[🔀] in (21014:11)❌ 
│ └─┬/_plugins/_ism/policies/{policy_id}
│   ├──[➕] head (3960:7)
│   └─┬GET
│     ├──[🔀] description (3946:20)
│     └─┬External Docs
│       └──[🔀] url (3948:14)
└─┬Components
  ├──[➕] responses (27918:7)
  ├──[➕] responses (27966:7)
  ├──[➕] responses (27922:7)
  ├──[➕] responses (27956:7)
  ├──[➕] responses (27941:7)
  ├──[➖] responses (27351:7)❌ 
  ├──[➕] responses (27931:7)
  ├──[➕] responses (27926:7)
  ├──[➕] requestBodies (25503:7)
  ├──[➕] requestBodies (25513:7)
  ├──[➕] requestBodies (25499:7)
  ├──[➖] parameters (20442:7)❌ 
  ├──[➕] parameters (20959:7)
  ├──[➕] parameters (20930:7)
  ├──[➕] parameters (20966:7)
  ├──[➕] parameters (20936:7)
  ├──[➕] parameters (21022:7)
  ├──[➕] parameters (20896:7)
  ├──[➕] parameters (20952:7)
  ├──[➕] parameters (21013:7)
  ├──[➕] parameters (21032:7)
  ├──[➕] parameters (20915:7)
  ├──[➕] parameters (20993:7)
  ├──[➕] schemas (49862:7)
  ├──[➕] schemas (49903:7)
  ├──[➕] schemas (49885:7)
  ├──[➕] schemas (49788:7)
  ├──[➕] schemas (49706:7)
  ├──[➕] schemas (49876:7)
  ├──[➕] schemas (49896:7)
  └─┬ism._common:GetPolicyResponse
    └──[🔀] $ref (49797:13)❌ 

Document Element Total Changes Breaking Changes
paths 50 16
components 31 3
  • BREAKING Changes: 19 out of 81
  • Modifications: 23
  • Removals: 8
  • Additions: 50
  • Breaking Removals: 2
  • Breaking Modifications: 15
  • Breaking Additions: 2

Report

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

API Coverage

Before After Δ
Covered (%) 560 (54.85 %) 584 (57.2 %) 24 (2.35 %)
Uncovered (%) 461 (45.15 %) 437 (42.8 %) -24 (-2.35 %)
Unknown 25 25 0

spec/namespaces/ism.yaml Outdated Show resolved Hide resolved
/_plugins/_ism/add:
post:
operationId: ism.add.0
x-operation-group: ism.add
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in the same group as ism.add_policy similar to {index}/_search and /_search

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as for change_policy they have different parameters, one specifies the index in the query, the other in the path.

/_plugins/_ism/remove:
post:
operationId: ism.remove.0
x-operation-group: ism.remove
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in the same group as ism.remove_policy

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as for change_policy they have different parameters, one specifies the index in the query, the other in the path.

Copy link
Collaborator

@nhtruong nhtruong Sep 16, 2024

Choose a reason for hiding this comment

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

That's correct. But as far as the clients users are concerned, which is what the operation group is for, where the index param is put in the URL request is irrelevant. We have many API actions, like search, that also allows the index to be specified as either a path param or a query param, the generated clients will favor use it as a path param regardless. In a generated client, client.ism.remove and client.ism_remove_policy will pretty much be identical to each other and it will lead to confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I had to add query to the one that doesn't really take it, but it won't matter from the client POV I think.

/_plugins/_ism/change_policy:
post:
operationId: ism.change.0
x-operation-group: ism.change
Copy link
Collaborator

@nhtruong nhtruong Sep 16, 2024

Choose a reason for hiding this comment

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

This should be in the same group as ism.change_policy

Copy link
Member Author

Choose a reason for hiding this comment

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

But they have different parameters, one specifies the index in the query, the other in the path. So if you try to make them the same group you get this:

{
  file: 'namespaces/ism.yaml',
  location: 'Operation Group: ism.change_policy',
  message: "2 'ism.change_policy' operations must have an identical set of query parameters."
}

Copy link
Collaborator

@nhtruong nhtruong Sep 16, 2024

Choose a reason for hiding this comment

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

I'm pretty sure you can still have index as a query string in the one that already accepts index in path, and the query string param will be ignored just like other API actions that allow this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Added HEAD /_plugins/_ism/policies/{policy_id}.
Added GET /_plugins/_ism/explain.
Added /_plugins/refresh_search_analyzers.
Added /_plugins/_ism/retry/{index}.

Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock requested a review from nhtruong September 17, 2024 13:55
@nhtruong nhtruong merged commit 3d49360 into opensearch-project:main Sep 17, 2024
18 checks passed
@dblock dblock deleted the ism-misc branch September 18, 2024 10:24
dblock added a commit to dblock/opensearch-api-specification that referenced this pull request Sep 23, 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.

[FEATURE] Add specs for ism namespace
2 participants