-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] OpenAPI linter #171851
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Looks good, tested adding an x-modify
prop and I see the linter failing:
LGTM 👍
One question though: I see that the Redocly CLI tool has a tool for bundling multiple OpenAPI files. Was that considered and discarded for some reason before creating the package in-house in #171526?
Right, there is |
749dc0d
to
ddd05aa
Compare
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.
Tested locally and got 95 linter warnings and 0 errors which makes the lining script succeed. Looks great @maximpn!
I do have a couple of suggestions though, please check my comments.
.buildkite/scripts/steps/code_generation/security_solution_codegen.sh
Outdated
Show resolved
Hide resolved
81b1d70
to
01294a4
Compare
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.
Using npx
in a scenario like this might have unintended side-effects and can be a potential security risk. I prefer if you call the installed "binary" directly without invoking it through npx
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.
@elastic/security-defend-workflows related changes (which is not much actually) look good 💯
(also, script runs nicely, was able to see the mentioned 95 warnings and fix one)
@maximpn Just in case you're watching this PR, I'm trying to fix CI and force-pushing into your origin branch. |
4b8d0aa
to
be9f265
Compare
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.
Changes LGTM - just found a tiny yarn.lock
nit 😊
be9f265
to
0c89d85
Compare
f8bf3b7
to
c50f445
Compare
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.
Entity Analytics changes LGTM 👍
7d50345
to
7be633f
Compare
39cb454
to
c4fec96
Compare
c4fec96
to
b5e12cd
Compare
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @maximpn |
I reviewed the changes requested by watson and verified they have all been addressed. I dismissed the review to unblock this PR. |
Resolves: https://github.com/elastic/security-team/issues/8099
Summary
This PR adds an a command to lint OpenAPI specs defined in Security Solution plugin.
Details
We have a number of OpenAPI specs defined in Security Solution plugin. While
@kbn/openapi-generator
package processes the specs and makes sure the specs are parsable and processable we don't have proper specs linter set up.This PR introduces OpenAPI specs linting by leveraging Redocly CLI's
lint
command with a custom configuration placed in@kbn/openapi-generator
package . Configuration includes reasonable best practices by using built-in Redocly rules.The lint utility fulfil the following requirements
3.0
and3.1
.x-modify
property to have onlypartial
,required
orrequiredOptional
values.The lint utility has been incorporated into the existing OpenAPI generator, and linting is performed before generation.
Tool selection
Swagger CLI is a well known tool to validate OpenAPI specs. On November 15th 2023 the repo has been archived with a message in README
Taking it into account choice falls on Redocly CLI.
How to test?
Change directory to Security Solution plugin's root and run linting by using the following commands from Kibana root
cd x-pack/plugins/security_solution yarn openapi:generate