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 automation for first two CAT APIs #8930

Merged
merged 6 commits into from
Dec 19, 2024
Merged

Add automation for first two CAT APIs #8930

merged 6 commits into from
Dec 19, 2024

Conversation

Naarcha-AWS
Copy link
Collaborator

@Naarcha-AWS Naarcha-AWS commented Dec 11, 2024

Adds automation for first two CAT APIs

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Editorial review -> Merged.

Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer.

When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). The doc reviewer will arrange for an editorial review.

@Naarcha-AWS Naarcha-AWS added backport 2.18 PR: Backport label for 2.18 4 - Doc review PR: Doc review in progress labels Dec 11, 2024
Copy link
Collaborator

@kolchfa-aws kolchfa-aws left a comment

Choose a reason for hiding this comment

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

Some suggestions to clarify the descriptions. Please include default values where appropriate. Thanks.

api: cat.aliases
component: paths_and_http_methods
-->
## Paths and HTTP methods
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Paths and HTTP methods
## Path and HTTP methods

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want Paths when there many and Path when there's one?
Or do want a universal one? If it's universal then Paths and HTTP methods makes more sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer Paths and HTTP methods. If we need to change to the singular (which in this case we don't, since multiple path types exist), we can exclude the header and modify it in the options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nhtruong: @kolchfa-aws and I discussed it. Path would be more accurate than Paths since each x-operation-id usually encompasses one path in the API. We'll use omit_header if the writer chooses to include multiple paths on a single page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Multiple paths on a single page would still be documented as different APIs and thus be in different sections.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use Path and HTTP methods per the API Style Guide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use "Endpoints" because "API operation" is not specific enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My primary concern here is consistency. If we want to use "Endpoints", that's fine, but let's reflect that in the template as well as in the relevant docs. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll change the template and the wording in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR is up #8959

Parameter | Type | Description
:--- | :--- | :---
local | Boolean | Whether to return information from the local node only instead of from the cluster manager node. Default is `false`.
expand_wildcards | Enum | Expands wildcard expressions to concrete indexes. Combine multiple values with commas. Supported values are `all`, `open`, `closed`, `hidden`, and `none`. Default is `open`.
`expand_wildcards` | String / String / String / String / List | Whether to expand wildcard expression to concrete indexes that are open, closed or both.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are there 4 instances of String in the type? Also, as a rule, we don't want slashes to signify "or". I would suggest "String or list". Also, the old description had supported values, which is helpful, and specified that the list should be comma-separated. The new description has less information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nhtruong: IS there something in the spec that would cause this?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's coming form the spec. I'll update the code to handle this edge case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. Let me know once that update is through and I'll update this PR with the updated descriptions.

bytes | Byte size | Specify the units for byte size. For example, `7kb` or `6gb`. For more information, see [Supported units]({{site.url}}{{site.baseurl}}/opensearch/units/).
local | Boolean | Whether to return information from the local node only instead of from the cluster manager node. Default is `false`.
cluster_manager_timeout | Time | The amount of time to wait for a connection to the cluster manager node. Default is 30 seconds.
`bytes` | String | The unit used to display byte values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`bytes` | String | The unit used to display byte values.
`bytes` | String | The units used to display byte values.

local | Boolean | Whether to return information from the local node only instead of from the cluster manager node. Default is `false`.
cluster_manager_timeout | Time | The amount of time to wait for a connection to the cluster manager node. Default is 30 seconds.
`bytes` | String | The unit used to display byte values.
`cluster_manager_timeout` | String | Operation timeout for connection to cluster-manager node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`cluster_manager_timeout` | String | Operation timeout for connection to cluster-manager node.
`cluster_manager_timeout` | String | A timeout for connection to the cluster manager node.

cluster_manager_timeout | Time | The amount of time to wait for a connection to the cluster manager node. Default is 30 seconds.
`bytes` | String | The unit used to display byte values.
`cluster_manager_timeout` | String | Operation timeout for connection to cluster-manager node.
`format` | String | A short version of the Accept header (for example, `json`, `yaml`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`format` | String | A short version of the Accept header (for example, `json`, `yaml`).
`format` | String | A short version of the HTTP accept header (for example, `json` or `yaml`).

`format` | String | A short version of the Accept header (for example, `json`, `yaml`).
`h` | List | Comma-separated list of column names to display.
`help` | Boolean | Return help information.
`local` | Boolean | Return local information, do not retrieve the state from cluster-manager node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`local` | Boolean | Return local information, do not retrieve the state from cluster-manager node.
`local` | Boolean | If `true`, specifies to retrieve information only from the local node. If `false`, specifies to retrieve local information from the cluster manager node.

`h` | List | Comma-separated list of column names to display.
`help` | Boolean | Return help information.
`local` | Boolean | Return local information, do not retrieve the state from cluster-manager node.
`s` | List | Comma-separated list of column names or column aliases to sort by.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`s` | List | Comma-separated list of column names or column aliases to sort by.
`s` | List | A comma-separated list of column names or column aliases to sort the response.

`help` | Boolean | Return help information.
`local` | Boolean | Return local information, do not retrieve the state from cluster-manager node.
`s` | List | Comma-separated list of column names or column aliases to sort by.
`v` | Boolean | Verbose mode. Display column headers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`v` | Boolean | Verbose mode. Display column headers.
`v` | Boolean | Whether to display column headers.

api: cat.allocation
component: query_parameters
-->
## Query parameters
Parameter | Type | Description
:--- | :--- | :---
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to specify defaults for the query parameters that have them (e.g., local).

api: cat.cluster_manager
component: query_parameters
-->
## Query parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as above.

@Naarcha-AWS Naarcha-AWS changed the title Add automation for first three CAT APIs Add automation for first two CAT APIs Dec 19, 2024
_api-reference/cat/cat-allocation.md Outdated Show resolved Hide resolved
Co-authored-by: kolchfa-aws <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
@Naarcha-AWS Naarcha-AWS merged commit 1006421 into main Dec 19, 2024
6 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 19, 2024
* Add automation for first three CAT APIs

Signed-off-by: Archer <[email protected]>

* Add pointers for Aliases and Allocation. Remove for cluster_manager.

Signed-off-by: Archer <[email protected]>

* Add updated specs

Signed-off-by: Archer <[email protected]>

* Update _api-reference/cat/cat-allocation.md

Co-authored-by: kolchfa-aws <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>

---------

Signed-off-by: Archer <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
Co-authored-by: kolchfa-aws <[email protected]>
(cherry picked from commit 1006421)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Doc review PR: Doc review in progress backport 2.18 PR: Backport label for 2.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants