Add reward-owners plugin #24937

matthyx · 2022-01-20T12:27:46Z

fixes #11994

parispittman · 2022-01-20T18:34:19Z

/lgtm

thank you SO MUCH for your work @matthyx :)) this is great for the community

dims · 2022-01-20T18:48:55Z

i have not reviewed prow plugins before. LGTM. thanks @matthyx

mrbobbytables · 2022-01-20T18:58:34Z

/assign @BenTheElder

stevekuznetsov · 2022-01-20T21:48:08Z

prow/plugins/reward-owners/reward-owners.go

+	RewardMessage = `
+Thanks for serving the community in this new capacity!
+
+Next steps:


Do we want this to be a Markdown-formatted link?

stevekuznetsov · 2022-01-20T21:48:17Z

prow/plugins/reward-owners/reward-owners.go

+Thanks for serving the community in this new capacity!
+
+Next steps:
+1- Reach out in #sig-contribex for your special badge swag!


Can we add a link to the channel?

I think that would be:

Suggested change

1- Reach out in #sig-contribex for your special badge swag!

1- Reach out in [#sig-contribex](https://app.slack.com/client/T09NY5SBT/C1TU9EB9S) for your special badge swag!

... Except the user also needs to be a member of the slack, so we might want to instead mention https://slack.k8s.io in this comment or some doc about joining the slack if there is one.

stevekuznetsov · 2022-01-20T21:48:23Z

prow/plugins/reward-owners/reward-owners.go

+Next steps:
+1- Reach out in #sig-contribex for your special badge swag!
+
+2- Join #kubernetes-contributors in slack and [dev@kubernetes.io](https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dev@kubernetes.io) for all upstream info


Can we add a link to the channel?

Suggested change

2- Join #kubernetes-contributors in slack and [dev@kubernetes.io](https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dev@kubernetes.io) for all upstream info

2- Join [#kubernetes-contributors](https://kubernetes.slack.com/archives/C09R23FHP) in slack and [dev@kubernetes.io](https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dev@kubernetes.io) for all upstream info

stevekuznetsov · 2022-01-20T21:51:08Z

prow/plugins/reward-owners/reward-owners.go

+	addedUsers := sets.NewString()
+	removedUsers := sets.NewString()
+	for _, change := range changes {
+		if (filepath.Base(change.Filename) == filenames.Owners || filepath.Base(change.Filename) == filenames.OwnersAliases) && change.Status != github.PullRequestFileRemoved {


IMO loading the YAML and doing a structured difference instead of using regex to parse a patch of a structured dataset might be nicer. Do we want to post this in all cases, or only if this is someone's first time becoming an OWNER? If so, we may just clone the new commit and traverse the owners set using our libraries to do the diff and figure out if this is a new member or someone owning new directories.

first time becoming an OWNER would be best but didn't want to make the scope too complicated out of the gate 🙇🏻

stevekuznetsov · 2022-01-20T21:52:09Z

prow/plugins/reward-owners/reward-owners.go

+			continue
+		}
+		fmt.Println("let's reward", user)
+		if err := ghc.CreateComment(org, repo, number, RewardMessage); err != nil {


We might make more of a coherent comment if we list the users, instead of posting (the same) information N times, once per user, in one PR.

BenTheElder · 2022-01-21T01:19:26Z

https://github.com/kubernetes/test-infra/pull/24937/files#r789177567 (@stevekuznetsov's comment) in particular

neat idea :)

matthyx · 2022-01-24T19:15:50Z

Thanks for the reviews, I'll come back to this tomorrow!

matthyx · 2022-01-26T13:16:01Z

prow/plugins/reward-owners/reward-owners.go

+	}
+
+	// Reward all new owners.
+	newOwners := headRepo.AllOwners().Difference(baseRepo.AllOwners())


@stevekuznetsov something like that? (please bear with me, I will fix tests and other details after you validate the approach)

Yes, I think this is an appropriate way to do it. @cjwagner might know more of the subtle bits of the owners impl, but this looks right to me

matthyx · 2022-01-28T08:15:45Z

@stevekuznetsov could you just check the new plugin logic, and then I'll fix the rest? Cheers!

stevekuznetsov · 2022-01-28T17:21:41Z

@matthyx sorry, I've not been at my desk - will look this morning :)

matthyx · 2022-01-28T17:31:54Z

Let's see what Cole has to say 😀
/cc @cjwagner

cjwagner · 2022-02-01T00:27:16Z

prow/repoowners/repoowners.go

+	log := c.logger.WithFields(logrus.Fields{"org": org, "repo": repo, "base": base, "sha": sha})
+	cloneRef := fmt.Sprintf("%s/%s", org, repo)
+	fullName := fmt.Sprintf("%s:%s", cloneRef, base)
+
 	entry, err := c.cacheEntryFor(org, repo, base, cloneRef, fullName, sha, log)


This cache only stores one value per org/repo which is normally perfectly efficient because we normally don't have look ups for old SHA values, only the current one. With this change we'll do look ups for old SHA values which will replace the cache value with a value that cause other consumers to cache miss.
We could mitigate this by adding an extra parameter to cacheEntryFor to optionally prevent updates to the cache and use that when LoadRepoOwnersSha is used rather than LoadRepoOwners.

cjwagner · 2022-02-01T00:34:05Z

prow/repoowners/repoowners.go

+func (o *RepoOwners) AllOwners() sets.String {
+	allOwners := sets.NewString()
+	for k := range o.approvers {
+		allOwners.Insert(k)


Both approvers and reviews are indexed by path, not by user so this would reward new OWNERS files rather than new users 🙃 . Instead loop over paths and regular expressions and union the sets.Strings at the leaf level.

Please also add a simple unit test for this.

Oh, yeah... let me do this a bit later.

cjwagner · 2022-02-01T00:43:21Z

prow/plugins/reward-owners/reward-owners.go

+}
+
+func handlePullRequest(pc plugins.Agent, pre github.PullRequestEvent) error {
+	if pre.Action != github.PullRequestActionOpened && pre.Action != github.PullRequestActionReopened && pre.Action != github.PullRequestActionSynchronize {


Shouldn't we post the message once the PR merges rather than when it is created and every time it is updated?

cjwagner · 2022-02-01T00:47:02Z

prow/plugins/reward-owners/reward-owners.go

+		log.Debug("No new owner to reward, exiting.")
+		return nil
+	}
+	return ghc.CreateComment(info.org, info.repo, info.number, fmt.Sprintf(RewardMessage, strings.Join(newOwners.List(), ", ")))


It might be a good idea to tag the users by prepending an @ to each username?

cjwagner · 2022-02-01T00:48:10Z

prow/plugins/reward-owners/reward-owners.go

+
+func helpProvider(_ *plugins.Configuration, _ []config.OrgRepo) (*pluginhelp.PluginHelp, error) {
+	pluginHelp := &pluginhelp.PluginHelp{
+		Description: fmt.Sprintf("The reward-owners plugin watches in %s and %s files for modifications and welcomes new approvers and reviewers.", ownersconfig.DefaultOwnersFile, ownersconfig.DefaultOwnersAliasesFile),


It doesn't look like this plugin actually considers the OWNERS_ALIAS files?

Well, repoowners looks into OWNERS and OWNERS_ALIAS, doesn't it?

Yeah, looking again this looks right. I think I forgot that the approvers and reviewers maps already have aliases expanded.

cjwagner · 2022-02-01T01:02:48Z

prow/plugins/reward-owners/reward-owners.go

+
+func handle(ghc githubClient, oc ownersClient, log *logrus.Entry, info info) error {
+	log.Debug("Resolving repository owners for base branch...")
+	baseRepo, err := oc.LoadRepoOwnersSha(info.org, info.repo, info.base.Ref, info.base.SHA)


It may be worth it to look up the files changed by the PR via the GH API before loading the owners files. Most PRs won't change OWNERS files at all and for those PRs it would probably be preferable to risk consuming some GH API rate limit in exchange for avoiding multiple repo fetches. We do this in verify-owners for example.

Yes I remember the clever chap that wrote it.

matthyx · 2022-02-01T09:15:01Z

Thanks @cjwagner I'll fix that soon!

matthyx · 2022-02-08T18:06:59Z

/retest

matthyx · 2022-02-08T18:11:08Z

@cjwagner I have added tests, it should be ready to review. Thanks in advance!

cjwagner

LGTM, thanks Matthias!

k8s-ci-robot · 2022-02-09T02:20:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~prow/OWNERS~~ [cjwagner]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2022

k8s-ci-robot requested review from michelle192837 and stevekuznetsov January 20, 2022 12:28

k8s-ci-robot added area/prow Issues or PRs related to prow area/prow/plugins Issues or PRs related to prow's plugins for the hook component sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 20, 2022

matthyx mentioned this pull request Jan 20, 2022

Add reward-owners plugin #19449

Closed

matthyx force-pushed the reward-bot branch from 345ea12 to e3e216b Compare January 20, 2022 12:39

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2022

matthyx force-pushed the reward-bot branch from e3e216b to 07ccc45 Compare January 20, 2022 12:56

k8s-ci-robot assigned parispittman Jan 20, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2022

k8s-ci-robot assigned BenTheElder Jan 20, 2022

stevekuznetsov reviewed Jan 20, 2022

View reviewed changes

matthyx force-pushed the reward-bot branch from 07ccc45 to b107abd Compare January 26, 2022 13:13

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2022

matthyx commented Jan 26, 2022

View reviewed changes

matthyx force-pushed the reward-bot branch 3 times, most recently from 910b52c to 0dfcb8f Compare January 26, 2022 13:43

k8s-ci-robot requested a review from cjwagner January 28, 2022 17:31

cjwagner reviewed Feb 1, 2022

View reviewed changes

matthyx force-pushed the reward-bot branch 3 times, most recently from 4bae078 to 21139bb Compare February 3, 2022 16:20

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2022

matthyx force-pushed the reward-bot branch from 21139bb to 2e3a0d7 Compare February 8, 2022 16:18

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 8, 2022

Add reward-owners plugin

493511c

matthyx force-pushed the reward-bot branch from 2e3a0d7 to 493511c Compare February 8, 2022 16:27

cjwagner approved these changes Feb 9, 2022

View reviewed changes

k8s-ci-robot assigned cjwagner Feb 9, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2022

k8s-ci-robot merged commit 52cbacb into kubernetes:master Feb 9, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 9, 2022

matthyx deleted the reward-bot branch February 9, 2022 06:02

parispittman mentioned this pull request Feb 23, 2022

swag ideas for OWNERs kubernetes/community#6492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reward-owners plugin #24937

Add reward-owners plugin #24937

matthyx commented Jan 20, 2022

parispittman commented Jan 20, 2022

dims commented Jan 20, 2022

mrbobbytables commented Jan 20, 2022

stevekuznetsov Jan 20, 2022

stevekuznetsov Jan 20, 2022

BenTheElder Jan 21, 2022

stevekuznetsov Jan 20, 2022

parispittman Jan 24, 2022

stevekuznetsov Jan 20, 2022

parispittman Jan 24, 2022

stevekuznetsov Jan 20, 2022

BenTheElder commented Jan 21, 2022

matthyx commented Jan 24, 2022

matthyx Jan 26, 2022

stevekuznetsov Jan 28, 2022

matthyx commented Jan 28, 2022

stevekuznetsov commented Jan 28, 2022

matthyx commented Jan 28, 2022

cjwagner Feb 1, 2022

cjwagner Feb 1, 2022

matthyx Feb 3, 2022

cjwagner Feb 1, 2022

cjwagner Feb 1, 2022

cjwagner Feb 1, 2022

matthyx Feb 3, 2022

cjwagner Feb 3, 2022

cjwagner Feb 1, 2022

matthyx Feb 3, 2022

matthyx commented Feb 1, 2022

matthyx commented Feb 8, 2022

matthyx commented Feb 8, 2022

cjwagner left a comment

k8s-ci-robot commented Feb 9, 2022

	1- Reach out in #sig-contribex for your special badge swag!
	1- Reach out in [#sig-contribex](https://app.slack.com/client/T09NY5SBT/C1TU9EB9S) for your special badge swag!

	2- Join #kubernetes-contributors in slack and [dev@kubernetes.io](https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dev@kubernetes.io) for all upstream info
	2- Join [#kubernetes-contributors](https://kubernetes.slack.com/archives/C09R23FHP) in slack and [dev@kubernetes.io](https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dev@kubernetes.io) for all upstream info

Add reward-owners plugin #24937

Add reward-owners plugin #24937

Conversation

matthyx commented Jan 20, 2022

parispittman commented Jan 20, 2022

dims commented Jan 20, 2022

mrbobbytables commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Jan 21, 2022

matthyx commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthyx commented Jan 28, 2022

stevekuznetsov commented Jan 28, 2022

matthyx commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthyx commented Feb 1, 2022

matthyx commented Feb 8, 2022

matthyx commented Feb 8, 2022

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 9, 2022