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

Use _list/indices API instead of _cat/index API in CatIndexTool #3243

Merged
merged 21 commits into from
Mar 12, 2025

Conversation

zane-neo
Copy link
Collaborator

@zane-neo zane-neo commented Nov 29, 2024

Description

Use _list/indices API instead of _cat/index API in CatIndexTool

Related Issues

#3182

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

I don't see where we're properly testing the pagination here.

@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env December 4, 2024 07:23 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env December 4, 2024 07:23 — with GitHub Actions Inactive
@zane-neo zane-neo force-pushed the migrate-cat-index-tool branch from 580e3ba to ef75795 Compare December 4, 2024 07:52
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env December 4, 2024 07:53 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env December 4, 2024 07:53 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env December 4, 2024 08:51 — with GitHub Actions Inactive
@ylwu-amzn
Copy link
Collaborator

Add a comment on the issue #3182 (comment)

Why not build another ListIndexTool?
List index API introduced in 2.18 https://opensearch.org/docs/latest/api-reference/list/list-indices/. Seems hard to backport fix/enhance of CatIndexTool to older versions by changing it to _list/indices ,

Any thoughts, @zane-neo , @dbwiddis ?

@dbwiddis
Copy link
Member

dbwiddis commented Dec 4, 2024

Any thoughts, @zane-neo , @dbwiddis ?

Agreed a new tool would be cleaner. RestIndicesListAction extends RestIndicesAction (that Cat Index uses) so it'd be better to logically extend the other tool with just the updated parameters.

zane-neo added 12 commits March 7, 2025 13:37
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
Signed-off-by: zane-neo <zaniu@amazon.com>
@zane-neo zane-neo force-pushed the migrate-cat-index-tool branch from 9d643f9 to 1b5d510 Compare March 7, 2025 05:39
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env March 7, 2025 05:40 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env March 7, 2025 05:40 — with GitHub Actions Error
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env March 7, 2025 05:40 — with GitHub Actions Error
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env March 7, 2025 05:40 — with GitHub Actions Failure
@zane-neo
Copy link
Collaborator Author

Sorry I'm not comfortable to merger a PR without passing any integration test suite. This is not a good practice IMO.

@dhrubo-os I've moved the failure assertion part of the ITs, now the failing ITs are not related to my change, please help approve, thanks.

@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env March 11, 2025 02:23 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env March 11, 2025 04:22 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env March 11, 2025 04:22 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env March 11, 2025 04:22 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env March 11, 2025 04:22 — with GitHub Actions Inactive
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env March 11, 2025 05:24 — with GitHub Actions Failure
@zane-neo zane-neo merged commit 17a9037 into opensearch-project:main Mar 12, 2025
11 of 14 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3243-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17a903736bafe2494795acb0481950af5899143a
# Push it to GitHub
git push --set-upstream origin backport/backport-3243-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3243-to-2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants