-
Notifications
You must be signed in to change notification settings - Fork 160
ci(openapi): Improve OpenAPI checks #2190
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
Conversation
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
I don't know, if Spectral doesn't take too much time, we can keep both 🤷
Looks like it's doing useful work then - unless it's the same as what Redocly reports
They are optional in most cases anyway, so no strong feelings. Which quotes produce the smaller diff in this PR?
cc @bliuchak as he added the original validation step. |
I have a slight preference towards double quotes, but I don't really care. I would see if there are more strings that need escaping single quotes or double quotes, and choose based on that. But whichever we choose, can we enforce them everywhere? I'd rather have it consistent than have to think "why are there quotes here but not here?" |
janbuchar
left a comment
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.
No way I'm gonna review all the changes, so I'm going to trust you 😁
package.json
Outdated
| "redoc:build:clean": "npm run redoc:build:clean:yaml && npm run redoc:build:clean:json", | ||
| "redoc:build:clean:yaml": "redocly bundle apify-api/openapi/openapi.yaml --skip-decorator=apify/legacy-doc-url-decorator --skip-decorator=apify/client-references-links-decorator --skip-decorator=apify/code-samples-decorator -o static/api/openapi.yaml", | ||
| "redoc:build:clean:json": "redocly bundle apify-api/openapi/openapi.yaml --skip-decorator=apify/legacy-doc-url-decorator --skip-decorator=apify/client-references-links-decorator --skip-decorator=apify/code-samples-decorator -o static/api/openapi.json", | ||
| "openapi:build": "npm run redoc:build:clean", |
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 feels like there are way too many package scripts now. Can we perhaps remove some of the older, redoc-prefixed ones? 😁
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.
Well, I didn't want to break commands people are already used to, but OK 😬.
All OpenAPI-related npm scripts are now grouped under the "openapi" prefix. Why? Because the workflow is no longer using only Redoc (it also uses Astral and ESLint for some commands).
Script mapping
redoc:start->openapi:previewredoc:build->openapi:bundleredoc:build:clean->openapi:buildredoc:build:clean:yaml->openapi:build:yamlredoc:build:clean:json->openapi:build:json
Removed scripts
redoc:test- This was justredocly lint && redoc:buildand was only used in CI, where it has been updated.redoc:test2- This referenced a non-existentbin/schemathesisscript and was not used anywhere.
If double quotes are configured but the string itself contains double quotes, for example And it enforces the use of quotes only where they are actually necessary.
You mean everywhere in this repository?
AFAIK this is not an example where YAML would require quotes - quoting is only necessary in cases where the value would otherwise be interpreted as a different type, for example So taking this together with the fact that it enforces the use of quotes only where they are necessary, this should be OK. |
|
Ah, thanks for the explanation, I thought it works differently 🤦 In that case I also don't care very much, whichever you prefer. |
- All OpenAPI-related npm scripts are now grouped under the "openapi" prefix, as the workflow is no longer using Redoc only (it also uses Spectral and ESLint for some commands). Script mapping - redoc:start -> openapi:preview - redoc:build -> openapi:bundle - redoc:build:clean -> openapi:build - redoc:build:clean:yaml -> openapi:build:yaml - redoc:build:clean:json -> openapi:build:json Removed scripts - redoc:test - This was just `redocly lint && redoc:build` and was only used in CI, where it has been updated. - redoc:test2 - This referenced a non-existent `bin/schemathesis` script and was not used anywhere.
fnesveda
left a comment
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 haven't checked the whole thing, just the important parts, and looks good 👍
|
Preview for this PR was built for commit |
Issue
Description
validatestep, where we validate the bundled specification with the Redocly and check its size.lint-check -> build -> validate.Testing
Note
Strengthens OpenAPI quality checks and streamlines CI.
OpenAPI checksworkflow (.github/workflows/openapi-ci.yaml) with Redocly/Spectral/YAML linting, bundle build/upload, and post-build validation (including bundle size check).github/workflows/openapi.yaml) and Go client codegen config (codegen/oapi-codegen-go/*), and cleans.gitignore.spectral.yaml) with rule overrides and adds YAML linting via ESLint (eslint.config.mjs) scoped toapify-api/**, with an exception foropenapi/openapi.yaml$refpaths, examples, required lists, and descriptionsWritten by Cursor Bugbot for commit b09198c. Configure here.