Skip to content
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

Add Zulip stream representation #1244

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jan 20, 2024

This PR adds a representation of Zulip streams to the team data. The idea is to represent streams in the TOML files, so that new streams can be declaratively created and their permissions (private/public) and membership (Zulip groups) can be modified.

This PR proposes attaching streams to teams, to avoid creating a separate streams directory. This means that each team is a an owner of a stream, but this doesn't need to be enforced anywhere, it can be only used for "bookkeeping" purposes. To add access to a stream, users can enumerate Zulip groups that have access to it.

@ehuss
Copy link
Contributor

ehuss commented Jan 20, 2024

Nice!

I'm not quite clear how the groups setting would work. My understanding is that stream access is solely based on who is subscribed, and subscription is individual-based (you can invite an entire group, but they end up as individuals, and you can't remove a group unless you iterate over each member of the group).

Does this mean that sync-team will force team members to be subscribed? Would it remove non-team members? If someone unsubscribes from a stream, would they be forced back into it?

Also, there are several streams that are not part of a team, or are shared by multiple teams. For the shared, would it just arbitrarily pick one team to own it? I suppose I don't quite understand why this would be part of a team.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 20, 2024

You make good points :) I created this PR exactly to discuss things like that.

  • Streams without a team: I'm not sure how to resolve that without creating streams as first class items, i.e. creating e.g. a zulip-streams directory and create one TOML file per stream. As a hackish alternative, we could add this shared streams under the all team or something like that?
  • Stream membership: I'm also not sure how to resolve that. If stream membership isn't tied to groups, we'd have to enumerate the individual members of private streams?

Also, one rather annoying team that you mentioned is that people can unsubscribe on their own. It's unclear what sync-team should do in that case (it probably couldn't distinguish it from the baseline case where the member hasn't been subscribed to the stream yet).

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional thoughts to add to @ehuss's:

  • It's a bit annoying that we might want to add a team to a Zulip stream but we're forced to add a Zulip user group. For example, if I create a T-compiler stream, I can't configure that to add the the compiler team, I must instead add the T-compiler user group. This requires knowing that user group name and changing it if the user group name changes. I think instead we should have the configuration understand the higher level concept of including teams instead of Zulip user groups.
    • On a related note beyond the scope of this PR - it might be nice to change the configuration such that teams can simply opt into having a Zulip user group for their team without having to give that user group a specific name - then the team repo can create user groups for all teams that opt into this that fit into a unified convention of how those user groups are named.
  • Re: unsubscribing - one possible solution is for there to be an excluded-subscribers key that allows folks to exclude themselves. The validator would confirm that those who are excluded would otherwise be included. Then sync-team can ensure that excluded-subscribers are not actually added.

[[zulip-streams]]
# The name of the Zulip stream (required)
name = "t-overlords"
# Zulip groups that will have access to the stream if it is private (optional)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what we want the behavior of this to be when the stream isn't private. Also, given that this stream is defined on the team itself, would any of the zulip-groups automatically be included here?

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 24, 2024

t's a bit annoying that we might want to add a team to a Zulip stream but we're forced to add a Zulip user group. For example, if I create a T-compiler stream, I can't configure that to add the the compiler team, I must instead add the T-compiler user group. This requires knowing that user group name and changing it if the user group name changes. I think instead we should have the configuration understand the higher level concept of including teams instead of Zulip user groups.

I understand the motivation, but this is already how it works for GitHub teams, and it's caused by the same thing - one team can have multiple Zulip groups (and GH teams). Would you rather include all Zulip groups of a team when you say that a team has access to a stream? And what if a team and a Zulip group has the same name?

Re: unsubscribing - one possible solution is for there to be an excluded-subscribers key that allows folks to exclude themselves. The validator would confirm that those who are excluded would otherwise be included. Then sync-team can ensure that excluded-subscribers are not actually added.

It sounds quite annoying to have to send a PR to unsubcribe from a stream. FWIW, @pietroalbini mentioned that he doesn't consider this to be a big issue. Is it common to want to unsubscribe from private streams?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants