From 7984dcd3e3c6f93ed29de86132d9a5ccb52543a1 Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Apr 2022 13:40:58 -0400 Subject: [PATCH 1/5] Add git-ls-tree implementation with subdirectory support --- README.md | 1 + action.yml | 4 ++++ docs/gh-action.md | 3 +++ internal/check/not_owned_file.go | 12 +++++++++--- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 0653666..338aea0 100644 --- a/README.md +++ b/README.md @@ -131,6 +131,7 @@ Use the following environment variables to configure the application: | OWNER_CHECKER_ALLOW_UNOWNED_PATTERNS | `true` | Specifies whether CODEOWNERS may have unowned files. For example:

`/infra/oncall-rotator/ @sre-team`
`/infra/oncall-rotator/oncall-config.yml`

The `/infra/oncall-rotator/oncall-config.yml` file is not owned by anyone. | | OWNER_CHEKER_OWNERS_MUST_BE_TEAMS | `false` | Specifies whether only teams are allowed as owners of files. | | NOT_OWNED_CHECKER_SKIP_PATTERNS | - | The comma-separated list of patterns that should be ignored by `not-owned-checker`. For example, you can specify `*` and as a result, the `*` pattern from the **CODEOWNERS** file will be ignored and files owned by this pattern will be reported as unowned unless a later specific pattern will match that path. It's useful because often we have default owners entry at the begging of the CODOEWNERS file, e.g. `* @global-owner1 @global-owner2` | +| NOT_OWNED_CHECKER_SUBDIRECTORIES | - | The comma-separated list of subdirectories to check in `not-owned-checker`. When specified, only files in the listed subdirectories will be checked if they do not have specified owners in CODEOWNERS. | * - Required diff --git a/action.yml b/action.yml index b08de56..6fee558 100644 --- a/action.yml +++ b/action.yml @@ -57,6 +57,10 @@ inputs: default: "false" required: false + not_owned_checker_subdirectories: + description: "Only check listed subdirectories for CODEOWNERS ownership that don't have owners." + required: false + runs: using: 'docker' image: 'docker://ghcr.io/mszostok/codeowners-validator:v0.7.2' diff --git a/docs/gh-action.md b/docs/gh-action.md index 2838f2f..b8a5626 100644 --- a/docs/gh-action.md +++ b/docs/gh-action.md @@ -73,6 +73,9 @@ jobs: # Specifies whether only teams are allowed as owners of files. owner_checker_owners_must_be_teams: "false" + + # Only check listed subdirectories for CODEOWNERS ownership that don't have owners. + not_owned_checker_subdirectories: "" ``` The best is to run this as a cron job and not only if you applying changes to CODEOWNERS file itself, e.g. the CODEOWNERS file can be invalidate when you removing someone from the organization. diff --git a/internal/check/not_owned_file.go b/internal/check/not_owned_file.go index 0f32d5b..cb45296 100644 --- a/internal/check/not_owned_file.go +++ b/internal/check/not_owned_file.go @@ -17,10 +17,12 @@ import ( type NotOwnedFileConfig struct { SkipPatterns []string `envconfig:"optional"` + Subdirectories []string `envconfig:"optional"` } type NotOwnedFile struct { skipPatterns map[string]struct{} + subDirectories []string } func NewNotOwnedFile(cfg NotOwnedFileConfig) *NotOwnedFile { @@ -29,8 +31,10 @@ func NewNotOwnedFile(cfg NotOwnedFileConfig) *NotOwnedFile { skip[p] = struct{}{} } + return &NotOwnedFile{ skipPatterns: skip, + subDirectories: cfg.Subdirectories, } } @@ -75,7 +79,7 @@ func (c *NotOwnedFile) Check(ctx context.Context, in Input) (output Output, err return Output{}, err } - out, err := c.GitListFiles(in.RepoDir) + out, err := c.GitListFileTree(in.RepoDir) if err != nil { return Output{}, err } @@ -167,10 +171,12 @@ func (c *NotOwnedFile) GitResetCurrentBranch(repoDir string) error { return nil } -func (c *NotOwnedFile) GitListFiles(repoDir string) (string, error) { +func (c *NotOwnedFile) GitListFileTree(repoDir string) (string, error) { + args := append([]string{"ls-tree", "-r", "--name-only", "HEAD"}, c.subDirectories...) + gitls := pipe.Script( pipe.ChDir(repoDir), - pipe.Exec("git", "ls-files"), + pipe.Exec("git", args...), ) stdout, stderr, err := pipe.DividedOutput(gitls) From 81e4655fd2cb29ec57c5eb23ae70ff0125ce147e Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Apr 2022 17:26:50 -0400 Subject: [PATCH 2/5] List files --- internal/check/not_owned_file.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/check/not_owned_file.go b/internal/check/not_owned_file.go index cb45296..75374e0 100644 --- a/internal/check/not_owned_file.go +++ b/internal/check/not_owned_file.go @@ -79,7 +79,7 @@ func (c *NotOwnedFile) Check(ctx context.Context, in Input) (output Output, err return Output{}, err } - out, err := c.GitListFileTree(in.RepoDir) + out, err := c.GitListFiles(in.RepoDir) if err != nil { return Output{}, err } @@ -171,8 +171,9 @@ func (c *NotOwnedFile) GitResetCurrentBranch(repoDir string) error { return nil } -func (c *NotOwnedFile) GitListFileTree(repoDir string) (string, error) { - args := append([]string{"ls-tree", "-r", "--name-only", "HEAD"}, c.subDirectories...) +func (c *NotOwnedFile) GitListFiles(repoDir string) (string, error) { + args := []string{"ls-files"} + args = append(args, c.subDirectories...) gitls := pipe.Script( pipe.ChDir(repoDir), From 86ad834af8d1e1deade68f46ed01b18465748669 Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Apr 2022 18:30:43 -0400 Subject: [PATCH 3/5] Need xargs -r in cases when git ls-files does not have an output --- internal/check/not_owned_file.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/check/not_owned_file.go b/internal/check/not_owned_file.go index 75374e0..344975c 100644 --- a/internal/check/not_owned_file.go +++ b/internal/check/not_owned_file.go @@ -134,7 +134,7 @@ func (c *NotOwnedFile) GitRemoveIgnoredFiles(repoDir string) error { pipe.ChDir(repoDir), pipe.Line( pipe.Exec("git", "ls-files", "-ci", "--exclude-standard", "-z"), - pipe.Exec("xargs", "-0", "git", "rm", "--cached"), + pipe.Exec("xargs", "-0", "-r", "git", "rm", "--cached"), ), ) From ebd4428660f6111a2d2c125dcead69d868a2dc2e Mon Sep 17 00:00:00 2001 From: Mateusz Szostok Date: Tue, 12 Apr 2022 14:53:32 +0200 Subject: [PATCH 4/5] E2E tests --- tests/integration/integration_test.go | 10 ++++++++++ .../testdata/TestCheckFailures/notowned.golden.txt | 3 ++- .../TestCheckFailures/notowned_sub_dirs.golden.txt | 5 +++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 tests/integration/testdata/TestCheckFailures/notowned_sub_dirs.golden.txt diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index e3e9ae8..3893f7f 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -215,6 +215,16 @@ func TestCheckFailures(t *testing.T) { "NOT_OWNED_CHECKER_SKIP_PATTERNS": "*", }, }, + { + name: "notowned_sub_dirs", + envs: Envs{ + "PATH": os.Getenv("PATH"), // need to be set to find the `git` binary + "CHECKS": "disable-all", + "EXPERIMENTAL_CHECKS": "notowned", + "NOT_OWNED_CHECKER_SKIP_PATTERNS": "*", + "NOT_OWNED_CHECKER_SUBDIRECTORIES": "notowned/dir", + }, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/tests/integration/testdata/TestCheckFailures/notowned.golden.txt b/tests/integration/testdata/TestCheckFailures/notowned.golden.txt index 87045db..c65b5d1 100644 --- a/tests/integration/testdata/TestCheckFailures/notowned.golden.txt +++ b/tests/integration/testdata/TestCheckFailures/notowned.golden.txt @@ -1,7 +1,8 @@ ==> Executing [Experimental] Not Owned File Checker () - [err] Found 3 not owned files (skipped patterns: "*"): + [err] Found 4 not owned files (skipped patterns: "*"): * .gitignore * CODEOWNERS * action.yml + * notowned/dir/example/sample.txt 1 check(s) executed, 1 failure(s) diff --git a/tests/integration/testdata/TestCheckFailures/notowned_sub_dirs.golden.txt b/tests/integration/testdata/TestCheckFailures/notowned_sub_dirs.golden.txt new file mode 100644 index 0000000..da79988 --- /dev/null +++ b/tests/integration/testdata/TestCheckFailures/notowned_sub_dirs.golden.txt @@ -0,0 +1,5 @@ +==> Executing [Experimental] Not Owned File Checker () + [err] Found 1 not owned files (skipped patterns: "*"): + * notowned/dir/example/sample.txt + +1 check(s) executed, 1 failure(s) From a068e8cc8aba7081153a3ca2de460c4907d87b81 Mon Sep 17 00:00:00 2001 From: Mateusz Szostok Date: Tue, 12 Apr 2022 14:54:47 +0200 Subject: [PATCH 5/5] Run gofmt --- internal/check/not_owned_file.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/check/not_owned_file.go b/internal/check/not_owned_file.go index 344975c..101743a 100644 --- a/internal/check/not_owned_file.go +++ b/internal/check/not_owned_file.go @@ -16,12 +16,12 @@ import ( ) type NotOwnedFileConfig struct { - SkipPatterns []string `envconfig:"optional"` + SkipPatterns []string `envconfig:"optional"` Subdirectories []string `envconfig:"optional"` } type NotOwnedFile struct { - skipPatterns map[string]struct{} + skipPatterns map[string]struct{} subDirectories []string } @@ -31,9 +31,8 @@ func NewNotOwnedFile(cfg NotOwnedFileConfig) *NotOwnedFile { skip[p] = struct{}{} } - return &NotOwnedFile{ - skipPatterns: skip, + skipPatterns: skip, subDirectories: cfg.Subdirectories, } }