Syntax Checker errors with "missing owner" on valid CODEOWNERS

[joshua-cannon-techlabs]

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 😞

Steps to reproduce

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

Troubleshooting

[mszostok]

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 #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]

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

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]

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

[instacart-spenser]

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]

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]

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]

+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]

I have a PR open that addresses this issue: #101

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

[electriquo]

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]

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

[mszostok]

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 #73. Additionally, I need to check whether this is still valid #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 + #73 which will change the way how validator is configured and #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]

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]

I tested it also under my example org.

Happy path scenario, where there is also unowned file:

• CODEOWNERS: https://github.com/gh-codeowners/codeowners-samples/blob/happy-path/CODEOWNERS
• Action run: https://github.com/gh-codeowners/codeowners-samples/runs/5169886423?check_suite_focus=true
  By default, owner_checker_allow_unowned_patterns is set to true.

Non-happy path scenario:

• CODEOWNERS: https://github.com/gh-codeowners/codeowners-samples/blob/failures/CODEOWNERS
• Action run: https://github.com/gh-codeowners/codeowners-samples/runs/5169888668?check_suite_focus=true
  But here I set owner_checker_allow_unowned_patterns set to false.