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

Link to Zulip stream from GitHub issue is broken #701

Closed
camelid opened this issue Jul 17, 2020 · 28 comments · Fixed by #711
Closed

Link to Zulip stream from GitHub issue is broken #701

camelid opened this issue Jul 17, 2020 · 28 comments · Fixed by #711
Assignees
Labels
invalid This doesn't seem right

Comments

@camelid
Copy link
Member

camelid commented Jul 17, 2020

Issue

Rustbot's link

https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/Use.20sane.20defaults.20in.20x.py.20compiler-team.23326

Actual link

https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Improve.20defaults.20in.20x.2Epy.20compiler-team.23326

Diff

-https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/Use.20sane.20defaults.20in.20x.py.20compiler-team.23326
+https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Improve.20defaults.20in.20x.2Epy.20compiler-team.23326

Summary

I think there are two issues here:

  1. Rustbot has xxx instead of t-compiler.2Fmajor-changes
  2. The GitHub issue name was changed and Rustbot didn't update the link. I'm not sure if it's worth it, but maybe there's a way to listen for a title change and then update the link or at least post the new one?

Cc https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Improve.20defaults.20in.20x.2Epy.20compiler-team.23326/near/204243761

This issue has been assigned to @camelid via this comment.

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Alternatively, the solution to problem 2 is to just have the issue number in the Zulip topic and not have the name. Rustbot would post the name as the first message in the topic. The downside is that you have to open the topic to see what it's about, which kind of defeats the purpose of a topic!

@Mark-Simulacrum
Copy link
Member

(1) is supposed to work -- according to the Zulip developers, that part of the URL is ignored. We don't actually know the stream name so can't readily insert it.

(2) would be feasible though a bit painful. It looks like there's not a documented way to rename topics through the API. I suspect that most proposals that change titles should probably just open a new proposal, or we can edit the comment for now.

Note that the Github issue name changing shouldn't break links AFAIK -- the problem is that someone changed the title of the topic in Zulip without also updating the issue comment on GitHub.

@Mark-Simulacrum
Copy link
Member

Alternatively, the solution to problem 2 is to just have the issue number in the Zulip topic and not have the name. Rustbot would post the name as the first message in the topic. The downside is that you have to open the topic to see what it's about, which kind of defeats the purpose of a topic!

Yeah I think this is basically not going to work, since the topic being useful is much more valuable than preserving the link -- the link, can, after all be readily updated by folks.

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

@rustbot modify labels to invalid

@rustbot rustbot added the invalid This doesn't seem right label Jul 17, 2020
@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

(1) is supposed to work -- according to the Zulip developers, that part of the URL is ignored. We don't actually know the stream name so can't readily insert it.

We don't know the stream name? Don't we need to know it to create the topic?

@Mark-Simulacrum
Copy link
Member

No, you only need the stream ID. We could also add stream names to our configuration, but regardless the stream name doesn't matter because it's not the problem -- URLs with any characters instead of the stream name will work. e.g.,

https://rust-lang.zulipchat.com/#narrow/stream/233931-this-is-a-sweet-stream/topic/Improve.20defaults.20in.20x.2Epy.20compiler-team.23326

works quite fine.

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

So do you think it's worth it to fix this? For example, by detecting GitHub issue title changes and updating the Zulip title so that people don't do it manually

@Mark-Simulacrum
Copy link
Member

Generally speaking, sure, I'd love to fix this. However, I think it's going to be quite hard.

  • We cannot detect edits to topic titles on Zulip with the existing hook, which only pings our backend if the bot's name is mentioned on Zulip.
  • If we go the other way, there's no API endpoint that I can find that lets us edit the topic name on Zulip. So we can't easily do things that way either.

What we could do relatively easily, though, is to detect changes in the issue title on GitHub and post a comment asking someone to edit the topic in Zulip and update the link in the first comment by the bot.

An alternative to that would be to, on changes to the GitHub issue title, post a new comment on Zulip saying "hey this is the continued discussion from " into a new topic. I'd rather not do that though as diverging the two topics on Zulip seems kinda annoying.

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Or maybe we could just post something to the original Zulip topic saying that the GitHub issue was renamed?

@Mark-Simulacrum
Copy link
Member

The github issue being renamed shouldn't affect the Zulip topic at all in the sense that all links etc should still work... I think a comment on the github issue makes a lot more sense, personally.

@joshtriplett
Copy link
Member

Could we ask the Zulip folks for a "rename topic" API endpoint?

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Yeah, that's what I was thinking

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Okay, I just filed an issue: zulip/zulip#15846

Feel free to add to it!

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

@Mark-Simulacrum Does github::IssuesAction::Edited represent a GitHub issue rename?

@Mark-Simulacrum
Copy link
Member

Seems to be -- https://docs.github.com/en/developers/webhooks-and-events/webhook-events-and-payloads#issues -- but I can't be 100% sure.

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Changes represents the old version, right?

@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Might as well mark this as claimed

@rustbot claim

@rustbot rustbot self-assigned this Jul 17, 2020
@camelid
Copy link
Member Author

camelid commented Jul 17, 2020

Apparently, there is actually a way to rename topics: zulip/zulip#15846 (comment)

@Mark-Simulacrum
Copy link
Member

Sounds great! We probably want to do that here then and edit the first comment ourselves.

@camelid
Copy link
Member Author

camelid commented Jul 18, 2020

Does Rustbot keep track of its comments? If not, maybe we edit the original issue text and add a Zulip link there instead of posting a separate MCP comment? (I.e., put some of the contents of this in the issue post.)

@camelid
Copy link
Member Author

camelid commented Jul 18, 2020

Is there a way to access the update-message endpoint from the zulip module?

@camelid
Copy link
Member Author

camelid commented Jul 18, 2020

Hmm, I think Rustbot will need to keep track of the message ID for the topic it creates (unless it is already). Maybe it makes more sense to just have someone manually rename the topic?

@Mark-Simulacrum
Copy link
Member

My impression was that if we specify change_all for propagate mode, we could change the topic by just posting a new message to it (and maybe using that message's ID?).

I don't believe we have an impl for the update message endpoint, so you'd have to add one.

@camelid
Copy link
Member Author

camelid commented Jul 18, 2020

My impression was that if we specify change_all for propagate mode, we could change the topic by just posting a new message to it (and maybe using that message's ID?).

I didn't think of that :)

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

The name of the topic was not changed. I started a new topic then linked to the new topic from the old one. The old one should still work: https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Use.20sane.20defaults.20in.20x.2Epy.20compiler-team.23326

@camelid
Copy link
Member Author

camelid commented Jul 18, 2020

Huh, it looks like we might have an additional issue here then.

For the original topic:

Rustbot link

https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/Use.20sane.20defaults.20in.20x.py.20compiler-team.23326

Actual link

https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Use.20sane.20defaults.20in.20x.2Epy.20compiler-team.23326

If we compare the Rustbot link (and elaborate the xxx to the full stream name) against the actual link, the only difference that I see is that the actual link encodes the . in x.py as .2E, whereas Rustbot encodes it as just ..

@Mark-Simulacrum Is this is an error with string encoding?

@camelid
Copy link
Member Author

camelid commented Jul 18, 2020

From what I see in the rest of the link, it looks like Zulip encodes special characters as a . and then the ASCII hex code. Because of that, it seems a . must be encoded as its hex code.

@Mark-Simulacrum
Copy link
Member

It's quite possible there's a bug in that code, I made a quick and best effort attempt at replicating what the Zulip codebase is doing - PRs appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
5 participants