From d82923193f304d45f388f8f716bb003437b1619e Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Wed, 26 Nov 2025 16:07:31 -0800 Subject: [PATCH] Fix regression in package.yml metadata.owner key As far as I can tell, this key was not intentionally removed and its presence is missed. Based on issues reported, We're restoring support rather than fix all the other things that depend on it. Tolerate having both keys set as long as they are the same. Fail when metadata.owner and owner are different. Since we don't know what other tooling reads this owner key, it seems better to error on ambiguity. It's possible other tooling reads this value from the package.yml. We can adjust if we find problems. Related to: rubyatscale/code_ownership#141 --- src/project.rs | 1 + src/project_builder.rs | 64 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/project.rs b/src/project.rs index 937a726..8293103 100644 --- a/src/project.rs +++ b/src/project.rs @@ -112,6 +112,7 @@ pub mod deserializers { #[derive(Deserialize)] pub struct RubyPackage { pub owner: Option, + pub metadata: Option, } #[derive(Deserialize)] diff --git a/src/project_builder.rs b/src/project_builder.rs index 487239d..518702f 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -314,7 +314,19 @@ fn ruby_package_owner(path: &Path) -> Result, Error> { let file = File::open(path).change_context(Error::Io)?; let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; - Ok(deserializer.owner) + let top_level_owner = deserializer.owner; + let metadata_owner = deserializer.metadata.and_then(|metadata| metadata.owner); + + // Error if both are present with different values + match (top_level_owner.as_ref(), metadata_owner.as_ref()) { + (Some(top), Some(meta)) if top != meta => Err(error_stack::report!(Error::Io).attach_printable(format!( + "Package at {} has conflicting owners: 'owner: {}' vs 'metadata.owner: {}'. Please use only one.", + path.display(), + top, + meta + ))), + _ => Ok(top_level_owner.or(metadata_owner)), + } } fn javascript_package_owner(path: &Path) -> Result, Error> { @@ -339,4 +351,54 @@ mod tests { fn test_glob_match() { assert!(glob_match(OWNED_GLOB, "script/.eslintrc.js")); } + + #[test] + fn test_ruby_package_owner_top_level() { + let yaml = "owner: TeamA\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).unwrap(); + assert_eq!(owner, Some("TeamA".to_string())); + } + + #[test] + fn test_ruby_package_owner_metadata() { + let yaml = "metadata:\n owner: TeamB\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).unwrap(); + assert_eq!(owner, Some("TeamB".to_string())); + } + + #[test] + fn test_ruby_package_owner_errors_when_both_present_and_different() { + let yaml = "owner: TeamA\nmetadata:\n owner: TeamB\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let result = ruby_package_owner(temp_file.path()); + assert!(result.is_err()); + } + + #[test] + fn test_ruby_package_owner_allows_both_when_same() { + let yaml = "owner: TeamA\nmetadata:\n owner: TeamA\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).unwrap(); + assert_eq!(owner, Some("TeamA".to_string())); + } + + #[test] + fn test_ruby_package_owner_no_owner() { + let yaml = "name: my_package\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).unwrap(); + assert_eq!(owner, None); + } }