From ec9e50ff933b9bb2d32a908a1c149f252b0730c7 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Tue, 19 Jul 2022 16:55:14 -0700 Subject: [PATCH 1/2] Always set PasswordAuthentication option --- internal/codespaces/ssh.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 0c81c4f32..9a146c4f6 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -56,7 +56,11 @@ func NewRemoteCommand(ctx context.Context, tunnelPort int, destination string, s // newSSHCommand populates an exec.Cmd to run a command (or if blank, // an interactive shell) over ssh. func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) (*exec.Cmd, []string, error) { - connArgs := []string{"-p", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes"} + connArgs := []string{ + "-p", strconv.Itoa(port), + "-o", "NoHostAuthenticationForLocalhost=yes", + "-o", "PasswordAuthentication=no", + } // The ssh command syntax is: ssh [flags] user@host command [args...] // There is no way to specify the user@host destination as a flag. @@ -101,6 +105,7 @@ func newSCPCommand(ctx context.Context, port int, dst string, cmdArgs []string) connArgs := []string{ "-P", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes", + "-o", "PasswordAuthentication=no", "-C", // compression } From 02881b4783db05be2249c27f36130e3ef82daa9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 21 Jul 2022 17:30:46 +0200 Subject: [PATCH 2/2] Correctly determine run started/elapsed timestamps (#5945) This switches `run list` and `run view` commands to measure how long ago did the run happen by using `run_started_at`, which is the timestamp of the latest run in a series, instead of `created_at`, which is the timestamp of the first run. This change also fixes accurate duration for in-progress runs. --- pkg/cmd/pr/checks/output.go | 4 +-- pkg/cmd/run/list/list.go | 27 +++++++--------- pkg/cmd/run/list/list_test.go | 24 ++++++++------ pkg/cmd/run/shared/shared.go | 24 +++++++++++++- pkg/cmd/run/shared/shared_test.go | 52 +++++++++++++++++++++++++++++++ pkg/cmd/run/shared/test.go | 23 +++++--------- pkg/cmd/run/view/view.go | 4 +-- pkg/cmd/run/watch/watch.go | 2 +- pkg/cmd/run/watch/watch_test.go | 5 ++- pkg/cmd/workflow/view/view.go | 10 +++--- 10 files changed, 121 insertions(+), 54 deletions(-) diff --git a/pkg/cmd/pr/checks/output.go b/pkg/cmd/pr/checks/output.go index 4e365bfdd..d1adb7c54 100644 --- a/pkg/cmd/pr/checks/output.go +++ b/pkg/cmd/pr/checks/output.go @@ -3,7 +3,6 @@ package checks import ( "fmt" "sort" - "time" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" @@ -12,9 +11,8 @@ import ( func addRow(tp utils.TablePrinter, io *iostreams.IOStreams, o check) { cs := io.ColorScheme() elapsed := "" - zeroTime := time.Time{} - if o.StartedAt != zeroTime && o.CompletedAt != zeroTime { + if !o.StartedAt.IsZero() && !o.CompletedAt.IsZero() { e := o.CompletedAt.Sub(o.StartedAt) if e > 0 { elapsed = e.String() diff --git a/pkg/cmd/run/list/list.go b/pkg/cmd/run/list/list.go index b913fce0f..5cbefb561 100644 --- a/pkg/cmd/run/list/list.go +++ b/pkg/cmd/run/list/list.go @@ -24,19 +24,21 @@ type ListOptions struct { HttpClient func() (*http.Client, error) BaseRepo func() (ghrepo.Interface, error) - PlainOutput bool - Exporter cmdutil.Exporter + Exporter cmdutil.Exporter Limit int WorkflowSelector string Branch string Actor string + + now time.Time } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { opts := &ListOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, + now: time.Now(), } cmd := &cobra.Command{ @@ -48,9 +50,6 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman // support `-R, --repo` override opts.BaseRepo = f.BaseRepo - terminal := opts.IO.IsStdoutTTY() && opts.IO.IsStdinTTY() - opts.PlainOutput = !terminal - if opts.Limit < 1 { return cmdutil.FlagErrorf("invalid limit: %v", opts.Limit) } @@ -126,7 +125,7 @@ func listRun(opts *ListOptions) error { cs := opts.IO.ColorScheme() - if !opts.PlainOutput { + if tp.IsTTY() { tp.AddField("STATUS", nil, nil) tp.AddField("NAME", nil, nil) tp.AddField("WORKFLOW", nil, nil) @@ -139,12 +138,12 @@ func listRun(opts *ListOptions) error { } for _, run := range runs { - if opts.PlainOutput { - tp.AddField(string(run.Status), nil, nil) - tp.AddField(string(run.Conclusion), nil, nil) - } else { + if tp.IsTTY() { symbol, symbolColor := shared.Symbol(cs, run.Status, run.Conclusion) tp.AddField(symbol, nil, symbolColor) + } else { + tp.AddField(string(run.Status), nil, nil) + tp.AddField(string(run.Conclusion), nil, nil) } tp.AddField(run.CommitMsg(), nil, cs.Bold) @@ -154,12 +153,8 @@ func listRun(opts *ListOptions) error { tp.AddField(string(run.Event), nil, nil) tp.AddField(fmt.Sprintf("%d", run.ID), nil, cs.Cyan) - elapsed := run.UpdatedAt.Sub(run.CreatedAt) - if elapsed < 0 { - elapsed = 0 - } - tp.AddField(elapsed.String(), nil, nil) - tp.AddField(utils.FuzzyAgoAbbr(time.Now(), run.CreatedAt), nil, nil) + tp.AddField(run.Duration(opts.now).String(), nil, nil) + tp.AddField(utils.FuzzyAgoAbbr(time.Now(), run.StartedTime()), nil, nil) tp.EndRow() } diff --git a/pkg/cmd/run/list/list_test.go b/pkg/cmd/run/list/list_test.go index fec49fe16..646f1a509 100644 --- a/pkg/cmd/run/list/list_test.go +++ b/pkg/cmd/run/list/list_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "testing" + "time" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/run/shared" @@ -115,13 +116,15 @@ func TestListRun(t *testing.T) { wantOut string wantErrOut string stubs func(*httpmock.Registry) - nontty bool + isTTY bool }{ { - name: "blank tty", + name: "default arguments", opts: &ListOptions{ Limit: defaultLimit, + now: shared.TestRunStartTime.Add(time.Minute*4 + time.Second*34), }, + isTTY: true, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/runs"), @@ -132,12 +135,12 @@ func TestListRun(t *testing.T) { wantOut: "STATUS NAME WORKFLOW BRANCH EVENT ID ELAPSED AGE\nX cool commit timed out trunk push 1 4m34s Feb 23, 2021\n* cool commit in progress trunk push 2 4m34s Feb 23, 2021\n✓ cool commit successful trunk push 3 4m34s Feb 23, 2021\nX cool commit cancelled trunk push 4 4m34s Feb 23, 2021\nX cool commit failed trunk push 1234 4m34s Feb 23, 2021\n- cool commit neutral trunk push 6 4m34s Feb 23, 2021\n- cool commit skipped trunk push 7 4m34s Feb 23, 2021\n* cool commit requested trunk push 8 4m34s Feb 23, 2021\n* cool commit queued trunk push 9 4m34s Feb 23, 2021\nX cool commit stale trunk push 10 4m34s Feb 23, 2021\n", }, { - name: "blank nontty", + name: "default arguments nontty", opts: &ListOptions{ - Limit: defaultLimit, - PlainOutput: true, + Limit: defaultLimit, + now: shared.TestRunStartTime.Add(time.Minute*4 + time.Second*34), }, - nontty: true, + isTTY: false, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/runs"), @@ -151,7 +154,9 @@ func TestListRun(t *testing.T) { name: "pagination", opts: &ListOptions{ Limit: 101, + now: shared.TestRunStartTime.Add(time.Minute*4 + time.Second*34), }, + isTTY: true, stubs: func(reg *httpmock.Registry) { var runID int64 runs := []shared.Run{} @@ -175,8 +180,7 @@ func TestListRun(t *testing.T) { { name: "no results", opts: &ListOptions{ - Limit: defaultLimit, - PlainOutput: true, + Limit: defaultLimit, }, stubs: func(reg *httpmock.Registry) { reg.Register( @@ -191,7 +195,9 @@ func TestListRun(t *testing.T) { opts: &ListOptions{ Limit: defaultLimit, WorkflowSelector: "flow.yml", + now: shared.TestRunStartTime.Add(time.Minute*4 + time.Second*34), }, + isTTY: true, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/flow.yml"), @@ -249,7 +255,7 @@ func TestListRun(t *testing.T) { } ios, _, stdout, stderr := iostreams.Test() - ios.SetStdoutTTY(!tt.nontty) + ios.SetStdoutTTY(tt.isTTY) tt.opts.IO = ios tt.opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.FromFullName("OWNER/REPO") diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index 81cbcc8f4..b2b17fbb1 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -49,6 +49,7 @@ var RunFields = []string{ "headSha", "createdAt", "updatedAt", + "startedAt", "status", "conclusion", "event", @@ -61,11 +62,13 @@ type Run struct { Name string CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` + StartedAt time.Time `json:"run_started_at"` Status Status Conclusion Conclusion Event string ID int64 WorkflowID int64 `json:"workflow_id"` + Attempts uint8 `json:"run_attempt"` HeadBranch string `json:"head_branch"` JobsURL string `json:"jobs_url"` HeadCommit Commit `json:"head_commit"` @@ -74,6 +77,25 @@ type Run struct { HeadRepository Repo `json:"head_repository"` } +func (r *Run) StartedTime() time.Time { + if r.StartedAt.IsZero() { + return r.CreatedAt + } + return r.StartedAt +} + +func (r *Run) Duration(now time.Time) time.Duration { + endTime := r.UpdatedAt + if r.Status != Completed { + endTime = now + } + d := endTime.Sub(r.StartedTime()) + if d < 0 { + return 0 + } + return d.Round(time.Second) +} + type Repo struct { Owner struct { Login string @@ -329,7 +351,7 @@ func PromptForRun(cs *iostreams.ColorScheme, runs []Run) (string, error) { symbol, _ := Symbol(cs, run.Status, run.Conclusion) candidates = append(candidates, // TODO truncate commit message, long ones look terrible - fmt.Sprintf("%s %s, %s (%s) %s", symbol, run.CommitMsg(), run.Name, run.HeadBranch, preciseAgo(now, run.CreatedAt))) + fmt.Sprintf("%s %s, %s (%s) %s", symbol, run.CommitMsg(), run.Name, run.HeadBranch, preciseAgo(now, run.StartedTime()))) } // TODO consider custom filter so it's fuzzier. right now matches start anywhere in string but diff --git a/pkg/cmd/run/shared/shared_test.go b/pkg/cmd/run/shared/shared_test.go index 53d941069..5d7768207 100644 --- a/pkg/cmd/run/shared/shared_test.go +++ b/pkg/cmd/run/shared/shared_test.go @@ -1,10 +1,12 @@ package shared import ( + "encoding/json" "net/http" "testing" "time" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" @@ -52,3 +54,53 @@ func TestGetAnnotations404(t *testing.T) { assert.NoError(t, err) assert.Equal(t, result, []Annotation{}) } + +func TestRun_Duration(t *testing.T) { + now, _ := time.Parse(time.RFC3339, "2022-07-20T11:22:58Z") + + tests := []struct { + name string + json string + wants string + }{ + { + name: "no run_started_at", + json: heredoc.Doc(` + { + "created_at": "2022-07-20T11:20:13Z", + "updated_at": "2022-07-20T11:21:16Z", + "status": "completed" + }`), + wants: "1m3s", + }, + { + name: "with run_started_at", + json: heredoc.Doc(` + { + "created_at": "2022-07-20T11:20:13Z", + "run_started_at": "2022-07-20T11:20:55Z", + "updated_at": "2022-07-20T11:21:16Z", + "status": "completed" + }`), + wants: "21s", + }, + { + name: "in_progress", + json: heredoc.Doc(` + { + "created_at": "2022-07-20T11:20:13Z", + "run_started_at": "2022-07-20T11:20:55Z", + "updated_at": "2022-07-20T11:21:16Z", + "status": "in_progress" + }`), + wants: "2m3s", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var r Run + assert.NoError(t, json.Unmarshal([]byte(tt.json), &r)) + assert.Equal(t, tt.wants, r.Duration(now).String()) + }) + } +} diff --git a/pkg/cmd/run/shared/test.go b/pkg/cmd/run/shared/test.go index 64f1c801a..b413e4386 100644 --- a/pkg/cmd/run/shared/test.go +++ b/pkg/cmd/run/shared/test.go @@ -5,23 +5,14 @@ import ( "time" ) -// Test data for use in the various run and job tests -func created() time.Time { - created, _ := time.Parse("2006-01-02 15:04:05", "2021-02-23 04:51:00") - return created -} - -func updated() time.Time { - updated, _ := time.Parse("2006-01-02 15:04:05", "2021-02-23 04:55:34") - return updated -} +var TestRunStartTime, _ = time.Parse("2006-01-02 15:04:05", "2021-02-23 04:51:00") func TestRun(name string, id int64, s Status, c Conclusion) Run { return Run{ Name: name, ID: id, - CreatedAt: created(), - UpdatedAt: updated(), + CreatedAt: TestRunStartTime, + UpdatedAt: TestRunStartTime.Add(time.Minute*4 + time.Second*34), Status: s, Conclusion: c, Event: "push", @@ -66,8 +57,8 @@ var SuccessfulJob Job = Job{ Status: Completed, Conclusion: Success, Name: "cool job", - StartedAt: created(), - CompletedAt: updated(), + StartedAt: TestRunStartTime, + CompletedAt: TestRunStartTime.Add(time.Minute*4 + time.Second*34), URL: "https://github.com/jobs/10", RunID: 3, Steps: []Step{ @@ -91,8 +82,8 @@ var FailedJob Job = Job{ Status: Completed, Conclusion: Failure, Name: "sad job", - StartedAt: created(), - CompletedAt: updated(), + StartedAt: TestRunStartTime, + CompletedAt: TestRunStartTime.Add(time.Minute*4 + time.Second*34), URL: "https://github.com/jobs/20", RunID: 1234, Steps: []Step{ diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 63518a632..03c7c017c 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -319,7 +319,7 @@ func runView(opts *ViewOptions) error { out := opts.IO.Out - ago := opts.Now().Sub(run.CreatedAt) + ago := opts.Now().Sub(run.StartedTime()) fmt.Fprintln(out) fmt.Fprintln(out, shared.RenderRunHeader(cs, *run, utils.FuzzyAgo(ago), prNumber)) @@ -416,7 +416,7 @@ func getLog(httpClient *http.Client, logURL string) (io.ReadCloser, error) { } func getRunLog(cache runLogCache, httpClient *http.Client, repo ghrepo.Interface, run *shared.Run) (*zip.ReadCloser, error) { - filename := fmt.Sprintf("run-log-%d-%d.zip", run.ID, run.CreatedAt.Unix()) + filename := fmt.Sprintf("run-log-%d-%d.zip", run.ID, run.StartedTime().Unix()) filepath := filepath.Join(os.TempDir(), "gh-cli-cache", filename) if !cache.Exists(filepath) { // Run log does not exist in cache so retrieve and store it diff --git a/pkg/cmd/run/watch/watch.go b/pkg/cmd/run/watch/watch.go index 4bb7367a8..c3ed5b61a 100644 --- a/pkg/cmd/run/watch/watch.go +++ b/pkg/cmd/run/watch/watch.go @@ -206,7 +206,7 @@ func renderRun(out io.Writer, opts WatchOptions, client *api.Client, repo ghrepo return nil, fmt.Errorf("failed to get run: %w", err) } - ago := opts.Now().Sub(run.CreatedAt) + ago := opts.Now().Sub(run.StartedTime()) jobs, err := shared.GetJobs(client, repo, *run) if err != nil { diff --git a/pkg/cmd/run/watch/watch_test.go b/pkg/cmd/run/watch/watch_test.go index fdbcf393d..bdb08a58d 100644 --- a/pkg/cmd/run/watch/watch_test.go +++ b/pkg/cmd/run/watch/watch_test.go @@ -288,7 +288,10 @@ func TestWatchRun(t *testing.T) { } else { assert.NoError(t, err) } - assert.Equal(t, tt.wantOut, stdout.String()) + // avoiding using `assert.Equal` here because it would print raw escape sequences to stdout + if got := stdout.String(); got != tt.wantOut { + t.Errorf("got stdout:\n%q\nwant:\n%q", got, tt.wantOut) + } reg.Verify(t) }) } diff --git a/pkg/cmd/workflow/view/view.go b/pkg/cmd/workflow/view/view.go index 4972f92a0..58d0fe7b6 100644 --- a/pkg/cmd/workflow/view/view.go +++ b/pkg/cmd/workflow/view/view.go @@ -5,6 +5,7 @@ import ( "net/http" "net/url" "strings" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -30,6 +31,8 @@ type ViewOptions struct { Prompt bool Raw bool YAML bool + + now time.Time } func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { @@ -37,6 +40,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman IO: f.IOStreams, HttpClient: f.HttpClient, Browser: f.Browser, + now: time.Now(), } cmd := &cobra.Command{ @@ -223,11 +227,7 @@ func viewWorkflowInfo(opts *ViewOptions, client *api.Client, repo ghrepo.Interfa tp.AddField(string(run.Event), nil, nil) if opts.Raw { - elapsed := run.UpdatedAt.Sub(run.CreatedAt) - if elapsed < 0 { - elapsed = 0 - } - tp.AddField(elapsed.String(), nil, nil) + tp.AddField(run.Duration(opts.now).String(), nil, nil) } tp.AddField(fmt.Sprintf("%d", run.ID), nil, cs.Cyan)