chore: reuse function from whoowns package#93
Conversation
dev_tools/check_ownership.py
Outdated
| GithubOwnerShip, | ||
| OwnerShipEntry, | ||
| check_git, | ||
| get_codeowners_path, |
There was a problem hiding this comment.
Oh, looks like I missed something. I though we remove this function entirely and instead use find_codeowners_file exclusively.
There was a problem hiding this comment.
That's a good idea. I'd also prefer to move find_codeowners_file to ownership_utils. That of course breaks the e2e tests in this PR. Do you think that that move makes sense?
There was a problem hiding this comment.
yes, let's move it. we probably need to disable again, release, and enable.
There was a problem hiding this comment.
Cool, already done and the e2e test fails as expected.
As an alternative, can we mark the e2e test as not required and merge with a red test? That would actually more transparently show what's going on and require less editing and reverting.
There was a problem hiding this comment.
ok, let's make this check optional. maybe we can find a better solution at some point.
There was a problem hiding this comment.
Do you want to make it optional or should we make this a separate PR?
There was a problem hiding this comment.
Isn't this a GitHub GUI setting for the repo? I.e., defining which job is required and which one isn't?
I don't think that there's something like allow-failure for GitHub actions, see actions/runner#2347
There was a problem hiding this comment.
That feels ugly. We could also put the check-ownership hook into
dev-tools/.github/workflows/check.yaml
Line 51 in fe85928
There was a problem hiding this comment.
True, that's possible, done.
And I agree, an alternative would be good. I'd also appreciate if we had a test that consumes whoowns from this repo and not from pypi, to not repeatedly run into this issue. I still think that we should also have a full e2e test that creates the hooks as if they were used on another repo, but maybe that's manual, allowed to fail (one can always do <possibly_failing_cmd> || true) or run on tags only. What do you think?
There was a problem hiding this comment.
We could have a list of gating and non gating try-repo calls. No gating by || true.
2 participants
As agreed in #90