From 02b30645c7d201d80152b7938c2b054cad3dc977 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 25 Apr 2025 11:37:24 -0500 Subject: [PATCH 1/5] DRY iter_team_globs --- src/ownership/mapper/team_glob_mapper.rs | 50 +++++++++++------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/src/ownership/mapper/team_glob_mapper.rs b/src/ownership/mapper/team_glob_mapper.rs index 01ec57f..00afb46 100644 --- a/src/ownership/mapper/team_glob_mapper.rs +++ b/src/ownership/mapper/team_glob_mapper.rs @@ -12,40 +12,36 @@ impl TeamGlobMapper { pub fn build(project: Arc) -> Self { Self { project } } + + fn iter_team_globs(&self) -> impl Iterator + '_ { + self.project.teams.iter().flat_map(|team| { + team.owned_globs + .iter() + .map(move |glob| (glob.as_str(), team.github_team.as_str(), team.name.as_str(), team.avoid_ownership)) + }) + } } impl Mapper for TeamGlobMapper { fn entries(&self) -> Vec { - let mut entries: Vec = Vec::new(); - - for team in &self.project.teams { - for owned_glob in &team.owned_globs { - entries.push(Entry { - path: owned_glob.to_owned(), - github_team: team.github_team.to_owned(), - team_name: team.name.to_owned(), - disabled: team.avoid_ownership, - }); - } - } - - entries + self.iter_team_globs() + .map(|(glob, github_team, team_name, disabled)| Entry { + path: glob.to_owned(), + github_team: github_team.to_owned(), + team_name: team_name.to_owned(), + disabled, + }) + .collect() } fn owner_matchers(&self) -> Vec { - let mut owner_matchers: Vec = Vec::new(); - - for team in &self.project.teams { - for owned_glob in &team.owned_globs { - owner_matchers.push(OwnerMatcher::Glob { - glob: owned_glob.clone(), - team_name: team.github_team.clone(), - source: Source::TeamGlob(owned_glob.clone()), - }) - } - } - - owner_matchers + self.iter_team_globs() + .map(|(glob, github_team, _, _)| OwnerMatcher::Glob { + glob: glob.to_owned(), + team_name: github_team.to_owned(), + source: Source::TeamGlob(glob.to_owned()), + }) + .collect() } fn name(&self) -> String { From 97634a2d6f35b872acdc10bce1987e2ce277bb62 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 25 Apr 2025 11:44:33 -0500 Subject: [PATCH 2/5] the behavior we want --- tests/fixtures/valid_project/config/teams/payments.yml | 2 ++ tests/fixtures/valid_project/ruby/app/payments/foo/.codeowner | 1 + .../valid_project/ruby/app/payments/foo/ownedby_payroll.rb | 3 +++ 3 files changed, 6 insertions(+) create mode 100644 tests/fixtures/valid_project/ruby/app/payments/foo/.codeowner create mode 100644 tests/fixtures/valid_project/ruby/app/payments/foo/ownedby_payroll.rb diff --git a/tests/fixtures/valid_project/config/teams/payments.yml b/tests/fixtures/valid_project/config/teams/payments.yml index 2f01f29..add6fe8 100644 --- a/tests/fixtures/valid_project/config/teams/payments.yml +++ b/tests/fixtures/valid_project/config/teams/payments.yml @@ -3,3 +3,5 @@ github: team: '@PaymentsTeam' owned_globs: - ruby/app/payments/**/* +unowned_globs: + - ruby/app/payments/foo/**/* diff --git a/tests/fixtures/valid_project/ruby/app/payments/foo/.codeowner b/tests/fixtures/valid_project/ruby/app/payments/foo/.codeowner new file mode 100644 index 0000000..4dc904f --- /dev/null +++ b/tests/fixtures/valid_project/ruby/app/payments/foo/.codeowner @@ -0,0 +1 @@ +Payroll \ No newline at end of file diff --git a/tests/fixtures/valid_project/ruby/app/payments/foo/ownedby_payroll.rb b/tests/fixtures/valid_project/ruby/app/payments/foo/ownedby_payroll.rb new file mode 100644 index 0000000..b4e5ca0 --- /dev/null +++ b/tests/fixtures/valid_project/ruby/app/payments/foo/ownedby_payroll.rb @@ -0,0 +1,3 @@ +class OwnedByPayroll + +end \ No newline at end of file From 5f4a201fd1d42c951b5fa2087f93481ed9a506a9 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 25 Apr 2025 13:53:37 -0500 Subject: [PATCH 3/5] introducing concept of subtracted globs --- src/ownership/mapper.rs | 67 +++++++++++++++++++++++- src/ownership/mapper/directory_mapper.rs | 60 ++++++++++----------- src/ownership/mapper/package_mapper.rs | 30 +++++------ src/ownership/mapper/team_gem_mapper.rs | 20 +++---- src/ownership/mapper/team_glob_mapper.rs | 21 ++++---- 5 files changed, 132 insertions(+), 66 deletions(-) diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index 62f64a4..da535a8 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -67,13 +67,49 @@ impl Source { #[derive(Debug, PartialEq)] pub enum OwnerMatcher { ExactMatches(HashMap, Source), - Glob { glob: String, team_name: TeamName, source: Source }, + Glob { + glob: String, + subtracted_globs: Vec, + team_name: TeamName, + source: Source, + }, } impl OwnerMatcher { + pub fn new_glob_with_candidate_subtracted_globs( + glob: String, + candidate_subtracted_globs: Vec, + team_name: TeamName, + source: Source, + ) -> Self { + let subtracted_globs = candidate_subtracted_globs.iter().filter(|candidate_subtracted_glob| { + glob_match(candidate_subtracted_glob, &glob) || glob_match(&glob, candidate_subtracted_glob) + }).cloned().collect(); + OwnerMatcher::Glob { + glob, + subtracted_globs, + team_name, + source, + } + } + + pub fn new_glob(glob: String, team_name: TeamName, source: Source) -> Self { + OwnerMatcher::Glob { + glob, + subtracted_globs: vec![], + team_name, + source, + } + } + pub fn owner_for(&self, relative_path: &Path) -> (Option<&TeamName>, &Source) { match self { - OwnerMatcher::Glob { glob, team_name, source } => { + OwnerMatcher::Glob { + glob, + subtracted_globs, + team_name, + source, + } => { if glob_match(glob, relative_path.to_str().unwrap()) { (Some(team_name), source) } else { @@ -94,6 +130,7 @@ mod tests { let team_name = "team1".to_string(); let owner_matcher = OwnerMatcher::Glob { glob: glob.to_string(), + subtracted_globs: vec![], team_name: team_name.clone(), source: source.clone(), }; @@ -150,4 +187,30 @@ mod tests { ); assert_eq!(Source::TeamYml.to_string(), "Teams own their configuration files"); } + + #[test] + fn test_new_glob_with_candidate_subtracted_globs() { + assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &[], &[]); + assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam/app/**/**"], &["packs/bam/app/**/**"]); + assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam/app/an/exceptional/path/it.rb"], &["packs/bam/app/an/exceptional/path/it.rb"]); + assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam.rb"], &[]); + assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/nope/app/**/**"], &[]); + assert_new_glob_with_candidate_subtracted_globs("packs/**", &["packs/yep/app/**/**"], &["packs/yep/app/**/**"]); + assert_new_glob_with_candidate_subtracted_globs("packs/foo.yml", &["packs/foo/**/**"], &[]); + } + + fn assert_new_glob_with_candidate_subtracted_globs(glob: &str, candidate_subtracted_globs: &[&str], expected_subtracted_globs: &[&str]) { + let owner_matcher = OwnerMatcher::new_glob_with_candidate_subtracted_globs( + glob.to_string(), + candidate_subtracted_globs.iter().map(|s| s.to_string()).collect(), + "team1".to_string(), + Source::TeamGlob(glob.to_string()), + ); + + if let OwnerMatcher::Glob { subtracted_globs, .. } = owner_matcher { + assert_eq!(subtracted_globs, expected_subtracted_globs); + } else { + panic!("Expected a Glob matcher"); + } + } } diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs index 7254b4b..4916ef1 100644 --- a/src/ownership/mapper/directory_mapper.rs +++ b/src/ownership/mapper/directory_mapper.rs @@ -40,11 +40,11 @@ impl Mapper for DirectoryMapper { let mut owner_matchers = Vec::new(); for file in &self.project.directory_codeowner_files { - owner_matchers.push(OwnerMatcher::Glob { - glob: format!("{}/**/**", escape_brackets(&file.directory_root().to_string_lossy())), - team_name: file.owner.to_owned(), - source: Source::Directory(file.directory_root().to_string_lossy().to_string()), - }); + owner_matchers.push(OwnerMatcher::new_glob( + format!("{}/**/**", escape_brackets(&file.directory_root().to_string_lossy())), + file.owner.to_owned(), + Source::Directory(file.directory_root().to_string_lossy().to_string()), + )); } owner_matchers @@ -125,21 +125,21 @@ mod tests { vecs_match( &mapper.owner_matchers(), &vec![ - OwnerMatcher::Glob { - glob: "app/consumers/**/**".to_owned(), - team_name: "Bar".to_owned(), - source: Source::Directory("app/consumers".to_string()), - }, - OwnerMatcher::Glob { - glob: "app/services/**/**".to_owned(), - team_name: "Foo".to_owned(), - source: Source::Directory("app/services".to_owned()), - }, - OwnerMatcher::Glob { - glob: "app/services/exciting/**/**".to_owned(), - team_name: "Bar".to_owned(), - source: Source::Directory("app/services/exciting".to_owned()), - }, + OwnerMatcher::new_glob( + "app/consumers/**/**".to_owned(), + "Bar".to_owned(), + Source::Directory("app/consumers".to_string()), + ), + OwnerMatcher::new_glob( + "app/services/**/**".to_owned(), + "Foo".to_owned(), + Source::Directory("app/services".to_owned()), + ), + OwnerMatcher::new_glob( + "app/services/exciting/**/**".to_owned(), + "Bar".to_owned(), + Source::Directory("app/services/exciting".to_owned()), + ), ], ); Ok(()) @@ -152,16 +152,16 @@ mod tests { vecs_match( &mapper.owner_matchers(), &vec![ - OwnerMatcher::Glob { - glob: "app/\\[consumers\\]/**/**".to_string(), - team_name: "Bar".to_string(), - source: Source::Directory("app/[consumers]".to_string()), - }, - OwnerMatcher::Glob { - glob: "app/\\[consumers\\]/deep/nesting/\\[nestdir\\]/**/**".to_string(), - team_name: "Foo".to_string(), - source: Source::Directory("app/[consumers]/deep/nesting/[nestdir]".to_string()), - }, + OwnerMatcher::new_glob( + "app/\\[consumers\\]/**/**".to_string(), + "Bar".to_string(), + Source::Directory("app/[consumers]".to_string()), + ), + OwnerMatcher::new_glob( + "app/\\[consumers\\]/deep/nesting/\\[nestdir\\]/**/**".to_string(), + "Foo".to_string(), + Source::Directory("app/[consumers]/deep/nesting/[nestdir]".to_string()), + ), ], ); Ok(()) diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs index fcb67d3..2279779 100644 --- a/src/ownership/mapper/package_mapper.rs +++ b/src/ownership/mapper/package_mapper.rs @@ -101,11 +101,11 @@ impl PackageMapper { let team = team_by_name.get(&package.owner); if let Some(team) = team { - owner_matchers.push(OwnerMatcher::Glob { - glob: format!("{}/**/**", package_root), - team_name: team.name.to_owned(), - source: Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)), - }); + owner_matchers.push(OwnerMatcher::new_glob( + format!("{}/**/**", package_root), + team.name.to_owned(), + Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)), + )); } } @@ -198,16 +198,16 @@ mod tests { vecs_match( &mapper.owner_matchers(&PackageType::Ruby), &vec![ - OwnerMatcher::Glob { - glob: "packs/bam/**/**".to_owned(), - team_name: "Bam".to_owned(), - source: Source::Package("packs/bam/package.yml".to_owned(), "packs/bam/**/**".to_owned()), - }, - OwnerMatcher::Glob { - glob: "packs/foo/**/**".to_owned(), - team_name: "Baz".to_owned(), - source: Source::Package("packs/foo/package.yml".to_owned(), "packs/foo/**/**".to_owned()), - }, + OwnerMatcher::new_glob( + "packs/bam/**/**".to_owned(), + "Bam".to_owned(), + Source::Package("packs/bam/package.yml".to_owned(), "packs/bam/**/**".to_owned()), + ), + OwnerMatcher::new_glob( + "packs/foo/**/**".to_owned(), + "Baz".to_owned(), + Source::Package("packs/foo/package.yml".to_owned(), "packs/foo/**/**".to_owned()), + ), ], ); Ok(()) diff --git a/src/ownership/mapper/team_gem_mapper.rs b/src/ownership/mapper/team_gem_mapper.rs index 254564e..9d74914 100644 --- a/src/ownership/mapper/team_gem_mapper.rs +++ b/src/ownership/mapper/team_gem_mapper.rs @@ -46,11 +46,11 @@ impl Mapper for TeamGemMapper { let vendored_gem = vendored_gem_by_name.get(owned_gem); if let Some(vendored_gem) = vendored_gem { - owner_matchers.push(OwnerMatcher::Glob { - glob: format!("{}/**/*", self.project.relative_path(&vendored_gem.path).to_string_lossy()), - team_name: team.name.clone(), - source: Source::TeamGem, - }); + owner_matchers.push(OwnerMatcher::new_glob( + format!("{}/**/*", self.project.relative_path(&vendored_gem.path).to_string_lossy()), + team.name.clone(), + Source::TeamGem, + )); } } } @@ -92,11 +92,11 @@ mod tests { let mapper = TeamGemMapper::build(ownership.project.clone()); vecs_match( &mapper.owner_matchers(), - &vec![OwnerMatcher::Glob { - glob: "gems/globbing/**/*".to_owned(), - team_name: "Bam".to_owned(), - source: Source::TeamGem, - }], + &vec![OwnerMatcher::new_glob( + "gems/globbing/**/*".to_owned(), + "Bam".to_owned(), + Source::TeamGem, + )], ); Ok(()) } diff --git a/src/ownership/mapper/team_glob_mapper.rs b/src/ownership/mapper/team_glob_mapper.rs index 00afb46..f82b23c 100644 --- a/src/ownership/mapper/team_glob_mapper.rs +++ b/src/ownership/mapper/team_glob_mapper.rs @@ -36,10 +36,13 @@ impl Mapper for TeamGlobMapper { fn owner_matchers(&self) -> Vec { self.iter_team_globs() - .map(|(glob, github_team, _, _)| OwnerMatcher::Glob { - glob: glob.to_owned(), - team_name: github_team.to_owned(), - source: Source::TeamGlob(glob.to_owned()), + .map(|(glob, github_team, _, _)| { + OwnerMatcher::new_glob_with_candidate_subtracted_globs( + glob.to_owned(), + vec![], + github_team.to_owned(), + Source::TeamGlob(glob.to_owned()), + ) }) .collect() } @@ -78,11 +81,11 @@ mod tests { let mapper = TeamGlobMapper::build(ownership.project.clone()); vecs_match( &mapper.owner_matchers(), - &vec![OwnerMatcher::Glob { - glob: "packs/bar/**".to_owned(), - team_name: "@Baz".to_owned(), - source: Source::TeamGlob("packs/bar/**".to_owned()), - }], + &vec![OwnerMatcher::new_glob( + "packs/bar/**".to_owned(), + "@Baz".to_owned(), + Source::TeamGlob("packs/bar/**".to_owned()), + )], ); Ok(()) } From 4d53a81d04bc8d92bfe01c9c1c4af4342a4874d2 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 25 Apr 2025 14:49:30 -0500 Subject: [PATCH 4/5] subtracting team globs --- src/common_test.rs | 7 ++ src/ownership/mapper.rs | 85 +++++++++++++------ src/ownership/mapper/team_glob_mapper.rs | 80 +++++++++++------ src/project.rs | 5 ++ .../fixtures/valid_project/.github/CODEOWNERS | 1 + tests/valid_project_test.rs | 1 + 6 files changed, 125 insertions(+), 54 deletions(-) diff --git a/src/common_test.rs b/src/common_test.rs index 6deb5d3..2e9db19 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -279,6 +279,13 @@ pub mod tests { }) } + pub fn build_ownership_with_subtracted_globs_team_glob_codeowners() -> Result> { + ownership!(TestProjectFile { + relative_path: "config/teams/baz.yml".to_owned(), + content: "name: Baz\ngithub:\n team: \"@Baz\"\n members:\n - Baz member\nowned_globs:\n - \"packs/bar/**\"\nunowned_globs:\n - \"packs/bar/excluded/**\"\n".to_owned(), + }) + } + pub fn build_ownership_with_team_yml_codeowners() -> Result> { let temp_dir = tempdir()?; diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index da535a8..c9fbddc 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -78,13 +78,17 @@ pub enum OwnerMatcher { impl OwnerMatcher { pub fn new_glob_with_candidate_subtracted_globs( glob: String, - candidate_subtracted_globs: Vec, + candidate_subtracted_globs: &[String], team_name: TeamName, source: Source, ) -> Self { - let subtracted_globs = candidate_subtracted_globs.iter().filter(|candidate_subtracted_glob| { - glob_match(candidate_subtracted_glob, &glob) || glob_match(&glob, candidate_subtracted_glob) - }).cloned().collect(); + let subtracted_globs = candidate_subtracted_globs + .iter() + .filter(|candidate_subtracted_glob| { + glob_match(candidate_subtracted_glob, &glob) || glob_match(&glob, candidate_subtracted_glob) + }) + .cloned() + .collect(); OwnerMatcher::Glob { glob, subtracted_globs, @@ -109,13 +113,10 @@ impl OwnerMatcher { subtracted_globs, team_name, source, - } => { - if glob_match(glob, relative_path.to_str().unwrap()) { - (Some(team_name), source) - } else { - (None, source) - } - } + } => relative_path + .to_str() + .filter(|path| glob_match(glob, path) && !subtracted_globs.iter().any(|subtracted| glob_match(subtracted, path))) + .map_or((None, source), |_| (Some(team_name), source)), OwnerMatcher::ExactMatches(path_to_team, source) => (path_to_team.get(relative_path), source), } } @@ -125,15 +126,15 @@ impl OwnerMatcher { mod tests { use super::*; - fn assert_owner_for(glob: &str, relative_path: &str, expect_match: bool) { + fn assert_owner_for(glob: &str, subtracted_globs: &[&str], relative_path: &str, expect_match: bool) { let source = Source::Directory("packs/bam".to_string()); let team_name = "team1".to_string(); - let owner_matcher = OwnerMatcher::Glob { - glob: glob.to_string(), - subtracted_globs: vec![], - team_name: team_name.clone(), - source: source.clone(), - }; + let owner_matcher = OwnerMatcher::new_glob_with_candidate_subtracted_globs( + glob.to_string(), + &subtracted_globs.iter().map(|s| s.to_string()).collect::>(), + team_name.clone(), + source.clone(), + ); let response = owner_matcher.owner_for(Path::new(relative_path)); if expect_match { assert_eq!(response, (Some(&team_name), &source)); @@ -144,26 +145,50 @@ mod tests { #[test] fn owner_for_without_brackets_in_glob() { - assert_owner_for("packs/bam/**/**", "packs/bam/app/components/sidebar.jsx", true); - assert_owner_for("packs/bam/**/**", "packs/baz/app/components/sidebar.jsx", false); - assert_owner_for("packs/bam/**/**", "packs/bam/app/[components]/gadgets/sidebar.jsx", true); - assert_owner_for("packs/bam/**/**", "packs/bam/app/sidebar_[component].jsx", true); + assert_owner_for("packs/bam/**/**", &[], "packs/bam/app/components/sidebar.jsx", true); + assert_owner_for("packs/bam/**/**", &[], "packs/baz/app/components/sidebar.jsx", false); + assert_owner_for("packs/bam/**/**", &[], "packs/bam/app/[components]/gadgets/sidebar.jsx", true); + assert_owner_for("packs/bam/**/**", &[], "packs/bam/app/sidebar_[component].jsx", true); + assert_owner_for( + "packs/bam/**/**", + &["packs/bam/app/excluded/**"], + "packs/bam/app/excluded/sidebar_[component].jsx", + false, + ); + } + + #[test] + fn subtracted_globs() { + assert_owner_for( + "packs/bam/**/**", + &["packs/bam/app/excluded/**"], + "packs/bam/app/excluded/some_file.rb", + false, + ); + assert_owner_for( + "packs/bam/**/**", + &["packs/bam/app/excluded/**"], + "packs/bam/app/not_excluded/some_file.rb", + true, + ); } #[test] fn owner_for_with_brackets_in_glob() { assert_owner_for( "packs/bam/app/\\[components\\]/**/**", + &[], "packs/bam/app/[components]/gadgets/sidebar.jsx", true, ); - assert_owner_for("packs/\\[bam\\]/**/**", "packs/[bam]/app/components/sidebar.jsx", true); + assert_owner_for("packs/\\[bam\\]/**/**", &[], "packs/[bam]/app/components/sidebar.jsx", true); } #[test] fn owner_for_with_multiple_brackets_in_glob() { assert_owner_for( "packs/\\[bam\\]/bar/\\[foo\\]/**/**", + &[], "packs/[bam]/bar/[foo]/app/components/sidebar.jsx", true, ); @@ -192,17 +217,25 @@ mod tests { fn test_new_glob_with_candidate_subtracted_globs() { assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &[], &[]); assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam/app/**/**"], &["packs/bam/app/**/**"]); - assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam/app/an/exceptional/path/it.rb"], &["packs/bam/app/an/exceptional/path/it.rb"]); + assert_new_glob_with_candidate_subtracted_globs( + "packs/bam/**/**", + &["packs/bam/app/an/exceptional/path/it.rb"], + &["packs/bam/app/an/exceptional/path/it.rb"], + ); assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam.rb"], &[]); assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/nope/app/**/**"], &[]); assert_new_glob_with_candidate_subtracted_globs("packs/**", &["packs/yep/app/**/**"], &["packs/yep/app/**/**"]); assert_new_glob_with_candidate_subtracted_globs("packs/foo.yml", &["packs/foo/**/**"], &[]); } - fn assert_new_glob_with_candidate_subtracted_globs(glob: &str, candidate_subtracted_globs: &[&str], expected_subtracted_globs: &[&str]) { + fn assert_new_glob_with_candidate_subtracted_globs( + glob: &str, + candidate_subtracted_globs: &[&str], + expected_subtracted_globs: &[&str], + ) { let owner_matcher = OwnerMatcher::new_glob_with_candidate_subtracted_globs( glob.to_string(), - candidate_subtracted_globs.iter().map(|s| s.to_string()).collect(), + &candidate_subtracted_globs.iter().map(|s| s.to_string()).collect::>(), "team1".to_string(), Source::TeamGlob(glob.to_string()), ); diff --git a/src/ownership/mapper/team_glob_mapper.rs b/src/ownership/mapper/team_glob_mapper.rs index f82b23c..be4230b 100644 --- a/src/ownership/mapper/team_glob_mapper.rs +++ b/src/ownership/mapper/team_glob_mapper.rs @@ -12,39 +12,42 @@ impl TeamGlobMapper { pub fn build(project: Arc) -> Self { Self { project } } - - fn iter_team_globs(&self) -> impl Iterator + '_ { - self.project.teams.iter().flat_map(|team| { - team.owned_globs - .iter() - .map(move |glob| (glob.as_str(), team.github_team.as_str(), team.name.as_str(), team.avoid_ownership)) - }) - } } impl Mapper for TeamGlobMapper { fn entries(&self) -> Vec { - self.iter_team_globs() - .map(|(glob, github_team, team_name, disabled)| Entry { - path: glob.to_owned(), - github_team: github_team.to_owned(), - team_name: team_name.to_owned(), - disabled, - }) - .collect() + let mut entries: Vec = Vec::new(); + + for team in &self.project.teams { + for owned_glob in &team.owned_globs { + entries.push(Entry { + path: owned_glob.to_owned(), + github_team: team.github_team.to_owned(), + team_name: team.name.to_owned(), + disabled: team.avoid_ownership, + }); + } + } + + entries } fn owner_matchers(&self) -> Vec { - self.iter_team_globs() - .map(|(glob, github_team, _, _)| { - OwnerMatcher::new_glob_with_candidate_subtracted_globs( - glob.to_owned(), - vec![], - github_team.to_owned(), - Source::TeamGlob(glob.to_owned()), - ) - }) - .collect() + let mut owner_matchers: Vec = Vec::new(); + + for team in &self.project.teams { + let team_subtracted_globs = team.subtracted_globs.clone(); + for owned_glob in &team.owned_globs { + owner_matchers.push(OwnerMatcher::new_glob_with_candidate_subtracted_globs( + owned_glob.clone(), + &team_subtracted_globs, + team.github_team.clone(), + Source::TeamGlob(owned_glob.clone()), + )) + } + } + + owner_matchers } fn name(&self) -> String { @@ -56,7 +59,10 @@ impl Mapper for TeamGlobMapper { mod tests { use std::error::Error; - use crate::common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_team_glob_codeowners, vecs_match}; + use crate::common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_subtracted_globs_team_glob_codeowners, + build_ownership_with_team_glob_codeowners, vecs_match, + }; use super::*; #[test] @@ -81,8 +87,26 @@ mod tests { let mapper = TeamGlobMapper::build(ownership.project.clone()); vecs_match( &mapper.owner_matchers(), - &vec![OwnerMatcher::new_glob( + &vec![OwnerMatcher::new_glob_with_candidate_subtracted_globs( + "packs/bar/**".to_owned(), + &[], + "@Baz".to_owned(), + Source::TeamGlob("packs/bar/**".to_owned()), + )], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_subtracted_globs() -> Result<(), Box> { + let ownership = build_ownership_with_subtracted_globs_team_glob_codeowners()?; + + let mapper = TeamGlobMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![OwnerMatcher::new_glob_with_candidate_subtracted_globs( "packs/bar/**".to_owned(), + &["packs/bar/excluded/**".to_owned()], "@Baz".to_owned(), Source::TeamGlob("packs/bar/**".to_owned()), )], diff --git a/src/project.rs b/src/project.rs index f7259f7..db47e10 100644 --- a/src/project.rs +++ b/src/project.rs @@ -37,6 +37,7 @@ pub struct Team { pub name: String, pub github_team: String, pub owned_globs: Vec, + pub subtracted_globs: Vec, pub owned_gems: Vec, pub avoid_ownership: bool, } @@ -50,6 +51,7 @@ impl Team { name: deserializer.name, github_team: deserializer.github.team, owned_globs: deserializer.owned_globs, + subtracted_globs: deserializer.subtracted_globs, owned_gems: deserializer.ruby.map(|ruby| ruby.owned_gems).unwrap_or_default(), avoid_ownership: deserializer.github.do_not_add_to_codeowners_file, }) @@ -132,6 +134,9 @@ pub mod deserializers { #[serde(default = "empty_string_vec")] pub owned_globs: Vec, + + #[serde(alias = "unowned_globs", default = "empty_string_vec")] + pub subtracted_globs: Vec, } fn empty_string_vec() -> Vec { diff --git a/tests/fixtures/valid_project/.github/CODEOWNERS b/tests/fixtures/valid_project/.github/CODEOWNERS index 38f4292..09a9fd7 100644 --- a/tests/fixtures/valid_project/.github/CODEOWNERS +++ b/tests/fixtures/valid_project/.github/CODEOWNERS @@ -18,6 +18,7 @@ # Owner in .codeowner /javascript/packages/items/**/** @PayrollTeam /javascript/packages/items/(special)/**/** @PaymentsTeam +/ruby/app/payments/foo/**/** @PayrollTeam /ruby/app/payroll/**/** @PayrollTeam # Owner metadata key in package.yml diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index bb5e0e8..6ee2caa 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -201,6 +201,7 @@ fn test_for_team() -> Result<(), Box> { ## Owner in .codeowner /javascript/packages/items/**/** + /ruby/app/payments/foo/**/** /ruby/app/payroll/**/** ## Owner metadata key in package.yml From 53d7642a3c4c304de857328ea481ee0c13604f25 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 25 Apr 2025 16:19:37 -0500 Subject: [PATCH 5/5] bumping version --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 928d46d..d9cf231 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -173,7 +173,7 @@ checksum = "1462739cb27611015575c0c11df5df7601141071f07518d56fcc1be504cbec97" [[package]] name = "codeowners" -version = "0.2.3" +version = "0.2.4" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index 3f77a40..df37c08 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.2.3" +version = "0.2.4" edition = "2024" [profile.release]