not-owned-checker: Add git-ls-tree implementation with subdirectory support

mszostok / codeowners-validator

The GitHub CODEOWNERS file validator

Apache License 2.0

212 stars 48 forks source link

not-owned-checker: Add git-ls-tree implementation with subdirectory support #141

Closed jeremycohen closed 2 years ago

jeremycohen commented 2 years ago

Description

Changes proposed in this pull request:

Support setting subdirectories when checking for non-existent owners using NOT_OWNED_CHECKER_SUBDIRECTORIES. If a file does not exist within these subdirectories, thenot-owned-checker will not return that file when seeing if it doesn't have an owner
Instead of using git ls-files after creating the artificial .gitignore we can use git ls-tree which returns the same output with the set arguments. The subdirectories specified are added as additional arguments for filtering

jeremycohen commented 2 years ago

@mszostok on second thought I'll need to hold off on this until I fix one thing. git ls-tree will check off of HEAD versus git ls-files which checks on the index. Will try to see if I can patch.

mszostok commented 2 years ago

Hi! Thanks for the PR. The feature is reasonable 👍

However, I see that CI fails. I check that locally, and I was also getting the same issues. It turns out that the git ls-tree works properly when the changes are committed, which makes sens base on the desc:

git-ls-tree - List the contents of a tree object.

Why the git ls-files -- <sub_dirs_pattern> cannot be used instead?

Tested on:

$ uname -prs 
Darwin 21.4.0 i38

$ git --version
git version 2.35.1

jeremycohen commented 2 years ago

Why the git ls-files -- cannot be used instead?

Ah, I might've missed that ls-files supports file arguments. Let me give it a run and will un-draft if it seems to work on my end.

jeremycohen commented 2 years ago

Fixed an issue with xargs that was causing some problems on our machines, and seems to be working when doing the approach you said.

Putting back in review.

jeremycohen commented 2 years ago

Great, thanks @mszostok! Pls me know if you're able to handle the release with this added.

mszostok commented 2 years ago

Hi @jeremycohen, yes I will create a new release, it will be today or tomorrow as I want to merge one more PR 👍

mszostok commented 2 years ago

Hi @jeremycohen the new release is available. However, I spot an issue: https://github.com/GitHubCODEOWNERS/codeowners-samples/runs/6004341721?check_suite_focus=true

This is due to the yesterday git release, see more in: https://stackoverflow.com/questions/71849415/cannot-add-parent-directory-to-safe-directory-on-git

Let me know if it's also a problem on your side. There are some workarounds: https://github.com/actions/checkout/issues/760

Unfortunately, I was not able to apply them with success. I will wait a bit more, as maybe there will be some official statement on how to approach that problem with GitHub Actions.

jeremycohen commented 2 years ago

Hey @mszostok, thanks for the release! Appreciate it.

I tried upgrading to git 2.35.2 locally and wasn't able to repro. Our CI machines haven't been upgraded to 2.35.2 yet, but will keep you posted if we see this issue. We can probably use that workaround in our checks if needed and codeowners-validator is failing for these reasons.