Fix run view --log when steps have slashes#3445
Conversation
mislav
left a comment
There was a problem hiding this comment.
Thanks for the quick fix! Good catch 🌟
pkg/cmd/run/view/view.go
Outdated
| for j, step := range job.Steps { | ||
| filename := fmt.Sprintf("%s/%d_%s.txt", job.Name, step.Number, step.Name) | ||
| filename := fmt.Sprintf("%s/%d_%s.txt", job.Name, step.Number, | ||
| strings.ReplaceAll(step.Name, "/", "")) |
There was a problem hiding this comment.
This looks good as a fix! I have a couple of additional asks:
- Could the step-to-filename–generating logic be extracted into a separate method?
- Could we have some tests exercising that method?
- There are possibly some characters other than
/that don't translate well to filenames (for example: the:character). @vilmibm do you think it would be possible/worthwhile to take a look at how our platform generates filenames and match that logic in our codebase? Or, could we somehow avoid guessing filename names altogether?
There was a problem hiding this comment.
(1) is done. (2) I don't speak go, so you will have to guide me through the testing framework. Or feel free add the tests yourself. (3) Slashes in particular are a quite serious bug because any step of the form uses: some-org/some-action@v1 will fail unless a name is specified. For example, run view --log is useless for reviewing test failures on virtually every Julia lang repo because of this issue. So, my preference is that this is merged and tagged quickly.
There was a problem hiding this comment.
@mislav @adamslc I think we can avoid special character handling all together by just matching on job name and step number. The job name only allows -, _, and alphanumeric characters and the step number is an integer so we shouldn't have to recreate the filename generating logic that actions uses and what characters it strips out. I wrote some code that changes this logic and adds tests that I will commit to this PR if that is okay with everyone?
Closes #3444, which incorrectly diagnosed the issue.