-
Notifications
You must be signed in to change notification settings - Fork 90
[api docs] increase schema validation strictness and start to fix issues #1633
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
base: master
Are you sure you want to change the base?
[api docs] increase schema validation strictness and start to fix issues #1633
Conversation
docs/.redocly.lint-ignore.yaml
Outdated
| @@ -0,0 +1,762 @@ | |||
| # This file instructs Redocly's linter to ignore the rules contained for specific parts of your API. | |||
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.
File created by (cd docs && npx --no @redocly/cli lint --extends=recommended api.yaml --generate-ignore-file) call. Having this file checked into repo enforces stricter rules on other contributions while working on improving existing problems.
Thanks - I really appreciate this approach 🙏 I will try to do a more detailed review this week; for now some quick queries:
I think this relates to getodk/central#1213 |
Makefile
Outdated
| .PHONY: api-docs-lint | ||
| api-docs-lint: | ||
| npx --no @redocly/cli lint --extends=minimal docs/api.yaml | ||
| (cd docs && npx --no @redocly/cli lint --extends=recommended api.yaml) |
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 reason to change the dir here?
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.
redocly hardcodes .redocly.lint-ignore.yaml file location to be in $PWD. cd allowed me to store this file along api.yaml, but after your comment that skipping entire rules would be better than ignoring individual errors in the file, I don't need .redocly.lint-ignore.yaml file now
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 think tracking individual rule-changes and the associated changes in docs/api.yaml will be eased by #1650, which defines the current config explicitly.
In the `info` section, then `description` is about 500 lines long. This makes it a little tricky to see other properties of the `info` section. This PR moves the `description` to the end of the `info` section. Noted while reviewing getodk#1633 and trying to understand what the `license` property belonged to.
In the `info` section, then `description` is about 500 lines long. This makes it a little tricky to see other properties of the `info` section. This PR moves the `description` to the end of the `info` section. Noted while reviewing #1633 and trying to understand what the `license` property belonged to.
2b93773 to
383ca9b
Compare
|
Hey @alxndrsn, Thanks for the initial review.
I like this idea! It can be implemented either as a
I don't think I can point single reference implementation that would confirm 100% spec compliance. Actually, there are many different implementations. It's my first time with Redocly, but it does comprehensive validation. For example, it validated and pointed out request examples that don't comply with the request schema. |
|
#1644 may cause conflicts with this PR, but the changes should be simple enough to iron out. |
@jbylina given this, and that your original aim is:
can you give any more info on how you're trying to generate the API client? |
|
@jbylina would the ruleset at https://github.com/Redocly/redocly-cli-cookbook/tree/main/rulesets/spec-compliant be a suitable target? It looks like there's a built-in It also seems like the current YAML parser that redocly uses is insufficient to enforce the full spec requirements, specifically re: Redocly/redocly-cli#1347. If this is important for consumers of the ODK docs then we could likely enforce this client easily with an additional check of our own. |
Per discussion on getodk#1633, it looks like redocly config needs to be more explicit. This PR extract the current `minimal` redocly config from source (https://github.com/Redocly/redocly-cli/blob/2367722a9fb173e55626dccbdac7b6694dd8b199/packages/core/src/config/minimal.ts#L52-L112) into a config file. This should simplify tracking of future changes to this config. Similarity to existing config can be confirmed with: ```sh $ diff <(npx --no @redocly/cli lint --extends=minimal docs/api.yaml 2>&1) <(npx --no @redocly/cli lint --config docs/redocly.conf.yaml docs/api.yaml 2>&1) 1,2d0 < No configurations were provided -- using built in recommended configuration by default. < 1447c1445 < docs/api.yaml: validated in 451ms --- > docs/api.yaml: validated in 415ms ```
Per discussion on #1633, it looks like `redocly` config needs to be more explicit. This PR extract the current "minimal" `redocly` config from source (https://github.com/Redocly/redocly-cli/blob/2367722a9fb173e55626dccbdac7b6694dd8b199/packages/core/src/config/minimal.ts#L52-L112) into a config file. This should simplify tracking of future changes to this config. Similarity to existing config can be confirmed with: ```sh $ diff <(npx --no @redocly/cli lint --extends=minimal docs/api.yaml 2>&1) <(npx --no @redocly/cli lint --config docs/redocly.conf.yaml docs/api.yaml 2>&1) 1,2d0 < No configurations were provided -- using built in recommended configuration by default. < 1447c1445 < docs/api.yaml: validated in 451ms --- > docs/api.yaml: validated in 415ms ```
|
#1688 may also help |
Hi,
Thanks all the great work you're doing here!! This is my first ever PR in this repo, so I will appreciate any extra guidance :)
I would like to improve the OpenAPI docs so that I can use them to generate an odk API client. Today's
docs/api.yamldoesn't comply with OpenAPI Specification v3.0.1 (as indicated in the first lines of the file). This seems to be a legacy of swagger-based api spec. #1593 introduces a great OpenAPI spec linter, starting with aminimalruleset. This PR aims to upgrade the ruleset torecommended, and slowly decrease the number of linting errors. On this PR,redoclyreports: 443 problems. I was able to get it down to164problems, which are mostly security-defined errors across all paths, but proposing such huge PR would be a reviewers' nightmare, so I would like to propose changes one by one.This PR introduces 3 api docs changes:
responses->Map[string, Response Object | Reference Object]) Eg. https://github.com/openapi-generators/openapi-python-client fails to open ODK spec due to this issue.licensenode following LICENSE fileIndividual FormdescriptionWhat has been done to verify that this works as intended?
CI
Why is this the best possible solution? Were any other approaches considered?
It's an improvement of the already set solution.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Updates artifact used by docs repo to render web docs. Initial change-set is minimal and shouldn't affect any user.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
It's a sole change to docs.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes