-
Notifications
You must be signed in to change notification settings - Fork 48
Add alias and data stream to list tool #108
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sun Ro Lee <sunro.lee@linecorp.com>
b18758f to
77912cc
Compare
rithin-pullela-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising the PR @ssunno, I believe this is a step in the right direction. Added few comments.
| if not isinstance(response, list): | ||
| logger.warning('Unexpected response type for cat.indices: %s', type(response)) | ||
| return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user is not authorized to perform the request, we might get an error response. This error should be propagated to the client making request in some form. But this would return an empty list
| if not isinstance(response, dict): | ||
| logger.warning('Unexpected response type for indices.get_alias: %s', type(response)) | ||
| return {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, should we handle it differently?
| if not isinstance(response, dict): | ||
| logger.warning('Unexpected response type for indices.get_data_stream: %s', type(response)) | ||
| return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| # List all the helper functions, these functions perform a single rest call to opensearch | ||
| # these functions will be used in tools folder to eventually write more complex tools | ||
| def list_indices(args: ListIndicesArgs) -> json: | ||
| def list_indices(args: ListIndicesArgs) -> list[dict]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
Would not block this PR, but something to perhaps think of
On a high level for list_indices, list_aliases, and list_data_streams
Would it make sense to create a class for the responses and parse the response into the classes? We can perhaps throw some sort of error if parsing fails. That way we can skip doing checks like isInstance etc?
Description
This PR add alias and data stream to return of
ListIndicesTool.The
aliasesanddata streamsare concepts that group multiple indices. But current version ofListIndicesTooldoes not support them, so it returns all indices equally, exposing the backing indices of data streams directly. To apply the intention of index designers, aliases and data streams should be returned as separate groups.Note: Indices that belong to an alias or a data stream are excluded from the general index list to avoid duplication.
When
include_detail = False, aliases and data streams return only the number of indices instead of their names.example of
ListIndicesTool(include_detail = False):Issues Resolved
Resolves #56
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.