Enable branch protection to prevent accidental pushes to master about apps HOT 23 CLOSED

+1 on protected branches.

I also have git configured like

❯ g remote -vv                                                                      
origin	[email protected]:durandom/operate-first.github.io.git (fetch)
origin	[email protected]:durandom/operate-first.github.io.git (push)
upstream	https://github.com/operate-first/operate-first.github.io.git (fetch)
upstream	https://github.com/operate-first/operate-first.github.io.git (push)

Note, my upstream is a https URL, which I cannot push to, but read from.
And "Merge UPstream" is aliased to:

❯ g mup --help
'mup' is aliased to '!git checkout master && git fetch upstream && git merge upstream/master && git push && git checkout -'

from apps.

I would strongly suggest 2. The only time we should be rushing PRs through is to resolve outages, etc.

from apps.

I've put the following protections to prevent direct pushes to master:

I've also applied similar rules to the following repos:

• toolbox
• argocd-apps
• continuous-deployment
• odh-moc-support
• sre

from apps.

relevant discussion here

from apps.

@anishasthana maybe the CODEOWNERS with * wildcard are messing with it?

https://docs.github.com/en/free-pro-team@latest/github/creating-cloning-and-archiving-repositories/about-code-owners

from apps.

I think we should stop using the /approve command. I don't see any other way around it, afaik a bot cannot github approve on another user's behalf. Open to suggestions.

from apps.

@HumairAK do you want to add main branch as well?

from apps.

Also, instead of setting this for individual repos, I think this can be done org wide by an admin.
@durandom would that be okay?

from apps.

my bad, it looks like this can't be done on the org level :(

from apps.

I'll leave this issue up for a couple of days in-case others have input/feedback on the protections I've added above.

from apps.

I welcome this! 👍

from apps.

Thanks folks, what about some feedback/discussion on the number of approvers. Right now it's 1. It helps get PR's in faster, but the cons are obvious. Less eyes, means more likely a problematic PR gets through. Upping this number can increase the rigor with which we merge our PR, but slow down and potentially block people as well (not always a bad thing).

from apps.

I'm a ++ for having at least 2 approvers, as it allows more people the chance to review PRs

from apps.

I agree with that. That goes hand in hand with the need of having more people reviewing and more actively - actually trying out the things and not just skim through the code. Also, many folks can gain much needed experience with unfamiliar areas of our codebase, when they get the chance to start reviewing it. Everybody should be encouraged to review and should get the time they need.

That reminds me @anishasthana , did you have any luck with the research on round robin review assignments?

from apps.

Not yet, I had some other work jump up and take priority. I'm wondering if Sesheta and Github are at odds with each other right now -- it seems like all owners are assigned by Github for reviews as of now. Probably some other setting I'm missing.

from apps.

Closing this issue, approvers were upped to 2 for the repositories listed above. If there's anything else that needs to be discussed, feel free to re-open.

from apps.

I think it started a bit of a war between bots, see here: #51 (comment)

We may want to think about this - either solve it somehow or stop using the /approve command. 😕

@goern @anishasthana any thoughts?

from apps.

@HumairAK @durandom ^

from apps.

@harshad16 ^

from apps.

I m a bit confused on the front, as I was under the impression branch protection of 2 approvals, didn't stop prow from merging stuff with one approval. If it does do let us know.

maybe stop using /approve command if there is such branch protection of 2 approvals seems to be the way to go.

from apps.

@harshad16 It works for the operate-first repositories. I think I know what the problem @anishasthana was having in the aicoe-cd repo (here). In the aicoe-cd repo, Sesheta is admin, so when applying branch protection, this check box needs to be enabled:

The aicoe-cd repo does not have this checked, so the restriction does not apply to sesheta, and will merge it based on the label. We have checked this box for the operate-first repo, so it seems to be working. This is an example branch protection rule for this repo:

from apps.

Thanks for this information @HumairAK .

from apps.

Closing this, if there's more to be discussed feel free to re-open.

from apps.