diff --git a/rust-toolchain.toml b/rust-toolchain.toml index bbf93c3..a1e0e05 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "1.85.0" +channel = "1.86.0" components = ["clippy", "rustfmt"] targets = ["x86_64-apple-darwin", "aarch64-apple-darwin", "x86_64-unknown-linux-gnu"] diff --git a/src/ownership.rs b/src/ownership.rs index aede707..67e3e71 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -125,8 +125,8 @@ impl Ownership { } #[instrument(level = "debug", skip_all)] - pub fn for_file(&self, file_path: &str) -> Result, ValidatorErrors> { - info!("getting file ownership for {}", file_path); + pub fn for_file(&self, file_path: &Path) -> Result, ValidatorErrors> { + info!("getting file ownership for {}", file_path.display()); let owner_matchers: Vec = self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(); let file_owner_finder = FileOwnerFinder { owner_matchers: &owner_matchers, @@ -186,7 +186,7 @@ mod tests { #[test] fn test_for_file_owner() -> Result<(), Box> { let ownership = build_ownership_with_all_mappers()?; - let file_owners = ownership.for_file("app/consumers/directory_owned.rb").unwrap(); + let file_owners = ownership.for_file(Path::new("app/consumers/directory_owned.rb")).unwrap(); assert_eq!(file_owners.len(), 1); assert_eq!(file_owners[0].team.name, "Bar"); assert_eq!(file_owners[0].team_config_file_path, "config/teams/bar.yml"); @@ -196,7 +196,7 @@ mod tests { #[test] fn test_for_file_no_owner() -> Result<(), Box> { let ownership = build_ownership_with_all_mappers()?; - let file_owners = ownership.for_file("app/madeup/foo.rb").unwrap(); + let file_owners = ownership.for_file(Path::new("app/madeup/foo.rb")).unwrap(); assert_eq!(file_owners.len(), 0); Ok(()) } diff --git a/src/ownership/file_generator.rs b/src/ownership/file_generator.rs index f2b8efb..6d8a34d 100644 --- a/src/ownership/file_generator.rs +++ b/src/ownership/file_generator.rs @@ -43,23 +43,25 @@ impl FileGenerator { fn to_sorted_lines(entries: &[Entry]) -> Vec { let mut lines: Vec = entries.iter().map(|entry| entry.to_row()).collect(); - lines.sort_by(|a, b| { - if let Some((prefix, _)) = a.split_once("**") { - if b.starts_with(prefix) { - return Ordering::Less; - } - } - if let Some((prefix, _)) = b.split_once("**") { - if a.starts_with(prefix) { - return Ordering::Greater; - } - } - a.cmp(b) - }); + lines.sort_by(compare_lines); lines } } +pub fn compare_lines(a: &String, b: &String) -> Ordering { + if let Some((prefix, _)) = a.split_once("**") { + if b.starts_with(prefix) { + return Ordering::Less; + } + } + if let Some((prefix, _)) = b.split_once("**") { + if a.starts_with(prefix) { + return Ordering::Greater; + } + } + a.cmp(b) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/ownership/parser.rs b/src/ownership/parser.rs index 57a3ae9..bd60bfa 100644 --- a/src/ownership/parser.rs +++ b/src/ownership/parser.rs @@ -4,6 +4,7 @@ use crate::{ }; use fast_glob::glob_match; use memoize::memoize; +use regex::Regex; use std::{ collections::HashMap, error::Error, @@ -12,6 +13,8 @@ use std::{ path::{Path, PathBuf}, }; +use super::file_generator::compare_lines; + pub struct Parser { pub project_root: PathBuf, pub codeowners_file_path: PathBuf, @@ -54,7 +57,6 @@ impl Parser { #[memoize] fn teams_by_github_team_name(team_file_glob: Vec) -> HashMap { - dbg!("in teams_by_github_team_name"); let mut teams = HashMap::new(); for glob in team_file_glob { let paths = glob::glob(&glob).expect("Failed to read glob pattern").filter_map(Result::ok); @@ -76,8 +78,6 @@ fn teams_by_github_team_name(team_file_glob: Vec) -> HashMap Vec { - dbg!("in build_codeowners_lines_in_priority"); - dbg!(&codeowners_file_path); let codeowners_file = match fs::read_to_string(codeowners_file_path) { Ok(codeowners_file) => codeowners_file, Err(e) => { @@ -89,13 +89,67 @@ fn build_codeowners_lines_in_priority(codeowners_file_path: String) -> Vec, +} + +impl Section { + fn new(heading: String, lines: Vec) -> Self { + let mut sorted_lines = lines.clone(); + sorted_lines.sort_by(compare_lines); + Self { + heading, + lines: sorted_lines, + } + } +} + +fn codeowner_sections(codeowners_file: &str) -> Result, Box> { + let un_ignore = Regex::new(r"^# \/")?; + let mut iter = codeowners_file.lines().peekable(); + let mut sections = Vec::new(); + let mut current_section = None; + let mut current_lines = Vec::new(); + + while let Some(line) = iter.next() { + let line = un_ignore.replace(line, "/").to_string(); + if line.is_empty() { + continue; + } + + if line.starts_with('#') { + if iter + .peek() + .map(|next| next.starts_with('/') || next.starts_with("# /")) + .unwrap_or(false) + { + if let Some(section_name) = current_section.take() { + sections.push(Section::new(section_name, std::mem::take(&mut current_lines))); + } + current_section = Some(line); + } + } else { + current_lines.push(line); + } + } + + if let Some(section_name) = current_section { + sections.push(Section::new(section_name, current_lines)); + } + + Ok(sections) +} + fn stripped_lines_by_priority(codeowners_file: &str) -> Vec { - codeowners_file - .lines() - .filter(|line| !line.trim().is_empty() && !line.trim().starts_with("#")) - .map(|line| line.trim().to_string()) - .rev() - .collect() + let mut lines = Vec::new(); + let sections = codeowner_sections(codeowners_file).unwrap_or_default(); + for section in sections { + lines.extend(section.lines); + } + lines.reverse(); + lines } pub fn parse_for_team(team_name: String, codeowners_file: &str) -> Result, Box> { @@ -136,6 +190,7 @@ pub fn parse_for_team(team_name: String, codeowners_file: &str) -> Result Result<(), Box> { + let codeownership_file = indoc! {" + # STOP! - DO NOT EDIT THIS FILE MANUALLY + # This file was automatically generated by \"bin/codeownership validate\". + # + # CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub + # teams. This is useful when developers create Pull Requests since the + # code/file owner is notified. Reference GitHub docs for more details: + # https://help.github.com/en/articles/about-code-owners + + + # Annotations at the top of file + # /app/assets/config/manifest.js @Prefix/team-foo + # /config/application.rb @Prefix/team-bar + /config/i18n-tasks.yml.erb @Prefix/language-team + + # Team-specific owned globs + # /.github/workflows/pull-translations.yml @Prefix/infra + # /.github/workflows/push-sources.yml @Prefix/infra + # /Dockerfile @Prefix/docker-team + # /components/create.rb @Prefix/component-team + /.codeclimate.yml @Prefix/climate-team + /.vscode/extensions/z/**/* @Prefix/zteam + /bin/brakeman @Prefix/psecurity + /config/brakeman.ignore @Prefix/security + "}; + + // build up each sections lines + // resort the lines without the '#' + // re-assemble the sections + // reverse sort + let codeowner_sections = codeowner_sections(codeownership_file)?; + assert_eq!( + codeowner_sections, + vec![ + Section { + heading: "# Annotations at the top of file".to_string(), + lines: vec![ + "/app/assets/config/manifest.js @Prefix/team-foo".to_string(), + "/config/application.rb @Prefix/team-bar".to_string(), + "/config/i18n-tasks.yml.erb @Prefix/language-team".to_string() + ] + }, + Section { + heading: "# Team-specific owned globs".to_string(), + lines: vec![ + "/.codeclimate.yml @Prefix/climate-team".to_string(), + "/.github/workflows/pull-translations.yml @Prefix/infra".to_string(), + "/.github/workflows/push-sources.yml @Prefix/infra".to_string(), + "/.vscode/extensions/z/**/* @Prefix/zteam".to_string(), + "/Dockerfile @Prefix/docker-team".to_string(), + "/bin/brakeman @Prefix/psecurity".to_string(), + "/components/create.rb @Prefix/component-team".to_string(), + "/config/brakeman.ignore @Prefix/security".to_string() + ] + }, + ] + ); + let stripped_lines = stripped_lines_by_priority(codeownership_file); + assert_eq!( + stripped_lines, + vec![ + "/config/brakeman.ignore @Prefix/security", + "/components/create.rb @Prefix/component-team", + "/bin/brakeman @Prefix/psecurity", + "/Dockerfile @Prefix/docker-team", + "/.vscode/extensions/z/**/* @Prefix/zteam", + "/.github/workflows/push-sources.yml @Prefix/infra", + "/.github/workflows/pull-translations.yml @Prefix/infra", + "/.codeclimate.yml @Prefix/climate-team", + "/config/i18n-tasks.yml.erb @Prefix/language-team", + "/config/application.rb @Prefix/team-bar", + "/app/assets/config/manifest.js @Prefix/team-foo" + ] + ); + Ok(()) + } + + #[test] + fn test_unignore_regex() -> Result<(), Box> { + let un_ignore = Regex::new(r"^# \/")?; + assert_eq!(un_ignore.replace("# /path/to/owned", "/"), "/path/to/owned"); + Ok(()) + } } diff --git a/src/project_builder.rs b/src/project_builder.rs index 396ab29..c9820b2 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -7,7 +7,7 @@ use error_stack::{Result, ResultExt}; use fast_glob::glob_match; use ignore::WalkBuilder; use rayon::iter::{IntoParallelIterator, ParallelIterator}; -use tracing::instrument; +use tracing::{instrument, warn}; use crate::{ cache::Cache, @@ -149,10 +149,14 @@ impl<'a> ProjectBuilder<'a> { owner, }); } - EntryType::TeamFile(absolute_path, _relative_path) => { - let team = Team::from_team_file_path(absolute_path).unwrap(); - team_files.push(team); - } + EntryType::TeamFile(absolute_path, _relative_path) => match Team::from_team_file_path(absolute_path) { + Ok(team) => { + team_files.push(team); + } + Err(e) => { + warn!("Error building team from team file path: {}", e); + } + }, EntryType::NullEntry() => {} } (project_files, pkgs, gems, codeowners, team_files) @@ -169,7 +173,10 @@ impl<'a> ProjectBuilder<'a> { acc }, ); - let teams_by_name = teams.iter().map(|team| (team.name.clone(), team.clone())).collect(); + let teams_by_name = teams + .iter() + .flat_map(|team| vec![(team.name.clone(), team.clone()), (team.github_team.clone(), team.clone())]) + .collect(); Ok(Project { base_path: self.base_path.to_owned(), diff --git a/src/runner.rs b/src/runner.rs index 25a3518..2e5fd11 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -69,6 +69,9 @@ fn for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> RunResul pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> Result, Error> { let config = config_from_path(&run_config.config_path)?; + let relative_file_path = Path::new(file_path) + .strip_prefix(&run_config.project_root) + .unwrap_or(Path::new(file_path)); let parser = crate::ownership::parser::Parser { project_root: run_config.project_root.clone(), @@ -76,7 +79,7 @@ pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> team_file_globs: config.team_file_glob.clone(), }; Ok(parser - .team_from_file_path(Path::new(file_path)) + .team_from_file_path(Path::new(relative_file_path)) .map_err(|e| Error::Io(e.to_string()))?) } @@ -198,7 +201,11 @@ impl Runner { } pub fn generate(&self) -> RunResult { - match std::fs::write(&self.run_config.codeowners_file_path, self.ownership.generate_file()) { + let content = self.ownership.generate_file(); + if let Some(parent) = &self.run_config.codeowners_file_path.parent() { + let _ = std::fs::create_dir_all(parent); + } + match std::fs::write(&self.run_config.codeowners_file_path, content) { Ok(_) => RunResult::default(), Err(err) => RunResult { io_errors: vec![err.to_string()], @@ -216,7 +223,10 @@ impl Runner { } pub fn for_file(&self, file_path: &str) -> RunResult { - let file_owners = match self.ownership.for_file(file_path) { + let relative_file_path = Path::new(file_path) + .strip_prefix(&self.run_config.project_root) + .unwrap_or(Path::new(file_path)); + let file_owners = match self.ownership.for_file(relative_file_path) { Ok(file_owners) => file_owners, Err(err) => { return RunResult { diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index d72277f..bb5e0e8 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -1,7 +1,7 @@ use assert_cmd::prelude::*; use indoc::indoc; use predicates::prelude::predicate; -use std::{error::Error, path::Path, process::Command}; +use std::{error::Error, fs, path::Path, process::Command}; #[test] fn test_validate() -> Result<(), Box> { @@ -56,6 +56,29 @@ fn test_for_file() -> Result<(), Box> { Ok(()) } +#[test] +fn test_for_file_full_path() -> Result<(), Box> { + let project_root = Path::new("tests/fixtures/valid_project"); + let for_file_absolute_path = fs::canonicalize(project_root.join("ruby/app/models/payroll.rb"))?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("for-file") + .arg(for_file_absolute_path.to_str().unwrap()) + .assert() + .success() + .stdout(predicate::eq(indoc! {" + Team: Payroll + Github Team: @PayrollTeam + Team YML: config/teams/payroll.yml + Description: + - Owner annotation at the top of the file + "})); + Ok(()) +} + #[test] fn test_fast_for_file() -> Result<(), Box> { Command::cargo_bin("codeowners")? @@ -74,6 +97,27 @@ fn test_fast_for_file() -> Result<(), Box> { Ok(()) } +#[test] +fn test_fast_for_file_full_path() -> Result<(), Box> { + let project_root = Path::new("tests/fixtures/valid_project"); + let for_file_absolute_path = fs::canonicalize(project_root.join("ruby/app/models/payroll.rb"))?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("for-file") + .arg("--fast") + .arg(for_file_absolute_path.to_str().unwrap()) + .assert() + .success() + .stdout(predicate::eq(indoc! {" + Team: Payroll + Team YML: config/teams/payroll.yml + "})); + Ok(()) +} + #[test] fn test_for_file_same_team_multiple_ownerships() -> Result<(), Box> { Command::cargo_bin("codeowners")?