Matticusau / pr-helper

Extremely powerful GitHub Action to streamline management of PRs through automation of common tasks. Very versatile with plenty of configuration settings to adapt to many different implementations.
MIT License
9 stars 1 forks source link

Support for key value lookup file on front matter reviewers #23

Closed Matticusau closed 4 years ago

Matticusau commented 4 years ago

Feature Request

Is your feature request related to a problem? Please describe. Currently in Jekyll like blog sites it is not mandatory to specify a GitHub username as the article Author or similar front matter value. It is common to use a friendly name which is then mapped to further details in an author file.

Describe the solution you'd like The solution should be to provide a path to a key:value YAML or JSON type file which is used to look up the GitHub username of the author. I prefer a YAML solution to match the configuration standard in GitHub Actions. I see two options:

  1. Store a custom YAML file in the .github directory. This file could be in the format of
Author Name: githubusername
Author Name: githubusername

Or

Author Name:
  ghaccount: githubusername
Author Name:
  ghaccount: githubusername
  1. [Preferred option] Support the use of the Jekyll Author file syntax. Similar to option 2 above. This could still be a path file in the .github directory or if it is a Jekyll blog then the _data/authors.yml. So adding an additional field to the author file for the GitHub UserName such as:
# Author details.
Author Name:
    name: Author Name
    email: name@email.com
    web: http://twitter.com/account
    ghaccount: githubusername
Author Name:
    name: Author Name
    email: name@email.com
    web: http://twitter.com/account
    ghaccount: githubusername

Describe alternatives you've considered Haven't found any alternative GitHub Actions to provide this functionality.

Alternatively we do not provide this functionality and simply continue with the current functionality which requires the GitHub account in the pages front matter. Which is not ideal as it duplicates the data needing to be maintained on each page if that is not already part of your Jekyll site.

Teachability, Documentation, Adoption, Migration Strategy TBD

Matticusau commented 4 years ago

@wbsmolen have a read of this and let me know if you think this solution would work for the problem we were discussing.

wbsmolen commented 4 years ago

I like the Jekyll Authors implementation.

If a PR comes in with an owner: value that isn't found in in authors.yml, it should also create a failing check.

Matticusau commented 4 years ago

Thanks, great idea on the failing check too.

wbsmolen commented 4 years ago

Is this bot mutually exclusive with the built-in codeowners functionality? Leveraging codeowners to assign review to a github team could be useful.

E.g.: Take Arc for servers' content in the DHT repo. The page owner would likely be Islam Gomaa, but ideally the entire vteam is assigned to a PR opened against its content.

The branch protection is still requiring only a single reviewer, but this would include more folks in the content review process. Plus, if Islam (sticking to my eg) is out of office, PRs against his content are not blocked.

You could implement this front matter parsing feature to allow for multiple authors, but a) that sounds complicated and b) having the github teams/groups capability would be beneficial.

Matticusau commented 4 years ago

The reviewer functionality can be leveraged by either/both:

  1. Parsing the front-matter where a comma delimited list of owners will be set as the reviewers. The current implementation should work with teams but I haven't tested that. The future state that this issue proposes should still allow for that too in the author file in theory.
  2. If you have a CODEOWNERS file, then this will be handled natively by GitHub, and can be used in addition to the front-matter parsing. This natively supports both individuals and teams.

Regardless of which method assigns the reviewers, the Bot/Action can be configured with either:

  1. Set prmerge-requirereviewcount = -1 will disable the minimum review count and instead require that there are no pending reviewers on the PR. Once all reviewers have approved it would proceed with the merge.
  2. Set prmerge-requirereviewcount = X will require that a minimum of X reviews are approved before auto merge will run. There may still be other reviewers requested. This is similar to the minimum reviewer functionality in GitHub Branch Protection. Unfortunately I am still looking for a way to pull that setting via the API. The value of X should match your Branch Protection Rules.

So I believe the scenario you describe is catered for with this configuration regardless of which method you use to assign reviewers. Actually using a combination of both could be beneficial as it would increase awareness of content changes to a larger group and allow backup coverage.

Matticusau commented 4 years ago

Let's settle on this syntax for Authors file:

# Author details.
Author Name:
    name: Author Name
    email: name@email.com
    web: http://twitter.com/account
    github: githubusername
Matticusau commented 4 years ago

Progress update - 2020-07-26

Initial dev work done on reading Author file and retrieving GitHub user. Implementation into work flow still required. TODO: Look at leveraging artifacts to avoid API call to file contents https://developer.github.com/v3/actions/artifacts/

Matticusau commented 4 years ago

Progress update - 2020-07-26 - Feature Preview testing

Preview feature testing will be completed in the Dev breach tomorrow. This initial release will perform an API call against the repo to get file contents. Am looking into using Action/Checkout on default branch and the Artifacts to reduce the chance of rate limiting on some membership levels.

Matticusau commented 4 years ago

Initial support for this is released. Please open new bugs/features and reference this issue where relevant