diff --git a/Cargo.lock b/Cargo.lock index 6dfce14..f944b14 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -203,6 +203,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml", + "similar", "tempfile", "tracing", "tracing-subscriber", @@ -868,6 +869,12 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "smallvec" version = "1.10.0" diff --git a/Cargo.toml b/Cargo.toml index 13916f7..dd093c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,7 @@ regex = "1.11.1" serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.143" serde_yaml = "0.9.34" +similar = "2.6.0" tempfile = "3.21.0" tracing = "0.1.41" tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 16916c5..1ff1f79 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -9,6 +9,7 @@ use error_stack::Context; use itertools::Itertools; use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; +use similar::{ChangeTag, TextDiff}; use tracing::debug; use tracing::instrument; @@ -29,7 +30,7 @@ enum Error { InvalidTeam { name: String, path: PathBuf }, FileWithoutOwner { path: PathBuf }, FileWithMultipleOwners { path: PathBuf, owners: Vec }, - CodeownershipFileIsStale { executable_name: String }, + CodeownershipFileIsStale { executable_name: String, diff: String }, } #[derive(Debug)] @@ -127,20 +128,15 @@ impl Validator { fn validate_codeowners_file(&self) -> Vec { let generated_file = self.file_generator.generate_file(); + let current_file = self.project.get_codeowners_file().unwrap_or_default(); - match self.project.get_codeowners_file() { - Ok(current_file) => { - if generated_file != current_file { - vec![Error::CodeownershipFileIsStale { - executable_name: self.executable_name.to_string(), - }] - } else { - vec![] - } - } - Err(_) => vec![Error::CodeownershipFileIsStale { + if generated_file == current_file { + vec![] + } else { + vec![Error::CodeownershipFileIsStale { executable_name: self.executable_name.to_string(), - }], + diff: codeowners_diff(¤t_file, &generated_file), + }] } } @@ -163,12 +159,32 @@ impl Validator { } } +/// Builds a line-oriented diff between the current (on-disk) CODEOWNERS file and the +/// freshly generated one, so that validation failures explain *what* is out of date +/// rather than just *that* it is. Only changed lines are emitted: removals (present +/// on disk but no longer expected) are prefixed with `-` and additions (expected but +/// missing) are prefixed with `+`. +fn codeowners_diff(current: &str, generated: &str) -> String { + let diff = TextDiff::from_lines(current, generated); + + diff.iter_all_changes() + .filter_map(|change| { + let line = change.value().trim_end_matches('\n'); + match change.tag() { + ChangeTag::Delete => Some(format!("-{line}")), + ChangeTag::Insert => Some(format!("+{line}")), + ChangeTag::Equal => None, + } + }) + .join("\n") +} + impl Error { pub fn category(&self) -> String { match self { Error::FileWithoutOwner { path: _ } => "Some files are missing ownership".to_owned(), Error::FileWithMultipleOwners { path: _, owners: _ } => "Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways".to_owned(), - Error::CodeownershipFileIsStale { executable_name } => { + Error::CodeownershipFileIsStale { executable_name, diff: _ } => { format!("CODEOWNERS out of date. Run `{}` to update the CODEOWNERS file", executable_name) } Error::InvalidTeam { name: _, path: _ } => "Found invalid team annotations".to_owned(), @@ -192,7 +208,13 @@ impl Error { vec![messages.join("\n")] } - Error::CodeownershipFileIsStale { executable_name: _ } => vec![], + Error::CodeownershipFileIsStale { executable_name: _, diff } => { + if diff.is_empty() { + vec![] + } else { + vec![format!("The following changes are required (- current, + expected):\n{diff}")] + } + } Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)], } } @@ -221,3 +243,43 @@ impl Display for Errors { } impl Context for Errors {} + +#[cfg(test)] +mod tests { + use super::*; + use indoc::indoc; + + #[test] + fn test_codeowners_diff_reports_added_and_removed_lines() { + let current = indoc! {" + # Team A + /app/a.rb @TeamA + /app/old.rb @TeamA + "}; + let generated = indoc! {" + # Team A + /app/a.rb @TeamA + /app/b.rb @TeamB + "}; + + let diff = codeowners_diff(current, generated); + + assert_eq!(diff, "-/app/old.rb @TeamA\n+/app/b.rb @TeamB"); + } + + #[test] + fn test_codeowners_diff_against_empty_file_is_all_additions() { + let generated = "# Team A\n/app/a.rb @TeamA\n"; + + let diff = codeowners_diff("", generated); + + assert_eq!(diff, "+# Team A\n+/app/a.rb @TeamA"); + } + + #[test] + fn test_codeowners_diff_is_empty_when_identical() { + let file = "# Team A\n/app/a.rb @TeamA\n"; + + assert_eq!(codeowners_diff(file, file), ""); + } +} diff --git a/tests/executable_name_config_test.rs b/tests/executable_name_config_test.rs index 5832eb8..e71da1d 100644 --- a/tests/executable_name_config_test.rs +++ b/tests/executable_name_config_test.rs @@ -40,11 +40,24 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box &["validate"], false, OutputStream::Stdout, - predicate::eq(indoc! {" + 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 + + + +# Annotations at the top of file + +/app/foo.rb @FooTeam + + + +# Team-specific owned globs + +/ruby/app/payments/** @PaymentTeam + + + +# Team YML ownership + +/config/teams/foo.yml @FooTeam + +/config/teams/payments.yml @PaymentTeam - "}), + "#}), )?; Ok(()) } @@ -57,11 +70,19 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box Result<(), Box> { 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\". + +# + +# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub + +# teams. This is useful when developers create Pull Requests since the + +# code/file owner is notified. Reference GitHub docs for more details: + +# https://help.github.com/en/articles/about-code-owners + + + +# Annotations at the top of file + +/gems/payroll_calculator/calculator.rb @PaymentTeam + +/ruby/app/models/bank_account.rb @PaymentTeam + +/ruby/app/models/payroll.rb @PayrollTeam + +/ruby/app/services/multi_owned.rb @PaymentTeam + + + +# Team-specific owned globs + +/ruby/app/payments/**/* @PaymentTeam + + + +# Owner in .codeowner + +/ruby/app/services/**/** @PayrollTeam + + + +# Owner metadata key in package.yml + +/ruby/packages/payroll_flow/**/** @PayrollTeam + + + +# Team YML ownership + +/config/teams/payments.yml @PaymentTeam + +/config/teams/payroll.yml @PayrollTeam + + + +# Team owned gems + +/gems/payroll_calculator/**/** @PayrollTeam Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways