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 generated Fleet API docs #2715

Merged
merged 11 commits into from
Mar 2, 2023
Merged

Conversation

kilfoyle
Copy link
Contributor

@kilfoyle kilfoyle commented Feb 27, 2023

As recommended here, this uses the process developed by @lcawl in elastic/kibana#142173 and follows on the testing work done by @dedemorton in elastic/kibana#146491 to add generated OpenAPI docs to the Fleet and Elastic Agent Guide.

  1. After merging, the generated OpenAPI docs will be accessible from the guide's Kibana Fleet APIs page. The generated docs are hidden from the ToC and search so that users won't come across them without any contextual info.

  2. A readme (essentially copying @lcawl's original) provides instructions to refresh the API docs by running the OpenAPI generator against the JSON spec in the Kibana repo, with the generated output going into the Fleet docs source in this repo.

  3. This PR removes the link we have going to the Fleet API docs in Swagger, but let me know, anyone, if you think we should still keep that as an additional way to view the API docs.

Preview is here.

@kilfoyle kilfoyle requested a review from kpollich February 27, 2023 17:08
@kilfoyle kilfoyle requested a review from a team as a code owner February 27, 2023 17:08
@github-actions
Copy link
Contributor

A documentation preview will be available soon:

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Feb 27, 2023
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Thanks for all your work so far on this! Left a few comments to hopefully continue moving forward here. So happy to see these landing in official Elastic docs ❤️

{{#apiInfo}}
{{#apis}}
{{#operations}}
<h4><a href="#{{baseName}}">{{baseName}}</a></h4>
Copy link
Member

Choose a reason for hiding this comment

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

Is this baseName variable where this Default string in the preview comes from?

image

If so, what controls this? Can the OpenAPI spec Fleet provides define something more meaningful that'll get rendered here?

Copy link
Contributor Author

@kilfoyle kilfoyle Feb 27, 2023

Choose a reason for hiding this comment

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

Similar to the above, it looks like we can just update the index.mustache file directly to adjust most of the formatting. I first changed the line <h4><a href="#{{baseName}}">{{baseName}}</a></h4> and confirmed that it does control that "Default" text. Then, I tried changing the text to something meaningful. But then, after adding tags to control the ordering (see comment) that text appeared above every endpoint. So, instead I just commented it out.

Comment on lines +8 to +15
<h2>Access</h2>
{{#hasAuthMethods}}
<ol>
{{#authMethods}}
<li>{{#isBasic}}HTTP Basic Authentication{{/isBasic}}{{#isOAuth}}OAuth AuthorizationUrl:{{authorizationUrl}}TokenUrl:{{tokenUrl}}{{/isOAuth}}{{#isApiKey}}APIKey KeyParamName:{{keyParamName}} KeyInQuery:{{isKeyInQuery}} KeyInHeader:{{isKeyInHeader}}{{/isApiKey}}</li>
{{/authMethods}}
</ol>
{{/hasAuthMethods}}
Copy link
Member

Choose a reason for hiding this comment

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

The "access" block seems to render a bit strangely, but maybe this is intentional? Seems like this might just be debugging info for now 🙂

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does look strange! It turns out that we can edit the index.mustache file directly (the warning at the top of the file says that it's generated, but my changes aren't overwritten when I regenerate the API docs). So, I just removed this section, or if we want it seems we can add anything we want here. I've pushed a new commit so the preview should refresh soon.


{{#apiInfo}}
{{#apis}}
{{#operations}}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way Fleet can control the order in which operations are rendered? It'd be great to order them the same way they appear in the spec file as we group endpoints by "namespace" - e.g. all routes prefixed with /epm are colocated, /setup appears first, all /agent_policy routes are colocated, etc.

image

Here's the order in the spec, for reference: https://github.com/elastic/kibana/blob/main/x-pack/plugins/fleet/common/openapi/entrypoint.yaml#L15-L153. Making this a predictably ordered list that aligns with the spec file would be awesome, so happy to do anything we can on Fleet's side to make that work.

Copy link
Contributor Author

@kilfoyle kilfoyle Feb 27, 2023

Choose a reason for hiding this comment

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

Happily, I have discovered that the output order is controlled by the tags in bundled.json. For example, I added "0001" to /setup, "0002" to /settings and and so on for the first ten or so endpoints (if that's the right term). Like this:

  "paths": {
    "/setup": {
      "post": {
        "summary": "Setup",
        "tags": ["0001"],
        "responses": {
          "200": {

The output now renders like this:
Screenshot 2023-02-27 at 4 01 32 PM

A couple of things I noticed:

  1. The post /health_check endpoint appears in the Fleet API docs if I give it a tag, and if I remove that tag, it disappears. Since I don't see that endpoint in the Swagger docs, I assume it's meant to be hidden. So, I guess we need to be careful about which endpoints have or don't have tags, assuming at least some aren't intended to be exposed in the docs.

  2. Some endpoints, such as "/agents/action_status" don't have a tag field at all, so perhaps one can be added.

@kpollich If you don't mind adjusting the spec to add tags I'd be very happy to retest.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for digging into this @kilfoyle - I think we can add some tags to group everything. Realistically, I don't care much about alphabetical order or anything as much as I care about grouping related endpoints together, which should be easily achievable by doing things like tags: ['agent-policies'] for each /agent_policies/* endpoint and so forth.

The post /health_check endpoint appears in the Fleet API docs if I give it a tag, and if I remove that tag, it disappears. Since I don't see that endpoint in the Swagger docs, I assume it's meant to be hidden.

Regarding /health_check - we probably don't want this documented in the public facing API spec, you're right. I'll probably just remove its path value from the spec.

I'll get a PR together with these changes and provide you a new spec file shortly. Many thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@kilfoyle - Here's that PR: elastic/kibana#152386 - please find the updated bundled.yaml or bundled.json attached to that PR and feel free to make a pass here with the updated spec. I took some time to clean up tags, descriptions, etc to hopefully group things consistently.

Here's an example with the official Swagger UI tool: https://petstore.swagger.io/?url=https://raw.githubusercontent.com/elastic/kibana/27b14796ddb26a390614396bfac1ba9190511786/x-pack/plugins/fleet/common/openapi/bundled.yaml#/. I'm sure we can go further in organizing here, but hopefully this represents a solid first step forward 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kpollich - Thanks for the terrific changes! I think the docs are looking great. I added the headers back in and they display the groups quite nicely.

I tested bundled.json and bundled.yaml. Both work nicely with exactly the same results.

One thing remaining would be to hide the "FleetInternals" section, but otherwise I think we're in pretty good shape now.

@elastic elastic deleted a comment from mergify bot Feb 27, 2023
@kilfoyle
Copy link
Contributor Author

@elasticmachine, run elasticsearch-ci/docs rebuild

kpollich added a commit to elastic/kibana that referenced this pull request Mar 1, 2023
## Summary

Ref elastic/observability-docs#2715

Adds relevant `tags` to all operations in order to help group them more
effectively in the generated docs.

---------

Co-authored-by: Kibana Machine <[email protected]>
@kpollich
Copy link
Member

kpollich commented Mar 1, 2023

The UI definitely looks better against the new spec, however I'm surprised we trim spaces and punctuation in tag names

image

This turns out especially poorly for the Elastic Package Manager (EPM) tag:

image

I assume this is part of the logic that's generating #AnchorLinks for each tag/section, but it'd be nice if our docs generation script could preserve the original tag name and render that as the HTML text node here, while using the "cleaned up" version of the tag name for the anchor link value.

Also, the sorting of individual operations seems like we could do better. Swagger UI honors the order we define in our entrypoint.yaml file that generates the eventual spec. It renders the operations in the order they appear in bundled.yaml.

image

If honoring the order in the spec is difficult, perhaps a sort on all the operation paths would suffice? e.g.

image

Not sure what pieces of this are controlled by our own process and which parts we're beholden to the OpenAPI generator for 🙂

@kilfoyle
Copy link
Contributor Author

kilfoyle commented Mar 2, 2023

@kpollich - I tried a few things but sadly without success:

  • Tested index.mustache to see if we can pull any other variables from the OpenAPI Generator CLI into the line <h4><a href="#{{baseName}}">{{baseName}}</a></h4>, but only baseName pushes any content at all into the HTML output. For reference, this is the list from which I tested other variables.
  • Checked the HTML Generator (the specific generator we're using in OpenAPI Generator) specs for any formatting options, but nothing there suits our needs.
  • Checked the Mustache 5 docs to see if we can implement any kind of sorting or string manipulation in index.mustache, but the only logic supported in a Mustache template file is show/hide based on whether or not a given variable is set.
  • Looked for examples where OpenAPI Generator is used to produce HTML API docs with complex tags, but in every example I found the tags are a single word (e.g. our own Kibana Cases APIs).
  • Checked for similar issues reported on Stack Exchange. No luck there either.
  • Checked HTML Generator code to see if that gives us any clues. I have zero knowledge of Java, but at least I can see that's where the baseName variable gets set (at line 721).

I'm certainly open to any ideas you may have! At this point though, the only options I can think of are to remove the tag names altogether, change them to be single words, or live with them in their current trimmed format.

On a more positive note, as @lcawl noted in the parent issue, these generated docs are planned only as a stop gap until we can get the API component working in the new docs system, so at least our solution here is not permanent (and probably not something we should invest a lot of time in). What do you think?

@kpollich
Copy link
Member

kpollich commented Mar 2, 2023

Thanks for the detailed investigation, @kilfoyle - since these docs are a stopgap and we're planning substantial improvements and a relocation in the future, I'm perfectly fine shipping them in the current state of this PR. I'll +1 it. Thanks again for all your work here!

@kilfoyle kilfoyle merged commit b2fa8db into elastic:main Mar 2, 2023
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
…2386)

## Summary

Ref elastic/observability-docs#2715

Adds relevant `tags` to all operations in order to help group them more
effectively in the generated docs.

---------

Co-authored-by: Kibana Machine <[email protected]>
bmorelli25 pushed a commit to bmorelli25/observability-docs that referenced this pull request Apr 11, 2023
* Add generated Fleet APIs

* Update to readme and paths to generate into Fleet repo

* Fix gen command; add ingored generator files

* Update readme

* Update readme

* Update readme

* Update readme

* Update APIs description page

* Fix up mustache file and regenerate

* Update output based on latest spec

* Update mustache file and output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants