diff --git a/Cargo.lock b/Cargo.lock index 944ebe3..1f9711d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -184,7 +184,7 @@ checksum = "1c133bc6a41be0d194c306b5506d15e6feeea7b1d6604bd3f8310dfb2ca96486" [[package]] name = "codeowners" -version = "0.5.0" +version = "0.6.0" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index 9dc4fcd..be84266 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.5.0" +version = "0.6.0" edition = "2024" license = "MIT" diff --git a/README.md b/README.md index bec715a..d99237b 100644 --- a/README.md +++ b/README.md @@ -68,8 +68,9 @@ cargo install --git https://github.com/rubyatscale/codeowners-rs codeowners js_package_paths: [] unowned_globs: - db/**/* - - app/services/some_file1.rb - frontend/javascripts/**/__generated__/**/* + optionally_owned_globs: + - app/services/some_file1.rb ``` 2. **Declare Teams** @@ -211,19 +212,20 @@ codeowners gv --no-cache - `ruby_package_paths` (default: `['packs/**/*', 'components/**']`) - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) -- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`) +- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. A team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. Use this for third-party or generated code. +- `optionally_owned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. It doesn't add to `owned_globs`, so a file must match both. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An optionally owned file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list". As in a full `validate`, an annotation, `.codeowner` or package file doesn't count if the file is untracked or in a top-level directory named in `ignore_dirs`, so a new file's own annotation counts once the file is staged. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `optionally_owned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) -- `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. +- `executable_name` (default: `'codeowners generate'`): The command that validation errors tell you to run to update the CODEOWNERS file. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. +- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself, which is always held to the default of 20,000 nodes, about one per list entry. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. Example configuration with custom executable name: ```yaml owned_globs: - '{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx}' -executable_name: 'bin/codeownership' # For Ruby gem wrapper +executable_name: 'bin/codeownership validate' # For Ruby gem wrapper ``` See examples in `tests/fixtures/**/config/` for reference setups. @@ -241,7 +243,7 @@ By default, cache is stored under `tmp/cache/codeowners` relative to the project 1. Only one mechanism defines ownership for any file. 2. All referenced teams are valid. -3. All files in `owned_globs` are owned, unless matched by `unowned_globs`. +3. All files in `owned_globs` are owned, unless matched by `unowned_globs` or `optionally_owned_globs`. 4. The generated `CODEOWNERS` file is up to date. Exit status is non-zero on errors. diff --git a/src/config.rs b/src/config.rs index 4894dac..23cbba1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -44,6 +44,9 @@ pub struct Config { #[serde(default = "default_codeowners_path", deserialize_with = "crate::yaml::string_or_null")] pub codeowners_path: String, + #[serde(default, deserialize_with = "crate::yaml::strings_or_nulls")] + pub optionally_owned_globs: Vec, + #[serde(default = "default_yaml_max_nodes")] pub yaml_max_nodes: usize, @@ -122,7 +125,7 @@ fn default_path() -> PathBuf { impl Config { pub fn load_from_path(path: &Path) -> std::result::Result { let bytes = std::fs::read(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; - // The config can't raise its own limit, and at a few hundred nodes it never needs to. + // The config can't raise its own limit, so its lists can hold about 20,000 entries in all. let config: Self = crate::yaml::from_slice(&bytes, crate::yaml::DEFAULT_MAX_NODES) .map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e))?; Ok(Self { @@ -173,6 +176,7 @@ mod tests { ); assert_eq!(config.vendored_gems_path, "vendored/"); assert_eq!(config.executable_name, "codeowners generate"); + assert!(config.optionally_owned_globs.is_empty()); assert_eq!(config.yaml_max_nodes, 20_000); Ok(()) } @@ -254,6 +258,23 @@ mod tests { Ok(()) } + #[test] + fn test_parse_config_with_optionally_owned_globs() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + optionally_owned_globs: + - \"**/deprecated/**/*\" + "}; + fs::write(&config_path, config_str)?; + let config = Config::load_from_path(&config_path)?; + assert_eq!(config.optionally_owned_globs, vec!["**/deprecated/**/*"]); + Ok(()) + } + #[test] fn test_codeowners_path_defaults_when_not_specified() -> Result<(), Box> { let temp_dir = tempdir()?; @@ -304,6 +325,8 @@ mod tests { - ignore_dirs: - + optionally_owned_globs: + - vendored_gems_path: cache_directory: executable_name: @@ -318,6 +341,7 @@ mod tests { &config.team_file_glob, &config.unowned_globs, &config.ignore_dirs, + &config.optionally_owned_globs, ] { assert_eq!(globs, &[""]); } diff --git a/src/ownership.rs b/src/ownership.rs index c0422e4..9cefbd7 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -4,7 +4,7 @@ use mapper::{OwnerMatcher, Source, TeamName}; use std::{ error::Error, fmt::{self, Display}, - path::Path, + path::{Path, PathBuf}, sync::Arc, }; use tracing::{info, instrument}; @@ -109,6 +109,27 @@ impl Entry { } } +/// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation naming one +/// still counts for its file, and a `.codeowner`, `package.yml` or `package.json` naming one for every path under its directory. +/// It sees only files the project reads, so an annotated file, `.codeowner` or package file names no owner here if it is +/// untracked or under a top-level directory named in ignore_dirs, as in `validate`. Building one goes through every project +/// file, so reuse it. +pub(crate) struct OwnerLookup { + owner_matchers: Vec, + unknown_team_roots: Vec, +} + +impl OwnerLookup { + pub(crate) fn has_owner(&self, relative_path: &Path) -> bool { + // FileOwnerFinder::find returns an owner exactly when some matcher names a team, but builds every owner to do it. + self.unknown_team_roots.iter().any(|root| relative_path.starts_with(root)) + || self + .owner_matchers + .iter() + .any(|owner_matcher| owner_matcher.owner_for(relative_path).0.is_some()) + } +} + impl Ownership { pub fn build(project: Project) -> Self { Self { @@ -156,6 +177,42 @@ impl Ownership { .collect()) } + pub(crate) fn owner_lookup(&self) -> OwnerLookup { + // `validate` reports a package.yml, package.json or `.codeowner` naming a team that doesn't exist, but a list of files + // isn't checked for that, so here each still names an owner for the files under its directory. The package mappers drop + // such a package, and the directory mapper's glob misses a `.codeowner` at the project root and reads braces as alternatives, + // matching `app/legacy` for one in `app/{legacy}`. So the directory is matched as a path prefix, which replaces that glob + // for such a `.codeowner`: brackets and braces are literal and the root covers every file. + let package_roots = self + .project + .packages + .iter() + .filter(|package| !self.project.teams_by_name.contains_key(&package.owner)) + .filter_map(|package| package.package_root()); + let directory_roots = self + .project + .directory_codeowner_files + .iter() + .filter(|directory_codeowner_file| !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner)) + .filter_map(|directory_codeowner_file| directory_codeowner_file.directory_root()); + OwnerLookup { + owner_matchers: self + .mappers() + .iter() + .flat_map(|mapper| mapper.owner_matchers()) + .filter(|owner_matcher| match owner_matcher { + OwnerMatcher::Glob { + source: Source::Directory(_), + team_name, + .. + } => self.project.teams_by_name.contains_key(team_name), + _ => true, + }) + .collect(), + unknown_team_roots: package_roots.chain(directory_roots).map(Path::to_path_buf).collect(), + } + } + #[instrument(level = "debug", skip_all)] pub fn for_team(&self, team_name: &str) -> Result, Box> { info!("getting team ownership for {}", team_name); diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 2aa2450..23a5fcc 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -385,6 +385,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), + optionally_owned_globs: vec![], yaml_max_nodes: crate::yaml::DEFAULT_MAX_NODES, path: PathBuf::from(crate::config::DEFAULT_PATH), } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 3f5339c..e291755 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,3 +1,4 @@ +use crate::path_utils::matches_globs; use crate::project::{Project, ProjectFile}; use core::fmt; use std::collections::HashSet; @@ -6,6 +7,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use itertools::Itertools; +use rayon::prelude::IntoParallelIterator; use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; use similar::{ChangeTag, TextDiff}; @@ -166,22 +168,29 @@ impl Validator { #[instrument(name = "validate_file_ownership", level = "debug", skip_all)] fn validate_file_ownership(&self) -> Vec { - let mut validation_errors = Vec::new(); - - for (file, owners) in self.file_to_owners() { - let relative_path = self.project.relative_path(&file.path).to_owned(); - - if owners.is_empty() { - validation_errors.push(Error::FileWithoutOwner { path: relative_path }) - } else if owners.len() > 1 { - validation_errors.push(Error::FileWithMultipleOwners { - path: relative_path, - owners, - }) - } - } - - validation_errors + let project = &self.project; + + self.file_to_owners() + .into_par_iter() + .filter_map(|(file, owners)| { + let relative_path = project.relative_path(&file.path).to_owned(); + + if owners.is_empty() { + if matches_globs(&relative_path, &project.optionally_owned_globs) { + None + } else { + Some(Error::FileWithoutOwner { path: relative_path }) + } + } else if owners.len() > 1 { + Some(Error::FileWithMultipleOwners { + path: relative_path, + owners, + }) + } else { + None + } + }) + .collect() } #[instrument(name = "validate_codeowners_file", level = "debug", skip_all)] diff --git a/src/path_utils.rs b/src/path_utils.rs index 230b1d1..dc98e00 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; /// Return `path` relative to `root` if possible; otherwise return `path` unchanged. pub fn relative_to<'a>(root: &'a Path, path: &'a Path) -> &'a Path { @@ -10,10 +10,51 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } +/// `path` without `.` components or repeated separators, or `None` if it has a `..`, a root or a prefix. Owners are matched +/// against plain project-relative paths, and no other path can be made into one without asking the file system. +pub(crate) fn plain_relative(path: &Path) -> Option { + let mut plain = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::Normal(name) => plain.push(name), + Component::ParentDir | Component::RootDir | Component::Prefix(_) => return None, + } + } + Some(plain) +} + +/// Returns true if `path` matches any of the provided glob patterns. +pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { + match path.to_str() { + Some(s) => globs.iter().any(|glob| fast_glob::glob_match(glob, s)), + None => false, + } +} + #[cfg(test)] mod tests { use super::*; + const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; + + #[test] + fn test_matches_globs() { + assert!(matches_globs(Path::new("script/.eslintrc.js"), &[OWNED_GLOB.to_string()])); + } + + #[test] + fn test_glob_match() { + assert!(fast_glob::glob_match(OWNED_GLOB, "script/.eslintrc.js")); + } + + #[test] + fn matches_globs_double_star_matches_root_level_and_nested_files() { + let globs = vec!["**/*".to_string()]; + assert!(matches_globs(Path::new("root.rb"), &globs)); + assert!(matches_globs(Path::new("app/models/user.rb"), &globs)); + } + #[test] fn relative_to_returns_relative_when_under_root() { let root = Path::new("/a/b"); @@ -46,4 +87,27 @@ mod tests { let rel_buf = relative_to_buf(root, path); assert_eq!(rel_ref, rel_buf.as_path()); } + + // Compared as strings, as globs see them, since Path equality ignores a repeated separator or a `.` past the start. + fn plain(path: &str) -> Option { + plain_relative(Path::new(path)).map(|plain| plain.to_string_lossy().into_owned()) + } + + #[test] + fn plain_relative_returns_a_plain_path_unchanged() { + assert_eq!(plain("a/b").as_deref(), Some("a/b")); + } + + #[test] + fn plain_relative_drops_dot_components_and_repeated_separators() { + assert_eq!(plain("./a/b").as_deref(), Some("a/b")); + assert_eq!(plain("a/./b").as_deref(), Some("a/b")); + assert_eq!(plain("a//b").as_deref(), Some("a/b")); + } + + #[test] + fn plain_relative_returns_none_for_parent_dirs_and_roots() { + assert_eq!(plain("a/../b"), None); + assert_eq!(plain("/a/b"), None); + } } diff --git a/src/project.rs b/src/project.rs index 4c6bb0a..1740172 100644 --- a/src/project.rs +++ b/src/project.rs @@ -17,6 +17,7 @@ pub struct Project { pub directory_codeowner_files: Vec, pub teams_by_name: HashMap, pub executable_name: String, + pub optionally_owned_globs: Vec, } #[derive(Clone, Debug)] @@ -229,6 +230,7 @@ mod tests { directory_codeowner_files: vec![], teams_by_name: HashMap::new(), executable_name: "codeowners generate".to_string(), + optionally_owned_globs: vec![], }; let map = project.vendored_gem_by_name(); diff --git a/src/project_builder.rs b/src/project_builder.rs index 769a185..a3e1b2d 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -5,7 +5,6 @@ use std::{ }; use error_stack::{Report, ResultExt}; -use fast_glob::glob_match; use ignore::{DirEntry, WalkBuilder, WalkParallel, WalkState}; use rayon::iter::{IntoParallelIterator, ParallelIterator}; use tracing::instrument; @@ -13,6 +12,7 @@ use tracing::instrument; use crate::{ cache::Cache, config::Config, + path_utils::matches_globs, project::{DirectoryCodeownersFile, Error, Package, PackageType, Project, ProjectFile, Team, VendoredGem, deserializers}, project_file_builder::ProjectFileBuilder, tracked_files, @@ -327,17 +327,11 @@ impl<'a> ProjectBuilder<'a> { directory_codeowner_files: directory_codeowners, teams_by_name, executable_name: self.config.executable_name.clone(), + optionally_owned_globs: self.config.optionally_owned_globs.clone(), }) } } -fn matches_globs(path: &Path, globs: &[String]) -> bool { - match path.to_str() { - Some(s) => globs.iter().any(|glob| glob_match(glob, s)), - None => false, - } -} - pub(crate) fn ruby_package_owner(path: &Path, node_limit: &NodeLimit) -> Result, Report> { let bytes = std::fs::read(path).change_context(Error::Io)?; // An empty, comment-only or `---`-only file is a null document, which serde-saphyr 2.1 no longer reads as a struct. @@ -375,18 +369,6 @@ mod tests { use super::*; - const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; - - #[test] - fn test_matches_globs() { - assert!(matches_globs(Path::new("script/.eslintrc.js"), &[OWNED_GLOB.to_string()])); - } - - #[test] - fn test_glob_match() { - assert!(glob_match(OWNED_GLOB, "script/.eslintrc.js")); - } - #[test] fn test_ruby_package_owner_top_level() { let yaml = "owner: TeamA\n"; diff --git a/src/runner.rs b/src/runner.rs index 5562979..f4397e5 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,8 +1,8 @@ +use std::cell::LazyCell; use std::path::{Path, PathBuf}; use std::process::Command; use error_stack::{Report, ResultExt}; -use fast_glob::glob_match; use serde::Serialize; use tracing::debug_span; @@ -10,6 +10,7 @@ use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, + path_utils::{matches_globs, plain_relative, relative_to_buf}, project_builder::ProjectBuilder, }; @@ -125,7 +126,7 @@ impl Runner { if file_paths.is_empty() { self.validate_all() } else { - self.validate_files(file_paths) + self.validate_files(file_paths, false) } } @@ -142,43 +143,90 @@ impl Runner { } } - fn validate_files(&self, file_paths: Vec) -> RunResult { + fn validate_files(&self, file_paths: Vec, codeowners_generated: bool) -> RunResult { let mut unowned_files = Vec::new(); + let mut optionally_owned_files_with_unlisted_owners = Vec::new(); let mut io_errors = Vec::new(); // Filter files based on owned_globs and unowned_globs configuration // Only validate files that match owned_globs and don't match unowned_globs - let filtered_paths: Vec = file_paths + let filtered_paths: Vec<(String, PathBuf)> = file_paths .into_iter() - .filter(|file_path| { - // Convert to relative path for glob matching - let path = Path::new(file_path); + .map(|file_path| { + let path = Path::new(&file_path); + // As in the CODEOWNERS lookup, a relative path is already relative to the project root, even when that root is relative. let relative_path = if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path) + relative_to_buf(&self.run_config.project_root, path) } else { - path + path.to_path_buf() }; - - // Mirror the filtering applied by ProjectBuilder when walking the project + (file_path, relative_path) + }) + // Mirror the filtering applied by ProjectBuilder when walking the project + .filter(|(_, relative_path)| { matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) }) .collect(); + // Built only if an optionally owned file needs it, and then once. + let owner_lookup = LazyCell::new(|| self.ownership.owner_lookup()); + let listed_in_codeowners = |path: &Path| { + matches!( + team_for_file_from_codeowners(&self.run_config, &path.to_string_lossy()), + Ok(Some(_)) + ) + }; + debug_span!("per_file_query").in_scope(|| { - for file_path in filtered_paths { + for (file_path, relative_path) in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} - Ok(None) => unowned_files.push(file_path), + // optionally_owned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or + // package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported, + // but not as unowned, which the key allows. Only files the project reads count, though, so an annotated + // file, `.codeowner` or package file names no owner here if it is untracked or under a top-level directory + // named in ignore_dirs, as in `validate`. The lookup matches plain project-relative paths only, so a path + // with a `..`, or an absolute one not under the project root, is never excused and is reported as unowned, + // as without the key. + Ok(None) => { + match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.optionally_owned_globs)) { + Some(path) if !owner_lookup.has_owner(&path) => {} + // The CODEOWNERS lookup above read the path as given, so a `./`, `//` or inner `.` missed the file's line. + Some(path) if path.as_os_str() != relative_path.as_os_str() && listed_in_codeowners(&path) => {} + Some(_) => optionally_owned_files_with_unlisted_owners.push(file_path), + None => unowned_files.push(file_path), + } + } Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } } }); + let mut validation_errors = Vec::new(); if !unowned_files.is_empty() { - let validation_errors = std::iter::once("Unowned files detected:".to_string()) - .chain(unowned_files.into_iter().map(|file| format!(" {}", file))) - .collect(); + validation_errors.push("Unowned files detected:".to_string()); + validation_errors.extend(unowned_files.into_iter().map(|file| format!(" {}", file))); + } + if !optionally_owned_files_with_unlisted_owners.is_empty() { + // `gv` has just written CODEOWNERS, so updating it again can't help, and only a team that doesn't exist is left out. + let or_update_codeowners = if codeowners_generated { + String::new() + } else { + format!(", or run `{}` to update the CODEOWNERS file", self.config.executable_name) + }; + validation_errors.push(format!( + "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name in each one's annotation, `.codeowner` or package{}:", + or_update_codeowners + )); + validation_errors.extend( + optionally_owned_files_with_unlisted_owners + .into_iter() + .map(|file| format!(" {}", file)), + ); + } + if !validation_errors.is_empty() { return RunResult { validation_errors, io_errors, @@ -220,7 +268,11 @@ impl Runner { if run_result.has_errors() { return run_result; } - self.validate(file_paths) + if file_paths.is_empty() { + self.validate_all() + } else { + self.validate_files(file_paths, true) + } } fn git_stage(&self) { @@ -442,14 +494,6 @@ impl RunResult { } } -/// Returns true if `path` matches any of the provided glob patterns. -fn matches_globs(path: &Path, globs: &[String]) -> bool { - match path.to_str() { - Some(s) => globs.iter().any(|glob| glob_match(glob, s)), - None => false, - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/tests/fixtures/optionally_owned_globs/.github/CODEOWNERS b/tests/fixtures/optionally_owned_globs/.github/CODEOWNERS new file mode 100644 index 0000000..b117ec9 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/.github/CODEOWNERS @@ -0,0 +1,15 @@ +# 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 + + +# Team YML ownership +/config/teams/foo.yml @FooTeam + +# Annotations at the top of file +/app/annotated.rb @FooTeam +/app/deprecated/annotated_old.rb @FooTeam diff --git a/tests/fixtures/optionally_owned_globs/app/annotated.rb b/tests/fixtures/optionally_owned_globs/app/annotated.rb new file mode 100644 index 0000000..cfab2e8 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/app/annotated.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated' diff --git a/tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb b/tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb new file mode 100644 index 0000000..d150983 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated, in an optionally owned glob' diff --git a/tests/fixtures/optionally_owned_globs/app/deprecated/old.rb b/tests/fixtures/optionally_owned_globs/app/deprecated/old.rb new file mode 100644 index 0000000..8eb0b57 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/app/deprecated/old.rb @@ -0,0 +1 @@ +puts 'no owner' diff --git a/tests/fixtures/optionally_owned_globs/config/code_ownership.yml b/tests/fixtures/optionally_owned_globs/config/code_ownership.yml new file mode 100644 index 0000000..cb352a7 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/config/code_ownership.yml @@ -0,0 +1,5 @@ +--- +owned_globs: + - "{app,config}/**/*.rb" +optionally_owned_globs: + - "app/deprecated/**/*" diff --git a/tests/fixtures/optionally_owned_globs/config/teams/foo.yml b/tests/fixtures/optionally_owned_globs/config/teams/foo.yml new file mode 100644 index 0000000..3b226c9 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/config/teams/foo.yml @@ -0,0 +1,4 @@ +--- +name: Foo +github: + team: '@FooTeam' diff --git a/tests/optionally_owned_globs_test.rs b/tests/optionally_owned_globs_test.rs new file mode 100644 index 0000000..c916837 --- /dev/null +++ b/tests/optionally_owned_globs_test.rs @@ -0,0 +1,780 @@ +use assert_cmd::{assert::Assert, prelude::*}; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, ffi::OsStr, fs, path::Path, process::Command}; +use tempfile::TempDir; + +mod common; +use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; + +const FIXTURE: &str = "optionally_owned_globs"; + +// `validate` suggests running the default executable_name, but `gv` has just written CODEOWNERS, so it doesn't. +fn unlisted_owner_heading(command: &str) -> &'static str { + if command == "gv" { + "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name in each one's annotation, `.codeowner` or package:" + } else { + "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name in each one's annotation, `.codeowner` or package, \ + or run `codeowners generate` to update the CODEOWNERS file:" + } +} + +type FixtureChange = fn(&Path) -> std::io::Result<()>; + +#[test] +fn test_validate_passes_with_unowned_files_in_optionally_owned_globs() -> Result<(), Box> { + // The fixture's CODEOWNERS lists `app/deprecated/annotated_old.rb`, so if annotations in optionally owned globs stop + // counting, this also fails, with CODEOWNERS out of date. + run_codeowners(FIXTURE, &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_generate_and_validate_passes_with_unowned_files_in_optionally_owned_globs() -> Result<(), Box> { + run_codeowners(FIXTURE, &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_unowned_file_in_an_optionally_owned_glob() -> Result<(), Box> { + run_codeowners( + FIXTURE, + &["validate", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::eq(""), + )?; + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_absolute_path_in_an_optionally_owned_glob() -> Result<(), Box> { + validate_absolute_path(|_| Ok(()), "app/deprecated/old.rb")? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_annotated_and_an_unowned_file_in_an_optionally_owned_glob() -> Result<(), Box> { + // What a pre-commit hook checks when a commit edits both. An optionally owned file that names an owner is reported only if + // CODEOWNERS doesn't list it, and CODEOWNERS lists the annotated one. + for command in ["validate", "gv"] { + run_codeowners( + FIXTURE, + &[command, "app/deprecated/annotated_old.rb", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::eq(""), + )?; + } + Ok(()) +} + +// The `for-file` tests pin that it reports an optionally owned file's owner as it would without optionally_owned_globs. +#[test] +fn test_for_file_reports_annotations_in_optionally_owned_globs() -> Result<(), Box> { + run_codeowners( + FIXTURE, + &["for-file", "app/deprecated/annotated_old.rb"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {" + Team: Foo + Github Team: @FooTeam + Team YML: config/teams/foo.yml + Description: + - Owner annotation at the top of the file + "}), + )?; + Ok(()) +} + +#[test] +fn test_for_file_reports_an_unowned_file_in_an_optionally_owned_glob_as_unowned() -> Result<(), Box> { + run_codeowners( + FIXTURE, + &["for-file", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::str::contains("Team: Unowned"), + )?; + Ok(()) +} + +fn modified_fixture(modify: impl FnOnce(&Path) -> std::io::Result<()>) -> Result> { + let temp_dir = setup_fixture_repo(&Path::new("tests/fixtures").join(FIXTURE)); + modify(temp_dir.path())?; + git_add_all_files(temp_dir.path()); + Ok(temp_dir) +} + +fn codeowners(project_root: &Path, args: &[impl AsRef]) -> Result> { + Ok(Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert()) +} + +fn run_on_modified_fixture(modify: impl FnOnce(&Path) -> std::io::Result<()>, args: &[&str]) -> Result> { + let temp_dir = modified_fixture(modify)?; + codeowners(temp_dir.path(), args) +} + +fn validate_absolute_path(modify: impl FnOnce(&Path) -> std::io::Result<()>, file: &str) -> Result> { + let temp_dir = modified_fixture(modify)?; + let file_absolute_path = temp_dir.path().join(file).canonicalize()?; + codeowners(temp_dir.path(), &[OsStr::new("validate"), file_absolute_path.as_os_str()]) +} + +// A config/code_ownership.yml that replaces the fixture's. A key with no globs is left out, so unowned_globs keeps its default. +#[derive(Default)] +struct Config { + owned_globs: &'static [&'static str], + unowned_globs: &'static [&'static str], + optionally_owned_globs: &'static [&'static str], + executable_name: Option<&'static str>, +} + +impl Config { + fn write(self, project_root: &Path) -> std::io::Result<()> { + let mut yaml = String::from("---\n"); + for (key, globs) in [ + ("owned_globs", self.owned_globs), + ("unowned_globs", self.unowned_globs), + ("optionally_owned_globs", self.optionally_owned_globs), + ] { + if !globs.is_empty() { + yaml += &format!("{key}:\n"); + yaml.extend(globs.iter().map(|glob| format!(" - \"{glob}\"\n"))); + } + } + if let Some(executable_name) = self.executable_name { + yaml += &format!("executable_name: {executable_name}\n"); + } + fs::write(project_root.join("config/code_ownership.yml"), yaml) + } +} + +fn add_unowned_file_outside_optionally_owned_globs(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/stray.rb"), "puts 'no owner'\n") +} + +#[test] +fn test_validate_reports_unowned_files_outside_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_optionally_owned_globs, &["validate"])? + .failure() + .stdout(predicate::str::contains("Some files are missing ownership")) + .stdout(predicate::str::contains("- app/stray.rb")) + .stdout(predicate::str::contains("app/deprecated/old.rb").not()); + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_unowned_file_outside_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_optionally_owned_globs, &["validate", "app/stray.rb"])? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/stray.rb")); + Ok(()) +} + +fn add_a_second_team_owning_the_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("config/teams/bar.yml"), + "---\nname: Bar\ngithub:\n team: '@BarTeam'\nowned_globs:\n - app/deprecated/**/*\n", + ) +} + +#[test] +fn test_validate_reports_multiple_owners_in_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_a_second_team_owning_the_optionally_owned_glob, &["validate"])? + .failure() + .stdout(predicate::str::contains( + "Code ownership should only be defined for each file in one way.", + )) + .stdout(predicate::str::contains("app/deprecated/annotated_old.rb\n owner:")); + Ok(()) +} + +fn add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/typo.rb"), "# @team Typo\nputs 'typo'\n") +} + +#[test] +fn test_validate_reports_invalid_teams_in_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, &["validate"])? + .failure() + .stdout(predicate::str::contains("Found invalid team references")) + .stdout(predicate::str::contains( + "- app/deprecated/typo.rb is referencing an invalid team - 'Typo'", + )); + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team() -> Result<(), Box> { + // A list of files isn't checked for invalid teams. CODEOWNERS has no line for this file, and its annotation keeps + // optionally_owned_globs from excusing it, so it's reported, but not as unowned. + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "app/deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("app/deprecated/typo.rb")) + .stdout(predicate::str::contains("Unowned files detected:").not()); + } + Ok(()) +} + +fn add_an_unowned_file_and_an_optionally_owned_one_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_unowned_file_outside_optionally_owned_globs(project_root)?; + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root) +} + +#[test] +fn test_validate_files_reports_optionally_owned_and_unowned_files_under_separate_headings() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_unowned_file_and_an_optionally_owned_one_naming_an_unknown_team, + &[command, "app/deprecated/typo.rb", "app/stray.rb"], + )? + .failure() + .stdout(predicate::eq(format!( + "Unowned files detected:\n app/stray.rb\n{}\n app/deprecated/typo.rb\n", + unlisted_owner_heading(command) + ))); + } + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team_by_absolute_path() +-> Result<(), Box> { + validate_absolute_path( + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, + "app/deprecated/typo.rb", + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading("validate"))) + .stdout(predicate::str::contains("app/deprecated/typo.rb")); + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team_by_path_with_dot_dot() +-> Result<(), Box> { + // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. It's reported + // as unowned, as without the key. + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "app/deprecated/../deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/../deprecated/typo.rb")); + } + Ok(()) +} + +fn own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; + Config { + owned_globs: &["**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), Box> { + // Unlike the fixture's owned_globs, `**/*.rb` matches a path starting with `./`. The optionally owned glob doesn't, so these + // reach the owner lookup only if it's matched against the plain path. + for command in ["validate", "gv"] { + run_on_modified_fixture( + own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team, + &[command, "./app/deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("./app/deprecated/typo.rb")); + run_on_modified_fixture( + own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team, + &[command, "./app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + +fn make_every_file_optionally_owned(project_root: &Path) -> std::io::Result<()> { + Config { + owned_globs: &["**/*.rb"], + optionally_owned_globs: &["**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_looks_up_an_optionally_owned_file_in_codeowners_by_its_plain_path() -> Result<(), Box> { + // Read as given, none of these paths matches annotated_old.rb's line in CODEOWNERS, and the owner lookup finds its + // annotation. + for command in ["validate", "gv"] { + for file in [ + "./app/deprecated/annotated_old.rb", + "app//deprecated/annotated_old.rb", + "app/./deprecated/annotated_old.rb", + ] { + run_on_modified_fixture(make_every_file_optionally_owned, &[command, file])? + .success() + .stdout(predicate::eq("")); + } + } + Ok(()) +} + +fn add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/typo/app"))?; + fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; + fs::write(project_root.join("packs/typo/app/thing.rb"), "puts 'typo'\n")?; + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/typo/**/*"], + ..Default::default() + } + .write(project_root) +} + +fn add_a_javascript_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("frontend/typo/src"))?; + fs::write( + project_root.join("frontend/typo/package.json"), + "{\"metadata\": {\"owner\": \"Typo\"}}\n", + )?; + fs::write(project_root.join("frontend/typo/src/thing.js"), "console.log('typo');\n")?; + Config { + owned_globs: &["{app,config,frontend}/**/*.{rb,js}"], + optionally_owned_globs: &["app/deprecated/**/*", "frontend/typo/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_in_a_package_naming_an_unknown_team() -> Result<(), Box> { + let packages: [(FixtureChange, &str); 2] = [ + ( + add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob, + "packs/typo/app/thing.rb", + ), + ( + add_a_javascript_package_naming_an_unknown_team_in_an_optionally_owned_glob, + "frontend/typo/src/thing.js", + ), + ]; + for (add_package, file) in packages { + for command in ["validate", "gv"] { + run_on_modified_fixture(add_package, &[command, file])? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains(file)); + } + } + Ok(()) +} + +// `packs/typo` is a string prefix of `packs/typo2` but not a path prefix, so the package names no owner for the file there. +fn add_an_optionally_owned_file_with_no_owner_in_a_sibling_of_a_package_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; + fs::create_dir_all(project_root.join("packs/typo2/app"))?; + fs::write(project_root.join("packs/typo2/app/thing.rb"), "puts 'no owner'\n")?; + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_passes_an_optionally_owned_file_in_a_sibling_of_a_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_optionally_owned_file_with_no_owner_in_a_sibling_of_a_package_naming_an_unknown_team, + &[command, "packs/typo2/app/thing.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + +// Read as a glob, `packs/[typo]` would match `packs/t` rather than the package's own directory. +fn add_a_bracketed_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/[typo]/app"))?; + fs::write(project_root.join("packs/[typo]/package.yml"), "owner: Typo\n")?; + fs::write(project_root.join("packs/[typo]/app/thing.rb"), "puts 'typo'\n")?; + fs::create_dir_all(project_root.join("packs/t"))?; + fs::write(project_root.join("packs/t/stray.rb"), "puts 'no owner'\n")?; + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_in_a_bracketed_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_bracketed_package_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "packs/[typo]/app/thing.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("packs/[typo]/app/thing.rb")); + } + Ok(()) +} + +#[test] +fn test_validate_files_passes_an_optionally_owned_file_outside_a_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_bracketed_package_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "packs/t/stray.rb", "app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + +// Read as a glob, `packs/{typo}` would match `packs/typo` rather than the package's own directory. +fn add_a_braced_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/{typo}/app"))?; + fs::write(project_root.join("packs/{typo}/package.yml"), "owner: Typo\n")?; + fs::write(project_root.join("packs/{typo}/app/thing.rb"), "puts 'typo'\n")?; + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_in_a_braced_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_braced_package_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "packs/{typo}/app/thing.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("packs/{typo}/app/thing.rb")); + } + Ok(()) +} + +fn add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/.codeowner"), "Typo\n") +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + } + Ok(()) +} + +// A glob built from the project root, `/**/**`, would match no project-relative path. +fn make_every_file_optionally_owned_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root( + project_root: &Path, +) -> std::io::Result<()> { + fs::write(project_root.join(".codeowner"), "Typo\n")?; + make_every_file_optionally_owned(project_root) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_at_the_root_naming_an_unknown_team() -> Result<(), Box> +{ + for command in ["validate", "gv"] { + run_on_modified_fixture( + make_every_file_optionally_owned_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root, + &[command, "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + } + Ok(()) +} + +// Read as a glob, `app/deprecated/{legacy}` would match `app/deprecated/legacy` rather than its own directory. +fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("app/deprecated/{legacy}"))?; + fs::write(project_root.join("app/deprecated/{legacy}/.codeowner"), "Typo\n")?; + fs::write(project_root.join("app/deprecated/{legacy}/thing.rb"), "puts 'typo'\n")?; + fs::create_dir_all(project_root.join("app/deprecated/legacy"))?; + fs::write(project_root.join("app/deprecated/legacy/thing.rb"), "puts 'no owner'\n") +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_under_a_braced_codeowner_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "app/deprecated/{legacy}/thing.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("app/deprecated/{legacy}/thing.rb")); + run_on_modified_fixture( + add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, + &[command, "app/deprecated/legacy/thing.rb", "app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + +#[test] +fn test_validate_reports_a_package_or_codeowner_naming_an_unknown_team_in_optionally_owned_globs() -> Result<(), Box> { + // The package mappers drop the package, leaving its file with no owner, but the file is optionally owned. The `.codeowner` + // gives annotated_old.rb a second owner, so `validate` would fail even if the `.codeowner` itself went unreported. + let files: [(FixtureChange, &str); 2] = [ + ( + add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob, + "packs/typo/package.yml", + ), + ( + add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, + "app/deprecated/.codeowner", + ), + ]; + for (add_file, file) in files { + let invalid_team = format!("- {file} is referencing an invalid team - 'Typo'"); + run_on_modified_fixture(add_file, &["validate"])? + .failure() + .stdout(predicate::str::contains(invalid_team)) + .stdout(predicate::str::contains("Some files are missing ownership").not()); + } + Ok(()) +} + +fn annotate_the_unowned_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("app/deprecated/old.rb"), + "# @team Foo\nputs 'no longer unowned'\n", + ) +} + +fn give_the_unowned_file_a_team_glob_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("config/teams/foo.yml"), + "---\nname: Foo\ngithub:\n team: '@FooTeam'\nowned_globs:\n - app/deprecated/old.rb\n", + ) +} + +fn add_a_codeowner_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/.codeowner"), "Foo\n") +} + +fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/foo/app"))?; + fs::write(project_root.join("packs/foo/package.yml"), "owner: Foo\n")?; + fs::write(project_root.join("packs/foo/app/thing.rb"), "puts 'foo'\n")?; + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/foo/**/*"], + ..Default::default() + } + .write(project_root) +} + +fn add_a_javascript_package_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("frontend/foo/src"))?; + fs::write( + project_root.join("frontend/foo/package.json"), + "{\"metadata\": {\"owner\": \"Foo\"}}\n", + )?; + fs::write(project_root.join("frontend/foo/src/thing.js"), "console.log('foo');\n")?; + Config { + owned_globs: &["{app,config,frontend}/**/*.{rb,js}"], + optionally_owned_globs: &["app/deprecated/**/*", "frontend/foo/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_missing_from_codeowners() -> Result<(), Box> { + // `validate`, not `gv`, which would add each file to CODEOWNERS. + let owners: [(FixtureChange, &str); 5] = [ + (annotate_the_unowned_file_without_regenerating_codeowners, "app/deprecated/old.rb"), + ( + give_the_unowned_file_a_team_glob_without_regenerating_codeowners, + "app/deprecated/old.rb", + ), + (add_a_codeowner_file_without_regenerating_codeowners, "app/deprecated/old.rb"), + (add_a_package_without_regenerating_codeowners, "packs/foo/app/thing.rb"), + ( + add_a_javascript_package_without_regenerating_codeowners, + "frontend/foo/src/thing.js", + ), + ]; + for (add_owner, file) in owners { + run_on_modified_fixture(add_owner, &["validate", file])? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading("validate"))) + .stdout(predicate::str::contains(file)); + } + Ok(()) +} + +fn name_an_executable(project_root: &Path) -> std::io::Result<()> { + Config { + owned_globs: &["{app,config}/**/*.rb"], + optionally_owned_globs: &["app/deprecated/**/*"], + executable_name: Some("bin/codeownership validate"), + ..Default::default() + } + .write(project_root) +} + +fn annotate_the_unowned_file_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { + annotate_the_unowned_file_without_regenerating_codeowners(project_root)?; + name_an_executable(project_root) +} + +#[test] +fn test_validate_files_suggests_the_configured_executable_for_an_optionally_owned_file_missing_from_codeowners() +-> Result<(), Box> { + run_on_modified_fixture( + annotate_the_unowned_file_and_name_an_executable, + &["validate", "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains( + "run `bin/codeownership validate` to update the CODEOWNERS file:\n app/deprecated/old.rb\n", + )); + Ok(()) +} + +fn annotate_an_optionally_owned_file_with_an_unknown_team_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; + name_an_executable(project_root) +} + +#[test] +fn test_generate_and_validate_files_does_not_suggest_the_configured_executable() -> Result<(), Box> { + // `gv` has just written CODEOWNERS, so running the executable to update it can't help. + run_on_modified_fixture( + annotate_an_optionally_owned_file_with_an_unknown_team_and_name_an_executable, + &["gv", "app/deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading("gv"))) + .stdout(predicate::str::contains("to update the CODEOWNERS file").not()); + Ok(()) +} + +fn put_the_optionally_owned_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; + Config { + owned_globs: &["{app,config}/**/*.rb"], + unowned_globs: &["app/deprecated/**/*"], + optionally_owned_globs: &["app/deprecated/**/*"], + ..Default::default() + } + .write(project_root) +} + +#[test] +fn test_unowned_globs_take_precedence_over_optionally_owned_globs() -> Result<(), Box> { + // A file in both is left out, so the unknown team its annotation names doesn't count. `gv`, not `validate`, because + // leaving files out changes CODEOWNERS. + run_on_modified_fixture(put_the_optionally_owned_glob_in_unowned_globs_too, &["gv"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +fn put_a_codeowner_file_naming_an_unknown_team_in_both_globs(project_root: &Path) -> std::io::Result<()> { + put_the_optionally_owned_glob_in_unowned_globs_too(project_root)?; + add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root) +} + +#[test] +fn test_validate_files_leaves_out_files_in_both_globs() -> Result<(), Box> { + // A `.codeowner` is read even in unowned_globs. Its team has no CODEOWNERS line, so the file passes only if it's left out. + for command in ["validate", "gv"] { + run_on_modified_fixture( + put_a_codeowner_file_naming_an_unknown_team_in_both_globs, + &[command, "app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + +#[test] +fn test_validate_reports_unowned_files_without_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture( + |project_root| { + Config { + owned_globs: &["{app,config}/**/*.rb"], + ..Default::default() + } + .write(project_root) + }, + &["validate"], + )? + .failure() + .stdout(predicate::str::contains("Some files are missing ownership")) + .stdout(predicate::str::contains("- app/deprecated/old.rb")); + Ok(()) +} + +fn make_every_file_optionally_owned_with_an_unowned_one_at_the_project_root(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("root.rb"), "puts 'no owner'\n")?; + make_every_file_optionally_owned(project_root) +} + +#[test] +fn test_double_star_glob_allows_unowned_files_at_the_project_root() -> Result<(), Box> { + run_on_modified_fixture( + make_every_file_optionally_owned_with_an_unowned_one_at_the_project_root, + &["validate"], + )? + .success() + .stdout(predicate::eq("")); + run_on_modified_fixture( + make_every_file_optionally_owned_with_an_unowned_one_at_the_project_root, + &["validate", "root.rb"], + )? + .success() + .stdout(predicate::eq("")); + Ok(()) +} diff --git a/tests/runner_api.rs b/tests/runner_api.rs index 8c9d4b1..3e5f3f7 100644 --- a/tests/runner_api.rs +++ b/tests/runner_api.rs @@ -187,3 +187,24 @@ javascript_package_paths: let content = std::fs::read_to_string(td.path().join(".github/CODEOWNERS")).unwrap(); assert!(!content.is_empty()); } + +#[test] +fn test_validate_files_takes_relative_paths_as_given_with_relative_root() { + let project_root = Path::new("tests/fixtures/valid_project"); + let run_config = RunConfig { + project_root: project_root.to_path_buf(), + codeowners_file_path: Some(project_root.join(".github/CODEOWNERS").to_path_buf()), + config_path: project_root.join("config/code_ownership.yml").to_path_buf(), + no_cache: true, + executable_name: None, + }; + + // Taken as given, as the CODEOWNERS lookup takes it, this path is outside owned_globs and skipped. Stripping the root for + // the glob check alone would match it as ruby/app/models/payroll.rb and then find no CODEOWNERS line for it. + let result = runner::validate( + &run_config, + vec!["tests/fixtures/valid_project/ruby/app/models/payroll.rb".to_string()], + ); + assert!(result.io_errors.is_empty(), "io: {:?}", result.io_errors); + assert!(result.validation_errors.is_empty(), "val: {:?}", result.validation_errors); +}