Closed konrad-jamrozik closed 8 months ago
Brain dump of potential rules to validate
Note: As of 5/22/2023 this spec has been implemented few months ago. In case of inconsistencies the implementation takes precedence. Please see the (massive) battery of unit tests included.
Note: this spec has been updated as of 1/23/2023 3:00 PM PST.
Per my chat with @weshaggard today, we want to add following restrictions on CODEOWNERS file which we will validate against:
/
.This section was added on 1/23/2023.
We forbid usage of **
of any other way than within slashes, i.e. as /**/
:
**/
is not possible because all paths must start with /
./**
is invalid because it is equivalent to /
.**
in file or dir names, like /foo**bar/
or /**foo**/
is forbidden, because it is equivalent to single star, like /foo*bar/
or /*foo*/
respectively.In addition, usage of suffix /**/
is invalid because it is equivalent to /**/**
which is equivalent to /
.
The characters [
, ]
, !
, and ?
are forbidden in CODEOWNERS paths. They are valid parts of file paths, but we forbid them to simplify implementation logic.
/
, there must be at least one existing directory it matches./
, there must be at least one existing file it matches.
/foo
and /foo/
.As of 1/23/2023 this section has been updated. Previously, we assumed that if CODEOWNERS paths are matched against target path /foo
, it wasn't known if it is a path to a file or directory. Now we assume that target path is always a path to a file.
As a consequence of the rules above, we can simplify our CODEOWNERS parser logic by making additional assumption:
If CODEOWNERS file is searched for a match for a target path, we can always match it against the first rule that matches it, using the same logic as GitHub parser.
This assumption works because:
foo
and our CODEOWNERS file has path /foo
, then foo
must be a path to an existing file, due to our validation. It is thus a match;foo
and our CODEOWNERS file has path /foo/
, then foo
must be a path to file that cannot exist, because our validation ensures that /foo/
is an existing directory. Thus no CODEOWNERS path will match foo
.In addition, we can assume in the parser that all CODEOWNERS paths are absolute to the repository root.
To ensure our existing language repositories CODEOWNER files match the rules outlined above, we must do the following:
/
, and file otherwise.
foo
(without any slashes in the beginning or middle) to /foo
(i.e. add slash at the beginning). This is because GitHub CODEOWNERS parser interprets foo
as **/foo
per .gitignore spec, per this example:
# In this example, @octocat owns any file in an apps directory
# anywhere in your repository.
apps/ @octocat
/docs
/docs/ @doctocat
- Ensure `notification-creator` properly parses the new CODEOWNERS files and generates relevant rules.
Every file and directory in the repository must match at least one path in the CODEOWNERS file.
I think that is a good goal but I don't know how feasible it will be.
Otherwise I believe what you wrote sounds correct based on our conversations.
Idea from @jsquire:
Have a weekly report with paths without owners or invalid owners. It would help understand where are the gaps while avoiding noise by avoiding "catch-all" ownership assignment.
The parsing and linting have been completely rewritten as part of the work being done for the new syntax. The updates include baselining. The PR is currently in review. Once that goes in the YML for the linter pipeline needs to be written.
Background
Our Azure SDK repositories leverage
CODEOWNERS
files (e.g. azure-sdk-for-python, azure-sdk-for-net) in two ways.First, to determine pull request reviewers, via GitHub interpreter of code owners file, as documented on GitHub docs "About code owners".
Second, to apply various additional augmentations, like automatically label PRs, notify about new issues being filed, or notify about relevant build failures. This is done by our own custom logic, as documented in azure-sdk/docs/pipelines/opensource.md.
Problem statement
The problem on the functionality relying on code owners contents, is that we do not have code owners file contents validation, thus resulting in ill-defined files, resulting in problems like notifications silently failing to be sent.
The GitHub-based logic, responsible for adding reviewers to PRs, has a GitHub-provided validator that results in a warnings banner if the file validation fails, e.g.:
The main issue here is these are just warnings, while we would like to prevent merging any changes to code owners file unless they pass validation (i.e. to gate the changes). For example, we would like to prevent folks from adding groups to the file, if they don't have necessary write permissions, as pointed out by this comment. We might also want to restrict the teams to a pre-vetted list, as seen here (Microsoft-internal).
More importantly, our own logic does not have any validation in place, hence if the code owners file is invalid, our augmentations will just silently be ignored, with no easy way to debug the issue.
Proposed solution
To address the code owners file contents issues described above, we propose to introduce a new pipeline and PR check that will validate our
azure-sdk-*
reposCODEOWNERS
files and report any issues, both for the features provided by GitHub-based interpreter, and our logic providing all the custom augmentations.The pipeline output would result in a set of well-defined problems, that can be manually uploaded beside the
CODEOWNERS
file as a text file used for baselining. Once baseline is available, the PR check will prevent any further changes to the code owners file unless they pass all validations.Once such validations are in place, we could start addressing the various issues and limitations we have, as already reported by GitHub validator (see links above), or reported by our users, e.g. #4487.
An example validation rule we could implement: flag all aliases mentioned in CODEOWNERS file for folks who are no longer Microsoft employees. Once we discover them, we probably could delete them right now as part of a cleanup effort, even before we establish the baselines.
Implementation considerations
We could handle a team assigned to given path in a code owners file by expanding it to its members in referentially transparent way. This is nontrivial because we have bunch of custom tooling ingesting as input the output of our own customer
CODEOWNERS
parser, likeFabricBot
(seeReference
section below for details). Bottom line, any additions or changes to our code owners parser require careful analysis, to ensure all our downstream tooling can properly interpret the changes/additions to supported format.One possible way to figure out how to expand GitHub teams into Microsoft aliases is through usage of CloudMine Kusto clusters. Patrick Hallisey is possible point of contact to help with this.
Related work
Linter by @heaths: https://github.com/heaths/gh-codeowners
If you have access, see also email thread
RE: Proposed API Stewardship Board process changes for PRs
from 2023/04.Reference
This doc (Microsoft-internal) (see also #4837) explains how build failure notifications via Azure DevOps notification system are implemented. The main executable source is configuration-creator, run via notficiations.yml.
Our other augmentations are provided by our other solutions, like
FabricBot
providing the PR labeling augmentation. This is explained in detail in https://github.com/Azure/azure-sdk-tools/issues/2770#issuecomment-1338923752.