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

[4/6] [openapi-manager] richer extra validation #6370

Merged

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Aug 17, 2024

With the Nexus external API, the validator generates a nexus_tags.txt file
that must be kept track of. Instead of the validation function simply erroring
out if the file is different, it is a better experience for users if it records
what it expects the file to be, and then the OpenAPI manager simply treats it
as an extra file similar to the document itself.

With this pattern, the check and generate functions can both work on the extra
file just like they work on the document.

In order for the there to be a richer protocol for validation, the interface
needs to be split into its own crate. This way, the API crates can depend on
this minimal interface, and the OpenAPI manager itself can depend on the API
crates.

This isn't used yet, but will be in #6373.

Created using spr 1.3.6-beta.1
sunshowers added a commit to sunshowers/omicron that referenced this pull request Aug 17, 2024
With the Nexus external API, the validator generates a `nexus_tags.txt` file
that must be kept track of.  Instead of the validation function simply erroring
out if the file is different, it is a better experience for users if it records
what it expects the file to be, and then the OpenAPI manager simply treats it
as an extra file similar to the document itself.

With this pattern, the check and generate functions can both work on the extra
file just like they work on the document.

In order for the there to be a richer protocol for validation, the interface
needs to be split into its own crate. This way, the API crates can depend on
this minimal interface, and the OpenAPI manager itself can depend on the API
crates.

Pull Request: oxidecomputer#6370
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the title [openapi-manager] richer extra validation [4/6] [openapi-manager] richer extra validation Aug 17, 2024
@sunshowers sunshowers requested review from hawkw and ahl August 17, 2024 03:23
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

My understanding of what validation is actually being performed here is fairly limited, but the code seems fine to me overall! I had a few minor comments.

dev-tools/openapi-manager/src/check.rs Outdated Show resolved Hide resolved
dev-tools/openapi-manager/src/output.rs Show resolved Hide resolved
dev-tools/openapi-manager/src/output.rs Outdated Show resolved Hide resolved
@sunshowers sunshowers enabled auto-merge (squash) August 21, 2024 04:17
@sunshowers sunshowers merged commit 6dde3f8 into main Aug 21, 2024
24 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/openapi-manager-richer-extra-validation branch August 21, 2024 05:31
sunshowers added a commit that referenced this pull request Aug 28, 2024
At long last, here's the last OpenAPI document converted into a trait: the
Nexus external API. This is a fairly large change, but with all the
prerequisites in place the conversion is overall pretty mechanical. I did need
to move some types around, but the bulk of the PR is just adding the very large
external API trait.

This PR also adds a `.git-blame-ignore-revs` entry corresponding to #6372, so that the indent doesn't ruin blame in the GitHub web view. (You can configure this locally as well with `git config blame.ignoreRevsFile .git-blame-ignore-revs`.)

In a subsequent PR I'll remove the old instructions.

Depends on:

* #6355
* #6364 
* #6369
* #6370
* #6372

Co-authored-by: David Crespo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants