Skip to content

Do not enforce branch protection in Headlamp #34950

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

Conversation

joaquimrocha
Copy link

In Headlamp we only have one branch where preventing force push makes sense. That's the main one.
All the rest we would like to force push and even to delete them since lots of branches were carried over from when we moved the repo and they are WIP or already merged ones.

I am not sure if this config is the correct way. I did read the source code for the prow and seems like it should work, but let me know otherwise.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: joaquimrocha
Once this PR has been reviewed and has the lgtm label, please assign dims for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Welcome @joaquimrocha!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 9, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @joaquimrocha. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 9, 2025
@cblecker
Copy link
Member

cblecker commented Jun 9, 2025

We don't typically allow this, as we enforce things like the CLA on all branches. Are you just trying to purge some branches?

@joaquimrocha
Copy link
Author

The way we were developing when we were a CNCF Sandbox project was that we'd just create new branches in the main repo (development directly in the main repo, which is a common use-case I believe), and when these were merged, we'd delete them. We had lots of WIP and left over branches which we were supposed to spring clean for a while but hadn't done it yet.

With the current enforcement, I'd have to go branch by branch in Github (or use the rest API) to stop the protection in order to delete it. Besides, we had to change the way we were developing and lots the fact that anyone in the team could just push to existing PR branches. To overcome this we even created a new fork of Headlamp so we all share the ability to push. Yet, the CI actions we got keeps failing because if something goes wrong and we re-run it, the temporary branches already exist and as a result every release is being problematic.

I believe enforcing the CLA for PRs is fine but we are not allowed to force push nor delete regardless of the branch and I think this only makes sense for the main branch in our case. What is the rationale behind preventing force-push/delete on "non-static" branches?

@cblecker
Copy link
Member

We have done a PoC previously on "feature branches" to allow for collaborative development in the main repo, and it ended up not being a pattern that wasn't sustainable so support was discontinued.

The typical development flow is to have a personal fork of the repo, and then PR from that personal fork into the kubernetes project repo.

/ok-to-test
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 13, 2025
@joaquimrocha
Copy link
Author

joaquimrocha commented Jun 16, 2025

The typical development flow is to have a personal fork of the repo, and then PR from that personal fork into the kubernetes project repo.

I understand this from external contributors (external to a core team). But it does get in the way for a team, e.g. if we work in the same team and you open a PR from your personal fork, I cannot add commits to it.
I do understand and respect that this is what kubernetes/kubernetes and many other projects use, but since we have the ability to have it per repo, can projects decide on how they handle these? i.e. what is the disadvantage of allowing projects to decide whether they have branch protection or not (since their teams are the ones handling the day to day development anyway)?

@BenTheElder
Copy link
Member

FWIW ...

But it does get in the way for a team, e.g. if we work in the same team and you open a PR from your personal fork, I cannot add commits to it.

You can set your fork to allow others to collaborate in a branch, Kubernetes contributors do this all the time because we're not going to let the 1000s of contributors create upstream branches and we need to be pretty careful with the core repo especially...

https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-access-to-your-personal-repositories/inviting-collaborators-to-a-personal-repository

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/managing-a-branch-protection-rule

Upstream branches are "official" and people can construe them as reflecting the project, so aside from attempting to ensure things like CLA it's just better to do this externally.

You can also do: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork to allow repo maintainers to push to your PR branch specifically, this is a checkbox when creating a PR.

@joaquimrocha
Copy link
Author

FWIW ...

I have not suggested projects should open the branches to 1000s of contributors. I suggested the Headlamp project remove the branch protections (except for the one branch that is valuable in our project: main) so the (very small) core team uses it for contributing, as it already has its permissions set in the mechanisms that Kubernetes promotes.

We also already have a fork as I said, but having a fork like that means that now I have to manage the same team in two places: the official repo + the fork. Besides not solving the problem we currently have which is releases failing because of this branch protection.

I wish projects could decide to change the default policies when security is not a concern. Nonetheless, I will abide and will close this PR + open one that removes the protection for individual branches.

Thanks.

@joaquimrocha
Copy link
Author

@BenTheElder , BTW this is the new one:
#34998

@BenTheElder
Copy link
Member

BenTheElder commented Jun 20, 2025

I have not suggested projects should open the branches to 1000s of contributors.

Never said you did.

My point was that we of course can't for Kubernetes/Kubernetes, and we've found that there are plenty of alternatives.

I wish projects could decide to change the default policies when security is not a concern.

It's as much compliance (IE enforcing the CLA) as security. Our projects have to abide by the CLA and also policies like https://github.com/cncf/foundation/blob/40bdbe8c908df30ab948e195049b77a404c81a89/policies-guidance/allowed-third-party-license-policy.md

Where possible enforcement is automated, given the 100s of repos we're hosting.

@joaquimrocha
Copy link
Author

No worries. The new PR should hopefully allow us to skip the problems for CI-created branches while keeping the protections for the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config Issues or PRs related to code in /config cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants