Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

jeremycohen
Copy link
Contributor

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

README.md Show resolved Hide resolved
@jeremycohen jeremycohen changed the title Add git-ls-tree implementation with subdirectory support not-owned-checker: Add git-ls-tree implementation with subdirectory support Apr 11, 2022
@mszostok mszostok self-assigned this Apr 11, 2022
@mszostok mszostok added the enhancement New feature or request label Apr 11, 2022
@jeremycohen
Copy link
Contributor Author

@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.

@jeremycohen jeremycohen marked this pull request as draft April 11, 2022 21:05
@mszostok
Copy link
Owner

mszostok commented Apr 11, 2022

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
Copy link
Contributor Author

Why the git ls-files -- <sub_dirs_pattern> 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
Copy link
Contributor Author

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 jeremycohen marked this pull request as ready for review April 11, 2022 22:31
Copy link
Owner

@mszostok mszostok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

I like the idea that you implemented. I only added e2e tests to ensure no regression in the future.

@mszostok mszostok merged commit a16e4b9 into mszostok:main Apr 12, 2022
@jeremycohen
Copy link
Contributor Author

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

@mszostok
Copy link
Owner

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

@jeremycohen jeremycohen deleted the jc/add-subdirs branch April 12, 2022 19:28
@mszostok
Copy link
Owner

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: actions/checkout#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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants