Conversation
code_ownership#140: a repo that assigns owners to only some files had no way to pass `validate` except `unowned_globs: ['**/*']`, and files matched by unowned_globs are never read, so that also switched off their `@team` annotations. With `allow_unowned_files: true`, a file in owned_globs that no mechanism assigns an owner is no longer an error, in both `validate` and `validate <files>`. Every mechanism, file annotations included, still applies to those files, so the annotations keep working. It defaults to false, and nothing else changes: unowned_globs files are still never read, which is what keeps large third-party trees cheap. Making annotations work inside unowned_globs instead would have meant opening every file those globs match. Measured on a synthetic tree of tracked vendored files, that made a cold `generate` about 7x slower at 100k files and 11x at 300k.
A boolean could only exempt every file at once, while a glob list can also scope the exemption, e.g. `**/deprecated/**/*`, and `['**/*']` still covers code_ownership#140's case. It matches how owned_globs and unowned_globs are configured. The difference from unowned_globs is that files here are still read, so annotations and every other mechanism still apply; they just don't have to have an owner. The globs are only checked for files that have no owner, using the same glob_match used for owned_globs on every walked file.
The validator imported matches_globs from project_builder, which made project_builder's copy crate-public just to share a one-line helper. It now has its own private copy, like runner.rs and file_owner_resolver.rs, and project_builder's copy is private again. validate_files' relative-path closure now calls path_utils::relative_to_buf instead of hand-rolling it. Also pin that `allow_unowned_globs: ['**/*']`, the README's way to allow every unowned file, covers files at the project root, for both `validate` and `validate <files>`.
runner.rs, project_builder.rs and the validator each had an identical private matches_globs. They now all use path_utils::matches_globs, next to relative_to and relative_to_buf, with a unit test that `**/*` matches both root-level and nested paths.
Nothing tested that `validate <files>` matched allow_unowned_globs against the project-relative path. Matching the raw argument instead passed the whole suite, because the only test passed a path that was already relative. A test with an absolute path now covers it. validate_files also relativizes each path once, carrying it through the owned_globs filter, instead of twice per file. Two more tests pin the README's claim that only the missing-owner check is waived: a file in an allowed glob still fails when it has more than one owner or names an invalid team. The validator now reads allow_unowned_globs from its project instead of keeping a copy, matches_globs is pub(crate) so it doesn't become public API, and its imports sit with the other crate imports.
The README said unowned_globs files are never read, but `for-file` reads the top of the file before ignoring its annotation. It also implied that no ownership mechanism applies to unowned_globs files, when directory, package and team-glob rules still cover them through CODEOWNERS. The real differences are that annotations count and that the other checks still run. It now also says that unowned_globs takes precedence, and how to move off `unowned_globs: ['**/*']`, the setup code_ownership#140 started from, where adding the new key alone changes nothing. The Getting Started example moves a hand-picked app file from unowned_globs to allow_unowned_globs, since unowned_globs is meant for third-party and generated code.
A minor bump. Since 0.5.0: - `allow_unowned_globs` (#135) is a new config key. It defaults to `[]`, so existing configs behave as before, but it adds a `pub` field to `Config`, which breaks downstream code that builds a `Config` with a struct literal. - Test git no longer follows a hook's GIT_DIR into this repo (#133). This only affects the test suite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an
allow_unowned_globsoption toconfig/code_ownership.yml, for rubyatscale/code_ownership#140, and bumps the version to 0.6.0.A repo that assigns owners to only some of its files had one way to get
validateto pass:unowned_globs: ['**/*']. But files matched byunowned_globsare left out of the project, so that also switched off their# @teamannotations.A file in
owned_globsthat matchesallow_unowned_globsdoesn't need an owner.validate/gvno longer reports it under "Some files are missing ownership", andvalidate <files>no longer reports it under "Unowned files detected". It is otherwise treated like any owned file. It's still read, its annotations still count, and every other check still runs, so it still fails if it has more than one owner or names an invalid team.['**/*']covers #140's case. This includes files at the project root, which a test checks.['**/deprecated/**/*']exempts only those paths. Files with no owner outside it still fail.[], and nothing else changes. In particular,unowned_globsfiles are still left out, andunowned_globstakes precedence overallow_unowned_globs. The README now spells out the difference between the two options, and how to move offunowned_globs: ['**/*'].Why not make annotations work inside
unowned_globsThat was the first approach I tried, and I benchmarked it. It means opening every file that
unowned_globsmatches, andunowned_globsis often a large third-party or generated tree. On a synthetic repo with git-tracked vendored files matching both globs:generatewas about 7x slower at 100k files and 11x slower at 300k;The cost is the per-file open, so reading only the first line didn't help. A gitignored
node_moduleswouldn't have been affected, but tracked vendored code would have been, and so would any checkout wheregit ls-filesfails (non-git checkouts, or CI containers with "dubious ownership").allow_unowned_globsonly adds a glob check for files that already have no owner, using the sameglob_matchthatowned_globsruns on every walked file.It would also have changed existing behavior: every repo with annotations inside
unowned_globswould see its CODEOWNERS change, and could start getting multiple-owner errors. An opt-in key avoids that.Changes
config.rs:allow_unowned_globs: Vec<String>, defaulting to empty.project.rs/project_builder.rs: carry it through toProject, the same wayexecutable_nameis carried.validator.rs: skipFileWithoutOwnerfor files that matchself.project.allow_unowned_globs. The multiple-owner and invalid-team checks are unchanged.runner.rs:validate <files>skips unowned files that matchallow_unowned_globs. It now makes each path project-relative once, withpath_utils::relative_to_buf, and uses that path for every glob check.path_utils.rs: one crate-privatematches_globs, which replaces identical private copies inrunner.rs,project_builder.rsand the validator. Two&strvariants infile_owner_resolver.rsand the perf binary predate this PR and are left alone.README.md: documents the option and contrasts it withunowned_globs, says which one wins and how to migrate, updates validation rule 3, and moves the Getting Started example's hand-picked app file fromunowned_globstoallow_unowned_globs.Cargo.toml/Cargo.lock: version 0.6.0. It's a minor bump because the newpubfield onConfigbreaks downstream code that builds aConfigwith a struct literal. No known consumer does; code_ownership only usescodeowners::runner.Test plan
allow_unowned_globsfixture, allowing onlyapp/deprecated/**/*, with 12 integration tests:validate,gvandvalidate app/deprecated/old.rbpass, andvalidatewith an absolute path to that file passes too.for-filereports the annotated file's team inside the allowed glob andUnownedfor the unannotated one.validateandvalidate <files>.['**/*']allows a root-level file, in bothvalidateandvalidate <files>.**/*matching root-level paths.validate <files>against the raw argument instead of the project-relative path.validate. It ignores the unknown key.validate!andvalidate!(files:), and annotated files still resolve and appear in CODEOWNERS.cargo test: 192 passed, 3 ignored (also ignored on main). fmt and clippy are clean.Follow-ups
code_ownership needs a release that picks up v0.6.0 before #140 can be closed:
codeownerstag inext/code_ownership/Cargo.toml, updateCargo.lock, rebuild the checked-in bundle, and bumpversion.rb.unowned_globsshould coverallow_unowned_globstoo. Its description ofunowned_globsas a TODO list of files to add owners to now fitsallow_unowned_globsbetter.validate!mention onlyunowned_globs.validatethen still lists the files.write_configuration(allow_unowned_globs: [...])writes a symbol key that serde ignores, so pass'allow_unowned_globs' => [...].