Skip to content

Commit 597b13d

Browse files
authored
feat: show CODEOWNERS diff when validate detects a stale file (#107)
`validate` previously reported only "CODEOWNERS out of date" with no indication of what differed, forcing developers to re-run the tool locally to discover the delta. The stale-file error now includes a line-oriented diff between the on-disk CODEOWNERS file and the freshly generated one (git-style `-`/`+` prefixes, changed lines only), so CI output explains what is out of date.
1 parent b43cc8a commit 597b13d

7 files changed

Lines changed: 142 additions & 21 deletions

File tree

Cargo.lock

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ regex = "1.11.1"
2727
serde = { version = "1.0.219", features = ["derive"] }
2828
serde_json = "1.0.143"
2929
serde_yaml = "0.9.34"
30+
similar = "2.6.0"
3031
tempfile = "3.21.0"
3132
tracing = "0.1.41"
3233
tracing-subscriber = { version = "0.3.19", features = ["env-filter"] }

src/ownership/validator.rs

Lines changed: 77 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use error_stack::Context;
99
use itertools::Itertools;
1010
use rayon::prelude::IntoParallelRefIterator;
1111
use rayon::prelude::ParallelIterator;
12+
use similar::{ChangeTag, TextDiff};
1213
use tracing::debug;
1314
use tracing::instrument;
1415

@@ -29,7 +30,7 @@ enum Error {
2930
InvalidTeam { name: String, path: PathBuf },
3031
FileWithoutOwner { path: PathBuf },
3132
FileWithMultipleOwners { path: PathBuf, owners: Vec<Owner> },
32-
CodeownershipFileIsStale { executable_name: String },
33+
CodeownershipFileIsStale { executable_name: String, diff: String },
3334
}
3435

3536
#[derive(Debug)]
@@ -127,20 +128,15 @@ impl Validator {
127128

128129
fn validate_codeowners_file(&self) -> Vec<Error> {
129130
let generated_file = self.file_generator.generate_file();
131+
let current_file = self.project.get_codeowners_file().unwrap_or_default();
130132

131-
match self.project.get_codeowners_file() {
132-
Ok(current_file) => {
133-
if generated_file != current_file {
134-
vec![Error::CodeownershipFileIsStale {
135-
executable_name: self.executable_name.to_string(),
136-
}]
137-
} else {
138-
vec![]
139-
}
140-
}
141-
Err(_) => vec![Error::CodeownershipFileIsStale {
133+
if generated_file == current_file {
134+
vec![]
135+
} else {
136+
vec![Error::CodeownershipFileIsStale {
142137
executable_name: self.executable_name.to_string(),
143-
}],
138+
diff: codeowners_diff(&current_file, &generated_file),
139+
}]
144140
}
145141
}
146142

@@ -163,12 +159,32 @@ impl Validator {
163159
}
164160
}
165161

162+
/// Builds a line-oriented diff between the current (on-disk) CODEOWNERS file and the
163+
/// freshly generated one, so that validation failures explain *what* is out of date
164+
/// rather than just *that* it is. Only changed lines are emitted: removals (present
165+
/// on disk but no longer expected) are prefixed with `-` and additions (expected but
166+
/// missing) are prefixed with `+`.
167+
fn codeowners_diff(current: &str, generated: &str) -> String {
168+
let diff = TextDiff::from_lines(current, generated);
169+
170+
diff.iter_all_changes()
171+
.filter_map(|change| {
172+
let line = change.value().trim_end_matches('\n');
173+
match change.tag() {
174+
ChangeTag::Delete => Some(format!("-{line}")),
175+
ChangeTag::Insert => Some(format!("+{line}")),
176+
ChangeTag::Equal => None,
177+
}
178+
})
179+
.join("\n")
180+
}
181+
166182
impl Error {
167183
pub fn category(&self) -> String {
168184
match self {
169185
Error::FileWithoutOwner { path: _ } => "Some files are missing ownership".to_owned(),
170186
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(),
171-
Error::CodeownershipFileIsStale { executable_name } => {
187+
Error::CodeownershipFileIsStale { executable_name, diff: _ } => {
172188
format!("CODEOWNERS out of date. Run `{}` to update the CODEOWNERS file", executable_name)
173189
}
174190
Error::InvalidTeam { name: _, path: _ } => "Found invalid team annotations".to_owned(),
@@ -192,7 +208,13 @@ impl Error {
192208

193209
vec![messages.join("\n")]
194210
}
195-
Error::CodeownershipFileIsStale { executable_name: _ } => vec![],
211+
Error::CodeownershipFileIsStale { executable_name: _, diff } => {
212+
if diff.is_empty() {
213+
vec![]
214+
} else {
215+
vec![format!("The following changes are required (- current, + expected):\n{diff}")]
216+
}
217+
}
196218
Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)],
197219
}
198220
}
@@ -221,3 +243,43 @@ impl Display for Errors {
221243
}
222244

223245
impl Context for Errors {}
246+
247+
#[cfg(test)]
248+
mod tests {
249+
use super::*;
250+
use indoc::indoc;
251+
252+
#[test]
253+
fn test_codeowners_diff_reports_added_and_removed_lines() {
254+
let current = indoc! {"
255+
# Team A
256+
/app/a.rb @TeamA
257+
/app/old.rb @TeamA
258+
"};
259+
let generated = indoc! {"
260+
# Team A
261+
/app/a.rb @TeamA
262+
/app/b.rb @TeamB
263+
"};
264+
265+
let diff = codeowners_diff(current, generated);
266+
267+
assert_eq!(diff, "-/app/old.rb @TeamA\n+/app/b.rb @TeamB");
268+
}
269+
270+
#[test]
271+
fn test_codeowners_diff_against_empty_file_is_all_additions() {
272+
let generated = "# Team A\n/app/a.rb @TeamA\n";
273+
274+
let diff = codeowners_diff("", generated);
275+
276+
assert_eq!(diff, "+# Team A\n+/app/a.rb @TeamA");
277+
}
278+
279+
#[test]
280+
fn test_codeowners_diff_is_empty_when_identical() {
281+
let file = "# Team A\n/app/a.rb @TeamA\n";
282+
283+
assert_eq!(codeowners_diff(file, file), "");
284+
}
285+
}

tests/executable_name_config_test.rs

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,24 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box<dyn Error>
4040
&["validate"],
4141
false,
4242
OutputStream::Stdout,
43-
predicate::eq(indoc! {"
43+
predicate::eq(indoc! {r#"
4444
4545
CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file
46+
The following changes are required (- current, + expected):
47+
-# Outdated content to trigger validation error
48+
-/app/old.rb @FooTeam
49+
+
50+
+# Annotations at the top of file
51+
+/app/foo.rb @FooTeam
52+
+
53+
+# Team-specific owned globs
54+
+/ruby/app/payments/** @PaymentTeam
55+
+
56+
+# Team YML ownership
57+
+/config/teams/foo.yml @FooTeam
58+
+/config/teams/payments.yml @PaymentTeam
4659
47-
"}),
60+
"#}),
4861
)?;
4962
Ok(())
5063
}
@@ -57,11 +70,19 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box<dyn Error
5770
&["validate"],
5871
false,
5972
OutputStream::Stdout,
60-
predicate::eq(indoc! {"
73+
predicate::eq(indoc! {r#"
6174
6275
CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file
76+
The following changes are required (- current, + expected):
77+
-# Outdated content to trigger validation error
78+
-/app/old.rb @BarTeam
79+
+# Annotations at the top of file
80+
+/app/bar.rb @BarTeam
81+
+
82+
+# Team YML ownership
83+
+/config/teams/bar.yml @BarTeam
6384
64-
"}),
85+
"#}),
6586
)?;
6687
Ok(())
6788
}

tests/fixtures/custom_executable_name/.github/CODEOWNERS

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# STOP! - DO NOT EDIT THIS FILE MANUALLY
2-
# This file was automatically generated by "codeowners".
2+
# This file was automatically generated by "bin/codeownership validate".
33
#
44
# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub
55
# teams. This is useful when developers create Pull Requests since the

tests/fixtures/default_executable_name/.github/CODEOWNERS

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# STOP! - DO NOT EDIT THIS FILE MANUALLY
2-
# This file was automatically generated by "codeowners".
2+
# This file was automatically generated by "bin/codeownership validate".
33
#
44
# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub
55
# teams. This is useful when developers create Pull Requests since the

tests/invalid_project_test.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,36 @@ fn test_validate() -> Result<(), Box<dyn Error>> {
1616
predicate::eq(indoc! {"
1717
1818
CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file
19+
The following changes are required (- current, + expected):
20+
+# STOP! - DO NOT EDIT THIS FILE MANUALLY
21+
+# This file was automatically generated by \"bin/codeownership validate\".
22+
+#
23+
+# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub
24+
+# teams. This is useful when developers create Pull Requests since the
25+
+# code/file owner is notified. Reference GitHub docs for more details:
26+
+# https://help.github.com/en/articles/about-code-owners
27+
+
28+
+# Annotations at the top of file
29+
+/gems/payroll_calculator/calculator.rb @PaymentTeam
30+
+/ruby/app/models/bank_account.rb @PaymentTeam
31+
+/ruby/app/models/payroll.rb @PayrollTeam
32+
+/ruby/app/services/multi_owned.rb @PaymentTeam
33+
+
34+
+# Team-specific owned globs
35+
+/ruby/app/payments/**/* @PaymentTeam
36+
+
37+
+# Owner in .codeowner
38+
+/ruby/app/services/**/** @PayrollTeam
39+
+
40+
+# Owner metadata key in package.yml
41+
+/ruby/packages/payroll_flow/**/** @PayrollTeam
42+
+
43+
+# Team YML ownership
44+
+/config/teams/payments.yml @PaymentTeam
45+
+/config/teams/payroll.yml @PayrollTeam
46+
+
47+
+# Team owned gems
48+
+/gems/payroll_calculator/**/** @PayrollTeam
1949
2050
Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways
2151

0 commit comments

Comments
 (0)