Skip to content

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Sep 6, 2025

The previous validate_zulip_group_ids check logic is not quite correct in two ways:

  1. The previous logic was too strict, in that it does not properly account for the case where a team member may not be included in any team-associated zulip user groups (such as via the excluded-people mechanism).
  2. The previous logic I think was also too loose, in that it is possible to attach additional extra people via extra-people. It is possible to include a person whose people/ entry contain only a github handle, and not also a zulip id.

Example

Consider the cloud-compute marker-team:

name = "cloud-compute"
kind = "marker-team"

[people]
leads = []
members = [
   "have_zulip_id",
   "no_zulip_id",
]

[[zulip-groups]]
name = "cloud-compute"
extra-people = [
   "extra_no_zulip_id",
]
excluded-people = [
   "no_zulip_id",
]

The "all zulip group members must have a zulip-id" validation should fail, but not against no_zulip_id, because that member is not part of any zulip groups associated with cloud-compute team. It should error on extra_no_zulip_id however.

Review remarks

Let me know if these validations have any tests... I can't figure out if the static-api tests are intended for this purpose, I don't think so. I tested these two cases manually, and the revised logic seems to behave as I would expect it to.

The previous `validate_zulip_group_ids` check logic is not quite correct
in two ways:

1. The previous logic was too *strict*, in that it does not properly
   account for the case where a team member may not be included in any
   team-associated zulip user groups (such as via the `excluded-people`
   mechanism).
2. The previous logic I think was also too *loose*, in that it is
   possible to attach additional extra people via `extra-people`. It is
   possible to include a person whose `people/` entry contain only a
   github handle, and not also a zulip id.

# Example

Consider the `cloud-compute` marker-team:

```markdown
name = "cloud-compute"
kind = "marker-team"

[people]
leads = []
members = [
   "have_zulip_id",
   "no_zulip_id",
]

[[zulip-groups]]
name = "cloud-compute"
extra-people = [
   "extra_no_zulip_id",
]
excluded-people = [
   "no_zulip_id",
]
```

The "all zulip group members must have a zulip-id" validation should
fail, but not against `no_zulip_id`, because that member is not part of
any zulip groups associated with `cloud-compute` team. It *should* error
on `extra_no_zulip_id` however.
@jieyouxu jieyouxu added the S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. label Sep 6, 2025
@jieyouxu jieyouxu added the T-infra Relevant to the infrastructure team. label Sep 6, 2025
@jieyouxu jieyouxu requested a review from marcoieni as a code owner September 6, 2025 07:53
@jieyouxu
Copy link
Member Author

jieyouxu commented Sep 6, 2025

Context: the zulip groups I was trying to create for infra announcement channel.

cf. #t-infra > Communication method for dev desktops @ 💬

Copy link

github-actions bot commented Sep 6, 2025

Dry-run check results

[WARN  sync_team] sync-team is running in dry mode, no changes will be applied.
[INFO  sync_team] synchronizing github
[INFO  sync_team] 💻 Team Diffs:
    📝 Editing team 'rust-lang/all':
      Adding member 'graciegregory' with member role
    📝 Editing team 'rust-lang/community-survey':
      Adding member 'graciegregory' with member role
    📝 Editing team 'rust-lang/wg-triage':
      Adding member 'edmilsonefs' with member role

@marcoieni
Copy link
Member

Does the old logic require that all members of teams have a zulip id? If yes, are we loosing this property with this change?

@jieyouxu jieyouxu added S-waiting-on-author Status: waiting on PR author and removed S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. labels Sep 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: waiting on PR author T-infra Relevant to the infrastructure team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants