-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Security Solution] Write additional metadata when generating types using kbn-openapi-generator
#174718
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
return sourcePath.replace(/\..+$/, '.gen.ts'); | ||
// Remove any double extension like `.schema.yaml` or `.schema.yml` and replace with `.gen.ts` | ||
const secondToLastDot = sourcePath.lastIndexOf('.', sourcePath.lastIndexOf('.') - 1); | ||
return `${sourcePath.substring(0, secondToLastDot)}.gen.ts`; |
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 fixing the firstname.
issue for some developers when running yarn openapi:generate
🙏
@elasticmachine merge upstream |
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 @spong for these enhancements and fixes to the openapi generator! 🙏
✅ Desk tested locally
LGTM 🚀
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.
Appreciate the fixes here for the firstname issue and improving the comments in generated code files!
I think we should reconsider whether the output directory option is necessary for the code generation though. To me it seems like additional complexity in both the generator script and the generated code structure. There's still a 1-1 correlation between .yaml
files and .gen.ts
files, but now the 2 files that correspond to each other might be in completely different places. For the use case here, I would argue that the .yaml
files for elastic assistant are part of the HTTP contract between client and server so they belong in common
the same way the generated .gen.ts
files do. What do you think?
.option('outputDir', { | ||
describe: 'Output directory for generated types', | ||
demandOption: false, | ||
type: 'string', | ||
}) |
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.
Commenting here @marshallmain so we can have a threaded discussion.
OG message:
Appreciate the fixes here for the firstname issue and improving the comments in generated code files!
I think we should reconsider whether the output directory option is necessary for the code generation though. To me it seems like additional complexity in both the generator script and the generated code structure. There's still a 1-1 correlation between .yaml files and .gen.ts files, but now the 2 files that correspond to each other might be in completely different places. For the use case here, I would argue that the .yaml files for elastic assistant are part of the HTTP contract between client and server so they belong in common the same way the generated .gen.ts files do. What do you think?
@andrew-goldstein and I had went back and forth on both approaches when reviewing this originating PR. I leaned toward keeping both in common
as you describe, and Andrew in keeping the schemas in plugin server
😅
I get both arguments, and so continued with this PR understanding that as we generate more outputs (route skeletons, mocks, hooks, client API, etc) we will need to support differing output destinations anyway. So in that sense, I figured the added complexity would be inevitable and so developer ergonomics could drive the actual decision here.
With those considerations in mind, does that seem like a reasonable path forward?
I am hoping to work both client api/hook and server route skeleton generation here in the near term (now that I'm more familiar with how everything works), and so would expand the outpurDir
functionality as part of those use cases. There would continue to be reasonable defaults (that would work 'out of the box' if shipping everything in single plugin), but also include optional output overrides (like here) to support the package<->plugin architecture, and for plugin architectures/directory structures that differ in any way.
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.
It seems clear to me that the API schemas as a common artifact should be separate from the (server) API routes and (public) client hooks, so I agree that picking output destinations for client api hooks and server route stubs will be good and necessary - but I think allowing a separate output directory for the generated schema files would still be extra complexity even if we need to allow output destinations for those other artifacts because each type of artifact will have different output destinations and likely different needs around the structure of the output destination.
It feels very natural to me to keep the schema yaml files right next to the corresponding generated TS files because they're so closely linked together, so it's easier to flip back and forth between yaml and .gen.ts to see the effects of changes, less configuration and script logic to maintain, and more standardization across plugins so devs can work across Kibana more easily.
Maybe @andrew-goldstein can share his perspective on why the yaml files belong in server
?
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.
Maybe @andrew-goldstein can share his perspective on why the yaml files belong in
server
?
In this use case, the yaml file (schema):
- Is the canonical definition of the
capabilities
server route, because thekbn-openapi-generator
takes a schema-first approach, where the schema is the authoritative definition of the server route. (For example, the typescript type describing the shape of the response returned by thecapabilities
server route, is derived / generated from the schema.) - The (one and only) implementation of the
capabilities
server route is in theelastic_assistant
plugin:x-pack/plugins/elastic_assistant/server/routes/capabilities/get_capabilities_route.ts
- In the originating PR, the schema for the
capabilities
route was (and still is) naturally defined inx-pack/plugins/elastic_assistant/server/schemas/capabilities/get_capabilities_route.schema.yaml
- I made this comment in the originating PR, requesting that the
GetCapabilitiesResponse
type, (generated from the schema), be moved to thekbn-elastic-assistant-common
package, because the generated type is required by both the clientpackages/kbn-elastic-assistant
, and the server implementationplugins/elastic_assistant/server
When viewed side-by-side, the paths in 2
and 3
above:
x-pack/plugins/elastic_assistant/server/routes/capabilities
x-pack/plugins/elastic_assistant/server/schemas/capabilities
are naturally co-located in x-pack/plugins/elastic_assistant/server
, because they are (respectively):
- The one-and-only implementation of the
capabilities
server route - The canonical schema definition of the
capabilities
server route
AFIK, the only driver to separate the implementation and schema above is an implementation detail: the GetCapabilitiesResponse
type derived from the schema is required by both the client (packages/kbn-elastic-assistant
) and server (plugins/elastic_assistant/server
).
In this use case, the outputDir
feature enables the server/routes
and server/schemas
to be (naturally) co-located in the same plugin, even though implementation details require the derived / generated types to live in a separate (common) package.
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.
I would say that the main driver for separating the route implementation from the yaml schema is that the schema defines the contract between API consumers and the API route while the API route defines only the server side portion of the API. In that sense, both the public and server code depend on the contract defined by the yaml schema, and that shows in the flow of dependencies: schema (yaml) -> generated schema (TS) -> API client/API route. If the yaml schema resides in server
, this means that the API client in public
ends up with a transitive dependency on files in server
which I don't think is good.
AFIK, the only driver to separate the implementation and schema above is an implementation detail: the GetCapabilitiesResponse type derived from the schema is required by both the client (packages/kbn-elastic-assistant) and server (plugins/elastic_assistant/server).
After writing the first paragraph above and comparing to what you wrote here, they're basically the same idea but with different conclusions. IMO if we're deriving a type from the yaml spec and then using that type in public
and server
, then the yaml spec must go in common
to avoid public
having a dependency on server
. I would think that, as a rule, if every server
folder were deleted we should still be able to build the client application with only the files in public
and common
.
If we're going to separate the OpenAPI specs from the generated zod schemas, to me it would make more sense to flip it around and put the yaml specs in common
and put the generated TS files in server
, as the public client does not care about the server side validation done by the schemas - it only cares about the HTTP API contract. As you mentioned, right now the public client does need the types from the generated zod schemas, so the zod schemas need to go in common as well. However, one of the additional goals we have for generation capabilities from OpenAPI specs is generating TS types directly from yaml, without having to use things like z.infer
. This can help avoid some implementation detail issues, such as TS type inference failing due to depth and difficulty with defining recursive schemas. This could also help here by allowing us to generate TS types for the API contract in common
but put the zod runtime validation schemas in server
. Even with this future capability, though, if the OpenAPI schemas are in server
then public
ends up with a transitive dependency on server
.
Let me know if you want to zoom to discuss this synchronously!
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.
Moving the schema to common
is reasonable. Also happy to discuss synchronously @marshallmain
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.
outputDir
removed in 4122a12, and *.schema.yaml
files (and corresponding scripts) moved to kbn-elastic-assistant-common
package.
@@ -6,5 +6,5 @@ source .buildkite/scripts/common/util.sh | |||
|
|||
echo --- Elastic Assistant OpenAPI Code Generation | |||
|
|||
(cd x-pack/plugins/elastic_assistant && yarn openapi:generate) | |||
(cd x-pack/packages/kbn-elastic-assistant-common && yarn openapi:generate) |
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.
outputDir
to kbn-openapi-generator
kbn-openapi-generator
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.
@YulNaumenko, here's the new location for the schema.yaml
's and corresponding types so they can be used in both the client package and server plugin. Codegen scripts have also been moved to kbn-elastic-assistant-common
, with documentation and instructions for running them over in the root readme: x-pack/packages/kbn-elastic-assistant-common/README.md
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.
I had some questions related to the move into a "common" package, but nothing that should be considered a blocker.
The output of the new info
fields looks good; I reviewed the EA endpoints specifically and they look correct 👍
@@ -7,12 +7,13 @@ | |||
|
|||
require('../../../../../src/setup_node_env'); | |||
const { bundle } = require('@kbn/openapi-bundler'); | |||
// eslint-disable-next-line import/no-nodejs-modules |
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.
Is there a higher-level way to get eslint to treat these files the same as they were previously? It seems like there's a discrepancy between how it treats plugin code vs package code. Unless this code could be run in a browser, and eslint
is correct?
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.
That's a good question! 🙂 I noticed the same thing and wasn't sure so was just following what folks had done in the other scripts. I thought it was interesting that the osquery
generate script (which is in a plugin) used @typescript-eslint/no-var-requires
:
Whereas the security_solution
generate script (also a plugin) didn't need it. And then of course as you mentioned, the package variants require import/no-nodejs-modules
.
Perhaps @jbudz can provide some insight here when he stops by for the operations review?
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 making the requested updates! Really appreciate the work here to improve on the generator script, and glad to see it is useful across a variety of plugins.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @spong |
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.
OPs changes lgtm
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.
Defend Workflows LGTM 👍 Thank you!
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.
Rule Management LGTM ✅ thanks for the upgrades to the OpenAPI generator.
Interesting discussion about the outputDir
and the placement of schema.yaml
vs gen.ts
files 👍
…sing `kbn-openapi-generator` (elastic#174718) ## Summary Follow up PR from elastic#174317 with the following fixes/enhancements to `kbn-openapi-generator`: * Fix extraneous `.`'s in paths causing generated files to not be written to disk * Updates `README.md` for latest method of adding CI actions * Adds `info` details to generated metadata comment for more easily tracing back to source schema * Moves assistant `*.schema.yaml` files from `elastic_assistant` plugin to `kbn-elastic-assistant-common` package > [!NOTE] > This PR includes a manual run of the `kbn-elastic-assistant-common` package `yarn openapi:generate` script as a reference example. Since this PR also updates the generation template to include the `info` metadata, CI will run the generator for the other consumers (`security_solution` & `osquery`) automatically, and commit those updates to this PR. <img width="16" src="https://user-images.githubusercontent.com/2946766/160040365-b1b8bb8a-d2d7-4187-b9b9-04817f8e2ae5.gif" /> ### Test instructions You can test against the `kbn-elastic-assistant-common` package using either the main CLI script from kibana root: ``` node scripts/generate_openapi --rootDir ./x-pack/packages/kbn-elastic-assistant-common ``` or via the yarn command: ``` cd x-pack/packages/kbn-elastic-assistant-common/ yarn openapi-generate ``` ### Checklist Delete any items that are not applicable to this PR. - [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…sing `kbn-openapi-generator` (elastic#174718) ## Summary Follow up PR from elastic#174317 with the following fixes/enhancements to `kbn-openapi-generator`: * Fix extraneous `.`'s in paths causing generated files to not be written to disk * Updates `README.md` for latest method of adding CI actions * Adds `info` details to generated metadata comment for more easily tracing back to source schema * Moves assistant `*.schema.yaml` files from `elastic_assistant` plugin to `kbn-elastic-assistant-common` package > [!NOTE] > This PR includes a manual run of the `kbn-elastic-assistant-common` package `yarn openapi:generate` script as a reference example. Since this PR also updates the generation template to include the `info` metadata, CI will run the generator for the other consumers (`security_solution` & `osquery`) automatically, and commit those updates to this PR. <img width="16" src="https://user-images.githubusercontent.com/2946766/160040365-b1b8bb8a-d2d7-4187-b9b9-04817f8e2ae5.gif" /> ### Test instructions You can test against the `kbn-elastic-assistant-common` package using either the main CLI script from kibana root: ``` node scripts/generate_openapi --rootDir ./x-pack/packages/kbn-elastic-assistant-common ``` or via the yarn command: ``` cd x-pack/packages/kbn-elastic-assistant-common/ yarn openapi-generate ``` ### Checklist Delete any items that are not applicable to this PR. - [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Follow up PR from #174317 with the following fixes/enhancements to
kbn-openapi-generator
:.
's in paths causing generated files to not be written to diskREADME.md
for latest method of adding CI actionsinfo
details to generated metadata comment for more easily tracing back to source schema*.schema.yaml
files fromelastic_assistant
plugin tokbn-elastic-assistant-common
packageNote
This PR includes a manual run of the
kbn-elastic-assistant-common
packageyarn openapi:generate
script as a reference example. Since this PR also updates the generation template to include theinfo
metadata, CI will run the generator for the other consumers (security_solution
&osquery
) automatically, and commit those updates to this PR.Test instructions
You can test against the
kbn-elastic-assistant-common
package using either the main CLI script from kibana root:or via the yarn command:
Checklist
Delete any items that are not applicable to this PR.