channable / hoff

A gatekeeper for your commits
Apache License 2.0
41 stars 3 forks source link

Fix web display of merge trains #178

Closed rudymatela closed 2 years ago

rudymatela commented 2 years ago

Closes: #162 Closes: #163

This PR fixes the web display of merge trains:


Before merge actions (unchanged)

0-before

:arrow_up: Nothing changes when all PRs are waiting for review.

These PRs were approved in the following order:


Before (repository view)

before-repo

:arrow_up: note PR #142 (Squidward) is missing and PRs are not listed in order of approval.


After (repository view)

repo-after

:arrow_up: correct order, no duplicates, icons


Before (global view)

before-global

:arrow_up: note PR #142 (Squidward) is missing and PRs are not listed in order of approval. There is one PR that is duplicate!


After (global view)

global-after

:arrow_up: correct order, no duplicates, icons


rudymatela commented 2 years ago

There were changes on master. Better rebase now than later...

rudymatela commented 2 years ago

This is ready for review.

@frank-channable or @alex-mckenna can either of you please review? (whoever is free first -- first-come-first-merged)

rudymatela commented 2 years ago

The emoji indicators are cute, but as we have more they become less obvious. The blue diamond for promoted feels odd, but I can't think of a good choice for this. Text only also has problems though, because it becomes a bit more difficult to just scan the page with your eyes... What do you think?

It does feel hard to parse in the case I presented. However in practice we expect to seldom encounter icons other than :yellow_circle: and :x::

The interface state shown here is a bit of an extreme example illustrating all states:

extreme

In practice we normally see something like:

Other icons will be more like exceptions:

... or only shown during a brief window:

PS: here are the two queries:

journalctl -u hoff -S2022-08-01 | grep "Posted.*Failed to rebase"
journalctl -u hoff -S2022-08-01 | grep "Posted.*Merge rejected:"
alex-mckenna commented 2 years ago

Hmm, okay. That seems reasonable then. Thanks for adding the comment :+1:

PS: Good to know, you don't need sudo for those commands by the way

rudymatela commented 2 years ago

@alex-mckenna Thanks for the review. :tada:

@OpsBotPrime merge :ship: :shipit: :rocket:

OpsBotPrime commented 2 years ago

Pull request approved for merge by @rudymatela, rebasing now.

OpsBotPrime commented 2 years ago

Rebased as 00dd4adcb0c1b46ef16426c8a1b15b29a8667327, waiting for CI โ€ฆ

OpsBotPrime commented 2 years ago

CI job :yellow_circle: started.