Added support for jobs with long filenames

[shayn-orca]

Fixes #7642

This is important for Orca since we use matrix steps whose names get pretty long sometimes.

We've also built and tested this manually and it works great for our use case.

[williammartin]

Yeh this is looking pretty much what I had in mind, good job! I'd really appreciate it if you could check how this plays with multi-byte chars as mentioned in the other comment.

[williammartin]

Could you do me a favour and create and run a workflow with some job names that include unicode chars? Something like:

😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅

Should be more than enough (something like kanji would be good too). I think this implementation is going to:

1. Count bytes rather than chars
2. Slice into the middle of a multi-byte char

I don't know what the underlying zip file restriction accounts for, but I'm pretty sure reading the code that the server is dealing in characters (runes in go) rather than bytes.

[shayn-orca]

This is super tricky - encoding, two-code vs three-code etc.

I also have no idea what the server does and what ZIP does, which ZIP version are we talking about, etc. - sounds brittle to change all these things here. It should be a different mechanism alltogheter - hash the job name, for example.

Could we separate this to a different PR? For our use case English should be enough.

[williammartin]

Yes, it is super tricky and I'm not really sure how to solve it right now.

I'd be willing to accept this PR if it were a strict improvement but I don't think I can accept it as is because it could break valid workflows, if my suspicion is correct (hence, me asking you to validate it). Consider the following case:

• A workflow with a job name that is greater than 89 bytes but less than 90 "chars" (in server-side C# UTF-16 land)
• The server does no truncation because it is less than 90 chars
• This PR slices the name and tries to match to the log file

In such a case, there will be a regression in behaviour. If we could resolve this I would accept the PR without having to handle every other nuanced case.

It should be a different mechanism alltogheter

Yes, I've spoken to the platform team before and have raised it again that the current solution is extremely fragile and I would like a manifest file (or some other solution) but I don't expect we'll get that any time soon, if ever.

FWIW here is the server side truncation function:

string TruncateAfter(string str, int max)
{
    string result = str.Length > max ? str.Substring(0, max) : str;
    result = result.Trim();
    return result;
}

[williammartin]

At a glance I think the following snippet would match the C# behaviour but I have not validated it:

func TruncateAfter(str string, max int) string {
	// Encode the string to UTF-16 to count code units
	utf16Encoded := utf16.Encode([]rune(str))
	if len(utf16Encoded) > max {
		// Decode back to UTF-8 up to the max length
		str = string(utf16.Decode(utf16Encoded[:max]))
	}
	return strings.TrimSpace(str)
}

This encodes the string as UTF-16 as used by C#, then compares the actual count of UTF-16 code units (as str.Length would do in C#), then slices the encoded utf-16 string and decodes that back to UTF-8.

[williammartin]

@shayn-orca, are you still interested in tackling this with the truncation logic I outlined above?

[shayn-orca]

@williammartin yes - it's on my TODO list, I'll probably get to it this week/next week.

Still now sure how to properly test it, though.

[williammartin]

Still now sure how to properly test it, though.

I would be comfortable seeing a test that demonstrates the correct slicing of utf-16 characters, that mimics the server (so, we don't need to care about grapheme clusters for example).

Specifically, I would create a real workflow on github.com that has 90~ multi-byte chars (choose your favourite kanji or non-grapheme cluster emoji?) in a job name, and then use the CLI to download the .zip, open it up and see what file name the server gave it.

With that, I would take the same workflow and turn it into an automated test case, and use the downloaded file name in the .zip asset, as you did in this PR already.

[williammartin]

I'm closing this since it appears to have gone stale. Of course I'd love for you to come back and reopen this when you're ready to work on it again but in the meantime I'm going to close it so that someone else can clearly see the state from the issue, and might choose to work on it.

Thanks for your work here, at the very least it gives an obvious direction and edge cases to handle when someone gets around to it.

[shayn-orca]

Hey @williammartin
I worked today trying to reproduce it. There's no point in your test since GitHub doesn't allow weird chars in job IDs:

Emojis

Unicode (hebrew)

Since you're worried about degradation - the case where degradation is possible looks impossible (or at least, well documented to not be supported).

If this is still unacceptable, maybe we can release this under a config/env var that will allow people to disable this behavior if it's problematic to them?

[williammartin]

That seems to be true of job-ids but not true of job-names which are used to construct the .zip structure from what I can tell. You can try yourself with this workflow:

name: Job Name Testing

on:
  push:

jobs:
  update-in-progress-issues:
    name: Emoji Test 😅😅😅😅😅😅😅
    runs-on: ubuntu-latest
    permissions:
      issues: write
    steps:
      - name: Emoji echo
        shell: bash
        run: echo "Emojis!"

But you can see a run in my test-repo, and fetching the logs via gh run -R williammartin-test-org/test-repo view --log --job 24553269126 you can see the contents of the zip:

[williammartin]

And here you can see the run of a workflow with a long emoji filled name.. Trying to view the log shows no content: gh run -R williammartin-test-org/test-repo view --log --job 24553563704 and it's almost definitely because the slicing is incorrectly matching the server:

In fact right now this PR branch actually panics on this workflow:

➜  test-repo git:(main) ~/workspace/cli/bin/gh run view --log --job 24553563704
panic: regexp: Compile("Emoji Test 😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅😅\xf0\x9f\\/1_.*\\.txt"): error parsing regexp: invalid UTF-8: `\/1_.*\.txt`

goroutine 1 [running]:
regexp.MustCompile({0x14000662000, 0x64})
        regexp/regexp.go:317 +0xb0
github.com/cli/cli/v2/pkg/cmd/run/view.logFilenameRegexp({0x5b781a638, {0x1400055b7e0, 0x9}, {0x1400055b7e9, 0x7}, {0x140000f4180, 0xba}, {0x140006cc000, 0x3, 0x4}, ...}, ...)
        github.com/cli/cli/v2/pkg/cmd/run/view/view.go:509 +0xc0
github.com/cli/cli/v2/pkg/cmd/run/view.attachRunLog(0x1400048d108, {0x1400063b988, 0x1, 0x104aa5810?})
        github.com/cli/cli/v2/pkg/cmd/run/view/view.go:541 +0x130
github.com/cli/cli/v2/pkg/cmd/run/view.runView(0x140005a6d20)
        github.com/cli/cli/v2/pkg/cmd/run/view/view.go:290 +0xb18
github.com/cli/cli/v2/pkg/cmd/run/view.NewCmdView.func1(0x140005a3b08?, {0x140007897d0?, 0x4?, 0x10382deb7?})
        github.com/cli/cli/v2/pkg/cmd/run/view/view.go:153 +0x30c
github.com/spf13/cobra.(*Command).execute(0x140005a3b08, {0x140007897a0, 0x3, 0x3})
        github.com/spf13/cobra@v1.6.1/command.go:916 +0x654
github.com/spf13/cobra.(*Command).ExecuteC(0x14000854008)
        github.com/spf13/cobra@v1.6.1/command.go:1044 +0x320
github.com/spf13/cobra.(*Command).ExecuteContextC(...)
        github.com/spf13/cobra@v1.6.1/command.go:977
main.mainRun()
        github.com/cli/cli/v2/cmd/gh/main.go:119 +0x4b0
main.main()
        github.com/cli/cli/v2/cmd/gh/main.go:46 +0x1c

I need to refresh my memory of how all of this fits together, it's been a couple of months since I looked at this.

[williammartin]

I think I'm just going to try and fix this PR. This is the third or fourth time I've had to remind myself of how this all fits together and I'd rather not do it again in the future. No blame towards you here, I appreciate you trying to improve the CLI and I understand your time is limited and also that this ended up being a lot more annoying than you initially imagined I'm sure.

I'm going to push on top of your branch and when I'm done I'll ask you to check whether my changes work for you.

[williammartin]

I believe I've fixed this in 4b28de8. Can you build from this branch and check that it works for your use cases?

[shayn-orca]

It looks good to me, I don't fully follow why the max was changed to 90 from 89.

I also don't fully remember how to test this locally 😂 I'll take a look in a few minutes.

Thanks for taking the branch all the way. Unicode is hard. I was mistaken with the job-ids vs job names, nice catch!

[shayn-orca]

Manual testing report. I can't share the output itself since it's a private repo. the structure is:

~/src/cli <--- Here's the GH cli on this branch
~/src/orca <--- Here's the repo where I'm testing

Built the gh cli with the scripts/build.go script:

go run script/build.go
...
❯ ls -l ./bin/gh
-rwxr-xr-x  1 shay  staff  54370498 May  4 12:11 ./bin/gh

Then, cd ../orca. From now on running in that repo.

Viewing the run without debug logs works (it's slow, but works):

GH_DEBUG=true ../cli/bin/gh run view 8942958501

And with --log-failed:

❯ wc -l gh_cli_local_build_output.txt
    3015 gh_cli_local_build_output.txt

Looks like it works :) :) :) 🚀 @williammartin Thanks a ton

[shayn-orca]

UTs pass locally for me, as well

@williammartin we seem to have conflicts on the runlogzip file and the view.go file. Do you want to solve them or should I? If me, should I merge, or rebase?

[williammartin]

I don't fully follow why the max was changed to 90 from 89.

I kind of hand-waved this mentally but I think it's to do with the change from []byte (utf-8) to []uint16 (utf-16). In any case the test I added to match reality requires 90 to slice the emoji in the middle leaving a trailing �.

I also don't fully remember how to test this locally

Yeh I know this feeling. This was one of the first bugs I found after I joined the team a year ago and every time reloading the context into my brain has proved slow.

Do you want to solve them or should I? If me, should I merge, or rebase?

I'll fix them on Monday, I just didn't want to force push over your changes and make you reset --hard to test my changes out.

Looks like it works :) :) :)

Hooray! I really wish the actions team would bundle a manifest in the .zip so that we could just map jobs and steps directly to files, but I didn't manage to get them to prioritise this last year. Maybe I'll try again.

[williammartin]

Having had a few people look at the changes I made, and having @shayn-orca check my changes work for them, I feel good about merging this. Cheers everyone.

[shayn-orca]

Thanks @williammartin !