Subject: Re: [PATCH 1/2] CI: use shorter names that fit in UX tooltips

From: Victoria Dye <[email-masked]>
Date: Fri, 19 Nov 2021 16:55:11 -0500

Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Nov 19 2021, Victoria Dye wrote:
> 
>> Ævar Arnfjörð Bjarmason wrote:
>>> Change the names used for the GitHub CI workflows to be short enough
>>> to (mostly) fit in the pop-up tool-tips that GitHub shows in the
>>> commit view. I.e. when mouse-clicking on the passing or failing
>>> check-mark next to the commit subject.
>>>
>>> That description is truncated to 24 characters, with the 3 at the end
>>> being placed by "...".
>>>
>>> E.g. the full job name (visible at [1]):
>>>
>>>     "regular (linux-gcc-default, gcc, ubuntu-latest)"
>>>
>>> Will, when shown in the tool-tip be truncated to:
>>>
>>>     "CI/PR / regular (linu..."
>>>
>>> There's then a further limit in the expanded view where the job names
>>> are observably truncated to 44 characters (including "..."). I.e.:
>>>
>>>     "regular (linux-gcc-default, gcc, ubuntu-l..."
>>>
>>
>> Tooltips like the ones you've pointed out here appear intended to be an "at
>> a glance" view of the jobs (mostly for showing pass/fail/skip status) - each
>> job in the tooltip has a "Details" link that takes you to the job summary
>> and logs. In the current state, although the names of the are truncated in
>> the tooltip, the information is still easily accessible in the full workflow
>> details (one click away). For example, the details for the "linux-leaks" job
>> [1] tell me the image, compiler, and job name right at the top of the page.
>>
>> [1] https://github.com/git/git/runs/4214606314?check_suite_focus=true
>>
>>> With this change we shorten both the job names, and change the
>>> top-level "name" from "CI/PR" to "CI", since it will be used as a
>>> prefix in the tooltips. We also remove redundant or superfluous
>>> information from the name, e.g. "ubuntu-latest" isn't really needed
>>> for "linux-leaks", it'll suffice to say linux. For discovering what
>>> image runs that specifically we can consult main.yml itself.
>>>
>>
>> By optimizing for the tooltip, this patch shortens names to the point that
>> they're more difficult to interpret (w32 vs. w32/VS) and/or removes valuable
>> context about platform/image/etc. When a user *does* want more information
>> on the job, they now have to: 
>>
>> 1) know that the "CI/PR" job definition is in ".github/workflows/main.yml"
>> 2) parse through the file to find the job they want
>> 3) correlate that back to the job in the workflow details they're
>>    investigating. 
>>
>> That's a strictly worse experience for an extremely common use-case. What
>> use-case is this patch attempting to improve?
> 
> That I can click on the button that your co-workers implemented and see
> the relevant information about the job :)
> 

I'm sure you meant this in good faith, but I don't see how where I work is
relevant. GitHub is a tool you can use to develop Git, and I'm reviewing
because this patch would affect how I work on Git.

> Given that it's truncated we need to pick and choose what we display if
> we're not going to force the user to have to go to the full view every
> time.
> 

This is what I wanted to dig into by asking for a use-case. Which users do
you expect are using this tooltip view so heavily that what's displayed
there justifies this change? If you're one of those users, then waiting for
more feedback on this patch will hopefully provide a better idea of what the
"average" user finds helpful.

> I'll change s/w32/win/ etc, and there's room to move stuff around here,
> but I think it's fine to just not display that it's e.g. "ubuntu" or
> "fedora" at all. That's almost never been relevant.
> 
> If we were trying to do the opposite and lengthen the names to shove
> every bit of useful information in there at a glance I can think of 5-10
> things we'd put there before "fedora". libc/version, compiler/version,
> kernel/version etc.
> 

I'm sure there's plenty of information that would be helpful to have there;
however, that's not really a justification for removing some of what we
already have.

> Whether it's a recent Gentoo or Ubuntu is something that's OK to omit.
> 
> But maybe I'm wrong, are there cases you can think of where we really
> need "ubuntu" or "fedora" etc.?
> 

Yes, absolutely - knowing that `fedora-latest` was the image used in the
failing builds for v2.34.0-rc1 led to learning that the image was updated
the day before the build failed, which was instrumental in quickly
identifying the root cause of the bug [1]. In that case, I got the
information to start debugging from the web UI, *not* from digging into
`main.yml`. 

In general, easily finding what image a CI job was built on can help with
investigating bugs that arise from environmental differences. It's a minor
quality-of-life improvement, but it's no less significant than the benefit
you're suggesting this patch provides.

[1] https://lore.kernel.org/git/[email-masked]/

>>> The above "regular (linux-gcc-default, gcc, ubuntu-latest)" job name
>>> then becomes a 1=1 match to the "$jobname" used in
>>> "ci/run-build-and-tests.sh". A "( push" or " (pull_request" is then
>>> added implicitly as before (from the top-level "on" parameter in
>>> "main.yml"). In the tooltip we'll now show:
>>>
>>>     "CI / linux-leaks (pu..."
>>>
>>> We then have no truncation in the expanded view. See [2] for a
>>> currently visible CI run using this commit, and [3] for the GitHub
>>> workflow syntax involved being changed here.
>>>
>>
>> If the only problem this patch really "solves" is making some job names fit
>> a bit better into the tooltip and, I think the costs (namely the loss of
>> accessible contextual info) outweigh any potential benefits you gain. 
> 
> Yeah it's a trade-off for sure, but now you can't see some of the
> relevant information at all in some views, so it's a gain for
> accessibility too.
> 

It may be a "gain for accessibility" in this one particular case but, in my
opinion, the relative decrease in accessibility by outright removing
information from the *all* CI views is greater. Of course, I don't represent
*all* users, so I'm interested in hearing what others have to say. For now,
though, I'm still unconvinced this patch is beneficial.