From dabef4e47e7f1b891b632308bc48a99526db4d84 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Tue, 5 Nov 2024 12:31:36 -0600 Subject: [PATCH] adding for-file (#29) --- README.md | 1 + src/main.rs | 17 +++- src/ownership.rs | 79 ++++++++++++++- src/ownership/file_owner_finder.rs | 97 +++++++++++++++++++ src/ownership/mapper.rs | 92 +----------------- src/ownership/validator.rs | 4 +- src/project.rs | 4 + .../ruby/app/services/.codeowner | 1 + .../ruby/app/services/multi_owned.rb | 1 + tests/invalid_project_test.rs | 33 +++++++ tests/valid_project_test.rs | 16 +++ 11 files changed, 246 insertions(+), 99 deletions(-) create mode 100644 src/ownership/file_owner_finder.rs create mode 100644 tests/fixtures/invalid_project/ruby/app/services/.codeowner create mode 100644 tests/fixtures/invalid_project/ruby/app/services/multi_owned.rb diff --git a/README.md b/README.md index b6a8167..e6a05f5 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ Commands: generate Generate the CODEOWNERS file and save it to '--codeowners-file-path' validate Validate the validity of the CODEOWNERS file. A validation failure will exit with a failure code and a detailed output of the validation errors generate-and-validate Chains both 'generate' and 'validate' commands + for-file Print the owners for a given file help Print this message or the help of the given subcommand(s) Options: diff --git a/src/main.rs b/src/main.rs index b7784f3..cde93a3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,4 +1,4 @@ -use ownership::Ownership; +use ownership::{FileOwner, Ownership}; use crate::project::Project; use clap::{Parser, Subcommand}; @@ -18,6 +18,8 @@ mod project; #[derive(Subcommand, Debug)] enum Command { + /// Responds with ownership for a given file + ForFile { name: String }, /// Generate the CODEOWNERS file and save it to '--codeowners-file-path'. Generate, @@ -113,6 +115,19 @@ fn cli() -> Result<(), Error> { std::fs::write(codeowners_file_path, ownership.generate_file()).change_context(Error::Io)?; ownership.validate().change_context(Error::ValidationFailed)? } + Command::ForFile { name } => { + let file_owners = ownership.for_file(&name).change_context(Error::Io)?; + match file_owners.len() { + 0 => println!("{}", FileOwner::default()), + 1 => println!("{}", file_owners[0]), + _ => { + println!("Error: file is owned by multiple teams!"); + for file_owner in file_owners { + println!("\n{}\n", file_owner); + } + } + } + } } Ok(()) diff --git a/src/ownership.rs b/src/ownership.rs index ebae88f..33ef776 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -1,14 +1,17 @@ -use mapper::TeamName; -use std::sync::Arc; +use file_owner_finder::FileOwnerFinder; +use mapper::{OwnerMatcher, TeamName}; +use std::{ + fmt::{self, Display}, + path::Path, + sync::Arc, +}; use tracing::{info, instrument}; mod file_generator; +mod file_owner_finder; mod mapper; mod validator; -#[cfg(test)] -mod tests; - use crate::{ownership::mapper::DirectoryMapper, project::Project}; pub use validator::Errors as ValidatorErrors; @@ -23,6 +26,26 @@ pub struct Ownership { project: Arc, } +pub struct FileOwner { + pub team_name: TeamName, + pub team_config_file_path: String, +} + +impl Display for FileOwner { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Team: {}\nTeam YML: {}", self.team_name, self.team_config_file_path) + } +} + +impl Default for FileOwner { + fn default() -> Self { + Self { + team_name: "Unowned".to_string(), + team_config_file_path: "Unowned".to_string(), + } + } +} + #[allow(dead_code)] #[derive(Debug, PartialEq)] pub struct Entry { @@ -62,6 +85,29 @@ impl Ownership { validator.validate() } + #[instrument(level = "debug", skip_all)] + pub fn for_file(&self, file_path: &str) -> Result, ValidatorErrors> { + info!("getting file ownership for {}", file_path); + let owner_matchers: Vec = self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(); + let file_owner_finder = FileOwnerFinder { + owner_matchers: &owner_matchers, + }; + let owners = file_owner_finder.find(Path::new(file_path)); + Ok(owners + .iter() + .map(|owner| match self.project.get_team(&owner.team_name) { + Some(team) => FileOwner { + team_name: owner.team_name.clone(), + team_config_file_path: team + .path + .strip_prefix(&self.project.base_path) + .map_or_else(|_| String::new(), |p| p.to_string_lossy().to_string()), + }, + None => FileOwner::default(), + }) + .collect()) + } + #[instrument(level = "debug", skip_all)] pub fn generate_file(&self) -> String { info!("generating codeowners file"); @@ -81,3 +127,26 @@ impl Ownership { ] } } + +#[cfg(test)] +mod tests { + use crate::common_test::tests::build_ownership_with_all_mappers; + + #[test] + fn test_for_file_owner() -> Result<(), Box> { + let ownership = build_ownership_with_all_mappers()?; + let file_owners = ownership.for_file("app/consumers/directory_owned.rb").unwrap(); + assert_eq!(file_owners.len(), 1); + assert_eq!(file_owners[0].team_name, "Bar"); + assert_eq!(file_owners[0].team_config_file_path, "config/teams/bar.yml"); + Ok(()) + } + + #[test] + fn test_for_file_no_owner() -> Result<(), Box> { + let ownership = build_ownership_with_all_mappers()?; + let file_owners = ownership.for_file("app/madeup/foo.rb").unwrap(); + assert_eq!(file_owners.len(), 0); + Ok(()) + } +} diff --git a/src/ownership/file_owner_finder.rs b/src/ownership/file_owner_finder.rs new file mode 100644 index 0000000..8a9a74c --- /dev/null +++ b/src/ownership/file_owner_finder.rs @@ -0,0 +1,97 @@ +use std::{collections::HashMap, path::Path}; + +use super::mapper::{directory_mapper::is_directory_mapper_source, OwnerMatcher, Source, TeamName}; + +#[derive(Debug)] +pub struct Owner { + pub sources: Vec, + pub team_name: TeamName, +} + +pub struct FileOwnerFinder<'a> { + pub owner_matchers: &'a [OwnerMatcher], +} + +impl<'a> FileOwnerFinder<'a> { + pub fn find(&self, relative_path: &Path) -> Vec { + let mut team_sources_map: HashMap<&TeamName, Vec> = HashMap::new(); + let mut directory_overrider = DirectoryOverrider::default(); + + for owner_matcher in self.owner_matchers { + let (owner, source) = owner_matcher.owner_for(relative_path); + + if let Some(team_name) = owner { + if is_directory_mapper_source(source) { + directory_overrider.process(team_name, source); + } else { + team_sources_map.entry(team_name).or_default().push(source.clone()); + } + } + } + + // Add most specific directory owner if it exists + if let Some((team_name, source)) = directory_overrider.specific_directory_owner() { + team_sources_map.entry(team_name).or_default().push(source.clone()); + } + + team_sources_map + .into_iter() + .map(|(team_name, sources)| Owner { + sources, + team_name: team_name.clone(), + }) + .collect() + } +} + +/// DirectoryOverrider is used to override the owner of a directory if a more specific directory owner is found. +#[derive(Debug, Default)] +pub struct DirectoryOverrider<'a> { + specific_directory_owner: Option<(&'a TeamName, &'a Source)>, +} + +impl<'a> DirectoryOverrider<'a> { + fn process(&mut self, team_name: &'a TeamName, source: &'a Source) { + if self + .specific_directory_owner + .map_or(true, |(_, current_source)| current_source.len() < source.len()) + { + self.specific_directory_owner = Some((team_name, source)); + } + } + + fn specific_directory_owner(&self) -> Option<(&TeamName, &Source)> { + self.specific_directory_owner + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_directory_overrider() { + let mut directory_overrider = DirectoryOverrider::default(); + assert_eq!(directory_overrider.specific_directory_owner(), None); + let team_name_1 = "team1".to_string(); + let source_1 = "src/**".to_string(); + directory_overrider.process(&team_name_1, &source_1); + assert_eq!(directory_overrider.specific_directory_owner(), Some((&team_name_1, &source_1))); + + let team_name_longest = "team2".to_string(); + let source_longest = "source/subdir/**".to_string(); + directory_overrider.process(&team_name_longest, &source_longest); + assert_eq!( + directory_overrider.specific_directory_owner(), + Some((&team_name_longest, &source_longest)) + ); + + let team_name_3 = "team3".to_string(); + let source_3 = "source/**".to_string(); + directory_overrider.process(&team_name_3, &source_3); + assert_eq!( + directory_overrider.specific_directory_owner(), + Some((&team_name_longest, &source_longest)) + ); + } +} diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index e147d72..601513c 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -1,11 +1,10 @@ -use directory_mapper::is_directory_mapper_source; use glob_match::glob_match; use std::{ collections::HashMap, path::{Path, PathBuf}, }; -mod directory_mapper; +pub(crate) mod directory_mapper; mod package_mapper; mod team_file_mapper; mod team_gem_mapper; @@ -51,99 +50,10 @@ impl OwnerMatcher { } } -#[derive(Debug)] -pub struct Owner { - pub sources: Vec, - pub team_name: TeamName, -} - -pub struct FileOwnerFinder<'a> { - pub owner_matchers: &'a [OwnerMatcher], -} - -impl<'a> FileOwnerFinder<'a> { - pub fn find(&self, relative_path: &Path) -> Vec { - let mut team_sources_map: HashMap<&TeamName, Vec> = HashMap::new(); - let mut directory_overrider = DirectoryOverrider::default(); - - for owner_matcher in self.owner_matchers { - let (owner, source) = owner_matcher.owner_for(relative_path); - - if let Some(team_name) = owner { - if is_directory_mapper_source(source) { - directory_overrider.process(team_name, source); - } else { - team_sources_map.entry(team_name).or_default().push(source.clone()); - } - } - } - - // Add most specific directory owner if it exists - if let Some((team_name, source)) = directory_overrider.specific_directory_owner() { - team_sources_map.entry(team_name).or_default().push(source.clone()); - } - - team_sources_map - .into_iter() - .map(|(team_name, sources)| Owner { - sources, - team_name: team_name.clone(), - }) - .collect() - } -} - -/// DirectoryOverrider is used to override the owner of a directory if a more specific directory owner is found. -#[derive(Debug, Default)] -struct DirectoryOverrider<'a> { - specific_directory_owner: Option<(&'a TeamName, &'a Source)>, -} - -impl<'a> DirectoryOverrider<'a> { - fn process(&mut self, team_name: &'a TeamName, source: &'a Source) { - if self - .specific_directory_owner - .map_or(true, |(_, current_source)| current_source.len() < source.len()) - { - self.specific_directory_owner = Some((team_name, source)); - } - } - - fn specific_directory_owner(&self) -> Option<(&TeamName, &Source)> { - self.specific_directory_owner - } -} - #[cfg(test)] mod tests { use super::*; - #[test] - fn test_directory_overrider() { - let mut directory_overrider = DirectoryOverrider::default(); - assert_eq!(directory_overrider.specific_directory_owner(), None); - let team_name_1 = "team1".to_string(); - let source_1 = "src/**".to_string(); - directory_overrider.process(&team_name_1, &source_1); - assert_eq!(directory_overrider.specific_directory_owner(), Some((&team_name_1, &source_1))); - - let team_name_longest = "team2".to_string(); - let source_longest = "source/subdir/**".to_string(); - directory_overrider.process(&team_name_longest, &source_longest); - assert_eq!( - directory_overrider.specific_directory_owner(), - Some((&team_name_longest, &source_longest)) - ); - - let team_name_3 = "team3".to_string(); - let source_3 = "source/**".to_string(); - directory_overrider.process(&team_name_3, &source_3); - assert_eq!( - directory_overrider.specific_directory_owner(), - Some((&team_name_longest, &source_longest)) - ); - } - fn assert_owner_for(glob: &str, relative_path: &str, expect_match: bool) { let source = "directory_mapper (\"packs/bam\")".to_string(); let team_name = "team1".to_string(); diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 684ff05..e7a4c21 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -13,8 +13,8 @@ use tracing::debug; use tracing::instrument; use super::file_generator::FileGenerator; -use super::mapper::FileOwnerFinder; -use super::mapper::Owner; +use super::file_owner_finder::FileOwnerFinder; +use super::file_owner_finder::Owner; use super::mapper::{Mapper, OwnerMatcher, TeamName}; pub struct Validator { diff --git a/src/project.rs b/src/project.rs index 31896a8..72de0ff 100644 --- a/src/project.rs +++ b/src/project.rs @@ -267,6 +267,10 @@ impl Project { .expect("Could not generate relative path") } + pub fn get_team(&self, name: &str) -> Option { + self.team_by_name().get(name).cloned() + } + pub fn team_by_name(&self) -> HashMap { let mut result: HashMap = HashMap::new(); diff --git a/tests/fixtures/invalid_project/ruby/app/services/.codeowner b/tests/fixtures/invalid_project/ruby/app/services/.codeowner new file mode 100644 index 0000000..4dc904f --- /dev/null +++ b/tests/fixtures/invalid_project/ruby/app/services/.codeowner @@ -0,0 +1 @@ +Payroll \ No newline at end of file diff --git a/tests/fixtures/invalid_project/ruby/app/services/multi_owned.rb b/tests/fixtures/invalid_project/ruby/app/services/multi_owned.rb new file mode 100644 index 0000000..bf47fae --- /dev/null +++ b/tests/fixtures/invalid_project/ruby/app/services/multi_owned.rb @@ -0,0 +1 @@ +# @team Payments \ No newline at end of file diff --git a/tests/invalid_project_test.rs b/tests/invalid_project_test.rs index 3b68726..8a9e154 100644 --- a/tests/invalid_project_test.rs +++ b/tests/invalid_project_test.rs @@ -17,3 +17,36 @@ fn test_validate() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_for_file() -> Result<(), Box> { + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg("tests/fixtures/invalid_project") + .arg("for-file") + .arg("ruby/app/models/blockchain.rb") + .assert() + .success() + .stdout(predicate::str::contains("Team: Unowned")) + .stdout(predicate::str::contains("Team YML: Unowned")); + + Ok(()) +} + +#[test] +fn test_for_file_multiple_owners() -> Result<(), Box> { + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg("tests/fixtures/invalid_project") + .arg("for-file") + .arg("ruby/app/services/multi_owned.rb") + .assert() + .success() + .stdout(predicate::str::contains("Error: file is owned by multiple teams!")) + .stdout(predicate::str::contains("Team: Payments")) + .stdout(predicate::str::contains("Team YML: config/teams/payments.yml")) + .stdout(predicate::str::contains("Team: Payroll")) + .stdout(predicate::str::contains("Team YML: config/teams/payroll.yml")); + + Ok(()) +} diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index b80ddd8..aab5892 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -1,4 +1,5 @@ use assert_cmd::prelude::*; +use predicates::prelude::predicate; use std::{error::Error, path::Path, process::Command}; #[test] @@ -31,3 +32,18 @@ fn test_generate() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_for_file() -> Result<(), Box> { + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg("tests/fixtures/valid_project") + .arg("for-file") + .arg("ruby/app/models/payroll.rb") + .assert() + .success() + .stdout(predicate::str::contains("Team: Payroll")) + .stdout(predicate::str::contains("Team YML: config/teams/payroll.yml")); + + Ok(()) +}