From 2dd02c3e7202c151e3429792851d39d5888aab11 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 1 Oct 2020 05:51:47 -0400 Subject: [PATCH 1/9] repokitteh: add support for randomized auto-assign. This will allow us to pick dedicated API shepherds for each PR, ensuring we have clear review ownership. Fixes #13350 Signed-off-by: Harvey Tuch --- api/envoy/config/cluster/v3/filter.proto | 1 + ci/repokitteh/modules/ownerscheck.star | 60 ++++++++++++++++++------ repokitteh.star | 1 + 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/api/envoy/config/cluster/v3/filter.proto b/api/envoy/config/cluster/v3/filter.proto index 74f4a1137dab6..6df41e109eda6 100644 --- a/api/envoy/config/cluster/v3/filter.proto +++ b/api/envoy/config/cluster/v3/filter.proto @@ -1,3 +1,4 @@ +// some test change syntax = "proto3"; package envoy.config.cluster.v3; diff --git a/ci/repokitteh/modules/ownerscheck.star b/ci/repokitteh/modules/ownerscheck.star index e93010f89a7f6..d0d4f2193b140 100644 --- a/ci/repokitteh/modules/ownerscheck.star +++ b/ci/repokitteh/modules/ownerscheck.star @@ -8,7 +8,8 @@ # "path": "api/", # "label": "api", # "allow_global_approval": True, -# "github_status_label" = "any API change", +# "github_status_label": "any API change", +# "auto_assign": True, # }, # ], # ) @@ -27,8 +28,13 @@ # # 'label' refers to a GitHub label applied to any matching PR. The GitHub check status # can be customized with `github_status_label`. +# +# If 'auto_assign' is set True, a randomly selected reviwer from the owner team will +# be selected and set as a reviewer on the PR if there is not already a member of the +# owner team set as reviewer or assignee for the PR. load("text", "match") +load("time", "now") load("github.com/repokitteh/modules/lib/utils.star", "react") def _store_partial_approval(who, files): @@ -64,7 +70,8 @@ def _get_relevant_specs(specs, changed_files): label=spec.get("label", None), path_match=path_match, allow_global_approval=allow_global_approval, - status_label=status_label)) + status_label=status_label, + auto_assign=spec.get("auto_assign", False))) print("specs: %s" % relevant) @@ -152,20 +159,19 @@ def _reconcile(config, specs=None): return results -def _comment(config, results, force=False): +def _comment(config, results, assignees, force=False): lines = [] for spec, approved in results: if approved: continue - mention = spec.owner + owner = spec.owner - if mention[0] != '@': - mention = '@' + mention + if owner[-1] == '!': + owner = owner[:-1] - if mention[-1] == '!': - mention = mention[:-1] + mention = '@' + owner match_description = spec.path_match if match_description: @@ -185,21 +191,47 @@ def _comment(config, results, force=False): elif mode == 'fyi': lines.append('CC %s: FYI only%s.' % (mention, match_description)) + if mode != 'skip' and spec.auto_assign: + api_assignee = None + # Find owners via github.team_get_by_name, github.team_list_members + team_name = owner.split('/')[1] + team = github.team_get_by_name(team_name) + members = [m['login'] for m in github.team_list_members(team['id'])] + # Is a team member already assigned? The first assigned team member is picked. Bad O(n^2) as + # Starlark doesn't have sets, n is small. + for assignee in assignees: + if assignee in members: + api_assignee = assignee + break + # Otherwise we need to see if there is a reviewer picked from the team? If so, first wins. + if not api_assignee: + reviewers = github.pr_list_reviews() + for reviewer in reviewers: + user = reviewer['login'] + if user in members: + api_assignee = user + break + # Otherwise, pick at "random" (we just use timestamp). + if not api_assignee: + api_assignee = members[now().second % len(members)] + lines.append('API shepherd assignee is @%s' % api_assignee) + github.issue_assign(api_assignee) + if lines: github.issue_create_comment('\n'.join(lines)) -def _reconcile_and_comment(config): - _comment(config, _reconcile(config)) +def _reconcile_and_comment(config, assignees): + _comment(config, _reconcile(config), assignees) -def _force_reconcile_and_comment(config): - _comment(config, _reconcile(config), force=True) +def _force_reconcile_and_comment(config, assignees): + _comment(config, _reconcile(config), assignees, force=True) -def _pr(action, config): +def _pr(action, config, assignees): if action in ['synchronize', 'opened']: - _reconcile_and_comment(config) + _reconcile_and_comment(config, assignees) def _pr_review(action, review_state, config): diff --git a/repokitteh.star b/repokitteh.star index bf4c78b592915..5181346d560a0 100644 --- a/repokitteh.star +++ b/repokitteh.star @@ -21,6 +21,7 @@ use( "path": "api/envoy/", "label": "api", "github_status_label": "any API change", + "auto_assign": True, }, { "owner": "envoyproxy/api-watchers", From 2c6bade62ac253bc8207b59e96ed26a13d30e4ab Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 13:28:33 -0500 Subject: [PATCH 2/9] Undo test change. Signed-off-by: Harvey Tuch --- api/envoy/config/cluster/v3/filter.proto | 1 - ci/repokitteh/modules/ownerscheck.star | 8 -------- 2 files changed, 9 deletions(-) diff --git a/api/envoy/config/cluster/v3/filter.proto b/api/envoy/config/cluster/v3/filter.proto index 6df41e109eda6..74f4a1137dab6 100644 --- a/api/envoy/config/cluster/v3/filter.proto +++ b/api/envoy/config/cluster/v3/filter.proto @@ -1,4 +1,3 @@ -// some test change syntax = "proto3"; package envoy.config.cluster.v3; diff --git a/ci/repokitteh/modules/ownerscheck.star b/ci/repokitteh/modules/ownerscheck.star index d0d4f2193b140..f89ba042155c2 100644 --- a/ci/repokitteh/modules/ownerscheck.star +++ b/ci/repokitteh/modules/ownerscheck.star @@ -203,14 +203,6 @@ def _comment(config, results, assignees, force=False): if assignee in members: api_assignee = assignee break - # Otherwise we need to see if there is a reviewer picked from the team? If so, first wins. - if not api_assignee: - reviewers = github.pr_list_reviews() - for reviewer in reviewers: - user = reviewer['login'] - if user in members: - api_assignee = user - break # Otherwise, pick at "random" (we just use timestamp). if not api_assignee: api_assignee = members[now().second % len(members)] From 8890345f74fdc6efbf5836cecc75076f0e48438d Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:12:31 -0500 Subject: [PATCH 3/9] Exclude author from assignment. Signed-off-by: Harvey Tuch --- ci/repokitteh/modules/ownerscheck.star | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/ci/repokitteh/modules/ownerscheck.star b/ci/repokitteh/modules/ownerscheck.star index f89ba042155c2..08b728a9daccf 100644 --- a/ci/repokitteh/modules/ownerscheck.star +++ b/ci/repokitteh/modules/ownerscheck.star @@ -159,7 +159,7 @@ def _reconcile(config, specs=None): return results -def _comment(config, results, assignees, force=False): +def _comment(config, results, assignees, sender, force=False): lines = [] for spec, approved in results: @@ -196,7 +196,8 @@ def _comment(config, results, assignees, force=False): # Find owners via github.team_get_by_name, github.team_list_members team_name = owner.split('/')[1] team = github.team_get_by_name(team_name) - members = [m['login'] for m in github.team_list_members(team['id'])] + # Exclude author from assignment. + members = [m['login'] for m in github.team_list_members(team['id']) if m['login'] != sender] # Is a team member already assigned? The first assigned team member is picked. Bad O(n^2) as # Starlark doesn't have sets, n is small. for assignee in assignees: @@ -213,17 +214,17 @@ def _comment(config, results, assignees, force=False): github.issue_create_comment('\n'.join(lines)) -def _reconcile_and_comment(config, assignees): - _comment(config, _reconcile(config), assignees) +def _reconcile_and_comment(config, assignees, sender): + _comment(config, _reconcile(config), assignees, sender) -def _force_reconcile_and_comment(config, assignees): - _comment(config, _reconcile(config), assignees, force=True) +def _force_reconcile_and_comment(config, assignees, sender): + _comment(config, _reconcile(config), assignees, sender, force=True) -def _pr(action, config, assignees): +def _pr(action, config, assignees, sender): if action in ['synchronize', 'opened']: - _reconcile_and_comment(config, assignees) + _reconcile_and_comment(config, assignees, sender) def _pr_review(action, review_state, config): From 36b8d63a81544309981f5200d2e10408b08a5e27 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:13:37 -0500 Subject: [PATCH 4/9] test Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/extension.proto | 1 + 1 file changed, 1 insertion(+) diff --git a/api/envoy/config/core/v3/extension.proto b/api/envoy/config/core/v3/extension.proto index ba66da6a8e363..6d673e070b04c 100644 --- a/api/envoy/config/core/v3/extension.proto +++ b/api/envoy/config/core/v3/extension.proto @@ -1,3 +1,4 @@ +// test syntax = "proto3"; package envoy.config.core.v3; From fdee4fd594f8116b93db3f7d6474f004efdb2a40 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:14:42 -0500 Subject: [PATCH 5/9] skip skip Signed-off-by: Harvey Tuch --- ci/repokitteh/modules/ownerscheck.star | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/repokitteh/modules/ownerscheck.star b/ci/repokitteh/modules/ownerscheck.star index 08b728a9daccf..533e84dcf3c4a 100644 --- a/ci/repokitteh/modules/ownerscheck.star +++ b/ci/repokitteh/modules/ownerscheck.star @@ -181,10 +181,10 @@ def _comment(config, results, assignees, sender, force=False): key = "ownerscheck/%s/%s" % (spec.owner, spec.path_match) - if (not force) and (store_get(key) == mode): - mode = 'skip' - else: - store_put(key, mode) + #if (not force) and (store_get(key) == mode): + # mode = 'skip' + #else: + # store_put(key, mode) if mode == 'approval': lines.append('CC %s: Your approval is needed%s.' % (mention, match_description)) From de848645c3aa1398ce0565fb698d05a76b175815 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:15:39 -0500 Subject: [PATCH 6/9] wip Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/extension.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/core/v3/extension.proto b/api/envoy/config/core/v3/extension.proto index 6d673e070b04c..303e24041e5ed 100644 --- a/api/envoy/config/core/v3/extension.proto +++ b/api/envoy/config/core/v3/extension.proto @@ -1,4 +1,4 @@ -// test +// test 2 syntax = "proto3"; package envoy.config.core.v3; From def5858bf827786f8b240cace7c89bb922a28163 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:16:51 -0500 Subject: [PATCH 7/9] wip Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/extension.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/core/v3/extension.proto b/api/envoy/config/core/v3/extension.proto index 303e24041e5ed..b2ade9fbd0519 100644 --- a/api/envoy/config/core/v3/extension.proto +++ b/api/envoy/config/core/v3/extension.proto @@ -1,4 +1,4 @@ -// test 2 +// test 3 syntax = "proto3"; package envoy.config.core.v3; From 17f20a4721c85640577a56df05ad3cb44de665a7 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:17:22 -0500 Subject: [PATCH 8/9] wip Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/extension.proto | 1 - 1 file changed, 1 deletion(-) diff --git a/api/envoy/config/core/v3/extension.proto b/api/envoy/config/core/v3/extension.proto index b2ade9fbd0519..ba66da6a8e363 100644 --- a/api/envoy/config/core/v3/extension.proto +++ b/api/envoy/config/core/v3/extension.proto @@ -1,4 +1,3 @@ -// test 3 syntax = "proto3"; package envoy.config.core.v3; From 29d2660100ad6eb77f2758997a0a2d8eacadb364 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 25 Nov 2020 19:17:37 -0500 Subject: [PATCH 9/9] Undo skip skip. Signed-off-by: Harvey Tuch --- ci/repokitteh/modules/ownerscheck.star | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/repokitteh/modules/ownerscheck.star b/ci/repokitteh/modules/ownerscheck.star index 533e84dcf3c4a..08b728a9daccf 100644 --- a/ci/repokitteh/modules/ownerscheck.star +++ b/ci/repokitteh/modules/ownerscheck.star @@ -181,10 +181,10 @@ def _comment(config, results, assignees, sender, force=False): key = "ownerscheck/%s/%s" % (spec.owner, spec.path_match) - #if (not force) and (store_get(key) == mode): - # mode = 'skip' - #else: - # store_put(key, mode) + if (not force) and (store_get(key) == mode): + mode = 'skip' + else: + store_put(key, mode) if mode == 'approval': lines.append('CC %s: Your approval is needed%s.' % (mention, match_description))