-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sync zulip stream membership #1604
base: master
Are you sure you want to change the base?
Conversation
Just so I'm clear, is the intention that this is only for private streams? I'm a little unclear, but it looks like it removes people if they are not explicitly listed in the team database, is that correct? Can you add some docs to https://github.com/rust-lang/team/blob/master/docs/toml-schema.md documenting the new TOML structure? |
Yeah, this would primarily be used for private streams. |
f9c7d59
to
b5878de
Compare
Added |
The Also we could use flatten to get rid of the duplicated code 👍 |
b5878de
to
af20765
Compare
Fixed the test and removed a bunch of the duplicated code |
af20765
to
d912ce0
Compare
Overall looks good to me! I just left a few minor comments 👍 |
d912ce0
to
c7ace6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good from my perspective.
Maybe we can merge it once rust-lang/sync-team#92 is also approved?
See rust-lang/sync-team#92 for the other half of this.
t-compiler want to use our private Zulip stream for some communications but we've held off on doing this as the membership of that stream is manually managed, that is inconvenient and we sometimes forget. In testing this patch, I found the stream membership was still out-of-sync!
I've tested this locally with the sync-team changes locally and absent the untested parts noted in that PR's description, it all works.
I've deliberately avoided adding an exhaustive schema for the streams, just their name as that's all that is necessary to adjust the membership, and that's all that the compiler team needs.
There's a reasonable amount of duplication with the types and functions for Zulip user groups, but we'll likely extend these types to manage more about streams, I think that's okay. Everything I've added should be reusable and compatible with representing streams fully, but I'll leave that for other patches like #1244.