Feature: trusted contributors

[martijnbastiaan]

When managing your own CI runners, it might not be desirable to let just anyone execute arbitrary code on it by creating a PR. Instead, a project might mark a number of regular contributors as trusted. Contributors marked as trusted would see their PRs automatically tested, while others need manual approval.

Two implementation strategies I can think of:

1. the bot could lookup the members of a specific team within the organization and consider them trusted
2. trusted users could be hardcoded through command line arguments

Would this fit within the scope of this project?

[Zimmi48]

Hello @martijnbastiaan,

I'd be interested to support your use case. Solution 1 looks like the most usable one and shouldn't be too difficult to implement.

I'd like to get your input on the way to organize this project to make it better suited for use beyond the Coq ecosystem. So far, it supports a few projects close to Coq that want to use GitLab CI, so it is @coqbot that is used to push branches to GitLab and status back to GitHub. Many features are used only in the Coq project itself and have a few hard-coded values. In your project, I assume that you would want to run your own instance with your own bot account, correct? What would be your preferred way of configuring the bot? Should it be a single application supporting all the use case we need and configured using a standard configuration language? Or should it be an OCaml library with building blocks allowing anyone to build the bot they need, at the cost of understanding a bit of OCaml syntax?

[martijnbastiaan]

Hi @Zimmi48 !

Thanks for the response and it's great to hear you're interested.

In your project, I assume that you would want to run your own instance with your own bot account, correct?

Good question. I guess we'd like the option to do so, but I don't think it's a priority. I think we'd even prefer to have someone else host it.

What would be your preferred way of configuring the bot?

The easiest way (both to implement and for us) would be to have a dot-(YAML)file in the root of our project. Ideally, the bot would have such good defaults that just adding it to the trusted teams on both github and gitlab would work.

If the bot refused to push a PR to GitLab due to the submitter not being trusted, I think the best way to manually instruct the bot to do it anyway would be a comment by one of the trusted collaborators looking like:

@coqbot approve

This way, the bot could work polling based too (simply checking mentions), which might help the design.

Should it be a single application supporting all the use case we need and configured using a standard configuration language? Or should it be an OCaml library with building blocks allowing anyone to build the bot they need, at the cost of understanding a bit of OCaml syntax?

I think I'd prefer the first. Generally, I think it helps projects to have a clear objective instead of trying to be a library supporting everyone's needs!

[Zimmi48]

Good question. I guess we'd like the option to do so, but I don't think it's a priority. I think we'd even prefer to have someone else host it.

Thanks for the feedback. Then, it'll be easier to adapt the bot to suit your needs quickly, and then we can make it more generic step by step.

Ideally, the bot would have such good defaults that just adding it to the trusted teams on both github and gitlab would work.

So I guess this would mean: if @coqbot is a member of no team (as is the case for us), then it runs everyone's pipelines. If it is the member of some teams, then it runs the pipelines of people that are members of one of those (and if none of the teams give it write-access it must get it from elsewhere anyway). That could work but it feels a bit counter-intuitive to me. I need to think about this.

As for the team on GitLab, I'm unsure what meaning it would have.

I think I'd prefer the first. Generally, I think it helps projects to have a clear objective instead of trying to be a library supporting everyone's needs!

Yes, although the Coq project itself does have quite varied needs, and we prefer to code a single bot supporting them all rather than several...

[martijnbastiaan]

So I guess this would mean: if @coqbot is a member of no team (as is the case for us), then it runs everyone's pipelines. If it is the member of some teams, then it runs the pipelines of people that are members of one of those (and if none of the teams give it write-access it must get it from elsewhere anyway).

I hadn't considered that. That does feel counter-intuitive.

I've reconsidered my earlier comment too:

The easiest way (both to implement and for us) would be to have a dot-(YAML)file in the root of our project.

This might not be as simple as I originally thought. The bot should probably ignore changes to the dot-file in PR made by untrusted contributors. But what should it do then: listen to the config file on master? Or maybe the config file before the changes in the PR? Neither feel like entirely obvious.

[Zimmi48]

I think the usual rule in the case of a configuration file that is not really meant to be tied to the code itself is to read it from the default branch. It avoids inconsistent behavior with old stable branches or outdated feature branches. That would be typically the case of a pull request template file. On the contrary, it makes sense for a code owners file or a CI configuration file to be tied to the code, so in this case, it is expected that the one that is used is the one of the pull request (or its base branch).

[martijnbastiaan]

Yes, I think you nailed it.

[Zimmi48]

@martijnbastiaan I found some time to work on this. Are you still interested? I would start by implementing the team membership check without any option to configure it, so I would just hardcode the name of your project and team in the code to start with. I think it is best to not try to solve all problems at once.

---

Reference for later:

GitHub's GraphQL API makes it easy to query for a specific user in a specific team:

query teamMember($org: String!, $team: String!, $user: String!) {
  organization(login:$org) {
    team(slug:$team) {
      members(query:$user, first:1) {
        nodes {
          login
        }
      }
    }
  }
}

If the returned list is non-empty, one has still to check that login is exactly the one that was passed as $user because it could also just contain the latter.

[martijnbastiaan]

I'm still interested! I don't think it's a good idea to test new features on the company's repo, but I can make a mock-organisation/team this weekend and experiment if you want :)

[Zimmi48]

Hi @martijnbastiaan, I've added the basic infrastructure to check that a user opening / updating a PR is in a given team before pushing to GitLab. The monitoring of comments is not done yet. This is still untested although OCaml being a pretty type-safe language, there are good chances that it will work since it type-checks.

I have a hardcoded map from organization names to team names. For now, all this map contains is:

bot/bot.ml

Line 592 in 3eab434

[("martijnbastiaan-test-org", "martijnbastiaan-test-team")]

If you want to start testing this, you'll need to follow the setup procedure described in the README.

[martijnbastiaan]

Thanks! I've setup the test repositories, but haven't managed to test the software yet. I hope to get to it some evening this week.

[Zimmi48]

BTW, you'll have to invite @coqbot to be a member of this org (and make the team public) or even make @coqbot a member of the team (if it is private) for it to be able to see the team members.

[Zimmi48]

Requesting @coqbot to run CI should now be supported with the review comment "@coqbot: Run CI now" or "@coqbot: run CI now". Support for doing this in a normal comment should be added soon.

[Zimmi48]

Support for normal comments added.

[martijnbastiaan]

Wow, you're on fire! Thanks so much! I'll test it tomorrow night (CEST)!

[martijnbastiaan]

I've added 'coqbot' to my test team on github, but it requires manual approval. I'll wait around and continue testing it if it's approved :)

[martijnbastiaan]

Oh my, I'm of course required to run my own bot. My bad!

[Zimmi48]

Done. @coqbot has accepted the invitation 😉

[Zimmi48]

Oh my, I'm of course required to run my own bot. My bad!

You are not. At least, for now I focused on implementing the feature rather than making it easier to run your own version of the bot. I'll try to make progress on this front too and on the documentation.

[martijnbastiaan]

Thanks @Zimmi48 !

Coqbot works fine on my own pull request, but doesn't run on an external one after instructing it to run with a comment. Can you see anything in the logs?

[Zimmi48]

Sorry @martijnbastiaan but I forgot to explicit that in order for this to work, you need to additionally select the "issue comment" and "review comment" events to be sent to the /github endpoint.

[Zimmi48]

So after investigating, the problem of @coqbot not receiving the event was not the only one. The token used by the bot did not give it read-access to organization content like teams (this is now fixed). The reason why CI ran anyway in your own PR is because you opened it from a branch in the repository: so GitLab mirroring kicked-in, and the bot had nothing to do with it (however, it pushed back the status check). You can see the difference by going to https://gitlab.com/martijnbastiaan-test-org/test and noticing that there is a test-1 branch but not any pr-1 branch which is the branch that the bot would have created. I created a new PR after fixing the access right and it worked this time: the log says "Unauthorized user: doing nothing." instead of "Error: Team @martijnbastiaan-test-org/martijnbastiaan-test-team does not exist.".

[martijnbastiaan]

(however, it pushed back the status check)

Right! That's why I thought coqbot was responsible.

you need to additionally select the "issue comment" and "review comment"

Right, I had selected "review comment", but missed "issue comment". The latter seemed like it wouldn't do much.

I've "approved" your PR, but it still doesn't seem to do much :(. These are the hooks I've set on GitHub:

And these ones are set on GitLab:

[Zimmi48]

Thanks for the feedback, there was an additional bug which I have fixed now: instead of checking the author of the comment, the bot was checking the author of the pull request. Of course, it was rejecting the request with “Unauthorized user: doing nothing”.

The “issue comments” event is the way GitHub sends events about “normal” comments whereas the “pull request reviews” is the way it sends events about main review comments. I thought it useless to support running the CI from a diff comment so the “pull request review comments” box doesn't have to be checked.

[martijnbastiaan]

Alright! It almost works. Coqbot is complaining about needing a rebase, but I'm unsure why?

[Zimmi48]

For some reason there was an error when fetching the remote branch. Unfortunately, the way it was coded, @coqbot was unable to distinguish actual merge conflicts from other types of errors. It is only a minor annoyance but has produced a few confusing behaviors in the past (see #34) so I've changed this and now @coqbot should be able to make the difference (in particular, it will say a rebase is needed only if it's actually true). Can you try again to see if the error persist or not?

[Zimmi48]

Remaining To-Dos:

• Support checking secret to guarantee authenticity of the webhook (otherwise there is not much point in adding this protection)
• Make it configurable with a file in the repo
• Documentation

[martijnbastiaan]

@Zimmi48 Sure thing! I'll check tomorrow night, maybe even tonight if I'm home early :).

[Zimmi48]

First to-do done: to guarantee authenticity of the requests, there is now an app at https://github.com/apps/coqbot-app. Certain operations (for now the ones introduced to grant this feature request) are now restricted to authenticated requests (in the future, we'll deprecate all unauthenticated requests for existing users). This means that you should remove the webhook you manually installed, and instead follow the previous link to install the app on your organization.

[Zimmi48]

Since there was no follow-up on this issue, I'll remove support for this feature and close it, but we can reintroduce it and use per-repo configuration files if there is still interest.