diff --git a/Cargo.lock b/Cargo.lock index 9be5280..9b07a78 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -168,7 +168,7 @@ checksum = "b94f61472cee1439c0b966b47e3aca9ae07e45d070759512cd390ea2bebc6675" [[package]] name = "codeowners" -version = "0.5.0" +version = "0.6.0" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index d95b654..b352a71 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.5.0" +version = "0.6.0" edition = "2024" [profile.release] diff --git a/README.md b/README.md index ebbafcf..13c7703 100644 --- a/README.md +++ b/README.md @@ -66,8 +66,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__/**/* + allow_unowned_globs: + - app/services/some_file1.rb ``` 2. **Declare Teams** @@ -209,11 +210,12 @@ 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. Use this for third-party or generated code. - `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. +- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, file annotations in them still count, and every other check still runs, so a file there can't have more than one owner or name an invalid team. 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 `allow_unowned_globs: ['**/*']` to make annotations work. Those files then get the other checks too, which may report problems that were hidden before. Example configuration with custom executable name: @@ -238,7 +240,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 `allow_unowned_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 620185f..232b48d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -31,6 +31,9 @@ pub struct Config { #[serde(default = "default_codeowners_path")] pub codeowners_path: String, + + #[serde(default)] + pub allow_unowned_globs: Vec, } #[allow(dead_code)] @@ -168,6 +171,25 @@ mod tests { let config_file = File::open(&config_path)?; let config: Config = serde_yaml::from_reader(config_file)?; assert_eq!(config.executable_name, "codeowners generate"); + assert!(config.allow_unowned_globs.is_empty()); + Ok(()) + } + + #[test] + fn test_parse_config_with_allow_unowned_globs() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + allow_unowned_globs: + - \"**/deprecated/**/*\" + "}; + fs::write(&config_path, config_str)?; + let config_file = File::open(&config_path)?; + let config: Config = serde_yaml::from_reader(config_file)?; + assert_eq!(config.allow_unowned_globs, vec!["**/deprecated/**/*"]); Ok(()) } diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index cc7f9fa..ab6d2ec 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -377,6 +377,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), + allow_unowned_globs: vec![], } } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 3f5339c..e796572 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; @@ -172,7 +173,9 @@ impl Validator { let relative_path = self.project.relative_path(&file.path).to_owned(); if owners.is_empty() { - validation_errors.push(Error::FileWithoutOwner { path: relative_path }) + if !matches_globs(&relative_path, &self.project.allow_unowned_globs) { + validation_errors.push(Error::FileWithoutOwner { path: relative_path }) + } } else if owners.len() > 1 { validation_errors.push(Error::FileWithMultipleOwners { path: relative_path, diff --git a/src/path_utils.rs b/src/path_utils.rs index 230b1d1..6c42be8 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -10,10 +10,25 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } +/// 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::*; + #[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"); diff --git a/src/project.rs b/src/project.rs index 12b09a6..3f21b7c 100644 --- a/src/project.rs +++ b/src/project.rs @@ -18,6 +18,7 @@ pub struct Project { pub directory_codeowner_files: Vec, pub teams_by_name: HashMap, pub executable_name: String, + pub allow_unowned_globs: Vec, } #[derive(Clone, Debug)] @@ -222,6 +223,7 @@ mod tests { directory_codeowner_files: vec![], teams_by_name: HashMap::new(), executable_name: "codeowners generate".to_string(), + allow_unowned_globs: vec![], }; let map = project.vendored_gem_by_name(); diff --git a/src/project_builder.rs b/src/project_builder.rs index e0d06ee..34fc37e 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, @@ -325,17 +325,11 @@ impl<'a> ProjectBuilder<'a> { directory_codeowner_files: directory_codeowners, teams_by_name, executable_name: self.config.executable_name.clone(), + allow_unowned_globs: self.config.allow_unowned_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) -> Result, Report> { let file = File::open(path).change_context(Error::Io)?; let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; @@ -365,6 +359,7 @@ fn javascript_package_owner(path: &Path) -> Result, Report #[cfg(test)] mod tests { use super::*; + use fast_glob::glob_match; const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; diff --git a/src/runner.rs b/src/runner.rs index 5562979..b9c5ce4 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -2,7 +2,6 @@ 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 +9,7 @@ use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, + path_utils::{matches_globs, relative_to_buf}, project_builder::ProjectBuilder, }; @@ -148,26 +148,23 @@ impl Runner { // 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); - let relative_path = if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path) - } else { - path - }; - - // Mirror the filtering applied by ProjectBuilder when walking the project + .map(|file_path| { + let relative_path = relative_to_buf(&self.run_config.project_root, Path::new(&file_path)); + (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(); 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) if matches_globs(&relative_path, &self.config.allow_unowned_globs) => {} Ok(None) => unowned_files.push(file_path), Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } @@ -442,14 +439,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/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs new file mode 100644 index 0000000..78c877b --- /dev/null +++ b/tests/allow_unowned_globs_test.rs @@ -0,0 +1,192 @@ +use assert_cmd::prelude::*; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; + +const FIXTURE: &str = "tests/fixtures/allow_unowned_globs"; + +#[test] +fn test_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { + run_codeowners("allow_unowned_globs", &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_generate_and_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { + run_codeowners("allow_unowned_globs", &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result<(), Box> { + run_codeowners( + "allow_unowned_globs", + &["validate", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::eq(""), + )?; + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + let file_absolute_path = project_root.join("app/deprecated/old.rb").canonicalize()?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(file_absolute_path) + .assert() + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box> { + run_codeowners( + "allow_unowned_globs", + &["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_unowned_file_in_an_allowed_glob_is_reported_as_unowned() -> Result<(), Box> { + run_codeowners( + "allow_unowned_globs", + &["for-file", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::str::contains("Team: Unowned"), + )?; + Ok(()) +} + +fn run_on_modified_fixture( + modify: impl FnOnce(&Path) -> std::io::Result<()>, + args: &[&str], +) -> Result> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + modify(project_root)?; + git_add_all_files(project_root); + let assert = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert(); + Ok(assert) +} + +fn add_unowned_file_outside_allowed_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_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_allowed_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_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_allowed_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_allowed_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_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_a_second_team_owning_the_allowed_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_allowed_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_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_an_annotation_naming_an_unknown_team_in_an_allowed_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_reports_unowned_files_without_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture( + |project_root| { + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\n", + ) + }, + &["validate"], + )? + .failure() + .stdout(predicate::str::contains("Some files are missing ownership")) + .stdout(predicate::str::contains("- app/deprecated/old.rb")); + Ok(()) +} + +fn allow_every_unowned_file_with_one_at_the_project_root(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("root.rb"), "puts 'no owner'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", + ) +} + +#[test] +fn test_double_star_glob_allows_unowned_files_at_the_project_root() -> Result<(), Box> { + run_on_modified_fixture(allow_every_unowned_file_with_one_at_the_project_root, &["validate"])? + .success() + .stdout(predicate::eq("")); + run_on_modified_fixture(allow_every_unowned_file_with_one_at_the_project_root, &["validate", "root.rb"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} diff --git a/tests/fixtures/allow_unowned_globs/.github/CODEOWNERS b/tests/fixtures/allow_unowned_globs/.github/CODEOWNERS new file mode 100644 index 0000000..b117ec9 --- /dev/null +++ b/tests/fixtures/allow_unowned_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/allow_unowned_globs/app/annotated.rb b/tests/fixtures/allow_unowned_globs/app/annotated.rb new file mode 100644 index 0000000..cfab2e8 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/app/annotated.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated' diff --git a/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb b/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb new file mode 100644 index 0000000..ca506d0 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated, in an allowed glob' diff --git a/tests/fixtures/allow_unowned_globs/app/deprecated/old.rb b/tests/fixtures/allow_unowned_globs/app/deprecated/old.rb new file mode 100644 index 0000000..8eb0b57 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/app/deprecated/old.rb @@ -0,0 +1 @@ +puts 'no owner' diff --git a/tests/fixtures/allow_unowned_globs/config/code_ownership.yml b/tests/fixtures/allow_unowned_globs/config/code_ownership.yml new file mode 100644 index 0000000..8fd3663 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/config/code_ownership.yml @@ -0,0 +1,5 @@ +--- +owned_globs: + - "{app,config}/**/*.rb" +allow_unowned_globs: + - "app/deprecated/**/*" diff --git a/tests/fixtures/allow_unowned_globs/config/teams/foo.yml b/tests/fixtures/allow_unowned_globs/config/teams/foo.yml new file mode 100644 index 0000000..3b226c9 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/config/teams/foo.yml @@ -0,0 +1,4 @@ +--- +name: Foo +github: + team: '@FooTeam'