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

Fail if target is missing #28

Open
JensRantil opened this issue Oct 23, 2020 · 1 comment
Open

Fail if target is missing #28

JensRantil opened this issue Oct 23, 2020 · 1 comment

Comments

@JensRantil
Copy link

Rule:

codeowners(
    name = "my_team",
    patterns = [
        "/application-a/",
        "/application-b/",
    ],
    teams = [
        "@tink-ab/my-team",
    ],
)

I just had a case where I removed all files in the application-a, including the directory. However, the generate_codeowners(...) target happily executed. It would nice if it would have failed telling me that I should removed patterns not patching anything (and possibly support disabling this check if files can be checked in dynamically or something).

@sudoforge
Copy link

sudoforge commented Oct 23, 2020

Hmm, so really what you're asking for is validation that the arbitrary pattern entered by a user of the rule matches paths in the workspace.

This gets a little trickier when considering that different platforms infer the patterns differently, e.g. Gitlab to Github have slightly different ownership syntax.

@zegl do you have thoughts about handling this cleanly? One thought I have is that we could do a little refactoring to build the workflow around something like:

codeowners(
    name = "foo",
    paths = glob([
        "README.md",
        "docs:all_files",
    ]),
    ...
)

... which would be a little more consistent with other rules and would effectively accomplish this goal. There are some downsides, though:

  • Our rule would get a little more complex as it would have to merge all the runfiles from the inputs before building the ownership strings.
  • We'd be removing arbitrary pattern matching from this library. This mainly affects usability of the rule and the verbosity of the generated file (which will now always have explicit paths and not patterns).
  • We wouldn't be able to define ownership for nonexistent paths (like we currently can), eg. a single entry for global or default ownership would no longer be possible.

I'd personally be okay with these trade-offs, but only because my teams have a hard requirement for ensuring owners are defined for paths at review time, and I have a test that ensures all paths in my repo are covered by an owners entry.

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

No branches or pull requests

2 participants