test-infra
test-infra copied to clipboard
kubernetes
Support GitHub Apps in branchprotector
/kind enhancement /sig testing
Closes https://github.com/kubernetes/test-infra/issues/24530
The main motivation of this PR is that we run TIde as a GitHub App. Currently, we have to add it manually to branch protection rules every time we create a new release branch. This PR is going to change that. It introduces an apps property to restrictions
branch-protection:
orgs:
foo:
repos:
bar:
enforce_admins: true
restrictions:
users: []
apps:
- my-prow-app # still allow this app to push/merge
teams: []
Apps restrictions are modified only if a --enable-apps-restrictions=true flag is set. Its default value is false.
This is done because apps cannot be specified in current prow configurations and I assume that there are some GitHub Apps added manually to branchprotector controlled branches. Those should not be removed silently when these changes become active.
When the flag is set apps behave the same way as teams and users.
We might remove the flag future when people had the chance to adapt their branch protection settings.
If verify-restrictions=true branch protector checks whether the apps are installed on GitHub Org level and requests write permissions for content.
https://docs.github.com/en/rest/orgs/orgs#list-app-installations-for-an-organization
Unlike Restrictions DismissalRestrictions do not support apps (see github documentation), so there are own types now.
@oliver-goetz: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them.
In response to this:
/kind enhancement /sig testing
Closes https://github.com/kubernetes/test-infra/issues/24530
The main motivation of this PR is that we run TIde as a GitHub App. Currently, we have to add it manually to branch protection rules every time we create a new release branch. This PR is going to change that. It introduces an
appsproperty torestrictionsbranch-protection: orgs: foo: repos: bar: enforce_admins: true restrictions: users: [] apps: - my-prow-app # still allow this app to push/merge teams: []Unlike
usersandteamsapp restrictions are not removed from GitHub branch protection whenappsare not specified in the configuration. They have to be set explicitly to an empty listapps: []that branch protector is removing them. This is done becauseappscannot be specified in current prow configurations and I assume that there are some GitHub Apps added manually to branchprotector controlled branches. Those should not be removed silently when these changes become active. We might harmonize that in the future when people had the chance to adapt their branch protection settings.If
verify-restrictions=truebranch protector checks whether the apps are installed on GitHub Org level and requestswritepermissions forcontent. https://docs.github.com/en/rest/orgs/orgs#list-app-installations-for-an-organizationUnlike
RestrictionsDismissalRestrictionsdo not support apps (see github documentation), so there are own types now.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Hi @oliver-goetz. Thanks for your PR.
I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@alvaroaleman @cjwagner could you kindly take a look when you have some time?
@alvaroaleman @cjwagner friendly reminder for reviewing this 😄 /auto-cc
Will a user be able to dump/refresh their current config to add in apps that had not been specified in the config, but were manually added? That would make the onboarding strategy nice here and parallel what we did for other new fields.
@oliver-goetz: Adding label: do-not-merge/blocked-paths because PR changes a protected file.
Reasons for blocking this PR:
[All Prow documentation (Markdown files) have moved to https://github.com/kubernetes-sigs/prow as part of https://github.com/kubernetes-sigs/prow/issues/4.]
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@oliver-goetz: Adding label:
do-not-merge/blocked-pathsbecause PR changes a protected file.
I'm not sure what I'm supposed to do here?
- revert changes in k/test-infra and apply them in k-s/prow
- apply the same changes in both repos
- do nothing because someone/a bot will do it at some point
- something else 😄
Will a user be able to dump/refresh their current config to add in apps that had not been specified in the config, but were manually added? That would make the onboarding strategy nice here and parallel what we did for other new fields.
I did not plan a dump function so far. Are you suggesting a generic --dump-config flag or something like a --dump-app-restrictions-config flag for the migration use case?
The function prow branch protector config -> github branch protection is a non-invertible function, because you are able to maintain the branch protection policies on config, org, repo and branch levels. Thus, there could be different branch protector config settings resulting in the same github branch protection rules.
We could try estimating what could be the "most useful" config, but this could be complex and there is no guarantee getting the desired output.
LMK which you'd prefer - if we merge this behind a feature flag, such that people must opt-in to use it (and dump their current, manual config first), we can have the logic handling empty slices be the same for this field and the rest of the fields. There's some merit to keeping the UX of config here similar. Barring that, though, the logic here looks reasonable.
A feature flag looks cleaner. I'll build one 😄
Hm - I could have sworn there was a dumping functionality, but it's been so long since I was in this code - perhaps that was some other GH automation tool. I think a feature flag + the verification behavior would gloss over the invertability problem you mentioned, though, so the user flow would be to keep verfiying until the provided config matched preexisting rules, then turn on the feature flag?
I created a feature flag as command line argument enable-apps-restrictions. When it's active apps restrictions now behave the same way as teams and users.
The only config dump function I found in a short research is peribolos, but this is different from what we need here.
@listx what am I supposed to do regarding the do-not-merge/blocked-paths label?
Sorry for the delay; I was on leave. I've removed the label for now.
If you're wondering why we have the label in the first place, there's a documentation migration effort to move contents out of here into https://github.com/kubernetes-sigs/prow. Any help there would be greatly appreciated.
(Looks like the PR needs rebase so once you get it rebased I can remove the label again.)
Thanks for coming back to this PR @listx @stevekuznetsov. @oliver-goetz is currently on leave as well and will be back on Monday :)
@oliver-goetz: Adding label: do-not-merge/blocked-paths because PR changes a protected file.
Reasons for blocking this PR:
[All Prow documentation (Markdown files) have moved to https://github.com/kubernetes-sigs/prow as part of https://github.com/kubernetes-sigs/prow/issues/4. Instructions for updating existing docs are at https://docs.prow.k8s.io/docs/contribution-guidelines/#updating-existing-docs.]
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Thanks @listx 😄 The PR is rebased now. Could you please remove the label again?
@stevekuznetsov I'm back from vacation and just rebased the PR.
@oliver-goetz: Adding label: do-not-merge/blocked-paths because PR changes a protected file.
Reasons for blocking this PR:
[All Prow documentation (Markdown files) have moved to https://github.com/kubernetes-sigs/prow as part of https://github.com/kubernetes-sigs/prow/issues/4. Instructions for updating existing docs are at https://docs.prow.k8s.io/docs/contribution-guidelines/#updating-existing-docs.]
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Maybe, we can be faster than the changes this time 😄
@listx @stevekuznetsov could you please remove the label and approve?
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: oliver-goetz, stevekuznetsov
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~prow/OWNERS~~ [stevekuznetsov]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment