diff --git a/README.md b/README.md index 526e83a..0be70e9 100644 --- a/README.md +++ b/README.md @@ -186,12 +186,13 @@ revup --remote-name origin --fork-name myfork upload Revup can also add reviewers, assignees, and labels to pull requests. Add the appropriate tags to any commit in a topic. ``` -Reviewers: alice, bob +Reviewers: alice, bob, myorg/backend-team Assignees: eve Labels: bug, feature, draft ``` Github usernames can be abbreviated and will match the shortest name with the given prefix. +Teams are specified as `org/team-slug` and can be used as reviewers (Github does not support teams as assignees). Labels must match exactly. The `draft` label is special and will make a pull request a draft if present and unmake draft if removed. diff --git a/docs/upload.md b/docs/upload.md index 633d179..d067a43 100644 --- a/docs/upload.md +++ b/docs/upload.md @@ -54,11 +54,16 @@ is specified, the auto detected base branch will be used. : Specifies reviewers that will be added on github. Names as given here can be any prefix of that user's github login name. If multiple users match a name, the user with the shortest login name will be used. If a reviewer cannot -be found a warning is printed. +be found a warning is printed. A reviewer can also be a team, specified as +`org/team-slug`; teams are matched exactly by their org and slug. If it looks +like the team has previously been resolved by Github's code review +auto-assignment (i.e. if one of its members is already a reviewer on the PR), +revup will not re-request the team, to avoid triggering auto-assignment again. **Assignees:** : Specifies assignees that will be added on github. Semantics are the same as -for reviewers. +for reviewers, except that Github does not support teams as assignees so +`org/team-slug` entries are rejected as an error. **Labels:** : Specifies labels that will be added on github. Labels must match the label diff --git a/revup/github_utils.py b/revup/github_utils.py index 65bc4b5..2cc8a3b 100644 --- a/revup/github_utils.py +++ b/revup/github_utils.py @@ -46,6 +46,8 @@ class PrInfo: state: str = "" reviewers: Set[str] = field(default_factory=set) reviewer_ids: Set[str] = field(default_factory=set) + reviewer_teams: Set[str] = field(default_factory=set) + reviewer_team_ids: Set[str] = field(default_factory=set) assignees: Set[str] = field(default_factory=set) assignee_ids: Set[str] = field(default_factory=set) labels: Set[str] = field(default_factory=set) @@ -66,6 +68,7 @@ class PrUpdate: title: Optional[str] = None id: str = "" reviewer_ids: Set[str] = field(default_factory=set) + reviewer_team_ids: Set[str] = field(default_factory=set) assignee_ids: Set[str] = field(default_factory=set) label_ids: Set[str] = field(default_factory=set) is_draft: Optional[bool] = None @@ -115,7 +118,16 @@ async def query_everything( head_refs: List[str], user_ids: List[str], labels: List[str], -) -> Tuple[str, List[Optional[PrInfo]], Dict[str, str], Dict[str, str], Dict[str, str]]: + teams: List[Tuple[str, str]], +) -> Tuple[ + str, + List[Optional[PrInfo]], + Dict[str, str], + Dict[str, str], + Dict[str, str], + Dict[str, str], + Dict[str, Optional[Set[str]]], +]: """ This function does all necessary graphql querying in one request. This dramatically reduces the amount of time spent on querying. @@ -126,19 +138,27 @@ async def query_everything( - Dict of user_ids as given to graphql node ids - Dict of user_ids as given to their full login name - Dict of labels to their graphql node ids + - Dict of "org/slug" team refs to graphql node ids + - Dict of "org/slug" team refs to their member logins. None if the team has more members + than we fetched (meaning membership is unknown / incomplete). """ head_refs_args = get_args_dict(head_refs, "pr") user_id_args = get_args_dict(user_ids, "user") label_args = get_args_dict(labels, "label") + team_org_args = get_args_dict([t[0] for t in teams], "team_org") + team_slug_args = get_args_dict([t[1] for t in teams], "team_slug") prs_out = get_result_args(len(head_refs), "pr_out") user_id_out = get_result_args(len(user_ids), "user_out") label_out = get_result_args(len(labels), "label_out") + team_out = get_result_args(len(teams), "team_out") arg_str = ", ".join( get_args_declaration(head_refs_args, "String!") + get_args_declaration(user_id_args, "String!") + get_args_declaration(label_args, "String!") + + get_args_declaration(team_org_args, "String!") + + get_args_declaration(team_slug_args, "String!") ) # NOTE: There are possible limitations here because we depend on PRs being returned in order of @@ -164,12 +184,21 @@ async def query_everything( label_str = "".join(len(labels) * ["{}: label (name: ${}) {{...LabelResult}},"]) label_str = label_str.format(*zip_and_flatten(label_out, label_args.keys())) + team_str = "" + for i in range(len(teams)): + team_str += ( + f"{team_out[i]}: organization(login: ${list(team_org_args.keys())[i]}) " + f"{{team(slug: ${list(team_slug_args.keys())[i]}) " + f"{{id, members(first: 100) {{nodes {{login}}, totalCount}}}}}}," + ) + multi_query_str = f""" query GetPrResults($owner: String!, $name: String!, {arg_str}) {{ repository(name: $name, owner: $owner) {{ id {request_str}{user_str}{label_str} }} + {team_str} }}""" if user_str: multi_query_str += """ @@ -222,6 +251,13 @@ async def query_everything( login id }} + ... on Team {{ + slug + id + organization {{ + login + }} + }} }} }} }} @@ -267,6 +303,8 @@ async def query_everything( **head_refs_args, **user_id_args, **label_args, + **team_org_args, + **team_slug_args, ) prs: List[Optional[PrInfo]] = [] @@ -278,17 +316,23 @@ async def query_everything( pr_label_ids = set() reviewers = set() reviewer_ids = set() + reviewer_teams = set() + reviewer_team_ids = set() assignees = set() assignee_ids = set() for label in this_node["labels"]["nodes"]: pr_labels.add(label["name"]) pr_label_ids.add(label["id"]) for revs in this_node["reviewRequests"]["nodes"]: - if not revs["requestedReviewer"]: + requested = revs["requestedReviewer"] + if not requested: continue - elif "login" in revs["requestedReviewer"]: - reviewers.add(revs["requestedReviewer"]["login"]) - reviewer_ids.add(revs["requestedReviewer"]["id"]) + elif "slug" in requested: + reviewer_teams.add(f"{requested['organization']['login']}/{requested['slug']}") + reviewer_team_ids.add(requested["id"]) + elif "login" in requested: + reviewers.add(requested["login"]) + reviewer_ids.add(requested["id"]) for revs in this_node["latestReviews"]["nodes"]: # Ignore self reviews and bot reviews (without a login) if not revs["viewerDidAuthor"] and "login" in revs["author"]: @@ -329,6 +373,8 @@ async def query_everything( title=this_node["title"], reviewers=reviewers, reviewer_ids=reviewer_ids, + reviewer_teams=reviewer_teams, + reviewer_team_ids=reviewer_team_ids, assignees=assignees, assignee_ids=assignee_ids, labels=pr_labels, @@ -376,12 +422,31 @@ async def query_everything( else: logging.warning("Couldn't find an existing label named {}".format(label)) + teams_to_ids = {} + teams_to_members: Dict[str, Optional[Set[str]]] = {} + for i, (org, slug) in enumerate(teams): + team_node = pr_result["data"][team_out[i]] + if team_node is not None and team_node["team"] is not None: + team_ref = f"{org}/{slug}" + teams_to_ids[team_ref] = team_node["team"]["id"] + members_node = team_node["team"]["members"] + member_logins = {m["login"] for m in members_node["nodes"]} + if members_node["totalCount"] > len(members_node["nodes"]): + # Team has more members than we fetched; we can't check membership reliably. + teams_to_members[team_ref] = None + else: + teams_to_members[team_ref] = member_logins + else: + logging.warning("Couldn't find a team matching {}/{}".format(org, slug)) + return ( pr_result["data"]["repository"]["id"], prs, names_to_ids, names_to_logins, labels_to_ids, + teams_to_ids, + teams_to_members, ) @@ -474,9 +539,10 @@ async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpd "labelableId": pr.id, }) - if pr.reviewer_ids: + if pr.reviewer_ids or pr.reviewer_team_ids: reviewers.append({ "userIds": list(pr.reviewer_ids), + "teamIds": list(pr.reviewer_team_ids), "clientMutationId": "revup", "pullRequestId": pr.id, "union": True, diff --git a/revup/topic_stack.py b/revup/topic_stack.py index a8ab17c..da1d0b4 100644 --- a/revup/topic_stack.py +++ b/revup/topic_stack.py @@ -50,6 +50,14 @@ def format_remote_branch(uploader: str, base_branch: str, topic: str, branch_for RE_TAGS = re.compile(r"^(?P[a-zA-Z\-]+):(?P.*)$", re.MULTILINE) +# Github team references use the form "org/team-slug". Anything with a "/" is treated as a team. +RE_TEAM = re.compile(r"^(?P[A-Za-z0-9][A-Za-z0-9-]*)/(?P[A-Za-z0-9_.-]+)$") + + +def is_team(name: str) -> bool: + return RE_TEAM.match(name) is not None + + TAG_REVIEWER = "reviewer" TAG_ASSIGNEE = "assignee" TAG_BRANCH = "branch" @@ -248,6 +256,13 @@ class TopicStack: # Github node ids of labels labels_to_ids: Optional[Dict[str, str]] = None + # Github node ids of teams, keyed by "org/slug" + teams_to_ids: Optional[Dict[str, str]] = None + + # Team member logins keyed by "org/slug". None means the team is too large to + # enumerate fully, and membership should be treated as unknown. + teams_to_members: Optional[Dict[str, Optional[Set[str]]]] = None + # Relative branch names to pr_info for those branches relative_infos: Dict[str, PrInfo] = field(default_factory=dict) @@ -587,10 +602,21 @@ async def populate_reviews( topic.tags[tag].add(user_target) if auto_add_users in ("r2a", "both"): - topic.tags[TAG_ASSIGNEE].update(topic.tags[TAG_REVIEWER]) + topic.tags[TAG_ASSIGNEE].update( + r for r in topic.tags[TAG_REVIEWER] if not is_team(r) + ) if auto_add_users in ("a2r", "both"): topic.tags[TAG_REVIEWER].update(topic.tags[TAG_ASSIGNEE]) + # Github does not support teams as assignees, so reject any team-style entries + # up front rather than silently ignoring them. + team_assignees = {a for a in topic.tags[TAG_ASSIGNEE] if is_team(a)} + if team_assignees: + raise RevupUsageException( + f"Topic '{name}' has team(s) listed as assignees, but Github only allows" + f" users as assignees: {sorted(team_assignees)}" + ) + # Track the last actually used topic for the relative-chain feature last_topic = name @@ -1097,14 +1123,25 @@ async def query_github(self) -> None: pr_targets = [] user_ids = set() + team_refs: Set[str] = set() labels = set() for _, topic, base_branch, review in self.all_reviews_iter(): pr_targets.append(review.remote_head) - user_ids |= topic.tags[TAG_REVIEWER] + for name in topic.tags[TAG_REVIEWER]: + if is_team(name): + team_refs.add(name) + else: + user_ids.add(name) user_ids |= topic.tags[TAG_ASSIGNEE] labels |= topic.tags[TAG_LABEL] labels.add(self.git_ctx.remove_branch_prefix(base_branch)) + team_tuples = [] + for team_ref in team_refs: + m = RE_TEAM.match(team_ref) + if m: + team_tuples.append((m.group("org"), m.group("slug"))) + relative_targets = set() # Add queries for relative branches at the end for _, topic, _, review in self.all_reviews_iter(): @@ -1120,8 +1157,15 @@ async def query_github(self) -> None: self.names_to_ids, self.names_to_logins, self.labels_to_ids, + self.teams_to_ids, + self.teams_to_members, ) = await github_utils.query_everything( - self.github_ep, self.repo_info, pr_targets, list(user_ids), list(labels) + self.github_ep, + self.repo_info, + pr_targets, + list(user_ids), + list(labels), + team_tuples, ) i = 0 @@ -1148,7 +1192,13 @@ def populate_update_info( """ if not self.topics: return - if self.names_to_ids is None or self.names_to_logins is None or self.labels_to_ids is None: + if ( + self.names_to_ids is None + or self.names_to_logins is None + or self.labels_to_ids is None + or self.teams_to_ids is None + or self.teams_to_members is None + ): raise RuntimeError("Need to query before updating") for topic in self.topics.values(): @@ -1200,13 +1250,40 @@ def populate_update_info( # Don't request reviewers that are already added, otherwise the request will clear # the "reviewed" status in the UI. + user_reviewer_tags = {r for r in topic.tags[TAG_REVIEWER] if not is_team(r)} + team_reviewer_tags = {r for r in topic.tags[TAG_REVIEWER] if is_team(r)} reviewer_ids = translate_if_exists( - topic.tags[TAG_REVIEWER], self.names_to_ids + user_reviewer_tags, self.names_to_ids ).difference(review.pr_info.reviewer_ids) reviewer_logins = translate_if_exists( - topic.tags[TAG_REVIEWER], self.names_to_logins + user_reviewer_tags, self.names_to_logins ).difference(review.pr_info.reviewers) + # A team may have been "resolved" by Github's code review assignment (the team + # gets dropped from reviewRequests and individuals get added). Re-requesting + # the team would trigger auto-assignment again, so skip teams whose members + # are already among the existing reviewers. + teams_to_request = set() + for team_ref in team_reviewer_tags: + if team_ref not in self.teams_to_ids: + continue + if team_ref in review.pr_info.reviewer_teams: + # Team is still pending on the PR; nothing to do. + continue + members = self.teams_to_members.get(team_ref) if self.teams_to_members else None + if members is not None and members & review.pr_info.reviewers: + overlap = sorted(members & review.pr_info.reviewers) + logging.warning( + f"Not re-requesting team '{team_ref}' on " + f"{review.pr_info.url or review.remote_head} because existing " + f"reviewer(s) {overlap} are members of it. Re-requesting would trigger" + " Github code review auto-assignment again." + ) + continue + teams_to_request.add(team_ref) + reviewer_team_ids = translate_if_exists(teams_to_request, self.teams_to_ids) + reviewer_team_refs = teams_to_request + assignee_ids = translate_if_exists( topic.tags[TAG_ASSIGNEE], self.names_to_ids ).difference(review.pr_info.assignee_ids) @@ -1229,9 +1306,11 @@ def populate_update_info( review.pr_update.is_draft = review.is_draft review.pr_update.label_ids = label_ids review.pr_update.reviewer_ids = reviewer_ids + review.pr_update.reviewer_team_ids = reviewer_team_ids review.pr_update.assignee_ids = assignee_ids review.pr_info.reviewers |= reviewer_logins + review.pr_info.reviewer_teams |= reviewer_team_refs review.pr_info.assignees |= assignee_logins review.pr_info.labels |= valid_labels @@ -1353,6 +1432,7 @@ async def update_prs(self) -> None: or review.pr_update.body is not None or review.pr_update.title is not None or review.pr_update.reviewer_ids + or review.pr_update.reviewer_team_ids or review.pr_update.assignee_ids or review.pr_update.label_ids or review.pr_update.is_draft is not None @@ -1423,11 +1503,11 @@ def print(self, skip_empty: bool) -> None: if review.new_commits: logging.debug(f"New head: {review.new_commits[-1]}") - reviewers = topic.tags[TAG_REVIEWER] + reviewers = set(topic.tags[TAG_REVIEWER]) assignees = topic.tags[TAG_ASSIGNEE] labels = topic.tags[TAG_LABEL] if review.pr_info: - reviewers = review.pr_info.reviewers + reviewers = review.pr_info.reviewers | review.pr_info.reviewer_teams assignees = review.pr_info.assignees labels = review.pr_info.labels if reviewers: diff --git a/tests/test_upload.py b/tests/test_upload.py index 9305a72..99f375b 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -491,6 +491,36 @@ async def test_both_copies_bidirectionally(self): assert "asn1" in topics.topics["alpha"].tags["reviewer"] +class TestUploadTeamReviewers: + @async_test + async def test_team_reviewer_kept_on_reviewer_tag(self): + async with GitTestEnvironment() as env: + await setup_repo(env) + await env.commit("feat\n\nTopic: alpha\nReviewer: myorg/backend", {"a.txt": "a"}) + + topics = await run_upload_pipeline(env) + assert "myorg/backend" in topics.topics["alpha"].tags["reviewer"] + + @async_test + async def test_team_assignee_raises(self): + async with GitTestEnvironment() as env: + await setup_repo(env) + await env.commit("feat\n\nTopic: alpha\nAssignee: myorg/team, realuser", {"a.txt": "a"}) + + with pytest.raises(RevupUsageException): + await run_upload_pipeline(env) + + @async_test + async def test_r2a_does_not_copy_team_into_assignees(self): + async with GitTestEnvironment() as env: + await setup_repo(env) + await env.commit("feat\n\nTopic: alpha\nReviewer: myorg/team, user1", {"a.txt": "a"}) + + topics = await run_upload_pipeline(env, auto_add_users="r2a") + assert topics.topics["alpha"].tags["assignee"] == {"user1"} + assert "myorg/team" in topics.topics["alpha"].tags["reviewer"] + + class TestUploadUserAliases: @async_test async def test_alias_replaces_reviewer_name(self):