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

ci: add a github bot to support advanced PR review workflows #3037

Merged
merged 44 commits into from
Nov 27, 2024

Conversation

aeddi
Copy link
Contributor

@aeddi aeddi commented Oct 28, 2024

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 #1007
Related to #1466, #2788

  • The config.go file contains all the conditions and requirements in an 'If - Then' format.
// 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.
// 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.
Screenshot 2024-11-05 at 18 37 34
  • 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.
Screenshot 2024-11-06 at 01 36 42

To do

  • implement base version of the bot
  • cleanup code / comments
  • setup a demo repo
  • add debug printing on dry run
  • add some tests on requirements and conditions
Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@aeddi aeddi self-assigned this Oct 28, 2024
@aeddi aeddi marked this pull request as draft October 28, 2024 10:00
@zivkovicmilos zivkovicmilos self-requested a review October 28, 2024 10:04
@thehowl
Copy link
Member

thehowl commented Oct 30, 2024

@aeddi would you like a preliminary review?

@aeddi aeddi force-pushed the dev/aeddi/github-bot branch from 8da569e to 4a3e93d Compare November 4, 2024 10:12
@ltzmaxwell
Copy link
Contributor

could you please add a brief description to give people an early preview, even if it's draft?

@aeddi
Copy link
Contributor Author

aeddi commented Nov 5, 2024

@thehowl Sorry for the delayed response. I hadn't seen your message. I still have some cleanup to do and need to figure out how to write some tests, but I think the PR is pre-reviewable now.
Thanks!

@ltzmaxwell Done ;)

@aeddi aeddi force-pushed the dev/aeddi/github-bot branch from 7b63f89 to 1a34f18 Compare November 5, 2024 23:31
@aeddi aeddi marked this pull request as ready for review November 6, 2024 00:51
@aeddi aeddi force-pushed the dev/aeddi/github-bot branch from 1a34f18 to 7aa06b8 Compare November 6, 2024 00:55
@ltzmaxwell
Copy link
Contributor

I haven't looked into this in depth yet, but I'm considering whether we could introduce an LLM, like ChatGPT, to assist with code review. It could help summarize PR modifications and potentially identify issues in the code. This is outside the current scope, but I wanted to leave some thoughts here for discussion.

@aeddi
Copy link
Contributor Author

aeddi commented Nov 6, 2024

Yes, why not have this kind of tool just to detect details we might have missed?
I know it's a controversial topic, but personally, knowing that our code is completely open-source, I wouldn't mind having a report from an LLM to pick out only the relevant suggestions.

Just quickly checking, I see there are Actions on the Github marketplace dedicated to this.

@moul
Copy link
Member

moul commented Nov 12, 2024

When do you think we should start using it and providing feedback for iteration?

@aeddi
Copy link
Contributor Author

aeddi commented Nov 12, 2024

@moul As soon as it's reviewed and merged, I think we can start by setting up a few simple rules to make sure everything is working well, then iterate from there.
The problem is that it's tricky to test a bot based on GitHub Actions. I've done a lot of testing on two private repos, but I hope there aren't any issues I haven't anticipated.
For example, we can test safe operations like requesting reviews from certain users + adding a manual check, or something like this.

BTW, the failing check is because the bot workflow added to this PR is trying to run while the repo isn't configured yet (specifically, the workflow requires setting a GitHub Actions secret and variable).

Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this way of implementing a bot is not secure. Giving random access to code execution to anyone creating a PR is not a good idea IMO.

.github/workflows/bot.yml Show resolved Hide resolved
.github/workflows/bot.yml Show resolved Hide resolved
.github/workflows/bot.yml Show resolved Hide resolved
.github/workflows/bot.yml Show resolved Hide resolved
@aeddi
Copy link
Contributor Author

aeddi commented Nov 13, 2024

I'm not sur to understand what differ from any other action. I mean to run code when opening a PR or any other GitHub event, we already have more than 130 checks that run for each PR we open.
But maybe I didn't fully understand what you meant?

Also in the case of this bot, we own its code in our repo and have total control over it. Which looks more secure to me than using third-party actions.

Still following the PoLP, the only permissions the bot requires right now are those listed in the README, which are not very sensitive IMO. The write permissions are for the scopes Pull Requests (managing comments, labels, milestones, etc.) and Commit Statuses (showing a check at the bottom of the PR). It can't change any code on the repo, for instance.

Do you have another approach to suggest or specific guideline to follow?

@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 13, 2024
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, my only requirement is to get ride of this // nolint:govet and fix cancel leaking.

I'm not sure about @ajnavarro's concern; as long as the token is correctly scoped, it should be fine IMO.

To me, the main issue is that if the PR comes from a fork, the workflow could not be able to grab the bot secret and therefore cannot access the token. Can you (have you?) check the scenario where you trigger this workflow from a branch of an external fork?

from: https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

contribs/github-bot/client/client.go Outdated Show resolved Hide resolved
contribs/github-bot/client/client.go Outdated Show resolved Hide resolved
contribs/github-bot/main.go Show resolved Hide resolved
.github/workflows/bot.yml Show resolved Hide resolved
contribs/github-bot/comment.go Outdated Show resolved Hide resolved
@aeddi aeddi force-pushed the dev/aeddi/github-bot branch from 6526e12 to f7e7902 Compare November 27, 2024 14:33
@aeddi aeddi force-pushed the dev/aeddi/github-bot branch from f7e7902 to 00bc576 Compare November 27, 2024 14:36
@thehowl
Copy link
Member

thehowl commented Nov 27, 2024

@aeddi, please avoid force-pushing and re-basing: 1. we use squash and merge, so everything is merged as a single commit eventually; 2. using merge commits allows reviewers to better see the differences from the last review.

@thehowl thehowl merged commit 310938d into master Nov 27, 2024
101 of 102 checks passed
@thehowl thehowl deleted the dev/aeddi/github-bot branch November 27, 2024 17:53
@Kouteki Kouteki removed the in focus Core team is prioritizing this work label Nov 29, 2024
@aeddi aeddi mentioned this pull request Dec 2, 2024
@gnolang gnolang deleted a comment from Gno2D2 Dec 2, 2024
r3v4s pushed a commit to gnoswap-labs/gno that referenced this pull request 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
None yet
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Proposal: improving review process with bot-assisted checkboxes
8 participants