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

CAT API automation (Part II) #9060

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

CAT API automation (Part II) #9060

wants to merge 13 commits into from

Conversation

Naarcha-AWS
Copy link
Collaborator

@Naarcha-AWS Naarcha-AWS commented Jan 13, 2025

This PR also fixes the previous automation markers with the latest changes to spec-insert tool..

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.

@Naarcha-AWS Naarcha-AWS added the 4 - Doc review PR: Doc review in progress label Jan 13, 2025
@Naarcha-AWS Naarcha-AWS self-assigned this Jan 13, 2025
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 marked this pull request as ready for review January 13, 2025 22:46
@Naarcha-AWS Naarcha-AWS added the backport 2.18 PR: Backport label for 2.18 label Jan 13, 2025
@Naarcha-AWS Naarcha-AWS mentioned this pull request Jan 13, 2025
26 tasks
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.

The addition of the default column results in incorrect formatting.


The following table lists the available query parameters. All query parameters are optional.

Parameter | Data type | Description | Default
Copy link
Collaborator

@kolchfa-aws kolchfa-aws Jan 14, 2025

Choose a reason for hiding this comment

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

The "Default" column is not populated for some rows so the table is not displayed correctly (in all these files).

Screenshot 2025-01-14 at 8 55 55 AM

Also, since the table is automatically generated, could we have consistency in pipe symbols: either no pipe symbols on either end of the row, or pipe symbols on both ends of the row (the latter is more extensible).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense to add |'s at the end and the beginning to me. @nhtruong, would that be difficult to change in the workflow?

The pretty setting will add pipes but also add additional spacing. One solution is we could enable that by default, since the tables aren't going to be touched directly anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just add pipe symbols regardless of pretty. Also, there should be no empty cells in the table. Where there is no default, we should add N/A. However, it's strange that there was no default automatically provided for expand_wildcards. Is it missing from the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not difficult. Are you saying with pretty: false (the default), the pipes are never present and with pretty: true they are there? OR are you saying that with pretty: false the pipes are there sometimes (this inconsistency would be a bug)

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding Default values not showing: Are they on the spec but not in the generated tables?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would go with adding pipes at the ends in any case (for pretty either true or false). This way, all tables are consistent (have pipes at either end at all times).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, according to the expected output, pretty: true does include pipes at the end and the beginning.

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.

3 participants