Skip to content

Commit fc4e148

Browse files
authored
Improving test tooling before tackling overrides (#70)
* overrides fixture * overrides prompt * adding verify compare for file * valid project with overrides test * from codeowners only * updating fixtures for overrides * ignoring overrides as they are not yet implemented * renaming verify to crosscheck * updating toolchain * linting updates
1 parent ca456be commit fc4e148

File tree

48 files changed

+504
-78
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+504
-78
lines changed

Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "codeowners"
3-
version = "0.2.7"
3+
version = "0.2.8"
44
edition = "2024"
55

66
[profile.release]

rust-toolchain.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
[toolchain]
2-
channel = "1.86.0"
2+
channel = "1.89.0"
33
components = ["clippy", "rustfmt"]
44
targets = ["x86_64-apple-darwin", "aarch64-apple-darwin", "x86_64-unknown-linux-gnu"]

src/cache/file.rs

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,26 +21,24 @@ const DEFAULT_CACHE_CAPACITY: usize = 10000;
2121

2222
impl Caching for GlobalCache {
2323
fn get_file_owner(&self, path: &Path) -> Result<Option<FileOwnerCacheEntry>, Error> {
24-
if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
25-
if let Ok(cache) = cache_mutex.lock() {
26-
if let Some(cached_entry) = cache.get(path) {
27-
let timestamp = get_file_timestamp(path)?;
28-
if cached_entry.timestamp == timestamp {
29-
return Ok(Some(cached_entry.clone()));
30-
}
31-
}
24+
if let Some(cache_mutex) = self.file_owner_cache.as_ref()
25+
&& let Ok(cache) = cache_mutex.lock()
26+
&& let Some(cached_entry) = cache.get(path)
27+
{
28+
let timestamp = get_file_timestamp(path)?;
29+
if cached_entry.timestamp == timestamp {
30+
return Ok(Some(cached_entry.clone()));
3231
}
3332
}
3433
Ok(None)
3534
}
3635

3736
fn write_file_owner(&self, path: &Path, owner: Option<String>) {
38-
if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
39-
if let Ok(mut cache) = cache_mutex.lock() {
40-
if let Ok(timestamp) = get_file_timestamp(path) {
41-
cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner });
42-
}
43-
}
37+
if let Some(cache_mutex) = self.file_owner_cache.as_ref()
38+
&& let Ok(mut cache) = cache_mutex.lock()
39+
&& let Ok(timestamp) = get_file_timestamp(path)
40+
{
41+
cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner });
4442
}
4543
}
4644

src/cli.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ enum Command {
1616
default_value = "false",
1717
help = "Find the owner from the CODEOWNERS file and just return the team name and yml path"
1818
)]
19-
fast: bool,
19+
from_codeowners: bool,
2020
name: String,
2121
},
2222

@@ -46,6 +46,9 @@ enum Command {
4646

4747
#[clap(about = "Delete the cache file.", visible_alias = "d")]
4848
DeleteCache,
49+
50+
#[clap(about = "Compare the CODEOWNERS file to the for-file command.", hide = true)]
51+
CrosscheckOwners,
4952
}
5053

5154
/// A CLI to validate and generate Github's CODEOWNERS file.
@@ -110,9 +113,10 @@ pub fn cli() -> Result<RunResult, RunnerError> {
110113
Command::Validate => runner::validate(&run_config, vec![]),
111114
Command::Generate { skip_stage } => runner::generate(&run_config, !skip_stage),
112115
Command::GenerateAndValidate { skip_stage } => runner::generate_and_validate(&run_config, vec![], !skip_stage),
113-
Command::ForFile { name, fast: _ } => runner::for_file(&run_config, &name),
116+
Command::ForFile { name, from_codeowners } => runner::for_file(&run_config, &name, from_codeowners),
114117
Command::ForTeam { name } => runner::for_team(&run_config, &name),
115118
Command::DeleteCache => runner::delete_cache(&run_config),
119+
Command::CrosscheckOwners => runner::crosscheck_owners(&run_config),
116120
};
117121

118122
Ok(runner_result)

src/crosscheck.rs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
use std::path::Path;
2+
3+
use crate::{
4+
cache::Cache,
5+
config::Config,
6+
ownership::for_file_fast::find_file_owners,
7+
project::Project,
8+
project_builder::ProjectBuilder,
9+
runner::{RunConfig, RunResult, config_from_path, team_for_file_from_codeowners},
10+
};
11+
12+
pub fn crosscheck_owners(run_config: &RunConfig, cache: &Cache) -> RunResult {
13+
match do_crosscheck_owners(run_config, cache) {
14+
Ok(mismatches) if mismatches.is_empty() => RunResult {
15+
info_messages: vec!["Success! All files match between CODEOWNERS and for-file command.".to_string()],
16+
..Default::default()
17+
},
18+
Ok(mismatches) => RunResult {
19+
validation_errors: mismatches,
20+
..Default::default()
21+
},
22+
Err(err) => RunResult {
23+
io_errors: vec![err],
24+
..Default::default()
25+
},
26+
}
27+
}
28+
29+
fn do_crosscheck_owners(run_config: &RunConfig, cache: &Cache) -> Result<Vec<String>, String> {
30+
let config = load_config(run_config)?;
31+
let project = build_project(&config, run_config, cache)?;
32+
33+
let mut mismatches: Vec<String> = Vec::new();
34+
for file in &project.files {
35+
let (codeowners_team, fast_display) = owners_for_file(&file.path, run_config, &config)?;
36+
let codeowners_display = codeowners_team.clone().unwrap_or_else(|| "Unowned".to_string());
37+
if !is_match(codeowners_team.as_deref(), &fast_display) {
38+
mismatches.push(format_mismatch(&project, &file.path, &codeowners_display, &fast_display));
39+
}
40+
}
41+
42+
Ok(mismatches)
43+
}
44+
45+
fn load_config(run_config: &RunConfig) -> Result<Config, String> {
46+
config_from_path(&run_config.config_path).map_err(|e| e.to_string())
47+
}
48+
49+
fn build_project(config: &Config, run_config: &RunConfig, cache: &Cache) -> Result<Project, String> {
50+
let mut project_builder = ProjectBuilder::new(
51+
config,
52+
run_config.project_root.clone(),
53+
run_config.codeowners_file_path.clone(),
54+
cache,
55+
);
56+
project_builder.build().map_err(|e| e.to_string())
57+
}
58+
59+
fn owners_for_file(path: &Path, run_config: &RunConfig, config: &Config) -> Result<(Option<String>, String), String> {
60+
let file_path_str = path.to_string_lossy().to_string();
61+
62+
let codeowners_team = team_for_file_from_codeowners(run_config, &file_path_str)
63+
.map_err(|e| e.to_string())?
64+
.map(|t| t.name);
65+
66+
let fast_owners = find_file_owners(&run_config.project_root, config, Path::new(&file_path_str))?;
67+
let fast_display = match fast_owners.len() {
68+
0 => "Unowned".to_string(),
69+
1 => fast_owners[0].team.name.clone(),
70+
_ => {
71+
let names: Vec<String> = fast_owners.into_iter().map(|fo| fo.team.name).collect();
72+
format!("Multiple: {}", names.join(", "))
73+
}
74+
};
75+
76+
Ok((codeowners_team, fast_display))
77+
}
78+
79+
fn is_match(codeowners_team: Option<&str>, fast_display: &str) -> bool {
80+
match (codeowners_team, fast_display) {
81+
(None, "Unowned") => true,
82+
(Some(t), fd) if fd == t => true,
83+
_ => false,
84+
}
85+
}
86+
87+
fn format_mismatch(project: &Project, file_path: &Path, codeowners_display: &str, fast_display: &str) -> String {
88+
let rel = project.relative_path(file_path).to_string_lossy().to_string();
89+
format!("- {}: CODEOWNERS={} fast={}", rel, codeowners_display, fast_display)
90+
}

src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
pub mod cache;
22
pub(crate) mod common_test;
33
pub mod config;
4+
pub mod crosscheck;
45
pub mod ownership;
56
pub(crate) mod project;
67
pub mod project_builder;

src/ownership/file_generator.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,15 +49,15 @@ impl FileGenerator {
4949
}
5050

5151
pub fn compare_lines(a: &String, b: &String) -> Ordering {
52-
if let Some((prefix, _)) = a.split_once("**") {
53-
if b.starts_with(prefix) {
54-
return Ordering::Less;
55-
}
52+
if let Some((prefix, _)) = a.split_once("**")
53+
&& b.starts_with(prefix)
54+
{
55+
return Ordering::Less;
5656
}
57-
if let Some((prefix, _)) = b.split_once("**") {
58-
if a.starts_with(prefix) {
59-
return Ordering::Greater;
60-
}
57+
if let Some((prefix, _)) = b.split_once("**")
58+
&& a.starts_with(prefix)
59+
{
60+
return Ordering::Greater;
6161
}
6262
a.cmp(b)
6363
}

src/ownership/for_file_fast.rs

Lines changed: 25 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,11 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path)
3232
if let Some(rel_str) = relative_file_path.to_str() {
3333
let is_config_owned = glob_list_matches(rel_str, &config.owned_globs);
3434
let is_config_unowned = glob_list_matches(rel_str, &config.unowned_globs);
35-
if is_config_owned && !is_config_unowned {
36-
if let Some(team) = teams_by_name.get(&team_name) {
37-
sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamFile);
38-
}
35+
if is_config_owned
36+
&& !is_config_unowned
37+
&& let Some(team) = teams_by_name.get(&team_name)
38+
{
39+
sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamFile);
3940
}
4041
}
4142
}
@@ -194,32 +195,30 @@ fn nearest_package_owner(
194195
if let Some(rel_str) = parent_rel.to_str() {
195196
if glob_list_matches(rel_str, &config.ruby_package_paths) {
196197
let pkg_yml = current.join("package.yml");
197-
if pkg_yml.exists() {
198-
if let Ok(owner) = read_ruby_package_owner(&pkg_yml) {
199-
if let Some(team) = teams_by_name.get(&owner) {
200-
let package_path = parent_rel.join("package.yml");
201-
let package_glob = format!("{rel_str}/**/**");
202-
return Some((
203-
team.name.clone(),
204-
Source::Package(package_path.to_string_lossy().to_string(), package_glob),
205-
));
206-
}
207-
}
198+
if pkg_yml.exists()
199+
&& let Ok(owner) = read_ruby_package_owner(&pkg_yml)
200+
&& let Some(team) = teams_by_name.get(&owner)
201+
{
202+
let package_path = parent_rel.join("package.yml");
203+
let package_glob = format!("{rel_str}/**/**");
204+
return Some((
205+
team.name.clone(),
206+
Source::Package(package_path.to_string_lossy().to_string(), package_glob),
207+
));
208208
}
209209
}
210210
if glob_list_matches(rel_str, &config.javascript_package_paths) {
211211
let pkg_json = current.join("package.json");
212-
if pkg_json.exists() {
213-
if let Ok(owner) = read_js_package_owner(&pkg_json) {
214-
if let Some(team) = teams_by_name.get(&owner) {
215-
let package_path = parent_rel.join("package.json");
216-
let package_glob = format!("{rel_str}/**/**");
217-
return Some((
218-
team.name.clone(),
219-
Source::Package(package_path.to_string_lossy().to_string(), package_glob),
220-
));
221-
}
222-
}
212+
if pkg_json.exists()
213+
&& let Ok(owner) = read_js_package_owner(&pkg_json)
214+
&& let Some(team) = teams_by_name.get(&owner)
215+
{
216+
let package_path = parent_rel.join("package.json");
217+
let package_glob = format!("{rel_str}/**/**");
218+
return Some((
219+
team.name.clone(),
220+
Source::Package(package_path.to_string_lossy().to_string(), package_glob),
221+
));
223222
}
224223
}
225224
}

src/ownership/mapper/package_mapper.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,10 +122,10 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> {
122122

123123
for package in packages.iter().sorted_by_key(|package| package.package_root()) {
124124
if let Some(last_package) = top_level_packages.last() {
125-
if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) {
126-
if !current_root.starts_with(last_root) {
127-
top_level_packages.push(package);
128-
}
125+
if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root())
126+
&& !current_root.starts_with(last_root)
127+
{
128+
top_level_packages.push(package);
129129
}
130130
} else {
131131
top_level_packages.push(package);

0 commit comments

Comments
 (0)