diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 71e5247..b0a23dd 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -185,16 +185,24 @@ fn nearest_package_owner( if let Some(rel_str) = parent_rel.to_str() { if glob_list_matches(rel_str, &config.ruby_package_paths) { let pkg_yml = current.join("package.yml"); - if pkg_yml.exists() - && let Ok(owner) = read_ruby_package_owner(&pkg_yml) - && let Some(team) = teams_by_name.get(&owner) - { - let package_path = parent_rel.join("package.yml"); - let package_glob = format!("{rel_str}/**/**"); - return Some(( - team.name.clone(), - Source::Package(package_path.to_string_lossy().to_string(), package_glob), - )); + if pkg_yml.exists() { + match crate::project_builder::ruby_package_owner(&pkg_yml) { + Ok(owner) => { + if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) { + let package_path = parent_rel.join("package.yml"); + let package_glob = format!("{rel_str}/**/**"); + return Some(( + team.name.clone(), + Source::Package(package_path.to_string_lossy().to_string(), package_glob), + )); + } + } + // validate rejects this package, so don't fall through to an enclosing package's owner. + Err(e) => { + eprintln!("Error reading ruby package: {e:?}, path: {}", pkg_yml.display()); + return None; + } + } } } if glob_list_matches(rel_str, &config.javascript_package_paths) { @@ -225,12 +233,6 @@ fn glob_list_matches(path: &str, globs: &[String]) -> bool { globs.iter().any(|g| glob_match(g, path)) } -fn read_ruby_package_owner(path: &Path) -> std::result::Result { - let file = std::fs::File::open(path).map_err(|e| e.to_string())?; - let deserializer: crate::project::deserializers::RubyPackage = serde_yaml::from_reader(file).map_err(|e| e.to_string())?; - deserializer.owner.ok_or_else(|| "Missing owner".to_string()) -} - fn read_js_package_owner(path: &Path) -> std::result::Result { let file = std::fs::File::open(path).map_err(|e| e.to_string())?; let deserializer: crate::project::deserializers::JavascriptPackage = serde_json::from_reader(file).map_err(|e| e.to_string())?; @@ -354,6 +356,95 @@ mod tests { assert_eq!(result.0, "DeepTeam"); } + #[test] + fn test_nearest_package_owner_ruby_metadata_owner() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let ruby_pkg = project_root.join("packs/payroll"); + std::fs::create_dir_all(&ruby_pkg).unwrap(); + std::fs::write(ruby_pkg.join("package.yml"), "---\nmetadata:\n owner: Payroll\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + let t = team_named("Payroll"); + tbn.insert(t.name.clone(), t); + + let rel_ruby = Path::new("packs/payroll/app/models/thing.rb"); + let ruby_owner = nearest_package_owner(project_root, rel_ruby, &config, &tbn).unwrap(); + assert_eq!(ruby_owner.0, "Payroll"); + match ruby_owner.1 { + Source::Package(pkg_path, glob) => { + assert!(pkg_path.ends_with("packs/payroll/package.yml")); + assert_eq!(glob, "packs/payroll/**/**"); + } + _ => panic!("expected Package source for ruby"), + } + } + + #[test] + fn test_nearest_package_owner_ruby_conflicting_owners_yields_none() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let ruby_pkg = project_root.join("packs/payroll"); + std::fs::create_dir_all(&ruby_pkg).unwrap(); + std::fs::write(ruby_pkg.join("package.yml"), "---\nowner: Payroll\nmetadata:\n owner: Benefits\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + for name in ["Payroll", "Benefits"] { + let t = team_named(name); + tbn.insert(t.name.clone(), t); + } + + let rel_ruby = Path::new("packs/payroll/app/models/thing.rb"); + assert!(nearest_package_owner(project_root, rel_ruby, &config, &tbn).is_none()); + } + + #[test] + fn test_nearest_package_owner_ruby_conflict_does_not_fall_through_to_outer_package() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let outer_pkg = project_root.join("packs/outer"); + let inner_pkg = outer_pkg.join("inner"); + std::fs::create_dir_all(&inner_pkg).unwrap(); + std::fs::write(outer_pkg.join("package.yml"), "---\nowner: Outer\n").unwrap(); + std::fs::write(inner_pkg.join("package.yml"), "---\nowner: InnerA\nmetadata:\n owner: InnerB\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + for name in ["Outer", "InnerA", "InnerB"] { + let t = team_named(name); + tbn.insert(t.name.clone(), t); + } + + let rel_ruby = Path::new("packs/outer/inner/x.rb"); + assert!(nearest_package_owner(project_root, rel_ruby, &config, &tbn).is_none()); + } + + #[test] + fn test_nearest_package_owner_ruby_ownerless_inner_package_falls_through_to_outer() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let outer_pkg = project_root.join("packs/outer"); + let inner_pkg = outer_pkg.join("inner"); + std::fs::create_dir_all(&inner_pkg).unwrap(); + std::fs::write(outer_pkg.join("package.yml"), "---\nowner: Outer\n").unwrap(); + std::fs::write(inner_pkg.join("package.yml"), "---\nenforce_dependencies: true\n").unwrap(); + + let mut tbn: HashMap = HashMap::new(); + let t = team_named("Outer"); + tbn.insert(t.name.clone(), t); + + let rel_ruby = Path::new("packs/outer/inner/x.rb"); + let owner = nearest_package_owner(project_root, rel_ruby, &config, &tbn).unwrap(); + assert_eq!(owner.0, "Outer"); + } + #[test] fn test_nearest_package_owner_ruby_and_js() { let td = tempdir().unwrap(); diff --git a/src/project_builder.rs b/src/project_builder.rs index 193791b..e0d06ee 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -336,7 +336,7 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool { } } -fn ruby_package_owner(path: &Path) -> Result, Report> { +pub(crate) fn ruby_package_owner(path: &Path) -> Result, Report> { let file = File::open(path).change_context(Error::Io)?; let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?;