Skip to content

Commit d829231

Browse files
committed
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
1 parent 68a5e19 commit d829231

File tree

2 files changed

+64
-1
lines changed

2 files changed

+64
-1
lines changed

src/project.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ pub mod deserializers {
112112
#[derive(Deserialize)]
113113
pub struct RubyPackage {
114114
pub owner: Option<String>,
115+
pub metadata: Option<Metadata>,
115116
}
116117

117118
#[derive(Deserialize)]

src/project_builder.rs

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,19 @@ fn ruby_package_owner(path: &Path) -> Result<Option<String>, Error> {
314314
let file = File::open(path).change_context(Error::Io)?;
315315
let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?;
316316

317-
Ok(deserializer.owner)
317+
let top_level_owner = deserializer.owner;
318+
let metadata_owner = deserializer.metadata.and_then(|metadata| metadata.owner);
319+
320+
// Error if both are present with different values
321+
match (top_level_owner.as_ref(), metadata_owner.as_ref()) {
322+
(Some(top), Some(meta)) if top != meta => Err(error_stack::report!(Error::Io).attach_printable(format!(
323+
"Package at {} has conflicting owners: 'owner: {}' vs 'metadata.owner: {}'. Please use only one.",
324+
path.display(),
325+
top,
326+
meta
327+
))),
328+
_ => Ok(top_level_owner.or(metadata_owner)),
329+
}
318330
}
319331

320332
fn javascript_package_owner(path: &Path) -> Result<Option<String>, Error> {
@@ -339,4 +351,54 @@ mod tests {
339351
fn test_glob_match() {
340352
assert!(glob_match(OWNED_GLOB, "script/.eslintrc.js"));
341353
}
354+
355+
#[test]
356+
fn test_ruby_package_owner_top_level() {
357+
let yaml = "owner: TeamA\n";
358+
let temp_file = tempfile::NamedTempFile::new().unwrap();
359+
std::fs::write(temp_file.path(), yaml).unwrap();
360+
361+
let owner = ruby_package_owner(temp_file.path()).unwrap();
362+
assert_eq!(owner, Some("TeamA".to_string()));
363+
}
364+
365+
#[test]
366+
fn test_ruby_package_owner_metadata() {
367+
let yaml = "metadata:\n owner: TeamB\n";
368+
let temp_file = tempfile::NamedTempFile::new().unwrap();
369+
std::fs::write(temp_file.path(), yaml).unwrap();
370+
371+
let owner = ruby_package_owner(temp_file.path()).unwrap();
372+
assert_eq!(owner, Some("TeamB".to_string()));
373+
}
374+
375+
#[test]
376+
fn test_ruby_package_owner_errors_when_both_present_and_different() {
377+
let yaml = "owner: TeamA\nmetadata:\n owner: TeamB\n";
378+
let temp_file = tempfile::NamedTempFile::new().unwrap();
379+
std::fs::write(temp_file.path(), yaml).unwrap();
380+
381+
let result = ruby_package_owner(temp_file.path());
382+
assert!(result.is_err());
383+
}
384+
385+
#[test]
386+
fn test_ruby_package_owner_allows_both_when_same() {
387+
let yaml = "owner: TeamA\nmetadata:\n owner: TeamA\n";
388+
let temp_file = tempfile::NamedTempFile::new().unwrap();
389+
std::fs::write(temp_file.path(), yaml).unwrap();
390+
391+
let owner = ruby_package_owner(temp_file.path()).unwrap();
392+
assert_eq!(owner, Some("TeamA".to_string()));
393+
}
394+
395+
#[test]
396+
fn test_ruby_package_owner_no_owner() {
397+
let yaml = "name: my_package\n";
398+
let temp_file = tempfile::NamedTempFile::new().unwrap();
399+
std::fs::write(temp_file.path(), yaml).unwrap();
400+
401+
let owner = ruby_package_owner(temp_file.path()).unwrap();
402+
assert_eq!(owner, None);
403+
}
342404
}

0 commit comments

Comments
 (0)