Page MenuHomePhabricator

Leave a comment on the Gerrit change when it is scheduled for a backport
Closed, ResolvedPublicFeature

Description

It would be nice to be able to see on the Gerrit side when someone has scheduled a particular change for a backport window. The tool could leave a comment that says something like:

Scheduled for deployment in the 2024-06-05 06:00 SF backport window (https://wikitech.wikimedia.org/wiki/Deployments#deploycal-item-20240605T1300)

Event Timeline

The magic for figuring out the anchor tag is to take the UTC time of the window and format it as %Y%m%dT%H%M. https://wikitech.wikimedia.org/wiki/Module:Deployment_schedule#L-67

That message seems reasonable.

Random thoughts:

There was concern about adding too many comments in Gerrit history in T323750: Provide early feedback when a patch has job failures. As there is already a JS plugin for this tool, is it possible to use client-side code to fetch and display the information?

There was concern about adding too many comments in Gerrit history in T323750: Provide early feedback when a patch has job failures. As there is already a JS plugin for this tool, is it possible to use client-side code to fetch and display the information?

well, after thinking about this for more than 5 seconds 😅 the volume of messages should be low enough that there isn't any concern about adding additional messages in Git repo history. (And this is valuable metadata to have in the git history anyway IMHO.)

In case it's helpful to you, there's some prior art for bot comments in Gerrit here and setup is described in https://wikitech.wikimedia.org/wiki/Tool:Early_warning_bot.

Scheduled for deployment in the 2024-06-05 06:00 SF backport window (https://wikitech.wikimedia.org/wiki/Deployments#deploycal-item-20240605T1300)

Instead of "06:00 SF" could we use "UTC morning backport window", "UTC afternoon backport window" and "UTC late backport window"?

Instead of "06:00 SF" could we use "UTC morning backport window", "UTC afternoon backport window" and "UTC late backport window"?

Yeah, it just needs a bit of wikitext parsing to collect it again. The "id" for a deployment window that is present in the wikitext and passed around by the app is a string like "2024-06-05 06:00 SF" taken from the when parameter of a Template:Deployment calendar event card instance. The "UTC $TIME backport window" message can be extracted from the window parameter of the matching template instance.

I think you are thinking along similar lines as @kostajh here, but please tell me more about how you imagine this addition to the comment rendering in Gerrit.

Is there a way to write a URL + text in a Gerrit comment so that it renders as linked text and not the literal URL? Or would you be expected to see it rendered something like https://zonestamp.toolforge.org/1717704000?

Does this lead to different data for the end user than a deep link to a Deployments page like https://wikitech.wikimedia.org/wiki/Deployments#deploycal-item-20240605T1300. I think the https://wikitech.wikimedia.org/wiki/MediaWiki:Gadget-site-deploycal.js default gadget already renders the window's timestamp in the browser's current timezone.

Instead of "06:00 SF" could we use "UTC morning backport window", "UTC afternoon backport window" and "UTC late backport window"?

Having the window name might be nice. But only the window name (without the timestamp) would add an extra step in a few cases I can think of:

  1. Windows move times throughout the year which makes historical data harder—results in an extra step to find the window time at the time the patch was scheduled; e.g. schedule a patch for deploy July 2024 to UTC late window that means 20:00UTC (13:00 PDT). schedule a patch for deploy Dec 2024 to UTC late window that means 21:00UTC (13:00 PST). And if we change window times at some point in the future, this problem is even worse.
  2. Showing the time (if it were localized) would save the step of having to figure out what time the window is happening (which is what I just had to do for the previous example :)).

Is there a way to write a URL + text in a Gerrit comment so that it renders as linked text and not the literal URL? Or would you be expected to see it rendered something like https://zonestamp.toolforge.org/1717704000?

AFAIK it is not possible. You have to provide the plain URL, without the option to specify the link text.

Is there a way to write a URL + text in a Gerrit comment so that it renders as linked text and not the literal URL? Or would you be expected to see it rendered something like https://zonestamp.toolforge.org/1717704000?

AFAIK it is not possible. You have to provide the plain URL, without the option to specify the link text.

This was my understanding last week too, but poking around I found an upstream bug for Markdown support in the Gerrit UI and to my surprise it was marked Fixed. https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/1041168/1#message-c2f554f56432fc822c3c118917abd0c14e400bfe shows that our Gerrit is supporting quite a bit of Markdown rendering.

I also found some cute things that are possible with https://gerrit.wikimedia.org/r/Documentation/config-robot-comments.html in local testing. I plan on spending a bit more time working out some options locally before attempting to reproduce on our Gerrit deployment and asking for feedback from interested folks.

bd808 changed the task status from Open to In Progress.Jun 11 2024, 4:40 PM
bd808 claimed this task.
bd808 triaged this task as Medium priority.

I am working on some options for this to show folks and ask which they like better.

I made a helper function that can build a message describing the scheduled deployment and providing links to both the [[Deployment]] page and zonestamp.toolforge.org as suggested by @thcipriani in T366763#9866184. This message uses the markdown feature and produces messages that look something like:

Scheduled for deployment in the Thursday, June 13 UTC late backport window at 2024-06-13 20:00 UTC

With that handled the next question is how best to attach this message to a Gerrit change. The two options I have tried so far are as a comment (https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/1041168/1#message-7e0c9ac89641970fd815b97715cde671bb3662b1) and as a "robot comment" (https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/1041168/1#message-1754316a46f2c3918c22c4c7972ef2baa64ed4e7).

Here is what those two options look like in collapsed form:

Screenshot 2024-06-11 at 17.30.25.png (146×1 px, 48 KB)

And here they are expanded:

Screenshot 2024-06-11 at 17.32.35.png (582×1 px, 134 KB)

Here is what wikibugs reported as these comments were added to the change:

[22:06]  < wikibugs> (CR) ScheduleDeploymentBot: "Scheduled for deployment in the [Thursday, June 13 UTC late backport window](https://wikitech.wikimedia.org/wiki/User:BryanDavis/Sandbox/D" [mediawiki-config] - https://gerrit.wikimedia.org/r/1041168 (owner: BryanDavis)

[22:06]  < wikibugs> (CR) ScheduleDeploymentBot: [DNM] Testing things in Gerrit UI (1 comment) [mediawiki-config] - https://gerrit.wikimedia.org/r/1041168 (owner: BryanDavis)

My personal preference is for the plain comment version. I find the robot comment to be very nice as an inline comment, but I also find that style of comment to be less useful for showing something that affects the entire change and not just some code review aspect. All robot comments are attached to files in the change without an ability to make that sort of stylized output show for the "review" that the comment is attached to.

  • Should probably use robot comment for gerrit so it can be easily differentiated from review.

I just found upstream at https://gerrit-review.googlesource.com/Documentation/config-robot-comments.html that robot comments are deprecated in newer Gerrit versions, so maybe we shouldn't choose that path. The upstream replacement is the checks api which is pretty great for reporting linter failures and things like that, but it seems like it would be a stretch to use that system to report a scheduled deployment.

[23:50]  <    bd808> Feedback on what style of comment schedule-deployment should leave on a Gerrit change to signify that the change is scheduled for backport is welcome -- https://phabricator.wikimedia.org/T366763#9882611
[00:05]  < MatmaRex> bd808: i think it would be nice to use a normal comment style that allows replying to it, so that we can clarify it if the deployment gets cancelled or something.
[00:06]  < MatmaRex> neither of those seems to allow normal replies :/
[00:07]  <    bd808> MatmaRex: hmm.. so you would like to see it as an open issue on the review?
[00:08]  < MatmaRex> hmm, i guess? i didn't really think of that. but i suppose it does represent an action item waiting to be done
[00:09]  < MatmaRex> or it could be marked as a resolved comment
[00:09]  < MatmaRex> (i feel like most people don't really pay attention to that anyway)
[00:10]  <    bd808> yeah, I think that would end up looking like the robot comment version but as you said you would be able to reply to it. Let me mess about a bit and see what options there might be for that.
[00:10]  < MatmaRex> i wouldn't mind either of the options you've already implemented, though

Poking around in the docs and my web console a bit, I think the magic to get the output that @matmarex is asking to see is sending the message body as a comment attached to the /PATCHSET_LEVEL magic path. This seems to be what the web UI does when you use the "reply" button to leave a comment as a human.

My personal preference is for the plain comment version. I find the robot comment to be very nice as an inline comment, but I also find that style of comment to be less useful for showing something that affects the entire change and not just some code review aspect. All robot comments are attached to files in the change without an ability to make that sort of stylized output show for the "review" that the comment is attached to.

I think a plain comment is better. However, I think it would make sense to add autogenerated:scheduledeployment to the tag attribute on the comment, so that these can be filtered out when looking at patches with many comments (from humans). In practice, there probably are going to be a lot of human comments on deployment patches, but it seems like a good idea to implement. From the docs:

Apply this tag to the review comment message, votes, and inline comments. Tags with an 'autogenerated:' prefix may be used by CI or other automated systems to distinguish them from human reviews. If another message was posted on a newer patchset, but with the same tag, then the older message will be hidden in the UI. Suffixes starting with ~ are not considered, so autogenerated:my-ci-system~trigger and autogenerated:my-ci-system~result will be considered being the same tag with regards to the hiding rule.

On a related note, I wonder if the ScheduleDeploymentBot should also comment on the patch on master that had a cherry-pick created. I think that would provide a useful path to see what happened with a particular patch.

I think a plain comment is better. However, I think it would make sense to add autogenerated:scheduledeployment to the tag attribute on the comment, so that these can be filtered out when looking at patches with many comments (from humans).

The current code includes a tag of autogenerated:{settings.GERRIT_ROBOT_ID} on the reviews it leaves. The default settings.GERRIT_ROBOT_ID value is wm-schedule-deployment which matches the name of the associated Gerrit plugin.

[00:05]  < MatmaRex> bd808: i think it would be nice to use a normal comment style that allows replying to it, so that we can clarify it if the deployment gets cancelled or something.

Demo comment at https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/1041168/1#message-0176bc821ab7bc3ebd3ab3c7da38b87482077d60:

Screenshot 2024-06-12 at 10.37.00.png (64×1 px, 31 KB)

Screenshot 2024-06-12 at 10.37.21.png (354×1 px, 75 KB)

Note: The screenshot does not show reply and quote buttons on the expanded view because it was taken from an unauthenticated session. Logged in it looks like this for me:

Screenshot 2024-06-12 at 10.51.11.png (420×2 px, 85 KB)

I didn't see a Wikibugs announce of this change and was worried that the autogenerated:wm-schedule-deployment tag (T366763#9885001) somehow caused that to fail. Deeper investigation makes me think the announce failure was just some sort of accident of timing related to a Kubernetes issue in Toolforge. A new attempt after the k8s issue was resolved produced the expected output:

[17:27]  < wikibugs> (CR) ScheduleDeploymentBot: "Scheduled for deployment in the [Thursday, June 13 UTC late backport window](https://wikitech.wikimedia.org/wiki/User:BryanDavis/Sandbox/D" [mediawiki-config] - https://gerrit.wikimedia.org/r/1041168 (owner: BryanDavis)

On a related note, I wonder if the ScheduleDeploymentBot should also comment on the patch on master that had a cherry-pick created. I think that would provide a useful path to see what happened with a particular patch.

Would you mind breaking this out into a new feature request @kostajh? I think this is an interesting idea, but I also think it will need a few rounds of discussion about the wording and type of comments to apply on the upstream change. It also will need some investigation into what information the tool can use to locate the upstream change. I'm fairly certain that Gerrit is tracking this with structured data, but I haven't needed to process that data yet.

Change #1041168 had a related patch set uploaded (by BryanDavis; author: BryanDavis):

[operations/mediawiki-config@master] [DNM] Testing things in Gerrit UI

https://gerrit.wikimedia.org/r/1041168

Mentioned in SAL (#wikimedia-cloud) [2024-06-12T22:57:59Z] <wmbot~bd808@tools-bastion-12> Built new image from 52a9d4a4 (T366763)

Mentioned in SAL (#wikimedia-cloud) [2024-06-12T22:59:13Z] <wmbot~bd808@tools-bastion-12> Restart to pick up new container image (T366763)

Change #1041168 abandoned by BryanDavis:

[operations/mediawiki-config@master] [DNM] Testing things in Gerrit UI

Reason:

Done with these tests

https://gerrit.wikimedia.org/r/1041168

The current code includes a tag of autogenerated:{settings.GERRIT_ROBOT_ID} on the reviews it leaves. The default settings.GERRIT_ROBOT_ID value is wm-schedule-deployment which matches the name of the associated Gerrit plugin.

Ah, perfect. (I had searched in the GitLab repo for the project for autogenerated and didn't see it, but that was because the code wasn't in GitLab yet.)