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 support for rebase and merge override labels #11979

Merged
merged 11 commits into from Apr 1, 2019

Conversation

@cji
Copy link
Contributor

commented Mar 28, 2019

Tide supports overriding the default merge method at the repo level to be squash or rebase. Tide also supports overriding the merge method at the PR level but only to squash. If the repo level setting is already squash, there's no way to override a particular PR to rebase or perform a standard merge.

This PR adds support for PRs that are in, for example, a squash default repo, but where users want to rebase or standard merge.

I outlined some other potential options for how to handle this here but thought the change was small enough that it might be worth just writing some code to get started as well.

Fixes #11877

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Welcome @cji!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Hi @cji. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@fejta
Copy link
Contributor

left a comment

Thanks, looks great!

@@ -447,6 +447,8 @@ tide:
pr_status_base_url: https://prow.k8s.io/pr
blocker_label: merge-blocker
squash_label: tide/squash
rebase_label: tide/rebase

This comment has been minimized.

Copy link
@fejta

fejta Mar 28, 2019

Contributor

Will you update label_sync/labels.yaml to include these values?

mergeMethod = github.MergeRebase
break
} else if string(prlabel.Name) == mergeLabel {
mergeMethod = github.MergeMerge

This comment has been minimized.

Copy link
@fejta

fejta Mar 28, 2019

Contributor

Don't we want to break here too?

This comment has been minimized.

Copy link
@cji

cji Mar 29, 2019

Author Contributor

I was missing a break here, yup! But, I took the breaks out now, since if we want to check for multiple labels, we need to always loop through all the labels.

This comment has been minimized.

Copy link
@deadmoose

deadmoose Mar 29, 2019

Could still break when setting MergeInvalid; no need to keep looping through labels to check if it's doubly-invalid. (Or flip the logic around; start out with a nil mergeMethod, loop looking for a label specifying one & set it. Continue looping, and if you get another explicit label, set to MergeInvalid and pull the cord. Outside the loop, assign to c.config().Tide.MergeMethod(sp.org, sp.repo) if nil. It's a beautifully hairy yak.)

squashLabel := c.config().Tide.SquashLabel
rebaseLabel := c.config().Tide.RebaseLabel
mergeLabel := c.config().Tide.MergeLabel
if squashLabel != "" || rebaseLabel != "" || mergeLabel != "" {
for _, prlabel := range pr.Labels.Nodes {

This comment has been minimized.

Copy link
@fejta

fejta Mar 28, 2019

Contributor

It is possible for a PR to have both a squash, rebase and merge label. It seems like we should detect this scenario and error out

This comment has been minimized.

Copy link
@cji

cji Mar 29, 2019

Author Contributor

I think this increases the complexity a bit, but took a shot at trying to do this in the latest commit.

Show resolved Hide resolved prow/tide/tide.go
@fejta

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

/ok-to-test

Show resolved Hide resolved prow/config.yaml
Show resolved Hide resolved prow/tide/tide.go Outdated

cji added some commits Mar 29, 2019

for _, prlabel := range pr.Labels.Nodes {
if string(prlabel.Name) == squashLabel {
switch string(prlabel.Name) {

This comment has been minimized.

Copy link
@deadmoose

deadmoose Mar 29, 2019

Forgive my golang rustiness asking this one, but is a switch where multiple cases are the same legal? The conditional around the whole things ensures that at least ONE of squashLabel/rebaseLabel/mergeLabel is non-empty, but it's possible that 2/3 are empty (or that they're non-empty but otherwise identical)

Probably worth checking that if they exist they aren't identical since that would be another way to accidentally get some surprising behavior with a non-obvious cause.

This comment has been minimized.

Copy link
@cji

cji Mar 29, 2019

Author Contributor

Yeah, this does seem confusing, and is making things a bit more complex than I had hoped here :(

go seems to be ok with allowing duplicate cases for variables (the same variable or variables with the same value), though it would be illegal for something like multiple case ""'s

This comment has been minimized.

Copy link
@midnightconman

midnightconman Mar 29, 2019

Contributor

Here we could use an empty switch, fall through, and a counter (labelCount)... if the counter is > 1, post a comment warning multiple label existence?

Fall through is not enabled by default in go: https://golang.org/test/switch.go

Show resolved Hide resolved prow/tide/tide.go Outdated

cji added some commits Mar 29, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 30, 2019

@cji

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

thanks for the feedback everyone, and patience with me making the changes. I think this is in a better state for a review now! 🤞

@stevekuznetsov
Copy link
Contributor

left a comment

This looks really nice!

/lgtm
/hold
@cjwagner did you want to do a final pass?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

LGTM label has been added.

Git tree hash: b576b0e70aa1eb69fdb96885dca1da2105fc57b3

@cjwagner
Copy link
Member

left a comment

I've got one nit, but this looks good overall so feel free to ignore with /hold cancel.

mergeLabel := c.config().Tide.MergeLabel
if squashLabel != "" || rebaseLabel != "" || mergeLabel != "" {
var err error
mergeMethod, err = checkMergeLabels(pr, squashLabel, rebaseLabel, mergeLabel, mergeMethod)

This comment has been minimized.

Copy link
@cjwagner

cjwagner Apr 1, 2019

Member

I think this would make more sense if it were in the tryMerge function. That would avoid the need to duplicate the error handling logic and seems semantically more correct.

This comment has been minimized.

Copy link
@cji

cji Apr 1, 2019

Author Contributor

In tryMerge itself? or in the anonymous mergeFunc function we're using to call tryMerge? I could be reading this wrong, but it looks like tryMerge is going to run after we try the merge with return c.ghc.Merge() in the mergeFunc.

This comment has been minimized.

Copy link
@cjwagner

cjwagner Apr 1, 2019

Member

We don't call tryMerge from an anonymous function, rather we pass an anonymous function to tryMerge. The anonymous function assigned to the mergeFunc parameter is not executed until it is called from within tryMerge.

I was imagining that your changes would be best as part of the tryMerge function itself, but that would necessitate parameterizing the mergeFunc with the merge method and passing all the label config and the PR's labels to tryMerge.
That seems equally as cumbersome as duplicating the error handling logic so I guess lets leave this as is for now. Sorry for the noise.

This comment has been minimized.

Copy link
@cji

cji Apr 1, 2019

Author Contributor

Apologies on mixing up the semantics 😅 Thank you for the feedback and clarification!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cji, cjwagner, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cjwagner
Copy link
Member

left a comment

/hold cancel

mergeLabel := c.config().Tide.MergeLabel
if squashLabel != "" || rebaseLabel != "" || mergeLabel != "" {
var err error
mergeMethod, err = checkMergeLabels(pr, squashLabel, rebaseLabel, mergeLabel, mergeMethod)

This comment has been minimized.

Copy link
@cjwagner

cjwagner Apr 1, 2019

Member

We don't call tryMerge from an anonymous function, rather we pass an anonymous function to tryMerge. The anonymous function assigned to the mergeFunc parameter is not executed until it is called from within tryMerge.

I was imagining that your changes would be best as part of the tryMerge function itself, but that would necessitate parameterizing the mergeFunc with the merge method and passing all the label config and the PR's labels to tryMerge.
That seems equally as cumbersome as duplicating the error handling logic so I guess lets leave this as is for now. Sorry for the noise.

@k8s-ci-robot k8s-ci-robot merged commit 0d66b18 into kubernetes:master Apr 1, 2019

15 checks passed

cla/linuxfoundation cji authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Job succeeded.
Details
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Skipped.
pull-test-infra-verify-file-perms Job succeeded.
Details
pull-test-infra-verify-github-spelling Job succeeded.
Details
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Job succeeded.
Details
pull-test-infra-verify-tslint Skipped.
tide In merge pool.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

@cji: Updated the following 2 configmaps:

  • label-config configmap in namespace test-pods using the following files:
    • key labels.yaml using file label_sync/labels.yaml
  • config configmap in namespace default using the following files:
    • key config.yaml using file prow/config.yaml

In response to this:

Tide supports overriding the default merge method at the repo level to be squash or rebase. Tide also supports overriding the merge method at the PR level but only to squash. If the repo level setting is already squash, there's no way to override a particular PR to rebase or perform a standard merge.

This PR adds support for PRs that are in, for example, a squash default repo, but where users want to rebase or standard merge.

I outlined some other potential options for how to handle this here but thought the change was small enough that it might be worth just writing some code to get started as well.

Fixes #11877

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.

@cji cji deleted the cji:mergelabels branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.