search

mszostok / codeowners-validator

The GitHub CODEOWNERS file validator
Apache License 2.0
212 stars 47 forks source link

Syntax Checker errors with "missing owner" on valid CODEOWNERS #94

Closed joshua-cannon-techlabs closed 2 years ago

joshua-cannon-techlabs commented 2 years ago

Description

It's totally valid to list a directory/file/pattern and leave off an owner as a way of specifying "no owner". See the docs: https://github.com/github/docs/blob/209c340039acc4c2f73c2eb1fcd607af00911118/content/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners.md#L103

# In this example, @octocat owns any file in the `/apps` 
# directory in the root of your repository except for the `/apps/github` 
# subdirectory, as its owners are left empty.
/apps/ @octocat
/apps/github

Notice the last line has no owner.

Expected result

No error when leaving off owner.

Actual result

Error :disappointed:

Steps to reproduce

Run the tool on a CODEOWNERS file which leaves off the owner

Troubleshooting

mszostok commented 2 years ago

Hi @joshua-cannon-techlabs Thanks for reporting this issue. I will fix that. Do you maybe know the use-case for that?

Because here is also a valid issue https://github.com/mszostok/codeowners-validator/issues/70 which mention that the validator should inform you about valid but problematic part of your CODEOWNERS file.

In this case such thing IMO shouldn't be an error as it's valid from the CODEOWNERS point on view but at least it should be displayed as warning because AFIK it will block you from merging changes to /apps/github if it's not owned by anyone. WDYT?

joshua-cannon-techlabs commented 2 years ago

(FWIW I would fix it and make a PR but we're still working with legal on open source contributions :sob: )

I think it all depends on your policies. If you don't have the strict owners policy than you can still merge changes to /apps/github.

If this kind of validation (and #70) was an optional check I can opt into or out of then thats good. But putting them in syntax check is wrong IMO as the file is syntactically correct.

joshua-cannon-techlabs commented 2 years ago

Actually, I might be able to take a stab at this. Good excuse to learn some Go

instacart-spenser commented 2 years ago

We have a use case for this. In our repo, we have one file which does not need a code owner (anyone with access to the repo should be able to review/approve it.) However, the rest of the files in the directory in which it is in should be owned by a specific team.

The file to be unowned is a configuration file for the oncall-rotator. The other files in the directory are an app that uses the configuration file.

/infra/oncall-rotator/                                  @sre-team
/infra/oncall-rotator/oncall-config.yml
joshua-cannon-techlabs commented 2 years ago

I have the implementation ready to go, but it'll need to be reviewed internally which could take time.

The implementation itself is quite simple, as we're just removing code so I wouldn't be brokenhearted if someone else beats me to this and makes a PR.

cktaylor commented 2 years ago

We have a similar use-case in our repo - of wanting to have an explicitly unowned file in an otherwise owned directory.

It would definitely be encouraging if codeowners-validator can handle this directly - so we can avoid other compromises (intentionally changing our file location/structure solely to allow codeowners-validator to pass).

peterdemin commented 2 years ago

+1 on this. We have auto-generated files in the same directory as configuration files, different owners assigned to different configuration files, but none should be tagged on the auto-generated files changes.

athtran commented 2 years ago

I have a PR open that addresses this issue: https://github.com/mszostok/codeowners-validator/pull/101

@mszostok If you could take a look when you have some time!

electriquo commented 2 years ago

I have the implementation ready to go, but it'll need to be reviewed internally which could take time.

The implementation itself is quite simple, as we're just removing code so I wouldn't be brokenhearted if someone else beats me to this and makes a PR.

@joshua-cannon-techlabs: Could you please update on the status?

joshua-cannon-techlabs commented 2 years ago

@athtran 's PR is basically the same as mine. We're in Open Source limbo :/

mszostok commented 2 years ago

Sorry guys!

Currently, I'm quite busy, and this project was developed 100% in the spare time, which I almost don't have right now.

The mentioned PR is quite problematic for me as it's only removing a given check instead of making it optional, so maybe I will adjust it a bit, so you can set a given env variable to disable strict checking. Configuring the codeowners validator in a more readable/customizable way will be handled by https://github.com/mszostok/codeowners-validator/issues/73. Additionally, I need to check whether this is still valid https://github.com/mszostok/codeowners-validator/pull/78 as it already landed on main branch.

Once again, sorry for such long waiting time. I also respect your time that you spent on it, so I will reserve some time this weekend to take care of this and do the release.

FYI: I plan to do the 1.0.x release when I will be able to address all reported bugs + https://github.com/mszostok/codeowners-validator/issues/73 which will change the way how validator is configured and https://github.com/mszostok/codeowners-validator/issues/50. Additionally, I would like to add an option to format CODEOWNERS files and an option to add/remove a given owner and create PRs automatically e.g. with such (draft) config:

updateRepositories:
  - path: "/tmp/cloned/repository"
  - url: "capactio/capact"
  - path: "capactio/website"

insert:
  - owners: [ "@hakuna", "@matata" ]
    condition:
      pattern: "*.go"
  - owners: [ "@baz", "@bar" ]
    condition:
      associatedWithOwner: "@ghost"

pullRequest:
  titleFormat: "Add {{ .NewOwner }} as a code owner in {{ .RepoName }}"

so maybe this project will be changed to sth like codeowners-manager than codeowners-checker.

mszostok commented 2 years ago

Hi all!

I fixed it in a way that the new release v0.7.0 won't report missing owner as an error anymore 🙂 Nevertheless, I wanted to keep it, so I moved this check under owner checker and made it optional. As a result, validator may work in a picky mode when needed, see new option:

Name Default Description
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.

Additionally, I changed it from error to warning:

==> Executing Valid Owner Checker (1.2s)
    [war] line 23: Missing owner, at least one owner is required

To enable it back, set:

      - name: GitHub CODEOWNERS Validator
        uses: mszostok/codeowners-validator@v0.7.0
        with:
          owner_checker_allow_unowned_patterns: "false"

Additionally, you can change the level on which the application should treat reported issues as failures. By default, it's set to warning, which treats both errors and warnings as failures, but as the missing owner is reported as warn you can also ignore it:

      - name: GitHub CODEOWNERS Validator
        uses: mszostok/codeowners-validator@v0.7.0
        with:
          owner_checker_allow_unowned_patterns: "false"
          check_failure_level: "error"

In this case, you will see it in logs, but the action will exit with 0 status code.

Once again, thanks for understanding and apologize for my absence. Hope v0.7 will be useful!

mszostok commented 2 years ago

I tested it also under my example org.

Happy path scenario, where there is also unowned file:

Non-happy path scenario: