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

Chore: enable branch protection eclipse-tractusx.github.io main branch #43

Merged

Conversation

FaGru3n
Copy link
Contributor

@FaGru3n FaGru3n commented Nov 30, 2023

Description

  • enable branch protection for eclipse-tractusx.github.io main branch
  • approval 2 committers needed

fixes eclipse-tractusx/eclipse-tractusx.github.io#515

FYI:
@stephanbcbauer @mhellmeier @Siegfriedk @danielmiehle

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

@FaGru3n FaGru3n requested review from a team as code owners November 30, 2023 08:22
@FaGru3n FaGru3n marked this pull request as draft November 30, 2023 08:22

This comment has been minimized.

@FaGru3n FaGru3n changed the title Xhore: enable branch protection eclipse-tractusx.github.io main branch Chore: enable branch protection eclipse-tractusx.github.io main branch Nov 30, 2023
@FaGru3n
Copy link
Contributor Author

FaGru3n commented Nov 30, 2023

@eclipse-tractusx/eclipsefdn-security will this modified for the hole org?

just want to enable this for eclipse-tractusx.github.io

Organization eclipse-tractusx[id=eclipse-tractusx]
--- original
+++ canonical
@@ -163,6 +163,7 @@
branch_protection_rules: [
orgs.newBranchProtectionRule('main') {
dismisses_stale_reviews: true

  •    required_approving_review_count: 2
       }
    

Copy link
Contributor

@SebastianBezold SebastianBezold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 2 reviews should be fine.
Our Code Reviews sections proposes a two step approach anyways. A "business" review and another one from a committer, that is anyway needed.

branch_protection_rules: [
orgs.newBranchProtectionRule('main') {
dismisses_stale_reviews: true,
required_approving_review_count: 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would highly vote for 1 required approving reviewer. 2 would be very hard for small teams. But maybe we can recommend 2 reviewers (in a TRG?) but enforce 1 reviewer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this PR is not introduceing branch protection and required review count to the GH Org but to the eclipse-tractusx/eclipse-tractusx.github.io repo only, I would like to require 2 reviewers. For this repository enough stakeholders should be available doing reviews.

Maybe we should also think about to introcude .github/CODEOWNERS (for eclipse-tractusx/eclipse-tractusx.github.io). Code owners are automatically assigned to new PRs:
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of a .github/CODEOWNERS file would be pretty useful. This would also solve the problem of Issues and PRs not associated. We could divide it for every section (like KIT etc.).

@netomi
Copy link
Contributor

netomi commented Nov 30, 2023

@eclipse-tractusx/eclipsefdn-security will this modified for the hole org?

no, the setting will only be used for the repo you defined it for, see also the summary:

image

btw. a required_approving_review_count of 2 is the default, so you dont have to specify it, unless you want to set it to a different value.

btw. other projects defined some custom branch protection rules that can then be easily applied for a repo like that:

https://github.com/eclipse-set/.eclipsefdn/blob/main/otterdog/eclipse-set.jsonnet#L3

Maybe of interest for tractus-x as well.

@FaGru3n
Copy link
Contributor Author

FaGru3n commented Nov 30, 2023

I think 2 reviews should be fine. Our Code Reviews sections proposes a two step approach anyways. A "business" review and another one from a committer, that is anyway needed.

that was my intention 1 vote from "business" and 1 from a committer

after reading the discussion with @carslen and @mhellmeier @SebastianBezold

we have:

and with the hint from @netomi i would clear this section that we can use the default setting for branch protection.

Think will wait for a project lead vote, because of the idea with "business" votes.. 😃

@ participants, thanks for your feedback.

@Siegfriedk
Copy link
Contributor

@FaGru3n lets do it. it will also be very good for proper coop

@stephanbcbauer
Copy link
Member

@FaGru3n sounds good. Thumbs up

Copy link

Diff for b500deb:
Printing local diff for configuration at '/home/runner/work/.eclipsefdn/.eclipsefdn/otterdog-configs/otterdog.json'

Actions are indicated with the following symbols:
+   create
!   modify
!   forced update
-   delete

Organization eclipse-tractusx[id=eclipse-tractusx]
  there have been 41 validation infos, enable verbose output with '-v' to to display them.

+   add branch_protection_rule[pattern="main", repository="eclipse-tractusx.github.io"] {
+     allows_deletions                                         = false
+     allows_force_pushes                                      = false
+     blocks_creations                                         = false
+     bypass_force_push_allowances                             = []
+     bypass_pull_request_allowances                           = []
+     dismisses_stale_reviews                                  = true
+     is_admin_enforced                                        = false
+     lock_allows_fetch_and_merge                              = false
+     lock_branch                                              = false
+     pattern                                                  = "main"
+     require_last_push_approval                               = false
+     required_approving_review_count                          = "2"
+     required_status_checks                                   = [
+       "eclipse-eca-validation:eclipsefdn/eca"
+     ],
+     requires_code_owner_reviews                              = false
+     requires_commit_signatures                               = false
+     requires_conversation_resolution                         = false
+     requires_deployments                                     = false
+     requires_linear_history                                  = false
+     requires_pull_request                                    = true
+     requires_status_checks                                   = true
+     requires_strict_status_checks                            = false
+     restricts_pushes                                         = false
+     restricts_review_dismissals                              = false
+   }
  
  Plan: 1 to add, 0 to change, 0 to delete.
Showing diff to a canonical version of the configuration at '/home/runner/work/.eclipsefdn/.eclipsefdn/otterdog-configs/otterdog.json'

Organization eclipse-tractusx[id=eclipse-tractusx]

@FaGru3n
Copy link
Contributor Author

FaGru3n commented Nov 30, 2023

FYI:

  • required_approving_review_count                          = "2"
    

@FaGru3n FaGru3n marked this pull request as ready for review November 30, 2023 14:39
@FaGru3n
Copy link
Contributor Author

FaGru3n commented Nov 30, 2023

we have:

@netomi
Copy link
Contributor

netomi commented Nov 30, 2023

let me know when you have agreed on a number of approvals.
Most projects use 1, which I find reasonable to assure that reviews are made, but progress is not blocked by having to chase reviewers to approve.

@Phil91
Copy link
Member

Phil91 commented Nov 30, 2023

@FaGru3n we might set it to 1 for other repos, but for this repo i think 2 reviewers are fine 👍

@netomi
Copy link
Contributor

netomi commented Dec 4, 2023

Looks like this has been stabilized, lets get this merged, you can easily change if needed ofc.

@netomi netomi merged commit af6fef2 into eclipse-tractusx:main Dec 4, 2023
3 checks passed
@netomi
Copy link
Contributor

netomi commented Dec 4, 2023

change is live.

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

Successfully merging this pull request may close these issues.

Enable branch protection for our web-page
8 participants