Skip to content

Commit

Permalink
[openapi-manager] richer extra validation
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@sunshowers
sunshowers committed Aug 17, 2024
1 parent 0131309 commit 3d621d5
Show file tree
Hide file tree
Showing 12 changed files with 442 additions and 126 deletions.
11 changes: 11 additions & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ members = [
"dev-tools/omicron-dev",
"dev-tools/omicron-dev-lib",
"dev-tools/openapi-manager",
"dev-tools/openapi-manager/types",
"dev-tools/oxlog",
"dev-tools/reconfigurator-cli",
"dev-tools/releng",
Expand Down Expand Up @@ -146,6 +147,7 @@ default-members = [
"dev-tools/omicron-dev",
"dev-tools/omicron-dev-lib",
"dev-tools/openapi-manager",
"dev-tools/openapi-manager/types",
"dev-tools/oxlog",
"dev-tools/reconfigurator-cli",
"dev-tools/releng",
Expand Down Expand Up @@ -456,6 +458,7 @@ oxlog = { path = "dev-tools/oxlog" }
oxnet = { git = "https://github.com/oxidecomputer/oxnet" }
once_cell = "1.19.0"
openapi-lint = { git = "https://github.com/oxidecomputer/openapi-lint", branch = "main" }
openapi-manager-types = { path = "dev-tools/openapi-manager/types" }
openapiv3 = "2.0.0"
# must match samael's crate!
openssl = "0.10"
Expand Down
1 change: 1 addition & 0 deletions dev-tools/openapi-manager/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ nexus-internal-api.workspace = true
omicron-workspace-hack.workspace = true
openapiv3.workspace = true
openapi-lint.workspace = true
openapi-manager-types.workspace = true
owo-colors.workspace = true
oximeter-api.workspace = true
serde_json.workspace = true
Expand Down
128 changes: 78 additions & 50 deletions dev-tools/openapi-manager/src/check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,16 @@
use std::{io::Write, process::ExitCode};

use anyhow::Result;
use camino::Utf8Path;
use indent_write::io::IndentWriter;
use owo_colors::OwoColorize;
use similar::TextDiff;

use crate::{
output::{
display_api_spec, display_error, display_summary, headers::*, plural,
write_diff, OutputOpts, Styles,
display_api_spec, display_api_spec_file, display_error,
display_summary, headers::*, plural, write_diff, OutputOpts, Styles,
},
spec::{all_apis, CheckStatus},
spec::{all_apis, CheckError, Environment},
FAILURE_EXIT_CODE, NEEDS_UPDATE_EXIT_CODE,
};

Expand All @@ -37,7 +36,7 @@ impl CheckResult {
}

pub(crate) fn check_impl(
dir: &Utf8Path,
env: &Environment,
output: &OutputOpts,
) -> Result<CheckResult> {
let mut styles = Styles::default();
Expand All @@ -48,6 +47,7 @@ pub(crate) fn check_impl(
let all_apis = all_apis();
let total = all_apis.len();
let count_width = total.to_string().len();
let count_section_indent = count_section_indent(count_width);
let continued_indent = continued_indent(count_width);

eprintln!("{:>HEADER_WIDTH$}", SEPARATOR);
Expand All @@ -59,56 +59,85 @@ pub(crate) fn check_impl(
plural::documents(total),
);
let mut num_up_to_date = 0;
let mut num_stale = 0;
let mut num_missing = 0;
let mut num_out_of_date = 0;
let mut num_failed = 0;

for (ix, spec) in all_apis.iter().enumerate() {
let count = ix + 1;

match spec.check(&dir) {
Ok(status) => match status {
CheckStatus::Ok(summary) => {
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}: {}",
UP_TO_DATE.style(styles.success_header),
display_api_spec(spec, &styles),
display_summary(&summary, &styles),
);
match spec.check(env) {
Ok(status) => {
let total_errors = status.total_errors();
let total_errors_width = total_errors.to_string().len();

if total_errors == 0 {
// Success case.
let extra = if status.extra_files_len() > 0 {
format!(
", {} extra files",
status.extra_files_len().style(styles.bold)
)
} else {
"".to_string()
};

num_up_to_date += 1;
}
CheckStatus::Stale { full_path, actual, expected } => {
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}",
STALE.style(styles.warning_header),
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}: {}{extra}",
UP_TO_DATE.style(styles.success_header),
display_api_spec(spec, &styles),
display_summary(&status.summary, &styles),
);

let diff = TextDiff::from_lines(&actual, &expected);
write_diff(
&diff,
&full_path,
&styles,
// Add an indent to align diff with the status message.
&mut IndentWriter::new(
&continued_indent,
std::io::stderr(),
),
)?;

num_stale += 1;
num_up_to_date += 1;
continue;
}
CheckStatus::Missing => {
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}",
MISSING.style(styles.warning_header),
display_api_spec(spec, &styles),
);

num_missing += 1;
// Out of date: print errors.
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}",
OUT_OF_DATE.style(styles.warning_header),
display_api_spec(spec, &styles),
);
num_out_of_date += 1;

for (error_ix, (spec_file, error)) in
status.iter_errors().enumerate()
{
let error_count = error_ix + 1;

match error {
CheckError::Stale { full_path, actual, expected } => {
eprintln!(
"{:>HEADER_WIDTH$}{count_section_indent}\
({error_count:>total_errors_width$}/{total_errors}) {}",
STALE.style(styles.warning_header),
display_api_spec_file(spec, spec_file, &styles),
);

let diff =
TextDiff::from_lines(&**actual, &**expected);
write_diff(
&diff,
&full_path,
&styles,
// Add an indent to align diff with the status message.
&mut IndentWriter::new(
&continued_indent,
std::io::stderr(),
),
)?;
}
CheckError::Missing => {
eprintln!(
"{:>HEADER_WIDTH$}{count_section_indent}\
({error_count:>total_errors_width$}/{total_errors}) {}",
MISSING.style(styles.warning_header),
display_api_spec_file(spec, spec_file, &styles),
);
}
}
}
},
}
Err(error) => {
eprint!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}",
Expand All @@ -131,20 +160,19 @@ pub(crate) fn check_impl(

let status_header = if num_failed > 0 {
FAILURE.style(styles.failure_header)
} else if num_stale > 0 {
STALE.style(styles.warning_header)
} else if num_out_of_date > 0 {
OUT_OF_DATE.style(styles.warning_header)
} else {
SUCCESS.style(styles.success_header)
};

eprintln!(
"{:>HEADER_WIDTH$} {} {} checked: {} up-to-date, {} stale, {} missing, {} failed",
"{:>HEADER_WIDTH$} {} {} checked: {} up-to-date, {} out-of-date, {} failed",
status_header,
total.style(styles.bold),
plural::documents(total),
num_up_to_date.style(styles.bold),
num_stale.style(styles.bold),
num_missing.style(styles.bold),
num_out_of_date.style(styles.bold),
num_failed.style(styles.bold),
);
if num_failed > 0 {
Expand All @@ -154,7 +182,7 @@ pub(crate) fn check_impl(
"cargo xtask openapi generate".style(styles.bold)
);
Ok(CheckResult::Failures)
} else if num_stale > 0 {
} else if num_out_of_date > 0 {
eprintln!(
"{:>HEADER_WIDTH$} (run {} to update)",
"",
Expand All @@ -170,14 +198,14 @@ pub(crate) fn check_impl(
mod tests {
use std::process::ExitCode;

use crate::spec::find_openapi_dir;
use crate::spec::Environment;

use super::*;

#[test]
fn check_apis_up_to_date() -> Result<ExitCode> {
let output = OutputOpts { color: clap::ColorChoice::Auto };
let dir = find_openapi_dir()?;
let dir = Environment::new(None)?;

let result = check_impl(&dir, &output)?;
Ok(result.to_exit_code())
Expand Down
8 changes: 4 additions & 4 deletions dev-tools/openapi-manager/src/dispatch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use clap::{Args, Parser, Subcommand};

use crate::{
check::check_impl, generate::generate_impl, list::list_impl,
output::OutputOpts, spec::openapi_dir,
output::OutputOpts, spec::Environment,
};

/// Manage OpenAPI specifications.
Expand Down Expand Up @@ -73,7 +73,7 @@ pub struct GenerateArgs {

impl GenerateArgs {
fn exec(self, output: &OutputOpts) -> anyhow::Result<ExitCode> {
let dir = openapi_dir(self.dir)?;
let dir = Environment::new(self.dir)?;
Ok(generate_impl(&dir, output)?.to_exit_code())
}
}
Expand All @@ -87,8 +87,8 @@ pub struct CheckArgs {

impl CheckArgs {
fn exec(self, output: &OutputOpts) -> anyhow::Result<ExitCode> {
let dir = openapi_dir(self.dir)?;
Ok(check_impl(&dir, output)?.to_exit_code())
let env = Environment::new(self.dir)?;
Ok(check_impl(&env, output)?.to_exit_code())
}
}

Expand Down
26 changes: 14 additions & 12 deletions dev-tools/openapi-manager/src/generate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use std::{io::Write, process::ExitCode};

use anyhow::Result;
use camino::Utf8Path;
use indent_write::io::IndentWriter;
use owo_colors::OwoColorize;

Expand All @@ -14,7 +13,7 @@ use crate::{
display_api_spec, display_error, display_summary, headers::*, plural,
OutputOpts, Styles,
},
spec::{all_apis, OverwriteStatus},
spec::{all_apis, Environment},
FAILURE_EXIT_CODE,
};

Expand All @@ -34,7 +33,7 @@ impl GenerateResult {
}

pub(crate) fn generate_impl(
dir: &Utf8Path,
env: &Environment,
output: &OutputOpts,
) -> Result<GenerateResult> {
let mut styles = Styles::default();
Expand Down Expand Up @@ -62,27 +61,30 @@ pub(crate) fn generate_impl(
for (ix, spec) in all_apis.iter().enumerate() {
let count = ix + 1;

match spec.overwrite(&dir) {
Ok((status, summary)) => match status {
OverwriteStatus::Updated => {
match spec.overwrite(env) {
Ok(status) => {
let updated_count = status.updated_count();

if updated_count > 0 {
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}: {}",
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}: {} ({} {} updated)",
UPDATED.style(styles.success_header),
display_api_spec(spec, &styles),
display_summary(&summary, &styles),
display_summary(&status.summary, &styles),
updated_count.style(styles.bold),
plural::files(updated_count),
);
num_updated += 1;
}
OverwriteStatus::Unchanged => {
} else {
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}: {}",
UNCHANGED.style(styles.unchanged_header),
display_api_spec(spec, &styles),
display_summary(&summary, &styles),
display_summary(&status.summary, &styles),
);
num_unchanged += 1;
}
},
}
Err(err) => {
eprintln!(
"{:>HEADER_WIDTH$} [{count:>count_width$}/{total}] {}",
Expand Down
Loading

0 comments on commit 3d621d5

Please sign in to comment.