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

Option to disable Check if a GitHub owner is in a given organization #73

Open
Nuru opened this issue Jun 7, 2021 · 3 comments · May be fixed by #222
Open

Option to disable Check if a GitHub owner is in a given organization #73

Nuru opened this issue Jun 7, 2021 · 3 comments · May be fixed by #222
Labels
breaking Introduces a breaking change enhancement New feature or request priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@Nuru
Copy link

Nuru commented Jun 7, 2021

Description

Please provide an option to disable only the "Check if a GitHub owner is in a given organization" part of the Valid Owner Checker.

Reasons

I frequently work in repos that have an outside collaborator as a Code Owner (sometimes it is me). I would like to be able to keep the other checks in the Valid Owner Checker active and disable only the "Check if a GitHub owner is in a given organization" part. It is excessive to completely disable all checks for outside collaborators.

@mszostok mszostok added the enhancement New feature or request label Jun 8, 2021
@mszostok
Copy link
Owner

mszostok commented Jun 10, 2021

Hi @Nuru

I can add opt to disable that e.g. OWNER_CHECKER_DISABLE_ORG_MEMBER_VALIDATION=true/false. But it will be a short-term solution. Later someone else can as about disabling other sub-check, maybe even disable it only for some owners etc.

Sometimes, I see that people introduce known_violations.txt and compare validator output with this file. In that way, some known issues can be ignored.

With codeowners-validator, it's a bit complicated as the output is not deterministic (parallel execution), so I thought to add native support to ignore/disable certain issues.

Example 1:

Global option to document excluded/ignored rules.

# config.yaml

# Option to ignore known and accepted issues that are detected by checks
issues:
  # Excluding checks per-text
  exclude-rules:
    # Exclude per-text message. Exact match.
    - text: 'Team "not-existing-team" does not exist in organization "gh-codeowners"'
      checks:
        - owner
    # Exclude per-text message. Regex match. In that way u exclude organization check completely for any team
    - regex: 'Team ".*" does not exist in organization "gh-codeowners"'
      checks:
        - owner

Pros:

  • it is easy to introduce
  • covers all possible scenarios as you can ignore new issues in the same way

Cons:

  • as it is text-based, all breaking changes in messages will break the flow

Example 2:

Check-scoped option to document excluded/ignored rules.

# config.yaml

# all available settings of specific linters
checks-settings:

  owner:
    # The owner and repository name separated by slash. Used to check if GitHub owner is in the given organization.
    # (required)
    repository: "gh-codeowners/codeowners-samples"

    # Excluding configuration
    exclude-rules: # support excluded only for some users so outside collaborators are not check but the rest are validated

      # excluded for all as no owners provided
      - rule: valid-definition
      - rule: has-github-account
      - rule: team-exists


      # excluded only for a given owners
      - rule: is-in-organization
        owners:
          - @owner1
          - @owner2
          - @owner3

Pros:

  • implementation details are hidden behind rule name.
  • option to disable sub-check completely in more readable way.

Cons:

  • Syntax is more complex.
  • Needs to be implemented per check.

At the moment I am leaning towards the first option as it will be easier to implement and ootb it's more generic. Is it ok with you guys?

/cc @Nuru @osterman @nitrocode

@mszostok mszostok added the needs-feedback Indicates an issue needs more information in order to work on it. label Jun 10, 2021
@nitrocode
Copy link

nitrocode commented Jun 15, 2021

I appreciate all the time you put into these examples. As for example 2, we currently ignore each individual using the owner_checker_ignored_owners input. I'd prefer not to ignore each owner individually as it's difficult to scale.

If we're going with either solution, I'd favor example 1 as that will eliminate the need to maintain a list of ignored users. Like you mentioned, if the message changes, then the exception breaks. However, I'm not too concerned about that as 1) that message probably doesn't change often and 2) we would notice immediately if the PR failed for the updated text and would update the exclusion text accordingly.

@Nuru
Copy link
Author

Nuru commented Jun 28, 2021

@mszostok You are right that people will want to disable other sub-checks. If you look at other "linters" you will see that in general they give some kind of name/label/key to every single check at the finest possible detail level (such that there is no such thing as a "sub-check") and then allow people to enable or disable them one by one. Usually you can say "run all except these" or "run only these". See mega-linter and rubocop for just 2 examples. These linters have been around a long time and have much guidance to offer about how to structure the configuration of a linter such as codeowners-validator.

So I suggest you start there. You do not need to restructure the code that implements the tests, just expand the list of options to cover every test.

@mszostok mszostok added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-feedback Indicates an issue needs more information in order to work on it. labels Feb 10, 2022
@mszostok mszostok added this to the v1.0.0 milestone Feb 10, 2022
@mszostok mszostok added the breaking Introduces a breaking change label Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change enhancement New feature or request priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants