Merge pull request #3445 from adamslc/log_fix

Fix `run view --log` when steps have slashes

pkg/cmd/run/view/view.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strconv"
 	"time"
@@ -98,16 +99,16 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
 		Example: heredoc.Doc(`
 			# Interactively select a run to view, optionally selecting a single job
 			$ gh run view
-			
+
 			# View a specific run
 			$ gh run view 12345
-			
+
 			# View a specific job within a run
 			$ gh run view --job 456789
-			
+
 			# View the full log for a specific job
 			$ gh run view --log --job 456789
-			
+
 			# Exit non-zero if a run failed
 			$ gh run view 0451 --exit-status && echo "run pending or passed"
 		`),
@@ -460,6 +461,11 @@ func promptForJob(cs *iostreams.ColorScheme, jobs []shared.Job) (*shared.Job, er
 	return nil, nil
 }
 
+func logFilenameRegexp(job shared.Job, step shared.Step) *regexp.Regexp {
+	re := fmt.Sprintf(`%s\/%d_.*\.txt`, job.Name, step.Number)
+	return regexp.MustCompile(re)
+}
+
 // This function takes a zip file of logs and a list of jobs.
 // Structure of zip file
 // zip/
@@ -477,9 +483,9 @@ func promptForJob(cs *iostreams.ColorScheme, jobs []shared.Job) (*shared.Job, er
 func attachRunLog(rlz *zip.ReadCloser, jobs []shared.Job) {
 	for i, job := range jobs {
 		for j, step := range job.Steps {
-			filename := fmt.Sprintf("%s/%d_%s.txt", job.Name, step.Number, step.Name)
+			re := logFilenameRegexp(job, step)
 			for _, file := range rlz.File {
-				if file.Name == filename {
+				if re.MatchString(file.Name) {
 					jobs[i].Steps[j].Log = file
 					break
 				}

pkg/cmd/run/view/view_test.go

@@ -864,6 +864,97 @@ func TestViewRun(t *testing.T) {
 	}
 }
 
+// Structure of fixture zip file
+// run log/
+// ├── cool job/
+// │   ├── 1_fob the barz.txt
+// │   └── 2_barz the fob.txt
+// └── sad job/
+//     ├── 1_barf the quux.txt
+//     └── 2_quux the barf.txt
+func Test_attachRunLog(t *testing.T) {
+	tests := []struct {
+		name         string
+		job          shared.Job
+		wantMatch    bool
+		wantFilename string
+	}{
+		{
+			name: "matching job name and step number 1",
+			job: shared.Job{
+				Name: "cool job",
+				Steps: []shared.Step{{
+					Name:   "fob the barz",
+					Number: 1,
+				}},
+			},
+			wantMatch:    true,
+			wantFilename: "cool job/1_fob the barz.txt",
+		},
+		{
+			name: "matching job name and step number 2",
+			job: shared.Job{
+				Name: "cool job",
+				Steps: []shared.Step{{
+					Name:   "barz the fob",
+					Number: 2,
+				}},
+			},
+			wantMatch:    true,
+			wantFilename: "cool job/2_barz the fob.txt",
+		},
+		{
+			name: "matching job name and step number and mismatch step name",
+			job: shared.Job{
+				Name: "cool job",
+				Steps: []shared.Step{{
+					Name:   "mismatch",
+					Number: 1,
+				}},
+			},
+			wantMatch:    true,
+			wantFilename: "cool job/1_fob the barz.txt",
+		},
+		{
+			name: "matching job name and mismatch step number",
+			job: shared.Job{
+				Name: "cool job",
+				Steps: []shared.Step{{
+					Name:   "fob the barz",
+					Number: 3,
+				}},
+			},
+			wantMatch: false,
+		},
+		{
+			name: "mismatching job name",
+			job: shared.Job{
+				Name: "mismatch",
+				Steps: []shared.Step{{
+					Name:   "fob the barz",
+					Number: 1,
+				}},
+			},
+			wantMatch: false,
+		},
+	}
+	rlz, _ := zip.OpenReader("./fixtures/run_log.zip")
+	defer rlz.Close()
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			attachRunLog(rlz, []shared.Job{tt.job})
+			for _, step := range tt.job.Steps {
+				log := step.Log
+				logPresent := log != nil
+				assert.Equal(t, tt.wantMatch, logPresent)
+				if logPresent {
+					assert.Equal(t, tt.wantFilename, log.Name)
+				}
+			}
+		})
+	}
+}
+
 type testRunLogCache struct{}
 
 func (testRunLogCache) Exists(path string) bool {