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

Migrate lgtm_after_commit munger to prow lgtm plugin #6886

Merged
merged 1 commit into from Feb 28, 2018

Conversation

@MorrisLaw
Copy link
Contributor

commented Feb 17, 2018

This PR migrates the lgtm_after_commit munger into the prow lgtm plugin.

Closes #3795.
Refer to the original PR, #3821, for reference.

/assign @cjwagner

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

/ok-to-test
Thanks for working on this! 😀

@cblecker

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

/cc

@k8s-ci-robot k8s-ci-robot requested a review from cblecker Feb 17, 2018

@@ -270,3 +270,7 @@ func (f *FakeClient) ListCollaborators(org, repo string) ([]github.User, error)
}
return result, nil
}

func LabelID(org, repo string, number int, label string) string {

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Feb 19, 2018

Contributor

Not sure we really need an exported func for this -- saves a couple chars but at the same time we're unlikely to move every use of the %s/%s#%d:%s usage over.

}

for i, k := range c.labelsRemoved {
if got, want := k, fakeGitHub.labelsRemoved[i]; got != want {

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Feb 19, 2018

Contributor

Can you do a reflect.DeepEqual here for simplicity?

This comment has been minimized.

Copy link
@kargakis

kargakis Feb 23, 2018

Member

Please use k8s/apimachinery/pkg/equality.Semantic.DeepEqual.

if got, want := k, fakeGitHub.labelsRemoved[i]; got != want {
t.Logf("labelsRemoved: got %v, want: %v", fakeGitHub.labelsRemoved, c.labelsRemoved)
t.Fatalf("at index %d got key: %s, want key: %s", i, got, want)
break

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Feb 19, 2018

Contributor

dead code?

return fmt.Sprintf("label %q does not exist on %s/%s/%d", e.Label, e.Owner, e.Repo, e.Number)
}

type githubError struct {

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Feb 19, 2018

Contributor

Either inline or move the use in func (c *Client) Merge to use this too

This comment has been minimized.

Copy link
@MorrisLaw

MorrisLaw Feb 25, 2018

Author Contributor

Thanks for the review @stevekuznetsov , I've added the use of the githubError struct to the Merge function.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

/lint

@k8s-ci-robot
Copy link
Contributor

left a comment

@BenTheElder: 3 warnings.

In response to this:

/lint

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.

@@ -973,15 +973,55 @@ func (c *Client) AddLabel(org, repo string, number int, label string) error {
return err
}

// Use LabelNotFound to indicate that a label is not attached to an issue.

This comment has been minimized.

Copy link
@k8s-ci-robot

k8s-ci-robot Feb 21, 2018

Contributor

Golint comments: comment on exported type LabelNotFound should be of the form "LabelNotFound ..." (with optional leading article). More info.

@@ -270,3 +270,7 @@ func (f *FakeClient) ListCollaborators(org, repo string) ([]github.User, error)
}
return result, nil
}

func LabelID(org, repo string, number int, label string) string {

This comment has been minimized.

Copy link
@k8s-ci-robot

k8s-ci-robot Feb 21, 2018

Contributor

Golint comments: exported function LabelID should have comment or be unexported. More info.

lgtmLabel,
); err != nil {
if _, ok := err.(*github.LabelNotFound); !ok {
return fmt.Errorf("RemoveLabel error: %v", err)

This comment has been minimized.

Copy link
@k8s-ci-robot

k8s-ci-robot Feb 21, 2018

Contributor

Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

This comment has been minimized.

Copy link
@MorrisLaw

MorrisLaw Feb 25, 2018

Author Contributor

@BenTheElder , should I change "RemoveLabel" to "removelabel"?

This comment has been minimized.

Copy link
@kargakis

kargakis Feb 26, 2018

Member

Make this "failed removing lgtm label: %v", err

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

/area prow
/cc
everything is apparently on fire right now but I intend to take another pass over this later 😬

@k8s-ci-robot k8s-ci-robot requested a review from BenTheElder Feb 26, 2018

@cblecker

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

@MorrisLaw Can you also rebase this on top of master, instead of merging master into your branch? Thank you!

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:lgtm branch 2 times, most recently from 5b1dae7 to cbf83d9 Feb 27, 2018

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:lgtm branch from cbf83d9 to 2be64af Feb 27, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

Looking pretty nice, ping @cjwagner @stevekuznetsov for any more nits :-)

@cblecker

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

/test pull-test-infra-lint

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 28, 2018

@cjwagner
Copy link
Member

left a comment

This looks great! Thanks for making this change, this is the final munger in use by 2 of our mungegithub instances so once this is in we will be able to turn down both of those instances entirely!
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, MorrisLaw

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

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 0c3a70b into kubernetes:master Feb 28, 2018

10 checks passed

cla/linuxfoundation MorrisLaw authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-config Skipped
pull-test-infra-verify-deps Skipped
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
tide In tide pool.
Details

@MorrisLaw MorrisLaw deleted the MorrisLaw:lgtm branch Mar 1, 2018

}, &ge)

// If our code was 200 or 204, no error info.
if code != 404 {

This comment has been minimized.

Copy link
@kargakis

kargakis Mar 13, 2018

Member

This means you ignore all errors apart from 404 as non-errors.

return err
// On 404, do not retry. We will inspect and handle this case appropriately.
exitCodes: []int{200, 204, 404},
}, &ge)

This comment has been minimized.

Copy link
@kargakis

kargakis Mar 13, 2018

Member

This means request will try to unmarshal any kind of request body into githubError (which may yield an Unmarshal error; the test failure I am seeing in #7258)

This comment has been minimized.

Copy link
@MorrisLaw

MorrisLaw Mar 15, 2018

Author Contributor

Nice catch! Thanks for taking care of this in your PR!

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.