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

Add Repository for kuksa-java-sdk #33

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

wba2hi
Copy link
Contributor

@wba2hi wba2hi commented Dec 11, 2024

No description provided.

@wba2hi wba2hi requested a review from a team as a code owner December 11, 2024 14:22
Copy link

This is your friendly self-service bot.

Thank you for raising a pull request to update the configuration of your GitHub organization.
You can manually add reviewers to this PR to eventually enable auto-merging.

The following conditions need to fulfilled for auto-merging to be available:

  • valid configuration
  • approved by a project lead
  • does not require any secrets
  • does not update settings only accessible via the GitHub Web UI
  • does not remove any resource
Otterdog commands and options

You can trigger otterdog actions by commenting on this PR:

  • /otterdog team-info checks the team / org membership for the PR author
  • /otterdog validate validates the configuration change
  • /otterdog validate info validates the configuration change, printing also validation infos
  • /otterdog check-sync checks if the base ref is in sync with live settings
  • /otterdog merge merges and applies the changes if the PR is eligible for auto-merging (only accessible for the author)
  • /otterdog done notifies the self-service bot that a required manual apply operation has been performed (only accessible for members of the admin team)
  • /otterdog apply re-apply a previously failed attempt (only accessible for members of the admin team)

Copy link

This is your friendly self-service bot.

The author (wba2hi) of this PR is associated with this organization in the role of MEMBER.

Additionally, wba2hi is a member of the following teams:

This comment has been minimized.

This comment has been minimized.

@wba2hi
Copy link
Contributor Author

wba2hi commented Dec 11, 2024

Anything I have to do / can do here: #33 (comment)?

This comment has been minimized.

@netomi
Copy link
Contributor

netomi commented Dec 11, 2024

should be fine now.

@wba2hi
Copy link
Contributor Author

wba2hi commented Dec 11, 2024

Thanks for your (always) quick support and help @netomi :)

@netomi
Copy link
Contributor

netomi commented Dec 11, 2024

np, environments are annoying, as they are kept being auto-created by GitHub. We should probably remove them from the config to avoid problems with them. It is useful sometimes to be able to configure them though.

@@ -416,5 +416,19 @@ orgs.newOrg('eclipse-kuksa') {
kuksa_default_branch_protection_rule('main')
],
},
orgs.newRepo('kuksa-java-sdk') {
allow_merge_commit: true,
allow_rebase_merge: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we use the same merge strategy as for the android-repos, i.e. only allow merge-commits, do not allow rebase/squash. As I understood there were some reasons to have that setup for Android, but does those reasons apply here as well, or do we want to have same setup as for most other repos where we allow all three methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind changing the approach here. I would like some kind of consistency though. The approach here is based on GitLab Flow. Are the other repos based on some official branching flow?

However there are some disadvantages of squashing with the current commit approach (conventional commits). The commits are linted for a consistency. Automatic Squash Merges will reduce the usefulness of this. So I am against that one. However you can squash it manually and force push before the merge if you want.

We can switch from merge commits to always rebase here if that feels better.

Choose a reason for hiding this comment

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

would tend to rebase merge as well

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not have any official branching flow in other repos. Typically there seems to be two sorts of people, those who prefer a clean history and fast forward merge, and those who prefer merge-commits. So in the end regardless of policy we will annoy some people. Personally I prefer a clean history unless the source branch is something long-lived, like merging "main" to "production" if you use the GitLab flow

There are also different views on how to keeping feature branches up-to-date, some prefer rebase and force-push to keep linear history, other prefer merges to make sure that there are no "orphan" comments. The general risk I see if you use merge-commits (or rebase) is that contributors will not do a rebase/squash/force-push to the feature branch before merge, so you end up with a lot of in my view use-less commit messages like "Applying suggestions from code-review"

The problem with too aggressive commit linters is that you might get errors also for commits created by bots like in eclipse-kuksa/kuksa-android-sdk#119, see https://github.com/eclipse-kuksa/kuksa-android-sdk/actions/runs/12070583813, as dependabot does not follow "subject must be sentence-case"

Copy link
Contributor

@Chrylo Chrylo Dec 12, 2024

Choose a reason for hiding this comment

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

Always a hot topic because the pros and contras are difficult to clearly compare.^^

I suggest:

  • Doing rebase for the clean history so it is aligned with most other repos
  • Keep using conventional commit and automatic changelog / release note generation. The advantage is too big IMHO.
  • Do not allow merge commits for consistency. Either rebase or merge commits.
  • Do not allow automatic squashing because it should be done deliberately to ensure a good commit quality for the conventional commits standard.
  • Document a clear Branching / Release concept (Preferable taking an official one) so all the rules are clearly linked and can be easily copied to a new repo.

If we don't care about the commit history or automatic changelog generation then we can think about allowing "everything". Could be that the advantages are not worth the trade off. But that is currently the basis for my arguments. :)

The risk that OSS Contributors are restricted too much by linters etc. to contribute is always there with every quality gate we are introducing. Be it code quality or commit quality tools. My feeling is that a good commit quality is ignored too much. That is why the main contributor Role is so important. You probably never want to just merge a (bigger) PR from outside without cleaning it up.

We should probably also not enforce "subject must be sentence-case" since dependabot is already using the conventional commit standards OR change the config for the dependabot for the commit message (if possible).

@erikbosch I let you decide together with Andre as the main reviewer one here without hard feelings. My motivation is only to solve the "bad commit messages" issue and automate release notes generation.

Copy link

This is your friendly self-service bot.
Please find below the validation of the requested configuration changes:

Diff for 2d2c94c
Project automotive.kuksa[github_id=eclipse-kuksa]
  there have been 7 validation infos, enable verbose output to display them.

+  add repository[name="kuksa-java-sdk"] {
+    allow_auto_merge                  = false
+    allow_forking                     = true
+    allow_merge_commit                = true
+    allow_rebase_merge                = false
+    allow_squash_merge                = false
+    allow_update_branch               = false
+    archived                          = false
+    auto_init                         = true
+    code_scanning_default_setup_enabled = false
+    custom_properties                 = {}
+    default_branch                    = "main"
+    delete_branch_on_merge            = true
+    dependabot_alerts_enabled         = true
+    dependabot_security_updates_enabled = true
+    description                       = "The Java SDK for Eclipse KUKSA"
+    gh_pages_build_type               = "disabled"
+    has_discussions                   = false
+    has_issues                        = true
+    has_projects                      = true
+    has_wiki                          = true
+    is_template                       = false
+    merge_commit_message              = "PR_TITLE"
+    merge_commit_title                = "MERGE_MESSAGE"
+    name                              = "kuksa-java-sdk"
+    private                           = false
+    private_vulnerability_reporting_enabled = false
+    secret_scanning                   = "enabled"
+    secret_scanning_push_protection   = "enabled"
+    squash_merge_commit_message       = "COMMIT_MESSAGES"
+    squash_merge_commit_title         = "COMMIT_OR_PR_TITLE"
+    topics                            = []
+    web_commit_signoff_required       = false
+  }

+  add repo_workflow_settings[repository=kuksa-java-sdk] {
+    enabled                           = true
+  }

+  add branch_protection_rule[pattern="main", repository=kuksa-java-sdk] {
+    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        = true
+    required_approving_review_count   = 1
+    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     = true
+    restricts_pushes                  = false
+    restricts_review_dismissals       = false
+  }
  
  Plan: 3 to add, 0 to change, 0 to delete.

@SebastianSchildt
Copy link
Contributor

In case it needs an approve from me before merge, I will wait for @erikbosch approval and then follow up if I like what I see

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

lgtm 👍

Copy link

This is your friendly self-service bot.
This Pull Request is eligible for auto-merging as it passed the following checks:

  • valid configuration
  • approved by a project lead
  • does not require any secrets
  • does not update settings only accessible via the GitHub Web UI
  • does not remove any resource

In order to automatically merge and apply the changes, add a comment /otterdog merge. 🚀

@erikbosch
Copy link
Contributor

/otterdog merge

Copy link

This is your friendly self-service bot.
Only the author of the pull request, a project-lead or a member of the admin teams is allowed to auto-merge it.

Copy link
Contributor

@Chrylo Chrylo left a comment

Choose a reason for hiding this comment

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

LGTM

@wba2hi
Copy link
Contributor Author

wba2hi commented Dec 12, 2024

/otterdog merge

@eclipse-otterdog eclipse-otterdog bot merged commit e7a1c08 into eclipse-kuksa:main Dec 12, 2024
3 checks passed
Copy link

This is your friendly self-service bot.

The following changes have been successfully applied:

Project automotive.kuksa[github_id=eclipse-kuksa]
  there have been 7 validation infos, enable verbose output to display them.

+  add repository[name="kuksa-java-sdk"] {
+    allow_auto_merge                  = false
+    allow_forking                     = true
+    allow_merge_commit                = true
+    allow_rebase_merge                = false
+    allow_squash_merge                = false
+    allow_update_branch               = false
+    archived                          = false
+    auto_init                         = true
+    code_scanning_default_setup_enabled = false
+    custom_properties                 = {}
+    default_branch                    = "main"
+    delete_branch_on_merge            = true
+    dependabot_alerts_enabled         = true
+    dependabot_security_updates_enabled = true
+    description                       = "The Java SDK for Eclipse KUKSA"
+    gh_pages_build_type               = "disabled"
+    has_discussions                   = false
+    has_issues                        = true
+    has_projects                      = true
+    has_wiki                          = true
+    is_template                       = false
+    merge_commit_message              = "PR_TITLE"
+    merge_commit_title                = "MERGE_MESSAGE"
+    name                              = "kuksa-java-sdk"
+    private                           = false
+    private_vulnerability_reporting_enabled = false
+    secret_scanning                   = "enabled"
+    secret_scanning_push_protection   = "enabled"
+    squash_merge_commit_message       = "COMMIT_MESSAGES"
+    squash_merge_commit_title         = "COMMIT_OR_PR_TITLE"
+    topics                            = []
+    web_commit_signoff_required       = false
+  }

+  add repo_workflow_settings[repository=kuksa-java-sdk] {
+    enabled                           = true
+  }

+  add branch_protection_rule[pattern="main", repository=kuksa-java-sdk] {
+    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        = true
+    required_approving_review_count   = 1
+    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     = true
+    restricts_pushes                  = false
+    restricts_review_dismissals       = false
+  }

  
  Applying changes:
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
  
  Done.
  
  Executed plan: 3 added, 0 changed, 0 deleted.

!                                                                                
! Warning:   The following GitHub repos have been created:                       
!-            https://github.com/eclipse-kuksa/kuksa-java-sdk                   
!                                                                                
!            Committers will gain access to it once the sync script runs (~2h).  
!                                                                                

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.

6 participants