Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Feature suggestion: PR mode #82

Open
dennissivia opened this issue May 29, 2020 · 2 comments
Open

Feature suggestion: PR mode #82

dennissivia opened this issue May 29, 2020 · 2 comments
Labels
enhancement New feature or request

Comments

@dennissivia
Copy link

Motivation

At the moment all files that do not have an owner are always taking into consideration for the final result. We also introduced a whitelist concept to ignore certain files/directories.
However, for a code-base with a lot of a legacy fixing exiting issues would just block their progress. This is why tools like pronto allow rubocop and other linters to only run against "changed files".

Possible solution

Thus I suggest to add a similar mode for the codeowners-checker so that we can only test the files that have been changed. That flag could be named --pr-mode or --changed-only to indicate that only changed files are checked.
This should also have the benefit of being much faster for big code bases.

@dennissivia dennissivia added the enhancement New feature or request label May 29, 2020
@bamorim
Copy link
Contributor

bamorim commented Jun 8, 2020

What do you think if instead of having --pr-mode we implement a pronto runner? Something like pronto-codeowners-checker.

But extending on that, let me see if I get correctly.

This gem does, basically, two things:

  • Checks codeowners file, line by line, to see if there is an issue with each line
  • Checks all files in directory and compare the the full codeowners file

When you say we only check changed files, do you mean that we only do the second check for the files changed but also check all lines on codeowners?

I mean, if I remove a file, then it is definitely not a file missing from codeowners (Since the file does not exist anymore), but the other check we do is to see if there is any useless patterns on codeowners and that may change when removing a file.

My point is, when we say we test only the files that has been changed do we also check for when a removed file may make a pattern useless?

@bamorim
Copy link
Contributor

bamorim commented Jun 11, 2020

We already have the --from and --to flags that can be used to compare the PR. However, it has a few flaws:

  • It uses that to get the new files between branches and check if any one of these files have owners. This means that a rule removed from CODEOWNERS will not be caught because the file isn't new.
  • It does not work for the invalid owner or useless pattern checks, because these look at the current state of the CODEOWNERS/OWNERS file and not the diff

That being said, what we can do to have a Proof of Concept of a nice lint tool that checks only the differences is to use the --from {base_branch} and --to HEAD and introduce a new flag that disable the checks that are not "diff-aware", maybe something like --report-only-files-without-owners.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants