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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Nov 19 2021, Victoria Dye wrote:

> Æ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.

Yes, sorry. That came off as quite snarky, it was just meant as a lame
joke.

>> 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.

Will improve the description etc. in a re-roll.

>> 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.

I'll retain more information in an incoming re-roll.

>> 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/pull.1072.git.1635990465854.gitgitgadget@gmail.com/

I thought that case was a good example of us creating a problem for
ourselves that we didn't need to have. as I noted downthread in:
https://lore.kernel.org/git/211104.86v918i78r.gmgdl@evledraar.gmail.com/

I.e. the "fedora" part didn't actually matter, it just so happened that
fedora had a relatively recent glibc/gcc, and when it was updated from
under us CI runs all over the place started failing.

Which is an issue that's entirely avoidable by pinning the dependency,
and not using *-latest.

In general I really don't see why who deploys software anywhere thinks
using this pattern of fluid dependencies is worth the hassle. It's
rather trivial to just run a cron script that bumps your dependencies,
commits and pushes.

It's not at all trivial to take software that downloads whatever
someone's random website has available at the moment and try to figure
out what state it was in last week, month or year when the state of that
third party website wasn't what it was today.

Anyway, I take your general point that these image names (or a derived
name) may be useful to someone somewhere.

I just thought this example was a bad one, because the only reason the
image name or update would have been unclear to anyone (as it was to me
when my CI started breaking due to this) is because we're using this
anti-pattern.