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

Proposal: improving review process with bot-assisted checkboxes #1007

Closed
moul opened this issue Jul 31, 2023 · 8 comments · Fixed by #3037
Closed

Proposal: improving review process with bot-assisted checkboxes #1007

moul opened this issue Jul 31, 2023 · 8 comments · Fixed by #3037
Labels
🗺️good first issue🗺️ Ideal for newcomer contributors help wanted Want to contribute? We recommend these issues.

Comments

@moul
Copy link
Member

moul commented Jul 31, 2023

We've been mulling over boosting our review process (#998, #985, #1005, and more).

Here's what I'm thinking:

  1. We kick-off with a comment sporting a checkbox and easy-peasy instructions. A bot gets to tick this when the contributor's nailed all tasks, or a reviewer steps in to give it a nod.
  2. We keep a CI/CD check yellow - a signal we're on standby, not in error mode - until the checkbox gets a tick.
  3. Bonus: this special comment can be updated by the bot to give straightforward specific instructions.

This ensures that either the contributor's done their bit, or a reviewer's given it a thumbs up.

There's good stuff happening over at https://github.com/snyk/release-notes-preview - they're using a similar system for making commit messages chime with automatic changelog and releases.

Later on, our bot could step up - like ensuring devs have signed off on a DCO, or handling specific directives for crucial repo folders. For this, I'd go with a CI/CD reacting to PR changes, and invoking a homemade Go script we could nestle in misc/gh-action.

What do you think? Let's chew the fat over this.

@moul moul added 🗺️good first issue🗺️ Ideal for newcomer contributors help wanted labels Jul 31, 2023
@moul moul changed the title Proposal: Improving Review Process with Bot-Assisted Checkboxes Proposal: improving review process with bot-assisted checkboxes Jul 31, 2023
@moul
Copy link
Member Author

moul commented Jul 31, 2023

@moul
Copy link
Member Author

moul commented Jul 31, 2023

Update:

Changed settings to Title and description from Title and commit messages.

Realized the extension "refined github" already handles improvements: Read more.

We can maintain the current setup and explore configuring "refined github" to omit checkboxes while preserving other elements.

@ajnavarro
Copy link
Contributor

@moul a super simple required checked checker: https://github.com/marketplace/actions/require-checklist

@moul
Copy link
Member Author

moul commented Sep 9, 2024

Hey, I'm recycling this old PR because we now have an R&D engineer on the team (@aeddi). I'm also re-inviting external people who want to contribute to provide their help.

I think we should start having a bot that will perform basic, regular actions in the context of GitHub, but linked with our tools (the chains, the CI, some specific realms, etc.). This bot should be hosted in a standalone repository because it will be global to the organization and not only to the gno repository (even if we start with a limited scope).
It should be built in the open, but keep it simple with hardcoded values instead of making it overly generic / reusable.

See also (potentially related):

@aeddi
Copy link
Contributor

aeddi commented Sep 10, 2024

That sounds really interesting to work on. 👍 I could maybe bootstrap a base on which to iterate over time to add features.

@Kouteki Kouteki added help wanted Want to contribute? We recommend these issues. and removed help wanted labels Oct 2, 2024
@thehowl
Copy link
Member

thehowl commented Oct 17, 2024

cc/ @aeddi, other references relating to this issue

Bot makes a comment, with "requirements" that can be automatic or manual, and be restricted on who can check the boxes

ie.

AUTOMATED:
- [ ] IF (touches folder tm2) THEN (1 (review|author) from @gnolang/eu && 1 (review|author) from @gnolang/us)
- [ ] is PR up-to-date with master?
- [ ] are all CI checks green EXCEPT known flaky ones?
- [ ] is the PR allowing edits from maintainers?

MANUAL:
- [ ] IF (touches dir1) THEN ("Infra changes needed?") CHANGEABLE BY: (@gnolang/tech-staff)
- [ ] Is the code style good? CHANGEABLE BY: (@gnolang/tech-staff)

MANUAL, by author:
- [ ] have you reviewed CONTRIBUTING?
- [ ] IF (touches dir2) THEN ("...")

All checkboxes must be ticked for merge to work
(CI is red until everything is ticked; this is the only CI that is required for merge)

One thing that I was considering together with the review team is that we should probably look to disable CODEOWNERS in the long term, possibly assisted by this bot; so we can have external PRs be reviewed by the review team first (possibly using the label), and then they can assign a reviewer (or team) to take a look at specific PRs after the first round.

@aeddi
Copy link
Contributor

aeddi commented Oct 17, 2024

Thanks! 👍
Indeed, as the functionalities of the bot will overlap with those of CODEOWNERS, it seems logical to disable it (or replace it with the bot's config file).

@Kouteki
Copy link
Contributor

Kouteki commented Oct 21, 2024

@moul a proposal that we can discuss

Short term solution

  • Remove CODEOWNERS, PR reviewers aren't assigned automatically
  • EXTERNAL PR: Review team assign themselves for the 1st review, assign the appropriate core team member after the 1st review
  • INTERNAL PR: Assign directly by teammates (1st and 2nd review)

Long term solution

  • Remove CODEOWNERS
  • Bot differentiates between internal and external owners, assigns either Review team or individuals from core team
  • If the PR affects a critical component, bot assigns a specific individual. The bot can block a merge if that specific individual hasn't approved it.

r3v4s pushed a commit to gnoswap-labs/gno that referenced this issue Dec 10, 2024
…#3037)

This pull request aims to add a bot that extends GitHub's
functionalities like codeowners file and other merge protection
mechanisms. Interaction with the bot is done via a comment. You can test
it on the demo repo here : GnoCheckBot/demo#1

Fixes gnolang#1007 
Related to gnolang#1466, gnolang#2788

- The `config.go` file contains all the conditions and requirements in
an 'If - Then' format.
```go
// Automatic check

{
  Description: "Changes to 'tm2' folder should be reviewed/authored by at least one member of both EU and US teams",
  If: c.And(
    c.FileChanged(gh, "tm2"),
    c.BaseBranch("main"),
  ),
  Then: r.And(
    r.Or(
      r.ReviewByTeamMembers(gh, "eu", 1),
      r.AuthorInTeam(gh, "eu"),
    ),
    r.Or(
      r.ReviewByTeamMembers(gh, "us", 1),
      r.AuthorInTeam(gh, "us"),
    ),
  ),

}
```
- There are two types of checks: some are automatic and managed by the
bot (like the one above), while others are manual and need to be
verified by a specific org team member (like the one below). If no team
is specified, anyone with comment editing permission can check it.
```go
// Manual check
{
  Description: "The documentation is accurate and relevant",
  If:          c.FileChanged(gh, `.*\.md`),
  Teams: []string{
    "tech-staff",
    "devrels",
  },
},
```

- The conditions (If) allow checking, among other things, who the author
is, who is assigned, what labels are applied, the modified files, etc.
The list is available in the `condition` folder.
- The requirements (Then) allow, among other things, assigning a member,
verifying that a review is done by a specific user, applying a label,
etc. (List in `requirement` folder).
- A PR Check (the icon at the bottom with all the CI checks) will remain
orange/pending until all checks are validated, after which it will turn
green.

<img width="1065" alt="Screenshot 2024-11-05 at 18 37 34"
src="https://github.com/user-attachments/assets/efaa1657-c254-4fc1-b6d1-49c7b93d8cda">

- The Github Actions workflow associated with the bot ensures that PRs
are processed concurrently, while ensuring that the same PR is not
processed by two runners at the same time.
- We can manually process a PR by launching the workflow directly from
the [GitHub Actions
interface](https://github.com/GnoCheckBot/demo/actions/workflows/bot.yml).

<img width="313" alt="Screenshot 2024-11-06 at 01 36 42"
src="https://github.com/user-attachments/assets/287915cd-a50e-47a6-8ea1-c31383014b84">

#### To do

- [x] implement base version of the bot
- [x] cleanup code / comments
- [x] setup a demo repo
- [x] add debug printing on dry run
- [x] add some tests on requirements and conditions

<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗺️good first issue🗺️ Ideal for newcomer contributors help wanted Want to contribute? We recommend these issues.
Projects
Status: Done
Status: 🔵 Not Needed for Launch
Development

Successfully merging a pull request may close this issue.

5 participants