Skip to content

Commit 36f1355

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 c3cace6 commit 36f1355

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
@@ -111,6 +111,7 @@ pub mod deserializers {
111111
#[derive(Deserialize)]
112112
pub struct RubyPackage {
113113
pub owner: Option<String>,
114+
pub metadata: Option<Metadata>,
114115
}
115116

116117
#[derive(Deserialize)]

src/project_builder.rs

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

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

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

0 commit comments

Comments
 (0)