Test-infra: define label constants only once in prow

[BenTheElder]

At least const LgtmLabel = "lgtm" is defined in more than one place currently, imo we should define constants once and share the definitions. I'm proposing a labels or consts package (or similar...) for prow plugins to leverage where these are all defined once.

cons:

• another package
• non-functional refactor

pros:

• de-dupe constants / guarantees they match
• sort of creates a registry for plugin labels

/area prow
/kind cleanup

[BenTheElder]

/cc @cjwagner @stevekuznetsov

[cjwagner]

sort of creates a registry for plugin labels

I don't see much value to this given that not all plugins may be in use and additional relevant labels controlled by external plugins will be omitted.

Putting these constants into a separate package could make some binaries a bit smaller by avoiding importing logic from other plugins just to get the label value.

I think this would be an improvement, but it doesn't seem very important.

[BenTheElder]

I think this would be an improvement, but it doesn't seem very important.

Sure, this is a nit. That's why there's no priority label :-)

I do think it would be better to define these once though for correctness reasons. if it's _actually_ supposed to be the same constant, just use the same named constant.

[cjwagner]

I do think it would be better to define these once though for correctness reasons. if it's actually supposed to be the same constant, just use the same named constant.

Agreed. We can do this without a separate package though. We just make the constants publicly visible. For example, see one of Steve's recent PRs: https://github.com/kubernetes/test-infra/pull/9446/files

Having all the constants in one place could help discourage redefining them though.

[BenTheElder]

Agreed. We can do this without a separate package though. We just make the constants publicly visible. For example, see one of Steve's recent PRs: https://github.com/kubernetes/test-infra/pull/9446/files

In this case that seems fine since we're actually validating in relation to a plugin, but in the more general case that's a bad idea imo, it introduces unnecessary dependencies on other packages that will cause a headache later (dependency cycles anyone?). Having a package for "these are the labels" keeps these imports strictly linear from the get-go.

[BenTheElder]

/priority backlog
/help
/good-first-issue

[k8s-ci-robot]

@BenTheElder:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/priority backlog
/help
/good-first-issue

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.

[BenTheElder]

This is any easy change if anyone is up for it, just help us come up with a name for a labels package, and move the label definitions like prow/plugins/lgtm.LGTMLabel into there (which, now that I think about it will also help reduce "stuttering" as golint calls it)

[taragu]

@BenTheElder i'd like to work on this /assign

[taragu]

@BenTheElder Just created a pull request #9843 . I was trying to run the lgtm_test.go but got the following errors that seemed to be unrelated to my changes:

Running tool: /usr/local/go/bin/go test -timeout 30s kube_fork/test-infra/prow/plugins/lgtm -run ^TestLGTMComment|TestLGTMCommentWithLGTMNoti|TestLGTMFromApproveReview|TestHandlePullRequest|TestAddTreeHashComment|TestRemoveTreeHashComment$

# kube_fork/test-infra/prow/plugins/lgtm [kube_fork/test-infra/prow/plugins/lgtm.test]
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm.go:121:83: cannot use pc.Logger (type *"k8s.io/test-infra/vendor/github.com/sirupsen/logrus".Entry) as type *"kube_fork/test-infra/vendor/github.com/sirupsen/logrus".Entry in argument to handleGenericComment
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm.go:126:5: cannot use pc.Logger (type *"k8s.io/test-infra/vendor/github.com/sirupsen/logrus".Entry) as type *"kube_fork/test-infra/vendor/github.com/sirupsen/logrus".Entry in argument to handlePullRequest
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm.go:139:86: cannot use pc.Logger (type *"k8s.io/test-infra/vendor/github.com/sirupsen/logrus".Entry) as type *"kube_fork/test-infra/vendor/github.com/sirupsen/logrus".Entry in argument to handlePullRequestReview
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm.go:438:43: cannot use ro.Approvers(filename) (type "k8s.io/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String) as type "kube_fork/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String in argument to reviewers.Union
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm.go:438:73: cannot use ro.Reviewers(filename) (type "k8s.io/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String) as type "kube_fork/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String in argument to reviewers.Union(ro.Approvers(filename)).Union
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm_test.go:46:49: cannot use fakeRepoOwners literal (type *fakeRepoOwners) as type repoowners.RepoOwnerInterface in return argument:
    *fakeRepoOwners does not implement repoowners.RepoOwnerInterface (wrong type for Approvers method)
        have Approvers(string) "kube_fork/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String
        want Approvers(string) "k8s.io/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String
/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm_test.go:67:5: cannot use fakeRepoOwners literal (type *fakeRepoOwners) as type repoowners.RepoOwnerInterface in assignment:
    *fakeRepoOwners does not implement repoowners.RepoOwnerInterface (wrong type for Approvers method)
        have Approvers(string) "kube_fork/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String
        want Approvers(string) "k8s.io/test-infra/vendor/k8s.io/apimachinery/pkg/util/sets".String
FAIL    kube_fork/test-infra/prow/plugins/lgtm [build failed]
Error: Tests failed.

[cjwagner]

/Users/[email protected]/go/src/kube_fork/test-infra/prow/plugins/lgtm/lgtm.go:121:83: cannot use pc.Logger (type *"k8s.io/test-infra/vendor/github.com/sirupsen/logrus".Entry) as type *"kube_fork/test-infra/vendor/github.com/sirupsen/logrus".Entry

@taragu It looks like you checked out the your fork of the test-infra repo to the wrong directory.

The repo is checked out to "/Users/[email protected]/go/src/kube_fork/test-infra", but it should be checked out to "/Users/[email protected]/go/src/k8s.io/test-infra".

[taragu]

ahh thanks @cjwagner !

[BenTheElder]

I think https://github.com/kubernetes/test-infra/pull/9843 fully fixed this, thanks!