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

Reimplement branch protection rules that had to be removed #260

Closed
3 tasks
emteknetnz opened this issue May 31, 2024 · 8 comments
Closed
3 tasks

Reimplement branch protection rules that had to be removed #260

emteknetnz opened this issue May 31, 2024 · 8 comments
Assignees

Comments

@emteknetnz
Copy link
Member

emteknetnz commented May 31, 2024

The tag ruleset create restriction was removed in #259 so that gha-tag-release could function and automatically tag patch releases as part of gha-ci

Other rules were also causing problems:

Investigate if it's possible to reimplement that restriction by somehow adding bypass permisions (repository admin) to the github-actions user. Or alternatively find another method.

Acceptance criteria

  • A way to allow the github-action user to bypass the tag ruleset is implemented for gha-tag-release on gha-ci
  • Module standardiser is updated to add the tag create restriction back to the ruleset
  • Module standardiser is run to update all supported repos
@emteknetnz emteknetnz changed the title Reimplment tag rulset create restriction Reimplement tag rulset create restriction May 31, 2024
@emteknetnz emteknetnz changed the title Reimplement tag rulset create restriction Reimplement tag ruleset create restriction May 31, 2024
@maxime-rainville
Copy link
Contributor

We're not sure how todo this. This card is a bit "spikish".

If it looks like achieving this is impossible or extremely difficult, report back to the team.

@GuySartorelli GuySartorelli changed the title Reimplement tag ruleset create restriction Reimplement rulesets that had to be removed Jun 4, 2024
@GuySartorelli GuySartorelli changed the title Reimplement rulesets that had to be removed Reimplement back protection rules that had to be removed Jun 4, 2024
@GuySartorelli GuySartorelli changed the title Reimplement back protection rules that had to be removed Reimplement branch protection rules that had to be removed Jun 4, 2024
@GuySartorelli GuySartorelli self-assigned this Aug 6, 2024
@GuySartorelli
Copy link
Member

Looks like there's no way to give GitHub actions the ability to bypass branch protections rules using the built-in token. There's been a lot of discussion about it, the main threads being https://github.com/orgs/community/discussions/13836 and https://github.com/orgs/community/discussions/25305.

It seems like the most robust way to do this would be by creating a new GitHub App, granting that app the ability to bypass branch protections, and using a token from that app to perform the required actions.
We've done something similar to this in https://github.com/silverstripe/gha-add-pr-to-project/.

The scenarios where we currently need to be able to bypass branch protection rules seem to be:

  1. Creating a new tag
  2. Deleting an existing tag
  3. Pushing commits directly after a merge-up

We could have an app for each of these to limit the surface area for each permission, if we wanted to - though IMO that's overkill, and having a single app for all of these should be fine.

Note that creating a tag for gha-* repos currently spins off another workflow, and since we'll be using a token that isn't associated with GitHub Actions, we'll need to remove our workaround of manually triggering that spinoff workflow. We'll similarly need to make sure we avoid an infinite loop where we create a tag, which spins off a v1 tag generation, etc, which spins off the v1 tag generation, etc etc.
Simply making sure that workflow doesn't recreate the tag if it doesn't need to should be sufficient.

Similarly, on push the CI workflow will be triggered, so we'd want to remove our workaround that manually triggers the CI workflow after merge-ups, to avoid double-ups. There's no risk of infinite loops in this case because merge-ups isn't triggered in any way by the CI workflow.

@GuySartorelli
Copy link
Member

To clarify what I mean about removing overrides:

Right now, we have some GitHub actions that push a tag, or push code commits.
If a human user was performing those actions, it would trigger GitHub Actions workflows that are triggered by pushed tags or pushed commits - but GitHub actions cannot trigger more GitHub actions like this. Our workaround was to manually dispatch the workflows which would ordinarily be triggered, e.g. gha-merge-up pushes commits, so it manually dispatches the ci.yml workflow which would otherwise usually be triggered by the push.

If we do what I've suggested in the comment above and use a different token for pushing those commits, that token will not be associated with GitHub Actions, so it will naturally trigger the ci.yml workflow. In that case we won't want to also manually dispatch ci.yml from within gha-merge-up.

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 7, 2024

In that case we won't want to also manually dispatch ci.yml from within gha-merge-up.

If we were to do what's proposed, I think we would still need to manually dispatch. I believe the workaround was in place not because of token limitations, instead because one github workflow will not trigger a subsequent workflow via the 'push' event in order to prevent infinite loops. We work around that by curling the GitHub API to trigger it via the 'workflow_dispatch' trigger

@GuySartorelli
Copy link
Member

one github workflow will not trigger a subsequent workflow via the 'push' event in order to prevent infinite loops

That's true, but based on the conversations I've seen with people doing what I've proposed, that's based on the token. It doesn't trigger subsequent workflows because the token used to perform the push belongs to GitHub Actions.
But if that turns out to be wrong we can just keep the workaround, no harm done.

@emteknetnz
Copy link
Member Author

Yeah I guess it would just double trigger which shouldn't cause any issues

Seems like the github app a.k.a the permission holder is the correct way to go about implementing this

@GuySartorelli
Copy link
Member

I realised that this isn't viable - adding the app to the organisation would mean anyone with write access to any repo in this org would be able to use the app (and its permissions) to affect repos they shouldn't be allowed to affect.

Chatted with Steve offline - for now we'll proceed with the branch protection rules as they currently are. This means we will still be a bottleneck for merging community PRs, but we can invite community members to explicitly review pull requests. Once the PRs are reviewed, they can then escalate to us to have a last look and click merge.

This is a stepping stone in the right direction, if nothing else. I'll raise a card to update the docs to reflect the new position.

@GuySartorelli
Copy link
Member

For future reference a new option has emerged: https://github.com/orgs/community/discussions/25305#discussioncomment-10728028

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants