What would you like to be added: For what I know tide only allows to set default merge method for given repo using merge_method field which is an OrgRepo map.

tide:
  merge_method:
      org/repo: squash
...

It's useful, but for some repositories using only default merge method is quite tedious task to remember to manually change the merge method using /label flag. What I would like to suggest is to change the merge_method field to accept per-branch configuration for each repository. This field would accept a regex that matches against the branch tide is working on. The idea goes as follows:

tide:
  merge_method:
    '*': # default merge method configuration for every repository
     branches:
        '*': squash
    org/repo: # per-repository override
      branches:
        '*': squash
        release-x.y: merge
        '^hotfix-.*$': merge
        develop: squash

If the configuration for that branch is not defined then the default behaviour is assumed -> "merge". The configuration was roughly inspired by plugins configuration, where you can define enabled plugin on global(?), org or repo level.

Why is this needed: That would ensure that PRs are merged with the correct merge method when opened against a branch without requiring to remember about manually changing the merge method using /label command.

It would mainly impact the work we have in Kyma since by default we want all of our commits to be squashed into default branch, but have all PRs with cherry-picks merged with merge method to keep the commit history without manually setting the label tide/merge-method-merge.

/area prow /area prow/tide /kind feature /sig testing

Mar 29 '22 14:03 Ressetkk

@Ressetkk idea sounds fine to me, feel free to pick this up. We will need to keep supporting the existing format though to not break ppl.

Apr 01 '22 13:04 alvaroaleman

Sure, I'll take it when I get more free time. If anyone picks it up before me it would also be cool.

Apr 05 '22 09:04 Ressetkk

Duplicate of #24680

Apr 05 '22 10:04 Ressetkk

I can pick this one up. I would like to proceed step by step by:

changing the way we parse the configuration by allowing two possible formats in order to accommodate the new feature but being backward compatible at the same time

Standard repo-wide configuration

tide:
  merge_method:
    kubernetes/test-infra: rebase

Extended per-branch configuration

tide:
  merge_method:
   kubernetes/test-infra:
      branches:
        '^develop$': rebase
        '^feature/.+$': squash
        '^master$': merge

this should be a safe change since it introduces no behavioral changes in Tide

changing the way a merge strategy is chosen for a given PR. At this point we modify the procedure by taking into the account the new configuration. I would like to stick with regex format only for the matching part without introducing other wildcards like '*' because I find easier to reason about, but I am obviously open to discussion.

tide:
  merge_method:
-    '*':
+    '.*':
      branches:
-        '*': merge
+        '.*': merge
   
    kubernetes/test-infra:
       branches:
         '^develop$': rebase
         '^feature/.+$': squash
         '^master$': merge

May 05 '22 09:05 danilo-gemoli

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Aug 03 '22 09:08 k8s-triage-robot

/remove-lifecycle stale

Aug 03 '22 11:08 Ressetkk

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Nov 01 '22 11:11 k8s-triage-robot

/remove-lifecycle stale

Nov 04 '22 16:11 danilo-gemoli

Blueprint

Phase 1

[x] Change MergeType type in Tide config to fit the new configuration
[x] Modify the way config is loaded: check “yamlToConfig” function on prow/config/config.go
[x] Create a new “OrgRepoBranchMergeMethod” function that introduces the new matching criteria and is backwards compatible
[x] Make “MergeMethod” function (prow/config/tide.go) forwarding to “OrgRepoBranchMergeMethod”
[ ] Open and merge a PR

Phase 2

It is not mandatory to follow this plan. If "Phase 2" is too much to handle all at once it can be further broken down into smaller steps.

[ ] Add unit tests for “OrgRepoBranchMergeMethod”
[ ] Modify “parseProwConfig” function in prow/config/config.go
- [ ] Add new validation algorithm for "MergeType" type
- [ ] Add unit tests
[ ] Modify “mergeFrom” function in prow/config/tide.go
- [ ] Modify the function
- [ ] Add unit tests
[ ] Modify “HasConfigFor” function in prow/config/config.go
- [ ] Modify the function eventually
- [ ] Add unit tests
[ ] Modify “hasGlobalConfig” in prow/config/config.go
- [ ] Modify the function eventually
- [ ] Add unit tests
[ ] Write Tide doc regarding how new configuration works
[ ] Open and merge a PR

Merge Method

As-is

Matching criteria

Given an org/repo pair to match, the following matching process is applied:

match “org/repo”
match “org”
default to “merge”

Examples:

tide:
  merge_method:
    kubernetes-client: rebase
    kubernetes-client/csharp: squash

Org/Repo	Merge Method	Step	Criteria
kubernetes-client/csharp	squash	(1.)	org/repo match
kubernetes-client/fsharp	rebase	(2.)	org match
golang/go	merge	(3.)	default

To-be

Configuration

tide:
  merge_method:
    # Full notation
    ${ORG_REGEX_1}:
      ${REPO_REGEX_1}:
        ${BRANCH_REGEX_1}: ${MERGE_METHOD_1}
        ${BRANCH_REGEX_2}: ${MERGE_METHOD_2}
        ...
      ${REPO_REGEX_2}:
      ...
    ${ORG_REGEX_2}:
    ...
    
    # Org-wide merge method
    ${ORG_REGEX}: ${MERGE_METHOD}
    # Repo-wide merge method
    ${ORG_REGEX}
      ${REPO_REGEX}: ${MERGE_METHOD}
    
    # Org/Repo@Branch shorthand
    ${ORG}/${REPO}@${BRANCH}: ${MERGE_METHOD}
    # Org/Repo shorthand
    ${ORG}/${REPO}: ${MERGE_METHOD}
    # Org shorthand. It takes precedence over the org-wide config.
    ${ORG}: ${MERGE_METHOD}

Example:

tide:
  merge_method:
    "kube.+":
      kubernetes:
        master: rebase
        "release-1\.\d+": merge
      test-infra: rebase

Org-Repo wide config

Organization level:

tide:
  merge_method:
    kubernetes: rebase
# This is the same as:
tide:
  merge_method:
    kubernetes:
      ".*": rebase
# or:
tide:
  merge_method:
    kubernetes:
      ".*":
        ".*": rebase

Repository level:

tide:
  merge_method:
    kubernetes:
      test-infra: rebase
# This is the same as:
tide:
  merge_method:
    kubernetes:
      test-infra:
        ".*": rebase

Branch level:

tide:
  merge_method:
    kubernetes:
      test-infra:
        master: rebase

Shortands

$org/$repo@$branch shorthand

tide:
  merge_method:
    kubernetes/test-infra@master: rebase
# This is the same as:
tide:
  merge_method:
    "^kubernetes$":
      "^test-infra$":
        "^master$": rebase

$org/$repo shorthand is in place for backward compatibility

tide:
  merge_method:
    kubernetes/test-infra: rebase
# This is the same as:
tide:
  merge_method:
    "^kubernetes$":
      "^test-infra$": rebase

$org shorthand is in place for backward compatibility

tide:
  merge_method:
    kubernetes: rebase
# This is the same as:
tide:
  merge_method:
    "^kubernetes$": rebase

Important Shorthand match is an exact, literal match: regex symbols are NOT interpreted here. Example:

org/repo@branch shorthand kube.+/client@master matches kube.+/client@master literally but on the other end it doesn’t match kubernetes/client@master

New Matching criteria

Given an org/repo@branch to match, the following matching process is applied:

org/repo@branch shorthand match
org/repo shorthand match
org shorthand match
Regex matching: a. Match using regex in the following order: org, repo and branch b. In case of multiple matches, pick the last one: ordering matters.
Default merge method: “merge”

Examples:

tide:
  merge_method:
    "open.+":
      client:
        master: rebase
    openshift:
      ".*":
        ".*": rebase
      installer:
        main: squash
      client:
        master: squash
    openshift/ci-tools: squash
    openshift/ci-tools@experiment: merge
    golang: rebase
    face.*: rebase

Org/Repo@Branch	Merge Method	Step	Criteria
openshift/ci-tools@experiment	merge	(1.)	org/repo@branch shorthand
openshift/ci-tools@master	squash	(2.)	org/repo shorthand
golang/protobuf	rebase	(3.)	org shorthand
openshift/installer@main	squash	(4a.)	org/repo@branch regex
kubernetes/test-infra@master	merge	(5.)	default
openshift/client@master	squash	(4b.)	last regex match
openarena/client@master	rebase	(4a.)	org/repo@branch regex
face.+/server@master	rebase	(3.)	org shorthand

Nov 10 '22 13:11 danilo-gemoli

Hey guys, @alvaroaleman @Ressetkk how does it look like?

Nov 10 '22 13:11 danilo-gemoli

Phase 1 Create a new “OrgRepoBranchMergeMethod” function that introduces the new matching criteria and is backwards compatible Phase 2 Add unit tests for “OrgRepoBranchMergeMethod”

Pls write tests for OrgRepoBranchMergeMethod right away

Otherwise, I'm not too fond of offering regexes on the org and org/repo level personally. I don't believe there's a strong use case for that, compared to the amount of complexity (see below) it brings. Some arguments why not have that:

It is very easy to have a regex that covers something not intended, which would result in messing with someone elses repository (or even org, if it happens to be handled by the same Tide)
Regexes are hard to validate in general: it's hard to detect conflicting regexes; two regexes covering same thing result in weird/unexpected behavior...
How would you even distiguish the intent on the shorthand level? How do you know if a key string is supposed to be a $ORG_REGEX or $ORG in the snippet below? By the presence of regex-ish characters? I would rather not go there...

    # Org-wide merge method
    ${ORG_REGEX}: ${MERGE_METHOD}
    # Repo-wide merge method
    ${ORG_REGEX}
      ${REPO_REGEX}: ${MERGE_METHOD}
    
    # Org/Repo@Branch shorthand
    ${ORG}/${REPO}@${BRANCH}: ${MERGE_METHOD}
    # Org/Repo shorthand
    ${ORG}/${REPO}: ${MERGE_METHOD}
    # Org shorthand. It takes precedence over the org-wide config.
    ${ORG}: ${MERGE_METHOD}

I'd only allow regexes on the branch level, and not in the org/repo@branch shorthand. Speaking of shorthands, how do you want to achieve them? How do you want to parse a yaml where a value of a key can be either a string or a struct?

org: method

org:
  repo-1:
  repo-2:

Nov 11 '22 14:11 petr-muller

I'm late here. Regarding regex: I agree with you it's difficult to decide whether two regex will overlap or not, as a consequence it's easy to match unintended text. I wanted to explore that area a little bit and then start a discussion. What about leaving them on branches (as you proposed) and allow only the * wildcard at repository level?

kubernetes:
  '*': # "*" it's the only allowed wildcard so far
    master: merge # branch matches using a regex
    'release-\d+': rebase
  test-infra: # that's a simple text
    'release-\d+': squash

If it sounds reasonable we should then decide whether shorthands should follow the same approach (wildcard+regex) or not:

kubernetes/*@release-\d+: merge

vs

kubernetes/[email protected]: merge

While the latter is easier to understand it breaks a little bit the coherence between the short form (shorthands) and the full one. Do I, as a user, expect wildcards and regex to work even on shorhands as they do on the full configuration? If I can write this:

kubernetes:
  '*': # "*" it's the only allowed wildcard so far
    'release-\d+': rebase

Do I expect also to be allowed to write that?

kubernetes/*@release-\d+: rebase

On parsing a yaml field that can be either a struct or a string, I've tried to achieve it by using custom unmarshalers, like danilo-gemoli/test-infra@25799-extend-tide-merge-config /prow/config/tide.go#L55

Nov 24 '22 09:11 danilo-gemoli

What about leaving them on branches (as you proposed) and allow only the * wildcard at repository level?

That sounds good to me.

You raise some fair points about the shorthands... IMO the one thing that we really don't want to do is something like kubernetes/*@release-\d+ in shorthands where the language is an arbitrary combo of explicit (org), explicit-or-asterisk (repo) and regex (branches) language, which would be IMO very nonintuitive and confusing. So our options are full regex (which I dislike for reasons in my earlier comment) or no patterns, which I'd prefer. Personally I see shorthands as a complexity-escape hatch for simple use cases (also backward compatibility device, I guess). If you don't have an easy use case, then IMO you should be comfortable using the configuration format that has structure.

Nov 29 '22 13:11 petr-muller

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle stale
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Feb 27 '23 14:02 k8s-triage-robot

/lifecycle-remove stale

Danilo still wants this and there's an open PR, hopefully we can merge it

Feb 28 '23 15:02 petr-muller

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle rotten
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

Mar 30 '23 16:03 k8s-triage-robot

/remove-lifecycle rotten

Mar 31 '23 07:03 danilo-gemoli

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle stale
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Jun 29 '23 07:06 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle rotten
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

Jan 19 '24 06:01 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue with /reopen
Mark this issue as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Feb 18 '24 06:02 k8s-triage-robot

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue with /reopen

Mark this issue as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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/test-infra repository.

Feb 18 '24 06:02 k8s-ci-robot

test-infra
test-infra copied to clipboard

Tide: default merge method for specific branch in the repository

Blueprint

Phase 1

Phase 2

Merge Method

As-is

Matching criteria

To-be

Configuration

Org-Repo wide config

Shortands

New Matching criteria

test-infra test-infra copied to clipboard

Tide: default merge method for specific branch in the repository

Blueprint

Phase 1

Phase 2

Merge Method

As-is

Matching criteria

To-be

Configuration

Org-Repo wide config

Shortands

New Matching criteria

test-infra
test-infra copied to clipboard