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

[HTTP/OAS] Add descriptions for Logstash pipeline APIs #184266

Merged
merged 11 commits into from
May 29, 2024

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented May 24, 2024

Summary

Relates to #180056

This PR adds operation summaries for the Logstash configuration management APIs in the generated OpenAPI document.

NOTE: I've omitted the "access: public" from the "delete pipelines" and "get all pipelines" operations, since those don't currently exist in the documentation. I believe this means they'll use the default "internal" access value.

How to test

Per #181277:

  1. Start Elasticsearch (e.g. yarn es snapshot --license trial)
  2. Add server.oas.enabled: true to kibana.dev.yml
  3. Start Kibana (e.g. yarn start --no-base-path)
  4. curl -s -u elastic http://localhost:5601/api/oas\?pathStartsWith\=/api/logstash/pipeline | jq > test.json

The output will contain info like this:

   "paths": {
    "/api/logstash/pipeline/{id}": {
      "delete": {
        "summary": "Delete a Logstash pipeline",

@lcawl lcawl added Feature:Logstash Pipelines Logstash Pipeline UI related release_note:skip Skip the PR/issue when compiling release notes docs v8.15.0 labels May 24, 2024
@lcawl lcawl requested a review from karenzone May 24, 2024 21:48
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@lcawl lcawl marked this pull request as ready for review May 24, 2024 22:55
@lcawl lcawl requested a review from a team as a code owner May 24, 2024 22:55
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

I left some suggestions inline for consideration. Do we have a character limit? The fact that these APIs work only for centrally-managed Logstash pipelines is significant.

x-pack/plugins/logstash/server/routes/pipeline/delete.ts Outdated Show resolved Hide resolved
x-pack/plugins/logstash/server/routes/pipeline/load.ts Outdated Show resolved Hide resolved
x-pack/plugins/logstash/server/routes/pipeline/save.ts Outdated Show resolved Hide resolved
x-pack/plugins/logstash/server/routes/pipelines/list.ts Outdated Show resolved Hide resolved
@lcawl
Copy link
Contributor Author

lcawl commented May 28, 2024

Do we have a character limit? The fact that these APIs work only for centrally-managed Logstash pipelines is significant.

Yes, this is the "operation summary" which is used as the title in the navigation bar, so we've added a linting rule to limit its length. Originally we were linting for strings to be less than 30, though I've updated that now to go as much as 45.

If all of these APIs apply to the same type of pipeline, it would be my preference to cover that either at the group level (in a "tag" description) or in the API detailed description and leave these nav titles as short as they are in the existing docs.
The functionality to add tag descriptions and detailed operation descriptions are currently outstanding per #182649 and #182779

It's also worth calling out that our guidelines for these summaries is that they "use common verbs like Get, Update, Delete whenever possible" so the suggested changes to "Retrieve" and "List" would fail a linting rule too.

@karenzone
Copy link
Contributor

Thanks for explaining the constraints. I modified my comments to use standard terminology and to respect the character limit... assuming that only display text counts against character limits. :-) Please LMKWYT

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

LGTM (pending passing ci). :-) Thanks for your work on this, @lcawl

@lcawl lcawl enabled auto-merge (squash) May 29, 2024 18:43
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@lcawl lcawl merged commit b34ff52 into elastic:main May 29, 2024
23 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting docs Feature:Logstash Pipelines Logstash Pipeline UI related release_note:skip Skip the PR/issue when compiling release notes v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants