Repository navigation
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.
dduugg
changed the title
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 resolves the conflicts with #142, which moved YAML parsing to serde-saphyr and added yaml_max_nodes. README.md, Config and the Config literal in file_owner_resolver's tests keep both sides' additions: allow_unowned_globs, then yaml_max_nodes and path. project_builder.rs takes main's ruby_package_owner and keeps this branch's removal of matches_globs, which now lives in path_utils. serde_yaml is no longer a dependency, so the allow_unowned_globs config test loads its file through Config::load_from_path, as the other config tests do.
serde-saphyr rejects a list item with no value, `~` or `null`, so such an `allow_unowned_globs` entry made the whole config fail to parse. serde_yaml read a bare `-` as "" and kept `~` and `null` as text. Like the other list keys, the field now goes through strings_or_nulls, which reads all three as "", and reads an empty, `~` or `null` list as empty: serde-saphyr 2.0 does that by itself, but 2.1 stops doing it for a Vec. The empty-values config test now covers this key too.
Given a list of files, `validate` and `gv` check each one only against CODEOWNERS, and allow_unowned_globs excused every allowed file that CODEOWNERS had no line for. That included a file whose annotation names a team that doesn't exist: without the key it is reported as unowned, and a full `validate` reports the invalid team. Now the glob excuses only a file that nothing names an owner for, counting owners as a full `validate` does, so such a file is reported as unowned again, and so is one that CODEOWNERS doesn't list yet. The owner matchers are collected once, and only when an allowed file is missing from CODEOWNERS.
The README says a file matching both is left out, but no test checked it: making allow_unowned_globs win passed the whole suite. The new test gives a file matching both an annotation naming an unknown team and checks that `gv` passes, which it can only do with the file left out.
The previous commit made allow_unowned_globs excuse only an allowed file that nothing names an owner for, but the owner lookup missed one kind of owner. The package mappers drop a package whose package.yml or package.json names a team that doesn't exist, so a file in it still passed `validate <files>` and `gv <files>`. Without the key it is reported as unowned, and a full `validate` reports the package.yml or package.json. The lookup now adds a matcher for each such package, so the file is reported as unowned again. A full `validate` is unchanged.
Three changes to the code passed every test. The owner lookup could leave out `.codeowner` files, so an allowed file under one naming a team that doesn't exist would pass. It could be given the path as typed rather than the project-relative one, so an absolute path would miss the file's annotation. And `validate <files>` applies the precedence of unowned_globs with its own filter, where letting allow_unowned_globs win went unnoticed: an annotated file in unowned_globs is never read, but a `.codeowner` there still is. A new test catches each change. The comment on the annotation test no longer says a list of files is checked only against CODEOWNERS, which stopped being true when the lookup was added.
The README said files matching unowned_globs are left out and that validation doesn't check them. A `.codeowner`, package.yml or package.json among them is still read, though: its owner still goes into CODEOWNERS, and a team it names that doesn't exist is still reported. main behaves the same way; only the sentence is new here.
Reusing path_utils::relative_to_buf in validate_files changed how it reads a relative path. The helper strips the project root from any path, but the closure it replaced, like the CODEOWNERS lookup, stripped it only from an absolute one. The CLI canonicalizes the root, but a library caller can pass a relative one, as the README's example does. A relative path that starts with that root was then checked against owned_globs with the root cut off, while the CODEOWNERS lookup read it as given and found no line for it, so runner::validate and runner::generate_and_validate reported an owned file as unowned. Before that change the file was skipped. A relative path is already relative to the project root, so it is now left as given and only an absolute one has the root stripped. Such a file is skipped again, and nothing changes when the root is absolute.
allow_unowned_globs excuses a file in `validate <files>` and `gv <files>` only if the owner lookup finds nothing naming an owner for it. The lookup matches only paths in the project's own form, such as app/x.rb, but it was given the path as passed. So `./app/x.rb`, `app/../app/x.rb` or an absolute path not under the canonical root, as through a symlink, found no owner, and the file was excused even when its annotation, `.codeowner` or package names a team that doesn't exist. Without the key it is reported as unowned. The excuse now drops `.` components and repeated separators from the path before matching it against allow_unowned_globs and looking up its owners, so it treats `./app/x.rb` as app/x.rb, and such a file is reported again. A path with a `..`, or an absolute one not under the project root, can't be put in that form without asking the file system, so it is never excused and is reported as without the key. The owned_globs and unowned_globs checks and the CODEOWNERS lookup read the path as before, so nothing changes without the key.
The owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` gave each package naming a team that doesn't exist an unescaped glob of its directory. For a package in `packs/[typo]`, `[typo]` read as a character class, so a file in the package passed although a full `validate` reports its package.yml, while an allowed file with no owner in `packs/t` was reported as unowned. A package.yml at the project root got `/**/**`, which matches nothing, so the files under it passed too. The lookup now keeps the directory of each such package and finds an owner for any path under one. It compares paths a component at a time rather than as globs, so brackets are literal and a package at the project root covers every file. A file in such a package is reported as unowned again, as without the key, and the file in `packs/t` passes. A full `validate` is unchanged. The package test now covers package.json too.
The comments on the owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` said that an allowed file is excused only if nothing names an owner for it, and that 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 the lookup is built from the project, which leaves out untracked files and anything under ignore_dirs. So an owner named only in such a file, as in a new file's own annotation before it is staged, or in an untracked `.codeowner` or package.yml, doesn't count, and the allowed file is excused, although without the key it is reported as unowned. A full `validate` doesn't read those files either, so it doesn't report such a file, and `validate <files>` keeps excusing it. Both comments now say that only files the project reads count, as in `validate`. Nothing else changes.
A full `validate` or `gv` matched each file with no owner against allow_unowned_globs in a loop on one thread. Such a file can be tried against every glob in the list, so the time grows with the number of those files times the length of the list. That is small for a few globs, but the README's Getting Started example lists a file by path, and a list of files still to be given owners can run to thousands of paths. On main such a list goes in unowned_globs, which is matched in the parallel walker. The check now runs on rayon's thread pool, like the other per-file checks in the validator. rayon's collect keeps the order of the files, so it reports the same errors in the same order. On a synthetic project of 10,000 files with no owner, each allowed by its path, the best of 15 runs of `validate` took 1.0 s before and takes 0.45 s now.
The two absolute-path tests copied the fixture, staged it and built the codeowners command line themselves, as run_on_modified_fixture does, because that helper takes its arguments before the temp root that an absolute path is built from exists. The file also named its fixture two ways, as a path for setup_fixture_repo and as a bare name for run_codeowners. The fixture is now named once, and run_on_modified_fixture is split into a builder that copies, modifies and stages the fixture and a runner for the command. validate_absolute_path joins them to validate a file by its canonical absolute path, so each absolute-path test is now a single call. The assertions are unchanged, and no test is added or removed.
Six tests of `validate <files>` were named as if they reported an invalid team or a package naming one, but a list of files is never checked for invalid teams. Each asserts that an allowed file whose annotation, `.codeowner` or package names a team that doesn't exist is reported under "Unowned files detected:". Beside test_validate_reports_invalid_teams_in_allowed_globs, which does check a full `validate` for invalid teams, the near-identical names misled. They are now named after the allowed file they report. The two `for-file` tests can't be broken by anything allow_unowned_globs changes, since `for-file` never reads the key. They are kept, now named after `for-file`, and a comment says they pin that it reports an allowed file's owner as it would without the key. Another comment says the test that a full `validate` passes relies on the fixture's CODEOWNERS listing app/deprecated/annotated_old.rb, so it also fails, with CODEOWNERS out of date, if annotations in allowed globs stop counting. Only test names and comments change.
Five changes to the code passed every test. `validate <files>` could report an allowed file with an owner even when CODEOWNERS lists it, which would fail a pre-commit hook on a commit that edits an annotated file in an allowed glob. The owner lookup could leave out team globs, or packages naming a team that exists, because the only test of an allowed file that CODEOWNERS doesn't list yet gave it an annotation. And no test ran a full `validate` with a package.yml or `.codeowner` naming an unknown team in an allowed glob, so it could stop reporting them there. New tests pass the annotated file and an unowned one to `validate` and `gv`, and run that full `validate` with each. The latter checks for each message, not just the exit code, since the `.codeowner` also gives annotated_old.rb a second owner. It also checks that the package's file, which has no owner, isn't reported as missing ownership. The test of an allowed file that CODEOWNERS doesn't list yet now also covers an owner from a team glob and one from a package, not just an annotation. Only tests change.
When matches_globs moved from project_builder to path_utils, its test stayed behind, as did test_glob_match, which tests fast_glob itself and needed a new glob_match import in project_builder's tests. The only check of the allow_unowned_globs default went in the test of the executable_name default, so a change to it would fail a test named for another key. Both tests and their OWNED_GLOB now sit with the other matches_globs test in path_utils, calling fast_glob::glob_match by its full path as the code there does, and the import is gone. The default is checked in test_parse_config with most of the others, between executable_name and yaml_max_nodes as in the struct, and the executable_name test is back to what main has. Only tests change, and none is added or removed.
The README's list of configuration keys put allow_unowned_globs after executable_name, with four other keys between it and unowned_globs, though the bullet is written in terms of unowned_globs. It says how the two keys differ, that unowned_globs takes precedence, and when to switch from `unowned_globs: ['**/*']` to the new key, so a reader had to look back up the list to follow it. The bullet now comes directly after unowned_globs. Its text and the rest of the README are unchanged.
The README said a file in allow_unowned_globs can't have more than one owner or name an invalid team, since every other check still runs. That holds for a full `validate` or `gv`, but the README never mentioned what they do given a list of files, as from a git hook. They look each file up in CODEOWNERS and never check for more than one owner, and pass an allowed file with no line 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 "Unowned files detected". The advice to replace `unowned_globs: ['**/*']` with the new key left out that CODEOWNERS then gains a line for each annotated file, so until `codeowners gv` runs, `validate` reports it out of date and `validate <files>` reports those files as unowned. It also left out that the files are then read and checked, which takes longer. The bullet now covers all of this, says to keep third-party and generated code in unowned_globs, and says the key was added in 0.6.0, since older versions ignore it without a warning and still report these files. Only the README changes.
The owner lookup behind allow_unowned_globs in `validate <files>` and
`gv <files>` matched a `.codeowner` only by the glob the directory
mapper builds from its directory. For one at the project root that
glob is `/**/**`, which matches nothing, and the mapper escapes
brackets but not braces, so `app/deprecated/{legacy}/**/**` matches
files in `app/deprecated/legacy` but not in `app/deprecated/{legacy}`.
An allowed file under such a `.codeowner` naming a team that doesn't
exist therefore passed, although a full `validate` reports the
`.codeowner` and, without the key, the file is reported as unowned.
The lookup now keeps the directory of each `.codeowner` naming a team
that doesn't exist, as it already did for packages, and finds an owner
for any path under one. A file under such a `.codeowner` is reported
as unowned again, as without the key. Only the nearest `.codeowner`
gives a file its owner in `validate`, but the lookup asks only whether
anything names an owner. The directory mapper is unchanged, so
CODEOWNERS and a full `validate` are too. A test of a package in a
directory with braces is added as well. It passed before, but without
it no test fails if packages are matched by globs like the directory
mapper's.
Given a list of files, `validate` and `gv` report an allowed file that CODEOWNERS has no line for only if something names an owner for it: a team that doesn't exist, or one CODEOWNERS doesn't list yet. Such a file was still listed under "Unowned files detected:", with no hint, although the config says it may go unowned. With the README's `['**/*']`, every failure for a list of files is of this kind, and in the code_ownership gem's pre-commit hook, which runs `gv` and so regenerates CODEOWNERS first, the cause is almost always a team that doesn't exist. These files now get a heading of their own. It says that they match allow_unowned_globs but name an owner that CODEOWNERS doesn't list, and suggests checking the team name or running the configured executable_name to update CODEOWNERS, as the stale-CODEOWNERS message does. Files outside the globs, and a path with a `..` or an absolute one not under the project root, are still listed under "Unowned files detected:", and both headings can appear in one run. The exit status is unchanged, and the gem only joins these lines into the error it raises. The README names the new heading, and new tests pass both kinds of file at once and check that the hint names the configured executable.
The README said yaml_max_nodes doesn't apply to the config file itself, which can read as if the config had no limit. It is always parsed with the default of 20,000 nodes, and nothing raises that, so a config whose lists add up to about 20,000 entries fails with a `budget breached` error and, unlike a team file, no hint. A long literal allow_unowned_globs list is the likeliest way to get there, and 0.5.0, which predates #142's limits, read such a config. The sentence now gives the limit. Only the README changes.
The README's unowned_globs bullet says files matched there are left out of the project, then names `.codeowner`, `package.yml` and `package.json` as the files still read, which reads as the whole list. A team file is another: the project builder matches team_file_glob before owned_globs and unowned_globs, so a team file in unowned_globs still defines its team and keeps its own line in CODEOWNERS. The bullet now says so in a sentence of its own, since the invalid-team check the sentence before it mentions doesn't apply to team files. Only the README changes.
The owner lookup behind allow_unowned_globs in `validate <files>` and
`gv <files>` matches the directory of a `.codeowner` naming a team
that doesn't exist as a path prefix, but it still took the glob the
directory mapper builds from that directory as well. The mapper
escapes brackets but not braces, so the glob it builds for one in
`app/deprecated/{legacy}` matches files in `app/deprecated/legacy`
instead. An allowed file there, which nothing names an owner for, was
therefore listed under the heading for files that name an owner
CODEOWNERS doesn't list, and the run failed instead of passing.
The lookup now leaves out the glob for such a `.codeowner`, as the
prefix covers just the files under its directory, so that file passes
and a file under the `.codeowner` is still reported. The glob for a
`.codeowner` naming a team that exists is kept, since it names an
owner for files that CODEOWNERS may not list yet. The directory mapper
is unchanged, so CODEOWNERS and a full `validate` are too. The braced
test now also passes a file in `app/deprecated/legacy` and fails
without the change. The test of an allowed file that CODEOWNERS
doesn't list yet covers an owner from a `.codeowner` too, since
without it no test fails if the lookup drops every `.codeowner` glob.
Given a list of files, `validate` and `gv` look each one up in CODEOWNERS by the path as given, but match a file with no line against allow_unowned_globs and the owner lookup by its plain project-relative path. So an allowed file that CODEOWNERS lists, such as an annotated one, missed its line when passed as `./app/x.rb`, `app//x.rb` or `app/./x.rb`, the owner lookup found its owner, and the file was listed under the heading for files that name an owner CODEOWNERS doesn't list. That is false, and neither checking the team name nor regenerating CODEOWNERS made it pass. The `./` form passed before allowed files were matched by their plain path. Such a file is now looked up in CODEOWNERS again by its plain path, and passes if that finds a line. The second lookup is made only when the plain path differs from the one given and the owner lookup has found an owner, so only a file that would otherwise be listed pays for it. A file outside allow_unowned_globs is still looked up only by the path as given, as on main, so nothing changes without the key. A new test passes the annotated file to `validate` and `gv` by each of those paths and fails without the change. The test of a `./` path now allows only `app/deprecated/**/*`, which matches the plain path but not the one given, so it also catches allow_unowned_globs being matched against the path as given, which no test did before.
Given a list of files, `gv` writes CODEOWNERS before it looks each one up, so an allowed file it lists under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list" is almost always there because its annotation, `.codeowner` or package names a team that doesn't exist. The heading still suggested running the configured executable_name to update CODEOWNERS, which can't help. The default, `codeowners generate`, writes the same file again, and the code_ownership gem names `bin/codeownership validate`, which by default runs `gv` again. For a file under an empty `.codeowner`, "check the team name" didn't say where to look either. Both commands now say to check the team name in each file's annotation, `.codeowner` or package, and only `validate` still suggests running the executable, as its CODEOWNERS may be out of date. The heading's first sentence, which the README quotes, and the exit status are unchanged. The tests now expect the heading each command prints, and a new one runs `gv` with the gem's executable_name and checks that it doesn't suggest updating CODEOWNERS. They fail without the change.
Two changes to the owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` passed every test. It could leave out package.json files naming a team that exists, because each one the tests add names a team that doesn't, and those are matched by their directory instead. `validate` would then pass an allowed file in such a package that CODEOWNERS doesn't list yet, rather than list it under the heading for files that name an owner CODEOWNERS doesn't list. It could also match the directory of a package or `.codeowner` naming a team that doesn't exist as a string prefix rather than a component at a time, so a package in `packs/typo` would name an owner for an allowed file with none in `packs/typo2`, which would be listed under that heading instead of passing. The test of an allowed file that CODEOWNERS doesn't list yet now also covers an owner from a package.json, as it did one from a package.yml and a `.codeowner`, and a new test passes the file in `packs/typo2` to `validate` and `gv`. Each fails with its change. Only tests change.
Thirteen fixture changes in the allow_unowned_globs tests replaced
config/code_ownership.yml with an inline YAML string, each quote and
newline escaped, and several were copies of one another: owning
`**/*.rb` and allowing `**/*` three times, and owning
`{app,config,packs}/**/*.rb` and allowing `packs/**/*` three more.
Which globs a test's config owned or allowed had to be picked out of
the escapes.
A Config struct now takes each key's globs by name, and its write
method renders the YAML, leaving out a key with no globs so that
unowned_globs keeps its default. Every fixture change that writes the
config now uses it, including name_an_executable, which appended its
key to the fixture's config. The two that copied allow_every_file now
call it. Every fixture a test builds has the same files, byte for
byte, as before, and no test is added, removed or changed in what it
checks. Only tests change.
Two comments on the owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` said an annotated file, `.codeowner` or package file names no owner there if it is under ignore_dirs, but the project builder prunes only a directory at the project root that ignore_dirs names. With the default ignore_dirs, an annotation naming a team that doesn't exist keeps a file in `app/tmp` from passing, though not one in `tmp`. They now say a top-level directory named in ignore_dirs. The one in validate_files also said a file whose annotation, `.codeowner` or package names a team that doesn't exist, "or that CODEOWNERS doesn't list yet", is still reported, which can be read as a team that CODEOWNERS doesn't list. It now says "or whose owner CODEOWNERS doesn't list yet", as the README does, since an owner that CODEOWNERS lists for other files counts too. The comment where the config is read said that at a few hundred nodes it never needs a higher limit, but a long literal allow_unowned_globs list can run to thousands of entries, and the README says the config is held to the default of 20,000 nodes, about one per list entry. It now says the config's lists can hold about 20,000 entries in all. Only comments change.
In the README's unowned_globs bullet, "So is a team file, which still defines its team and keeps its own line in CODEOWNERS" followed "an invalid team in it is still reported", so the elliptical "So is" could be read as saying that a team file is still reported. It now says that a team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. The allow_unowned_globs bullet said that, given a list of files, an allowed file whose annotation, `.codeowner` or package names a team that doesn't exist is reported, but the owner lookup that decides this sees only the files the project reads. An untracked file's own annotation names no owner there, nor does an untracked `.codeowner` or package file, or any of them in a top-level directory named in ignore_dirs, as in a full `validate`. So `validate app/new.rb` passes a new allowed file whose annotation names a team that doesn't exist, and reports it only once the file is staged. A sentence after the one quoting the heading now says so. It says that such a file doesn't count, rather than that only a tracked one does, since outside a git repository no file is left out for being untracked. Only the README changes.
The README said executable_name defaults to `'codeowners'`, but the default, here and on main, is `codeowners generate`. Its example config set `bin/codeownership` for the Ruby gem, while the gem passes `bin/codeownership validate`, which regenerates CODEOWNERS by default. Both the stale-CODEOWNERS message and the new hint for allowed files in `validate <files>` print this value as the command to run to update CODEOWNERS. The gem's own runs aren't affected, since the value it passes replaces the config's, but a config copied from the example made `codeowners validate` tell users to run `bin/codeownership`, which only prints the gem's usage. The bullet now gives the real default and says the key is the whole command these messages tell you to run, rather than a command name, and the example sets `bin/codeownership validate`. Only the README changes.
`allow_unowned_globs` reads as `allow_` followed by the name of the `unowned_globs` key, as though it switched on or loosened that key. The relationship is the reverse: the new key narrows `owned_globs`, whose files it still reads and checks, and has nothing to do with `unowned_globs`, which takes precedence over it. The `allow_` prefix also reads like a boolean, as rubyatscale/code_ownership#140 proposed with `allow_unowned_files: true`, while the other list keys are noun phrases. `optionally_owned_globs` sits between the two existing keys: files in `owned_globs` must have an owner, files that also match `optionally_owned_globs` may have one, and files in `unowned_globs` are left out. Since the name could be read as adding files beyond `owned_globs`, the README now says it doesn't, and that a file must match both. The key, the `Config` and `Project` fields, the heading that `validate <files>` prints, the README, the fixture directory, the test file and the test names all use the new name, and prose that called these files "allowed" now calls them optionally owned. 0.6.0 hasn't been released, so nothing reads the old name and no alias is kept.
has_owner only needs to know whether anything names an owner, but it called FileOwnerFinder::find, which runs every owner matcher, clones each matching source into a map of teams and collects the owners, all to check that the list isn't empty. With `optionally_owned_globs: ['**/*']`, every file `validate <files>` is given that has no line in CODEOWNERS reaches it. find returns an owner exactly when some matcher names a team: a matched directory always survives as the most specific one. So has_owner now asks whether any matcher names a team, and stops at the first. Fresh Eyes reported this as an info-level performance finding.
dduugg
changed the title
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
1 participant
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
optionally_owned_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 matchesoptionally_owned_globsdoesn't need an owner.validate/gvno longer reports it under "Some files are missing ownership". It is otherwise treated like any owned file. It's still read, its annotations still count, and every other check still runs, sovalidate/gvstill reports it if it has more than one owner or names an invalid team.Given a list of files,
validateandgvlook each one up in CODEOWNERS, and never check for more than one owner, with or without the option. An optionally owned file that CODEOWNERS has no line for passes if nothing names an owner for it. One whose annotation,.codeowneror package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported, under its own heading, "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list", with a hint on where to look, rather than under "Unowned files detected". As in a fullvalidate, only files the project reads count: tracked files outside top-levelignore_dirs. A path that can't be put in plain project-relative form, such as one with a..or an absolute one outside the project, is reported as unowned, as without the option.['**/*']covers Feature Request: Add a flag to allow unowned files without disabling file annotations code_ownership#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 overoptionally_owned_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").optionally_owned_globsonly adds a glob check for files that already have no owner, using the sameglob_matchthatowned_globsruns on every walked file. Given a list of files,validateandgvalso collect the owner matchers once, from the project they have already read, if an optionally owned file has no line in CODEOWNERS. That took about 2 ms on a synthetic 52,000-file project.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.Why this name
The key was first
allow_unowned_files, a boolean, as rubyatscale/code_ownership#140 proposed, and thenallow_unowned_globs. That name reads asallow_plus theunowned_globskey, as though it loosened that key, but it narrowsowned_globsand has nothing to do withunowned_globs, which takes precedence over it. Theallow_prefix also reads like a boolean, while the other list keys are noun phrases.optionally_owned_globssits between the existing two: files inowned_globsmust have an owner, files that also matchoptionally_owned_globsmay have one, and files inunowned_globsare left out. It doesn't add files beyondowned_globs, and the README says so.Changes
config.rs:optionally_owned_globs: Vec<String>, defaulting to empty. Like the other list keys, it's read withstrings_or_nulls, so an empty,~ornullvalue is an empty list, and an item with no value,~ornullis"".project.rs/project_builder.rs: carry it through toProject, the same wayexecutable_nameis carried.validator.rs: skipFileWithoutOwnerfor files that matchself.project.optionally_owned_globs. The multiple-owner and invalid-team checks are unchanged. The file-ownership loop now runs on rayon, like the validator's other per-file checks, so a long literal list doesn't add single-threaded time; the errors are the same, in the same order.runner.rs:validate <files>makes an absolute path project-relative and leaves a relative one as given, as the CODEOWNERS lookup does. For a file that CODEOWNERS has no line for, it takes the plain form of the path (path_utils::plain_relative). If that matchesoptionally_owned_globs, the file passes unless something names an owner for it; a path given as./x,a//bora/./bis first looked up in CODEOWNERS again by its plain form. Optionally owned files with an owner CODEOWNERS doesn't list get their own heading. Its hint suggests runningexecutable_nameto update CODEOWNERS only forvalidate, sincegvhas just written it.ownership.rs: a crate-privateOwnerLookup, fromOwnership::owner_lookup, tells whether anything names an owner for a path, even a team that doesn't exist. It uses the owner matchersvalidateuses, and matches each package and.codeownernaming a team that doesn't exist by its directory, compared a component at a time: the package mappers drop such packages, and the directory mapper's glob matches nothing for a.codeownerat the project root and reads braces as alternatives.validate <files>builds it at most once, and only when an optionally owned file has no line in CODEOWNERS.path_utils.rs: one crate-privatematches_globs, which replaces identical private copies inrunner.rsandproject_builder.rs, with their tests. The validator uses it too. Two&strvariants infile_owner_resolver.rsand the perf binary predate this PR and are left alone. Alsoplain_relative, above.README.md: documents the option next tounowned_globsand contrasts the two: which one wins, what each still reads (including team files,.codeowner,package.ymlandpackage.jsoninsideunowned_globs), whatvalidate <files>does with optionally owned files, and how to migrate offunowned_globs: ['**/*'](runcodeowners gvonce; the files are then read, so keep third-party and generated code inunowned_globs). It says the key was added in 0.6.0, that the config file itself is held to 20,000 YAML nodes, about one per list entry, and givesexecutable_name's real default,codeowners generate. It also updates validation rule 3 and moves the Getting Started example's hand-picked app file fromunowned_globstooptionally_owned_globs.Cargo.toml/Cargo.lock: version 0.6.0. It's a minor bump because newpubfields onConfigbreak downstream code that builds aConfigwith a struct literal:optionally_owned_globshere, andyaml_max_nodesandpathfrom Replace serde_yaml with serde-saphyr to drop unsafe-libyaml #142. No known consumer does; code_ownership only usescodeowners::runner. Main hasn't changed the version since 0.5.0, so merging this releases everything merged since then too, including Replace serde_yaml with serde-saphyr to drop unsafe-libyaml #142's move to serde-saphyr.Test plan
optionally_owned_globsfixture, in which onlyapp/deprecated/**/*is optionally owned, with 33 integration tests:validate,gvandvalidate app/deprecated/old.rbpass, and so doesvalidatewith an absolute path to that file.validateandgvgiven the annotated and the unannotated optionally owned file together pass.for-filereports the annotated file's team inside the optionally owned glob andUnownedfor the unannotated one.validateandvalidate <files>, under "Unowned files detected".validate, and so does an annotation,package.ymlor.codeownernaming an unknown team.validate <files>andgv <files>report an optionally owned file under the new heading if its annotation,.codeowner,package.ymlorpackage.jsonnames an unknown team, including a package or.codeownerat the project root or in a directory with brackets or braces, while an optionally owned file with no owner in a sibling directory still passes. They also report an optionally owned file whose owner, from an annotation, a team glob, a.codeowneror a package, CODEOWNERS doesn't list yet, and list both headings when given both kinds of file...path is reported as unowned;./,//and inner.paths are matched againstoptionally_owned_globs, the owner lookup and CODEOWNERS by their plain form.executable_nameforvalidate, andgvdoesn't suggest it.unowned_globsandoptionally_owned_globsis left out:gvpasses although its annotation names an unknown team, andvalidate <files>andgv <files>pass it although a.codeownerover it names one.['**/*']allows a root-level file, in bothvalidateandvalidate <files>.**/*matching root-level paths, and forplain_relative. Arunner::validatetest with a relative project root checks that a relative path is taken as given.optionally_owned_globsor the owner lookup against the path as given instead of its plain form, matching unknown-team directories as strings or globs, dropping the annotation, team-glob, directory or package mappers from the owner lookup, lettingoptionally_owned_globswin overunowned_globs, and readingoptionally_owned_globswithoutstrings_or_nulls.validate,gv,generateandfor-file, andvalidate/gvfor every tracked file one at a time and all together, given plain,./,//, inner.,.., absolute and symlinked paths. Stdout, stderr, exit codes and generated files were identical. The commits after f40c570 only change code that runs with the option set.validate. It ignores the unknown key.validate!andvalidate!(files:), and annotated files still resolve and appear in CODEOWNERS. An optionally owned file whose annotation names a team that doesn't exist failsvalidate!(files:)under the new heading (with autocorrect off, its hint namesbin/codeownership validate), andvalidate!with "Found invalid team references". At f40c570 the key was still namedallow_unowned_globs, so the setup used that name and the heading printed it. The later commits rename it tooptionally_owned_globsand otherwise change only code that runs with it set.cargo test: 248 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 to resolve rubyatscale/code_ownership#140, which the stale bot has since closed as not planned:
codeownerstag inext/code_ownership/Cargo.toml, updateCargo.lock, rebuild the checked-in bundle, and bumpversion.rb. The new tag also brings in Replace serde_yaml with serde-saphyr to drop unsafe-libyaml #142, so the release notes should mention its YAML changes, theyaml_max_nodeskey, and the fixed limit of 20,000 nodes on the config file itself, which v0.5.0 didn't have andyaml_max_nodescan't raise.unowned_globsshould coveroptionally_owned_globstoo. Its description ofunowned_globsas a TODO list of files to add owners to now fitsoptionally_owned_globsbetter. Both of its example configs list hand-picked app files,app/services/some_file1.rbandsome_file2.rb, underunowned_globs; move them tooptionally_owned_globs, as this PR does in its own Getting Started example.validate!mention onlyunowned_globs, and callfilesignored (lib/code_ownership.rb:194), althoughvalidate!passes it on and a non-empty list runs codeowners' file-list mode.validatethen still lists the files.write_configuration(optionally_owned_globs: [...])writes a symbol key that serde ignores, so pass'optionally_owned_globs' => [...].Found while reviewing, unchanged here because they predate this PR and some would change CODEOWNERS output:
validate <files>reads./x,a//banda/./bas given in theowned_globsfilter and the CODEOWNERS lookup, so such a path to an owned file outsideoptionally_owned_globsis reported as unowned.PackageMapperdoesn't escape brackets or braces in package paths,DirectoryMapperdoesn't escape braces, and a package or.codeownerat the project root gets//**/**in CODEOWNERS, which matches nothing.