Skip to content

Commit

Permalink
Improve code owner "source" descriptions (#36)
Browse files Browse the repository at this point in the history
* providing location of package file

* improving glob source descriptions
  • Loading branch information
perryqh authored Nov 7, 2024
1 parent b0899d3 commit 210252a
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 26 deletions.
2 changes: 1 addition & 1 deletion src/ownership.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ impl Display for FileOwner {
.join(", ");
write!(
f,
"Team: {}\nTeam YML: {}\nSource(s): {}",
"Team: {}\nTeam YML: {}\nDescription: {}",
self.team_name, self.team_config_file_path, sources
)
}
Expand Down
36 changes: 22 additions & 14 deletions src/ownership/mapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,22 @@ pub enum Source {
Directory(String),
TeamFile,
TeamGem,
TeamGlob,
TeamGlob(String),
Package(String, String),
TeamYml,
}

impl Display for Source {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Source::Directory(path) => write!(f, "DirectoryMapper({})", path),
Source::TeamFile => write!(f, "TeamFileMapper"),
Source::TeamGem => write!(f, "TeamGemMapper"),
Source::TeamGlob => write!(f, "TeamGlobMapper"),
Source::Package(file_type, path) => write!(f, "PackageMapper({}, glob: {})", file_type, path),
Source::TeamYml => write!(f, "TeamYmlMapper"),
Source::Directory(path) => write!(f, "Owner specified in `{}/.codeowner`", path),
Source::TeamFile => write!(f, "Owner annotation at the top of the file"),
Source::TeamGem => write!(f, "Owner specified in Team YML's `owned_gems`"),
Source::TeamGlob(glob) => write!(f, "Owner specified in Team YML as an owned_glob `{}`", glob),
Source::Package(package_path, glob) => {
write!(f, "Owner defined in `{}` with implicity owned glob: `{}`", package_path, glob)
}
Source::TeamYml => write!(f, "Teams own their configuration files"),
}
}
}
Expand Down Expand Up @@ -131,14 +133,20 @@ mod tests {

#[test]
fn display_source() {
assert_eq!(Source::Directory("packs/bam".to_string()).to_string(), "DirectoryMapper(packs/bam)");
assert_eq!(Source::TeamFile.to_string(), "TeamFileMapper");
assert_eq!(Source::TeamGem.to_string(), "TeamGemMapper");
assert_eq!(Source::TeamGlob.to_string(), "TeamGlobMapper");
assert_eq!(
Source::Package("Ruby".to_string(), "packs/bam/**/**".to_string()).to_string(),
"PackageMapper(Ruby, glob: packs/bam/**/**)"
Source::Directory("packs/bam".to_string()).to_string(),
"Owner specified in `packs/bam/.codeowner`"
);
assert_eq!(Source::TeamFile.to_string(), "Owner annotation at the top of the file");
assert_eq!(Source::TeamGem.to_string(), "Owner specified in Team YML's `owned_gems`");
assert_eq!(
Source::TeamGlob("a/glob/**".to_string()).to_string(),
"Owner specified in Team YML as an owned_glob `a/glob/**`"
);
assert_eq!(
Source::Package("packs/bam/packag.yml".to_string(), "packs/bam/**/**".to_string()).to_string(),
"Owner defined in `packs/bam/packag.yml` with implicity owned glob: `packs/bam/**/**`"
);
assert_eq!(Source::TeamYml.to_string(), "TeamYmlMapper");
assert_eq!(Source::TeamYml.to_string(), "Teams own their configuration files");
}
}
6 changes: 3 additions & 3 deletions src/ownership/mapper/package_mapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl PackageMapper {
owner_matchers.push(OwnerMatcher::Glob {
glob: format!("{}/**/**", package_root),
team_name: team.name.to_owned(),
source: Source::Package(package_type.to_string(), format!("{}/**/**", package_root)),
source: Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)),
});
}
}
Expand Down Expand Up @@ -201,12 +201,12 @@ mod tests {
OwnerMatcher::Glob {
glob: "packs/bam/**/**".to_owned(),
team_name: "Bam".to_owned(),
source: Source::Package("Ruby".to_owned(), "packs/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("Ruby".to_owned(), "packs/foo/**/**".to_owned()),
source: Source::Package("packs/foo/package.yml".to_owned(), "packs/foo/**/**".to_owned()),
},
],
);
Expand Down
4 changes: 2 additions & 2 deletions src/ownership/mapper/team_glob_mapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl Mapper for TeamGlobMapper {
owner_matchers.push(OwnerMatcher::Glob {
glob: owned_glob.clone(),
team_name: team.github_team.clone(),
source: Source::TeamGlob,
source: Source::TeamGlob(owned_glob.clone()),
})
}
}
Expand Down Expand Up @@ -85,7 +85,7 @@ mod tests {
&vec![OwnerMatcher::Glob {
glob: "packs/bar/**".to_owned(),
team_name: "@Baz".to_owned(),
source: Source::TeamGlob,
source: Source::TeamGlob("packs/bar/**".to_owned()),
}],
);
Ok(())
Expand Down
34 changes: 28 additions & 6 deletions tests/invalid_project_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,30 @@ fn test_validate() -> Result<(), Box<dyn Error>> {
.arg("validate")
.assert()
.failure()
.stdout(predicate::str::contains("CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file"))
.stdout(predicate::str::contains("Some files are missing ownership\n- ruby/app/models/blockchain.rb\n- ruby/app/unowned.rb"))
.stdout(predicate::str::contains("Found invalid team annotations\n- ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3'"))
.stdout(predicate::str::contains("Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways\n- gems/payroll_calculator/calculator.rb (owner: Payments, source: TeamFileMapper)\n- gems/payroll_calculator/calculator.rb (owner: Payroll, source: TeamGemMapper)"));
.stdout(predicate::str::contains(
"CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file",
))
.stdout(predicate::str::contains(
"Some files are missing ownership\n- ruby/app/models/blockchain.rb\n- ruby/app/unowned.rb",
))
.stdout(predicate::str::contains(
"Found invalid team annotations\n- ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3'",
))
.stdout(predicate::str::contains(
"Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways",
))
.stdout(predicate::str::contains(
"gems/payroll_calculator/calculator.rb (owner: Payments, source: Owner annotation at the top of the file)",
))
.stdout(predicate::str::contains(
"gems/payroll_calculator/calculator.rb (owner: Payroll, source: Owner specified in Team YML's `owned_gems`)",
))
.stdout(predicate::str::contains(
"ruby/app/services/multi_owned.rb (owner: Payments, source: Owner annotation at the top of the file)",
))
.stdout(predicate::str::contains(
"ruby/app/services/multi_owned.rb (owner: Payroll, source: Owner specified in `ruby/app/services/.codeowner`",
));

Ok(())
}
Expand Down Expand Up @@ -45,10 +65,12 @@ fn test_for_file_multiple_owners() -> Result<(), Box<dyn Error>> {
.stdout(predicate::str::contains("Error: file is owned by multiple teams!"))
.stdout(predicate::str::contains("Team: Payments"))
.stdout(predicate::str::contains("Team YML: config/teams/payments.yml"))
.stdout(predicate::str::contains("Source(s): TeamFileMapper"))
.stdout(predicate::str::contains("Description: Owner annotation at the top of the file"))
.stdout(predicate::str::contains("Team: Payroll"))
.stdout(predicate::str::contains("Team YML: config/teams/payroll.yml"))
.stdout(predicate::str::contains("Source(s): DirectoryMapper(ruby/app/services)"));
.stdout(predicate::str::contains(
"Description: Owner specified in `ruby/app/services/.codeowner`",
));

Ok(())
}

0 comments on commit 210252a

Please sign in to comment.