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

How to use with branch protection? #33

Open
damemi opened this issue Aug 3, 2020 · 15 comments
Open

How to use with branch protection? #33

damemi opened this issue Aug 3, 2020 · 15 comments

Comments

@damemi
Copy link

damemi commented Aug 3, 2020

We have tried enabling this action in https://github.com/kubernetes-sigs/descheduler (see kubernetes-sigs/descheduler#298), but run into the problem that gh-pages has branch protection enabled (to verify the linuxfoundation CLA check).

This check is required for all Kubernetes repositories. Other projects have also hit this problem (kubernetes/autoscaler#3393 (comment)). As shown in that comment, this can be addressed by using 3rd party chart-releaser actions and taking steps like having them open PRs.

However given that this is the official chart-releaser action it should be usable by Kubernetes projects rather than having to rely on 3rd party scripts. Is there a way to configure this to work with branch protection?

If not, I believe one fix would be to update this action to optionally decouple the actual Git push (which is baked into the action) so that it can be replaced with a more configurable Pull Request action (like here, which is using a 3rd party action: https://github.com/kubernetes/dashboard/pull/5311/files#diff-29eba779142f216406df01a921ea49c3R59-R66)

@gjtempleton
Copy link

gjtempleton commented Aug 3, 2020

Beat me to it.

I intend to raise a PR to the autoscaler repo's workflows to work around as above for now, but would love to be able to return to using the official action.

@damemi
Copy link
Author

damemi commented Aug 3, 2020

@gjtempleton please keep us updated on what your workarounds are! I would love to continue using this official Helm action if possible, but the automated git push will cause it to fail (I think)

@unguiculus
Copy link
Member

unguiculus commented Aug 9, 2020

Pushing to the gh_pages branch is currently part of the chart-releaser action only. I guess it would be best to directly implement this in chart-releaser. We could add options to push (--push, default false) or create a PR (--pr, default false) to the index command. Only one of the two options should be allowed. Comments?

@unguiculus
Copy link
Member

I started working on this.

@scottrigby
Copy link
Member

Now that GitHub allows pages to build and deploy from any branch (no longer just master or gh-pages), maybe we should already add an optional github_pages_branch configuration to helm/chart-releaser-action to specify the branch used for GitHub pages?

This would also allow for the OP's use case where the GitHub pages branch is branch protected requiring PR review rules. For this use case, users could create a non-protected branch (example: gh-pages-review) intended for pull requests to whatever the GitHub pages branch (example: gh-pages). Just configure the github_pages_branch option to gh-pages-review so helm/chart-releaser-action pushes changes to that branch, then add a create pull request action step to your workflow to open up a PR against your GitHub pages branch (gh-pages-review -> gh-pages) for manual review.

I'm suggesting this in the action because helm/chart-releaser doesn't enforce using GitHub pages at all (it is just a recommendation). What do you think?

@davidkarlsen
Copy link
Member

Both comments are reasonable:

  • configurable branch name (default gh_branches)
  • pr or push, with configured branch as target, for a pr you need a pr branch name as well, which I guess can just be the current branch name.

@unguiculus
Copy link
Member

@scottrigby It's true that chart-releaser doesn't enforce using GitHub pages but it is actually meant to be used for this. However, with the additional flags I suggested, it would still be optional. In the end, I think it makes sense if chart-releaser and chart-releaser-action support the same features. Currently, the action does more. I wouldn't want to rely on a separate action that creates a PR while chart-releaser-action itself is able to do a direct push.

I think we should bake that functionality into chart-releaser and also support a configurable branch for GitHub pages. We would have trouble using a fixed branch to push changes to in order to use this branch to create a PR from. What if this branch already exists or if we have concurrent changes? We will have to generate a random branch name. chart-releaser should be able to handle that.

@damemi
Copy link
Author

damemi commented Aug 9, 2020

Both sound like great improvements to the action, though for our use case we would prefer the PR option (since every branch in the Kubernetes org needs to have branch protection, the option to use a different branch won't solve our problem).

Thanks for working on this!

@unguiculus
Copy link
Member

@damemi In order to create a PR, you need to push to a branch first. How would you do this, if every branch is protected?

@damemi
Copy link
Author

damemi commented Aug 10, 2020

@unguiculus that's a good question, and I actually didn't think of that (my mistake). But based on what they're trying in autoscaler, the branch becomes protected after the first push, so pushing to a new branch isn't a problem like I thought.

@stevehipwell
Copy link

@damemi it looks like the descheduler chart release is working correctly without the (undocumented) --pr flag, did you have to do anything for this to work? I've been seeing a protected branch block on gh-pages for both Metrics Server and External DNS recently which has been breaking the releases.

@damemi
Copy link
Author

damemi commented Nov 8, 2021

@stevehipwell I have been manually modifying the branch protection to allow for the release to run. getting the release to run automatically (with branch protection) is still a TODO for us. is the --pr flag something that can address this?

@stevehipwell
Copy link

@damemi I opened kubernetes/test-infra#24222 to fix this for External DNS and Metrics Server.

@damemi
Copy link
Author

damemi commented Nov 8, 2021

@stevehipwell oh awesome, thanks for the tip! I'll give that a try

@stevehipwell
Copy link

@damemi you're welcome.

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

No branches or pull requests

6 participants