Add an additional webhook event to trigger the eca check

The relevant webhook event should be this: https://docs.github.com/en/webhooks/webhook-events-and-payloads#merge_group

changed the description

Any update on this? We use merge queues to ensure that concurrent PRs cannot break the main branch, not being able to make the eca check required introduces an unnecessary risk.

@zacharysabourin @malowe - Can you guys please take a look at this issue?

I can look into this to figure out effort, and as long as it's not weirdly difficult have a patch for this early next week

assigned to @malowe

I've done some additional reading as I was unfamiliar with merge queues before now (mainly a GL user over github). In the documentation for merge queues, it has the following statement:

Once a pull request has passed all required branch protection checks, a user with write access to the repository can add the pull request to the queue. The merge queue will ensure the pull request's changes pass all required status checks when applied to the latest version of the target branch and any pull requests already in the queue.

To me, this reads as a PR needs to exist and pass it's checks to be added to the "Merge Queue". Branch protection rules can include rules like "an approving review or passing status checks for all pull requests merged into the protected branch.", as read on the Branch Protection Rule documentation page. The fix here sounds like we can just return 200 on the merge_queue events without processing, as the presence of the data in the queue means it passed the PR's ECA status check.

@netomi am I missing anything with my understanding that would mean we would have to process the requests?

I am also not an expert on this topic, and it will need further testing, but my understanding is the following:

to add a PR to a merge queue, all required status checks need to pass first, up to this point a PR acts as a normal PR, e.g. issues a pull_request/synchronize webhook event if the PR changes
after a PR has been added to a merge queue, the behavior changes, such that merge_group/checks_requested events are triggered if another PR gets added to the merge group

in the next section it is explained like that: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#configuring-continuous-integration-ci-workflows-for-merge-queues

so if you have a workflow that produces a status check that is required, you need to add an additional trigger merge_group as otherwise the workflow is not triggered and the status check is not reported and thus the PR cant get merged.

Maybe this section is the actual relevant one:

Triggering merge group checks with third-party CI providers

With third-party CI providers, you will need to update your CI configuration to run when a branch that begins with the special prefix gh-readonly-queue/{base_branch} is pushed to. These are the temporary branches that are created on your behalf by a merge queue and contain a different sha from the pull request.

Here is some more explanation: https://blog.devops.dev/githubs-pr-merge-queue-93991cc5cea0

So with a merge queue, temporary branch are created that combine all changes that are supposed to be merged together into the main branch. I guess you need to attach the eca status check this the HEAD of this branch as well. In order to do that, you should listen to the merge_group/checks_request event as it will contain the head sha as property afaict.

mentioned in commit 0bb12164

mentioned in merge request !197 (merged)

mentioned in commit 16bd9b7b

I've made these updates and will be pushing the change live early next week. The fix made here was to push a commit status for the head sha upon request. As the commits have all been validated as part of the PR, we don't need to check here. The link Thomas provided in his last comment explained why this was needed at all.

When a commit is added to a merge queue, it is added to a read-only branch and given a new SHA. This new SHA will not have the commit status that was given in the PR, which explains the issue. By pushing a new status, we should be able to get this working again. The only caveat is that if the merge_group commit is the one merged in, we won't have the URL associated with the status check as the merge_group scrubs references to the originating PR.

As an aside, adding this permission will be a manual update to the ECA app associated with the repositories, so projects will likely still need to ask for this to be enabled. The alternative is to add this as a default operation in Otterdog to ensure that the merge_group event has been checked in the app, though that is likely not an easy check to do.

regarding the permission update:

I had a similar case with otterdog, but I did change the permissions using the eclipsewebmaster account, who is the owner of the EclipseFdn and all other orgs. So in this case I did not have to approve the updated permissions for all installations which was really a relief.

So I am not sure if some other thing played into that, but I would do the permission change with your personal account even if you are a maintainer for this app as well afaict.

For the real app, I needed to add Merge Group::read permissions, and then Merge Group as a new event type. There was a merge queue event mentioned, but it wasn't the correct one in this case, so we did end up needing a permissions update as well, which will go to the inboxes of organization/repository owners that have the app installed.

mentioned in issue #170 (closed)

I was able to get this working in testing with the following settings in a branch protection rule set. Can you confirm that these settings make sense and reflect generally what was used?

Require merge queue checked
- Require all queue entries to pass required checks unchecked
Require a pull request before merging checked
Require status checks to pass checked
- eclipsefdn/eca added as a required check

I added the "Require a pull request" as a safety thing since the Merge Group calls are uniquely unhelpful for figuring out what was pushed, and the current state is essentially a rubber stamp. Doing a more in-depth implementation would be much longer to implement and a lot more complicated from what I can figure out by sniffing through the docs.

@mnonnenmacher would you be able to confirm whether these settings match the intended use case? If they don't work for the case, could you elaborate the differences in the given settings for context? We want to try and make it work for the community while not compromising on some of the checks we introduce like the ECA.

Yes, I think that matches our configuration. As I don't have access to our merge queue settings, please have a look on your own: https://github.com/eclipse-apoapsis/ort-server/settings

Our main goal is that we can make the eclipsefdn/eca check required again. We had to remove it from the list because it was not executed in the merge queue, and it sounds like this would be fixed with your changes.

Requiring pull requests also makes sense to me, actually I don't know how you could add something to the merge queue without creating a pull request first.

I actually don't have access to the admin creds as a point of security/separation, I just help write the sync and ECA code ;) @cguindon or I believe @fgurr will be able to help me confirm early next week, could one of you get back to me during NA office hours next week to check on this? I think Fred has admin access, but I could be wrong.

Requiring pull requests also makes sense to me, actually I don't know how you could add something to the merge queue without creating a pull request first.

From what I understand, if you push directly to the branch it would go into the queue if you've set up the queue in the ruleset for the branch. I'm not fully up to speed on exact branch protection rules, so I'm not sure if it's possible today, but is theoretically possible in general.

these are the current settings for the ort-server:

so pull requests are required as otherwise the whole things would be pointless:

required prerequisite for a merge queue, without a pull request a merge queue does not make sense
eca status checks are also only applied when a pull request is created
status checks is what we ultimately want so thats fine as well

Thanks Thomas! A new status check gets generated for required actions in merge queues as well, which is what had blocked this. I think it has to do with the fact that it technically has a different SHA in queues, which is how status checks are tracked/maintained.

fyi: otterdog has now the ability to list and approve requested permission changes from installed GitHub Apps, see the example below:

tn@proteus:~/workspace/eclipse/otterdog$ otterdog review-permissions OtterdogTest -c otterdog-test.json -a otterdog-app -g

Reviewing permission updates for app installations:

Organization OtterdogTest[id=OtterdogTest]

  app['otterdog-app'] {
    checks                            = "read"
    codespaces                        = "write"
  }

  Approve requested permissions?
    Do you want to continue? (Only 'yes' or 'y' will be accepted to approve)

  Enter a value: n

  Approval cancelled.

So for at least otterdog repos, we'll be able to easily update the permissions, that's handy!

Other than that, I was able to get the flow to merge with the above settings. Once they are confirmed, we can plan the rollout of these permissions and functionality.

so while running it for all orgs I noticed this:

Organization ee4j.jakartaee-platform[id=jakartaee]

  app['eclipse-eca-validation-v2-testing'] {
    merge                             = "read"
  }

Organization modeling.emf[id=eclipse-emf]

  app['eclipse-eca-validation'] {
    metadata                          = "read"
  }

V2 can be ignored as that's the testing app. To be honest, we can and probably should remove it from the jakarta ee org

@netomi @fgurr now that we have the general lay of how to use this, how do we want to roll this out to users? Adding an operation to otterdog to auto-accept permission updates on the ECA app would likely be a good start to reduce effort, but that doesn't cover all orgs yet I don't think.

Not accepting permissions wouldn't break anything that currently exists for projects, as this just adds the ability to have the ECA app as a "required check" for merge queues. With it being non breaking, we could likely approach this in a number of ways!

Once we figure that out, I can go into the app and add the new permission and event to the general app and start the process!

so once you have updated the requested permissions for the eca-validation app we can run otterdog to accept these permissions for all orgs that is has been installed to.

The remaining orgs can then be manually approved using the webmaster account. That should be rather simple as not many will be remaining and you see that in the UI interface afaict.

@fgurr let me know if you have any comments/questions/concerns! If there are none, we can move forward with this soon and add this permission set.

@cguindon had recommended that the handbook get updated, so I will look into that. I'm thinking something along the lines of the following added under the Github section, WDYT?

Merge Queue support

As of September 2024, the Eclipse Foundation has added merge queue support for projects hosted on GitHub. To enable these rule sets on your project, either reach out to the Release team on the [GitLab Helpdesk], or if you have permissions on the repository, a rule set may be added with the following settings, at minimum:

Require merge queue checked

Require all queue entries to pass required checks unchecked

Require a pull request before merging checked

Require status checks to pass checked

eclipsefdn/eca added as a required check

@malowe Projects have permission to set these settings themselves?

@malowe Projects have permission to set these settings themselves?

TBH, I'm fairly sure they shouldn't, but I added that in case. I know there are some cases where people get elevated permissions long term in some repos, and I want it more documented that this is how we support it in case someone does.

@fgurr let me know if you have any comments/questions/concerns! If there are none, we can move forward with this soon and add this permission set.

FWICTILGTM (From what I can tell, it looks good to me).

@malowe Projects have permission to set these settings themselves?

I assume, projects can set this via Otterdog.

Do we want to document this in the project handbook? I'm not the one that drives those conversations so I don't know what would be most useful in this case.

@malowe is this change now merged and operational so that we can re-add the ECA check when a merge queue is enabled?

I don't think the permission has been added to the app yet, but the changes in the API are done yes. The only thing stopping it now is that the checks don't trigger calls to the API AFAIK.

I've updated the app now, apologies for the delay. Let me know if there are any issues!

Update is running:

tn@proteus:~/workspace/eclipse/EclipseFdn/otterdog-configs$ otterdog review-permissions -a eclipse-eca-validation -g -f

Reviewing permission updates for app installations:

Organization adoptium[id=adoptium]

  app['eclipse-eca-validation'] {
    merge queues                      = "read"
  }

  requested permissions approved.
...

I dont have all things required to authenticate as the eca validation app. If I would have that I could easily get the list of installations and the permissions they have already granted, then we can identify the orgs that need to be handled manually.

Here is the Rest API call that can be used for that: https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#list-installations-for-the-authenticated-app

all otterdog enabled orgs should have approved this new permission now.

As this has been enabled everywhere with some basic testing on our end, I'm going to close this out! Thanks for the support in getting this live @netomi!

closed

@malowe @netomi Thanks for working on this!

I have just checked our repository (https://github.com/eclipse-apoapsis/ort-server) and so far the ECA check is not executed by the merge queue:

Do I first have to make the ECA check required again or should this already work?

I'm able to see it now at least on the latest PR, and I haven't touched anything yet. Which PR are you seeing this on?

So I checked the status of the latest commit that was merged via the merge queue, and it has an eca check attached:

so I would assume that the GitHub UI just shows required checks, but worth a try.

Ok, I will give it a try tomorrow and tell you if it works.

Add an additional webhook event to trigger the eca check

Designs

Child items ...

Activity

Merge Queue support