diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 664de3a..e3888c9 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -2,7 +2,7 @@ use crate::project::{Project, ProjectFile}; use core::fmt; use std::collections::HashSet; use std::fmt::Display; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use error_stack::Context; @@ -27,10 +27,31 @@ pub struct Validator { #[derive(Debug)] enum Error { - InvalidTeam { name: String, path: PathBuf }, - FileWithoutOwner { path: PathBuf }, - FileWithMultipleOwners { path: PathBuf, owners: Vec }, - CodeownershipFileIsStale { executable_name: String, diff: String }, + InvalidTeam { + name: String, + path: PathBuf, + }, + /// A `.codeowner` naming a team that isn't registered. Distinct from `InvalidTeam` only + /// so the message can name the ancestor the directory now silently inherits from, which + /// is the part a reader needs in order to understand what happened. `category()` + /// deliberately matches `InvalidTeam` so one typo'd team name across an annotation, a + /// `package.yml`, and a `.codeowner` still groups under a single headline. + InvalidDirectoryTeam { + name: String, + path: PathBuf, + inherits_from: Option, + }, + FileWithoutOwner { + path: PathBuf, + }, + FileWithMultipleOwners { + path: PathBuf, + owners: Vec, + }, + CodeownershipFileIsStale { + executable_name: String, + diff: String, + }, } #[derive(Debug)] @@ -65,6 +86,7 @@ impl Validator { errors.append(&mut self.invalid_team_annotation(&team_names)); errors.append(&mut self.invalid_package_ownership(&team_names)); + errors.append(&mut self.invalid_directory_ownership()); errors } @@ -107,6 +129,41 @@ impl Validator { .collect() } + /// `DirectoryMapper::entries` skips unresolvable owners, so the directory silently + /// inherits its ancestor's owner and nothing else reports the bad name. + /// + /// Resolves through `teams_by_name` rather than `teams[].name` so the predicate is + /// identical to the mapper's lookup: that map is keyed by both `name` and + /// `github_team`, and a `.codeowner` holding either one generates a correct line. + fn invalid_directory_ownership(&self) -> Vec { + let resolvable_roots: HashSet<&Path> = self + .project + .directory_codeowner_files + .iter() + .filter(|directory_codeowner_file| self.project.teams_by_name.contains_key(&directory_codeowner_file.owner)) + .filter_map(|directory_codeowner_file| directory_codeowner_file.directory_root()) + .collect(); + + self.project + .directory_codeowner_files + .iter() + .flat_map(|directory_codeowner_file| { + if !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner) { + Some(Error::InvalidDirectoryTeam { + name: directory_codeowner_file.owner.clone(), + path: self.project.relative_path(&directory_codeowner_file.path).to_owned(), + inherits_from: directory_codeowner_file + .directory_root() + .and_then(|root| root.ancestors().skip(1).find(|ancestor| resolvable_roots.contains(ancestor))) + .map(|ancestor| self.project.relative_path(ancestor).to_owned()), + }) + } else { + None + } + }) + .collect() + } + fn validate_file_ownership(&self) -> Vec { let mut validation_errors = Vec::new(); @@ -187,7 +244,8 @@ impl Error { 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(), + Error::InvalidTeam { name: _, path: _ } => "Found invalid team references".to_owned(), + Error::InvalidDirectoryTeam { .. } => "Found invalid team references".to_owned(), } } @@ -213,6 +271,16 @@ impl Error { // 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)], + Error::InvalidDirectoryTeam { name, path, inherits_from } => { + let mut message = format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name); + if let Some(inherits_from) = inherits_from { + message.push_str(&format!( + "; this directory is currently inheriting its owner from {}", + inherits_from.to_string_lossy() + )); + } + vec![message] + } } } } diff --git a/tests/directory_codeowner_github_team_test.rs b/tests/directory_codeowner_github_team_test.rs new file mode 100644 index 0000000..4cbca9b --- /dev/null +++ b/tests/directory_codeowner_github_team_test.rs @@ -0,0 +1,22 @@ +use predicates::prelude::*; +use std::error::Error; + +mod common; +use common::OutputStream; +use common::run_codeowners; + +/// `teams_by_name` is keyed by both `name` and `github_team`, so a `.codeowner` holding +/// either form generates a correct line. Validation has to accept both, or it fails a +/// project whose CODEOWNERS is already right — with no command that fixes it. +#[test] +fn test_validate_accepts_directory_codeowner_by_name_or_github_team() -> Result<(), Box> { + run_codeowners( + "directory-codeowner-github-team", + &["validate"], + true, + OutputStream::Stdout, + predicate::str::contains("invalid team").not(), + )?; + + Ok(()) +} diff --git a/tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS b/tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS new file mode 100644 index 0000000..a36866e --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS @@ -0,0 +1,15 @@ +# 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 + + +# Owner in .codeowner +/app/by_handle/**/** @footeam +/app/by_name/**/** @footeam + +# Team YML ownership +/config/teams/foo.yml @footeam diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner b/tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner new file mode 100644 index 0000000..d556bd1 --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner @@ -0,0 +1 @@ +@footeam diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb b/tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb new file mode 100644 index 0000000..083e45e --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb @@ -0,0 +1,2 @@ +class Handled +end diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner b/tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner new file mode 100644 index 0000000..bc56c4d --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner @@ -0,0 +1 @@ +Foo diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb b/tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb new file mode 100644 index 0000000..018c0f7 --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb @@ -0,0 +1,2 @@ +class Named +end diff --git a/tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml b/tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml new file mode 100644 index 0000000..c76f028 --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml @@ -0,0 +1,10 @@ +--- +owned_globs: + - "{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}" +unowned_globs: + - config/code_ownership.yml +javascript_package_paths: + - javascript/packages/** +vendored_gems_path: gems +team_file_glob: + - config/teams/**/*.yml \ No newline at end of file diff --git a/tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml b/tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml new file mode 100644 index 0000000..7c3977c --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml @@ -0,0 +1,5 @@ +name: Foo +github: + team: "@footeam" + members: + - fooer diff --git a/tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS b/tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS new file mode 100644 index 0000000..fed28cf --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS @@ -0,0 +1,14 @@ +# 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 + + +# Owner in .codeowner +/app/services/**/** @footeam + +# Team YML ownership +/config/teams/foo.yml @footeam diff --git a/tests/fixtures/invalid-directory-codeowner/app/services/.codeowner b/tests/fixtures/invalid-directory-codeowner/app/services/.codeowner new file mode 100644 index 0000000..bc56c4d --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/app/services/.codeowner @@ -0,0 +1 @@ +Foo diff --git a/tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner b/tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner new file mode 100644 index 0000000..c2075a5 --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner @@ -0,0 +1 @@ +Web3 diff --git a/tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb b/tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb new file mode 100644 index 0000000..d19bcc1 --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb @@ -0,0 +1,2 @@ +class NestedFile +end diff --git a/tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml b/tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml new file mode 100644 index 0000000..c76f028 --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml @@ -0,0 +1,10 @@ +--- +owned_globs: + - "{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}" +unowned_globs: + - config/code_ownership.yml +javascript_package_paths: + - javascript/packages/** +vendored_gems_path: gems +team_file_glob: + - config/teams/**/*.yml \ No newline at end of file diff --git a/tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml b/tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml new file mode 100644 index 0000000..7c3977c --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml @@ -0,0 +1,5 @@ +name: Foo +github: + team: "@footeam" + members: + - fooer diff --git a/tests/invalid_directory_codeowner_test.rs b/tests/invalid_directory_codeowner_test.rs new file mode 100644 index 0000000..f3d1d69 --- /dev/null +++ b/tests/invalid_directory_codeowner_test.rs @@ -0,0 +1,26 @@ +use indoc::indoc; +use predicates::prelude::*; +use std::error::Error; + +mod common; +use common::OutputStream; +use common::run_codeowners; + +/// A nested `.codeowner` naming an unregistered team, under one naming a real team: +/// ownership falls through to the ancestor, so nothing else reports the bad name. The +/// message names that ancestor, since the silent inheritance is the surprising part. +#[test] +fn test_validate_reports_directory_codeowner_with_invalid_team() -> Result<(), Box> { + run_codeowners( + "invalid-directory-codeowner", + &["validate"], + false, + OutputStream::Stdout, + predicate::str::contains(indoc! {" + Found invalid team references + - app/services/nested/.codeowner is referencing an invalid team - 'Web3'; this directory is currently inheriting its owner from app/services + "}), + )?; + + Ok(()) +} diff --git a/tests/invalid_project_test.rs b/tests/invalid_project_test.rs index 884c5df..3c00a7a 100644 --- a/tests/invalid_project_test.rs +++ b/tests/invalid_project_test.rs @@ -61,7 +61,7 @@ fn test_validate() -> Result<(), Box> { owner: Payroll - Owner specified in `ruby/app/services/.codeowner` - Found invalid team annotations + Found invalid team references - ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3' Some files are missing ownership