search

rust-lang / triagebot

Automation/tooling for Rust spaces
https://triage.rust-lang.org
Apache License 2.0
172 stars 73 forks source link

Update GitHub issue with renamed Zulip topic link #1589

Closed ghost closed 2 years ago

ghost commented 2 years ago

Post additional comment to the GitHub issue with an updated Zulip topic link when the issue/topic is renamed. Addendum to #1228 and #1588.

ghost commented 2 years ago

Again, please keep in mind I have no way to test this myself (and I'm also a total Rust noob...). Hopefully this change is correct. 😊

ghost commented 2 years ago

cc @Mark-Simulacrum since you approved my other change

apiraino commented 2 years ago

@skippy10110 @Mark-Simulacrum I am totally with the rationale for this patch (it is disorienting when a user clicks a link in a github comment and lands on a "dead" Zulip topic), but I'm slightly unsure about the implementation discussed in #1588.

Renaming a Github issue would generate comments in the issue itself that could potentially add noise. Github sometimes also compresses messages that are deemed "not relevant" to the discussion (example: some technical reports from our bot) so I wonder if these comments would end up being hidden.

Another way I could think to solve the issue of orphaned Zulip topics is: when a Github issue change name, send a message to the previous Zulip topic and forward users to the new topic ("Issue has been renamed, new Zulip topic here") (can't remember rn if it's already working like that).

I'm just thinking aloud here, so maybe my suggestion is not great :) What do you think?

ghost commented 2 years ago

You make an excellent point. This is definitely a clunky workaround, hopefully Zulip will get proper topic permalinks at some point.

I like your idea about a message on the previous topic. With the full topic rename implemented in #1588, we could then post an additional comment back under the old topic name pointing to the new topic name. The old topic name would then only have that single comment, and this would also mean anyone who had bookmarked the old topic isn't broken either.

I think that would work with multiple topic renames as well (like what happened on my MCP). A trail of breadcrumbs would be left behind with a single comment under the original topic name, which then points to the next rename, which then points to the final rename where all the discussion has already been moved.

Mark-Simulacrum commented 2 years ago

Doesn't Zulip already post such a message to both the old and new streams?

ghost commented 2 years ago

I didn't get the impression that it does, but maybe I'm wrong. There's only the "The associated GitHub issue has been renamed. Renaming this Zulip topic." that triagebot itself posts AFAIK.

I'm kind of flying blind here, but do you have access to rename a topic on Zulip and see if such a message is present under the old URL? If that was the case we wouldn't need any follow up fix at all and #1588 would be sufficient on its own.

Or is it very hard to setup a dev Zulip/triagebot instance to try these things out myself?

Mark-Simulacrum commented 2 years ago

I would test the end-to-end workflow -- all merged PRs should have already deployed, so you can likely open a compiler-team MCP (for example) and rename that, just marking it as test should be OK.

Setting up a dev instance for triagebot, Zulip, and GitHub is likely pretty annoying to do today, unfortunately.

ghost commented 2 years ago

Cool, I'll try testing it out with a dummy MCP then if you think that's ok. I already filed on the wrong repo originally (compiler-team), and then I filed incorrectly at lang-team next before I got it right, and then I ended up with extra streams from my renames.... so I was a little hesitant to make more noise. 😁

ghost commented 2 years ago

Ok, I've verified that everything in the topic does get correctly moved by #1588 now, but the old topic URLs do indeed get broken.

Broken:

Current:

Accessing the old URLs doesn't provide any indication of the renamed topic: image

In light of that, I prefer the suggestion from @apiraino, where we leave a single comment behind under the old topic name pointing to the new topic name. We could still apply the fix in this PR as well if we wanted to leave a comment with the updated link and save following the "trail of breadcrumbs", but it wouldn't be strictly necessary.

ghost commented 2 years ago

I left a review comment already, but just wanted to note that this PR as-is currently has a bug that would prevent it from working. I'm planning to close this in favor of #1590 instead if that lands, so haven't bothered to fix it.

Mark-Simulacrum commented 2 years ago

Sounds like we should close this now. Thanks!