diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 1ff1f79..664de3a 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -208,18 +208,34 @@ impl Error { vec![messages.join("\n")] } - Error::CodeownershipFileIsStale { executable_name: _, diff } => { - if diff.is_empty() { - vec![] - } else { - vec![format!("The following changes are required (- current, + expected):\n{diff}")] - } - } + // The diff is intentionally *not* rendered as part of the error. It is + // surfaced separately as an informational message (see `Errors::info_messages`) + // so that a long diff doesn't bury the actionable headline. + Error::CodeownershipFileIsStale { .. } => vec![], Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)], } } } +impl Errors { + /// Supplementary detail that explains *what* is wrong without itself being an error. + /// The stale-CODEOWNERS diff is surfaced here, as informational output, rather than + /// inline with the error so that a long diff doesn't bury the actionable headline in + /// CI logs (and so the wrapping `code_ownership` gem raises only the headline rather + /// than the entire diff). + pub fn info_messages(&self) -> Vec { + self.0 + .iter() + .filter_map(|error| match error { + Error::CodeownershipFileIsStale { diff, .. } if !diff.is_empty() => { + Some(format!("The following changes are required (- current, + expected):\n{diff}")) + } + _ => None, + }) + .collect() + } +} + impl Display for Errors { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let grouped_errors = self.0.iter().into_group_map_by(|error| error.category()); diff --git a/src/runner.rs b/src/runner.rs index 6f6c688..e7704b4 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -128,6 +128,9 @@ impl Runner { match self.ownership.validate() { Ok(_) => RunResult::default(), Err(err) => RunResult { + // The stale-CODEOWNERS diff (if any) rides along as informational output, + // printed ahead of the errors, so the actionable headline isn't buried. + info_messages: err.info_messages(), validation_errors: vec![format!("{}", err)], ..Default::default() }, diff --git a/tests/executable_name_config_test.rs b/tests/executable_name_config_test.rs index e71da1d..4dc135d 100644 --- a/tests/executable_name_config_test.rs +++ b/tests/executable_name_config_test.rs @@ -41,8 +41,6 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box false, OutputStream::Stdout, predicate::eq(indoc! {r#" - - CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file The following changes are required (- current, + expected): -# Outdated content to trigger validation error -/app/old.rb @FooTeam @@ -57,6 +55,8 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box +/config/teams/foo.yml @FooTeam +/config/teams/payments.yml @PaymentTeam + CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file + "#}), )?; Ok(()) @@ -71,8 +71,6 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box Result<(), Box Result<(), Box> { false, OutputStream::Stdout, predicate::eq(indoc! {" - - CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file The following changes are required (- current, + expected): +# STOP! - DO NOT EDIT THIS FILE MANUALLY +# This file was automatically generated by \"bin/codeownership validate\". @@ -47,6 +45,8 @@ fn test_validate() -> Result<(), Box> { +# Team owned gems +/gems/payroll_calculator/**/** @PayrollTeam + CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file + Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways gems/payroll_calculator/calculator.rb