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

Adding specs for new _list/indices and _list/shards APIs #613

Merged
merged 14 commits into from
Nov 11, 2024

Conversation

gargharsh3134
Copy link
Contributor

Description

Adding specs for _list/indices and _list/shards APIs, new paginated alternates of _cat/indices and _cat/shards. Both the new list APIs will continue to support all the params supported by existing cat API counterparts in addition to three new parameters which are specific to pagination (namely next_token, size and order).

Related Opensearch changes: opensearch-project/OpenSearch#14258 & opensearch-project/OpenSearch#14257

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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

Spec Test Coverage Analysis

Total Tested
515 319 (61.94 %)

@dblock
Copy link
Member

dblock commented Oct 15, 2024

Looks good so far! Needs tests (will require updates for the 2.18 and 3.x builds we use in CI).

Signed-off-by: Harsh Garg <[email protected]>
Copy link
Contributor

github-actions bot commented Oct 17, 2024

Changes Analysis

Commit SHA: ca3218f
Comparing To SHA: e41257a

API Changes

Summary

├─┬Paths
│ ├──[➕] path (2804:3)
│ ├──[➕] path (2873:3)
│ ├──[➕] path (2787:3)
│ ├──[➕] path (2903:3)
│ └──[➕] path (2838:3)
└─┬Components
  ├──[➕] responses (29222:7)
  ├──[➕] responses (29276:7)
  ├──[➕] responses (29216:7)
  ├──[➕] parameters (22251:7)
  ├──[➕] parameters (22107:7)
  ├──[➕] parameters (22141:7)
  ├──[➕] parameters (22052:7)
  ├──[➕] parameters (22084:7)
  ├──[➕] parameters (22115:7)
  ├──[➕] parameters (22201:7)
  ├──[➕] parameters (22194:7)
  ├──[➕] parameters (22099:7)
  ├──[➕] parameters (22175:7)
  ├──[➕] parameters (22059:7)
  ├──[➕] parameters (22073:7)
  ├──[➕] parameters (22091:7)
  ├──[➕] parameters (22183:7)
  ├──[➕] parameters (22215:7)
  ├──[➕] parameters (22208:7)
  ├──[➕] parameters (22278:7)
  ├──[➕] parameters (22133:7)
  ├──[➕] parameters (22287:7)
  ├──[➕] parameters (22242:7)
  ├──[➕] parameters (22293:7)
  ├──[➕] parameters (22234:7)
  ├──[➕] parameters (22168:7)
  ├──[➕] parameters (22260:7)
  ├──[➕] parameters (22159:7)
  ├──[➕] parameters (22045:7)
  ├──[➕] parameters (22271:7)
  ├──[➕] parameters (22152:7)
  ├──[➕] parameters (22035:7)
  ├──[➕] parameters (22066:7)
  ├──[➕] parameters (22226:7)
  └──[➕] parameters (22124:7)

Document Element Total Changes Breaking Changes
paths 5 0
components 35 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/11779538587/artifacts/2171382582

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 35 40 5

@gargharsh3134
Copy link
Contributor Author

@dblock Can you please take a look again? Added tests taking inspiration from existing _cat APIs. Verified the tests by running them against Amazon Opensearch domain.

@gargharsh3134
Copy link
Contributor Author

The staging 2.18 distribution seems to be missing the _list APIs, and as a result Spec test in CI are failing.

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.

This is looking good!

The next version (2.18 and 3.0) distribution uses a specific SHA from https://hub.docker.com/r/opensearchstaging/opensearch/tags to remain stable. Update both to the latest ones that have the _list API, and make sure tests pass.

spec/namespaces/list.yaml Outdated Show resolved Hide resolved
spec/namespaces/list.yaml Outdated Show resolved Hide resolved
spec/namespaces/list.yaml Outdated Show resolved Hide resolved
spec/namespaces/list.yaml Outdated Show resolved Hide resolved
spec/namespaces/list.yaml Outdated Show resolved Hide resolved
spec/namespaces/list.yaml Outdated Show resolved Hide resolved
tests/default/list/indices.yaml Outdated Show resolved Hide resolved
tests/default/list/shards.yaml Outdated Show resolved Hide resolved
@gargharsh3134
Copy link
Contributor Author

@dblock Thanks for reviewing. I have addressed previous set of comments, please do take a look again whenever you get a chance. Also, sorry for the delay in between the revisions, got occupied with other urgent items.

On the tests, i have updated the staging distributions for 2.18 and 3.0 and the newly added list tests are passing. Not sure why some of the existing ones have started to fail. Trying to root cause.

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.

The 3.0 build gets generated even if not all plugins build with it, so looks like the update SHA doesn't have all the plugins? Try another more recent SHA until the tests all pass, possibly hunt down why the build is failing for the plugins that are failing (geospatial looks like).

spec/namespaces/list.yaml Outdated Show resolved Hide resolved
spec/namespaces/list.yaml Outdated Show resolved Hide resolved
@gargharsh3134
Copy link
Contributor Author

The 3.0 build gets generated even if not all plugins build with it, so looks like the update SHA doesn't have all the plugins? Try another more recent SHA until the tests all pass, possibly hunt down why the build is failing for the plugins that are failing (geospatial looks like).

Got it, I had sorted the distributions via the newest and used the latest one already. Can this staging build be triggered manually to generate a latest distribution?

@dblock
Copy link
Member

dblock commented Oct 30, 2024

The 3.0 build gets generated even if not all plugins build with it, so looks like the update SHA doesn't have all the plugins? Try another more recent SHA until the tests all pass, possibly hunt down why the build is failing for the plugins that are failing (geospatial looks like).

Got it, I had sorted the distributions via the newest and used the latest one already. Can this staging build be triggered manually to generate a latest distribution?

You should try with a newer SHA. The builds happen every day.

If the build is broken (other tests failing, please try another SHA or chase that down).

Harsh Garg added 2 commits November 4, 2024 18:01
@dblock
Copy link
Member

dblock commented Nov 4, 2024

Looks like the style checker is failing for good reason (use of indices instead of indexes, etc.).

@nhtruong
Copy link
Collaborator

nhtruong commented Nov 7, 2024

Looks like the style checker is failing for good reason (use of indices instead of indexes, etc.).

We have a namespace called indices that has been around since the beginning of time. Shouldn't it be the other way around where the use indices over indexes?

@dblock
Copy link
Member

dblock commented Nov 8, 2024

Looks like the style checker is failing for good reason (use of indices instead of indexes, etc.).

We have a namespace called indices that has been around since the beginning of time. Shouldn't it be the other way around where the use indices over indexes?

The descriptive text was standardized as "indexes", but technically it's "indices" in APIs and such I believe. Let's open an issue in docs about potentially changing that, but for now the style checker is correct.

Harsh Garg added 5 commits November 11, 2024 11:27
Signed-off-by: Harsh Garg <[email protected]>
Signed-off-by: Harsh Garg <[email protected]>
Signed-off-by: Harsh Garg <[email protected]>
Signed-off-by: Harsh Garg <[email protected]>
@gargharsh3134
Copy link
Contributor Author

Test run against 3.0.0 build is the only failing task. Newly added spec tests though are passing, the existing plugin related tests are failing.

@gargharsh3134
Copy link
Contributor Author

@dblock @nhtruong Can you please help with suggestions on 3.0.0 build? I have picked up the latest SHA as part of this PR.
Thanks!

@dblock
Copy link
Member

dblock commented Nov 11, 2024

Looks like we don't have a good 3.0 build since with all the plugins, hence the failures :(

Undo the 3.0 hash change and add version: '< 3.0' in the new list tests, maybe with a # TODO: re-enable in 3.0, see https://github.com/opensearch-project/opensearch-api-specification/pull/613.

@gargharsh3134
Copy link
Contributor Author

Undo the 3.0 hash change and add version: '< 3.0' in the new list tests, maybe with a # TODO: re-enable in 3.0, see #613.

@dblock Thanks for the suggestions. The tasks are green now, please take a look whenever you get a chance.
Thanks!

@dblock dblock merged commit 62f1f5f into opensearch-project:main Nov 11, 2024
24 checks passed
@dblock
Copy link
Member

dblock commented Nov 11, 2024

Good work @gargharsh3134, thanks for hanging in here!

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.

3 participants