mirego / dispatch

🦀 Dispatch makes sure pull requests within a GitHub organization get reviewed by the right people.
https://open.mirego.com
BSD 3-Clause "New" or "Revised" License
25 stars 3 forks source link

Support usernames blacklist for a repository #18

Open garno opened 5 years ago

garno commented 5 years ago

Sometimes, someone might feel like he has lost track of a projet for various reasons. If that is the case, he might not be the appropriate person to review a Pull Request and he’s taking the spot of a more relevant reviewer.

To prevent this, we could allow a per-repository configuration with a blacklist.

{
  "repositories": {
    "mirego/dispatch": {
      "blacklist": [
        {
          "username": "garno"
        }
      ]
    }
  }
}

By keeping the same structure as the global blacklist, we can simply merge both of them before making the final reviewers selection.

This is the first time we’re adding repository configuration outside the query params. Thoughts? It could be use for various other improvements.

✌️

remi commented 5 years ago

Since the organization never changes, it’s not possible for Dispatch to handle a Webhook URL from another organization. Therefore I think we could drop the mirego/ from the repository key.

However, I’m not sure I’m on board with the structure you’re suggesting here, since it opens up an opportunity to store repository settings in the JSON file rather than trough URL query parameters. We want to avoid configuration.json becoming a database file (eg. by storing repository stacks and other settings in it).

I think we could extend our already-existing blacklist format to support specific repositories:

{
  "blacklist": [
    {
      "username": "foo"
    },
    {
      "username": "bar",
      "repositories": ["repository-bar-no-longer-wants-to-be-requested-on"]
    }
  ]
}

The behavior for the user foo doesn’t change, he’ll still be blacklisted for all repositories. However, bar will only be considered blacklisted for Webhook requests related to this specific repository.

What do you think?

garno commented 5 years ago

We want to avoid configuration.json becoming a database file (eg. by storing repository stacks and other settings in it).

I feel like it is already the case though. I think that using query parameters will eventually add an unnecessary complexity as we add more and more preferences for a repository.

That being said, I do not dislike the structure you are proposing. @thermech will also approve :)