From d110073d832ba319687ff56e18ffc73e7e3a4ab2 Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Thu, 17 Oct 2024 15:08:43 +0100 Subject: [PATCH 1/6] wip --- internal/daemon/daemon.go | 1 + internal/run/reporter.go | 35 ++++++++++++++++++++- internal/run/reporter_test.go | 59 +++++++++++++++++++++++++++++++---- 3 files changed, 88 insertions(+), 7 deletions(-) diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 5cfe62bd6..9f13a9336 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -480,6 +480,7 @@ func (d *Daemon) Start(ctx context.Context, started chan struct{}) error { Workspaces: d.Workspaces, Runs: d.Runs, Configs: d.Configs, + Cache: make(map[string]vcs.Status), }, }, { diff --git a/internal/run/reporter.go b/internal/run/reporter.go index 17f16a8a4..1f4333cc4 100644 --- a/internal/run/reporter.go +++ b/internal/run/reporter.go @@ -27,6 +27,14 @@ type ( Workspaces reporterWorkspaceClient VCS reporterVCSClient Runs reporterRunClient + + // Cache most recently set status for each incomplete run to ensure the + // same status is not set more than once on an upstream VCS provider. + // This is important to avoid hitting rate limits on VCS providers, e.g. + // GitHub has a limit of 1000 status updates on a commit. + // + // key is the run ID. + Cache map[string]vcs.Status } reporterWorkspaceClient interface { @@ -119,7 +127,19 @@ func (r *Reporter) handleRun(ctx context.Context, run *Run) error { default: return fmt.Errorf("unknown run status: %s", run.Status) } - return client.SetStatus(ctx, vcs.SetStatusOptions{ + + // Check status cache. If there is a hit for the same run and status then + // skip setting the status again. + if lastStatus, ok := r.Cache[run.ID]; ok && lastStatus == status { + r.V(8).Info("skipped setting duplicate run status on vcs", + "run_id", run.ID, + "run_status", run.Status, + "vcs_status", status, + ) + return nil + } + + err = client.SetStatus(ctx, vcs.SetStatusOptions{ Workspace: ws.Name, Ref: cv.IngressAttributes.CommitSHA, Repo: cv.IngressAttributes.Repo, @@ -127,4 +147,17 @@ func (r *Reporter) handleRun(ctx context.Context, run *Run) error { Description: description, TargetURL: r.URL(paths.Run(run.ID)), }) + if err != nil { + return err + } + + // Update status cache. If the run is complete then remove the run from the + // cache because no further status updates are expected. + if run.Done() { + delete(r.Cache, run.ID) + } else { + r.Cache[run.ID] = status + } + + return nil } diff --git a/internal/run/reporter_test.go b/internal/run/reporter_test.go index 118ec0b63..c04c1a11d 100644 --- a/internal/run/reporter_test.go +++ b/internal/run/reporter_test.go @@ -23,7 +23,7 @@ func TestReporter_HandleRun(t *testing.T) { want vcs.SetStatusOptions }{ { - name: "pending run", + name: "set pending status", run: &Run{ID: "run-123", Status: RunPending}, ws: &workspace.Workspace{ Name: "dev", @@ -64,12 +64,13 @@ func TestReporter_HandleRun(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var got vcs.SetStatusOptions + got := make(chan vcs.SetStatusOptions, 1) reporter := &Reporter{ Workspaces: &fakeReporterWorkspaceService{ws: tt.ws}, Configs: &fakeReporterConfigurationVersionService{cv: tt.cv}, - VCS: &fakeReporterVCSProviderService{got: &got}, + VCS: &fakeReporterVCSProviderService{got: got}, HostnameService: internal.NewHostnameService("otf-host.org"), + Cache: make(map[string]vcs.Status), } err := reporter.handleRun(ctx, tt.run) require.NoError(t, err) @@ -79,6 +80,52 @@ func TestReporter_HandleRun(t *testing.T) { } } +// TestReporter_DontSetStatusTwice tests that the same status is not set more +// than once for a given run. +func TestReporter_DontSetStatusTwice(t *testing.T) { + ctx := context.Background() + + run := &Run{ID: "run-123", Status: RunPending} + ws := &workspace.Workspace{ + Name: "dev", + Connection: &workspace.Connection{}, + } + cv := &configversion.ConfigurationVersion{ + IngressAttributes: &configversion.IngressAttributes{ + CommitSHA: "abc123", + Repo: "leg100/otf", + }, + } + + got := make(chan vcs.SetStatusOptions, 1) + reporter := &Reporter{ + Workspaces: &fakeReporterWorkspaceService{ws: ws}, + Configs: &fakeReporterConfigurationVersionService{cv: cv}, + VCS: &fakeReporterVCSProviderService{got: got}, + HostnameService: internal.NewHostnameService("otf-host.org"), + Cache: make(map[string]vcs.Status), + } + + // handle run the first time and expect status to be set + err := reporter.handleRun(ctx, run) + require.NoError(t, err) + + want := vcs.SetStatusOptions{ + Workspace: "dev", + Ref: "abc123", + Repo: "leg100/otf", + Status: vcs.PendingStatus, + TargetURL: "https://otf-host.org/app/runs/run-123", + } + assert.Equal(t, want, <-got) + + // handle run the second time with the same status and expect status to + // *not* be set + err = reporter.handleRun(ctx, run) + require.NoError(t, err) + assert.Equal(t, 0, len(got)) +} + type fakeReporterConfigurationVersionService struct { configversion.Service @@ -100,7 +147,7 @@ func (f *fakeReporterWorkspaceService) Get(context.Context, string) (*workspace. } type fakeReporterVCSProviderService struct { - got *vcs.SetStatusOptions + got chan vcs.SetStatusOptions } func (f *fakeReporterVCSProviderService) GetVCSClient(context.Context, string) (vcs.Client, error) { @@ -110,10 +157,10 @@ func (f *fakeReporterVCSProviderService) GetVCSClient(context.Context, string) ( type fakeReporterCloudClient struct { vcs.Client - got *vcs.SetStatusOptions + got chan vcs.SetStatusOptions } func (f *fakeReporterCloudClient) SetStatus(ctx context.Context, opts vcs.SetStatusOptions) error { - *f.got = opts + f.got <- opts return nil } From 18a9087cab02c60f2df450bedf4456ffa7fd8fb2 Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Thu, 17 Oct 2024 15:10:27 +0100 Subject: [PATCH 2/6] wip --- internal/run/reporter.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/run/reporter.go b/internal/run/reporter.go index 1f4333cc4..947027e64 100644 --- a/internal/run/reporter.go +++ b/internal/run/reporter.go @@ -31,7 +31,9 @@ type ( // Cache most recently set status for each incomplete run to ensure the // same status is not set more than once on an upstream VCS provider. // This is important to avoid hitting rate limits on VCS providers, e.g. - // GitHub has a limit of 1000 status updates on a commit. + // GitHub has a limit of 1000 status updates on a commit: + // + // https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status // // key is the run ID. Cache map[string]vcs.Status From 2c97cdbb5f79cdcfb7151f8c7fb1cd0d7f276c00 Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Thu, 17 Oct 2024 15:17:02 +0100 Subject: [PATCH 3/6] wip --- internal/run/reporter_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/run/reporter_test.go b/internal/run/reporter_test.go index c04c1a11d..692085805 100644 --- a/internal/run/reporter_test.go +++ b/internal/run/reporter_test.go @@ -20,7 +20,9 @@ func TestReporter_HandleRun(t *testing.T) { run *Run ws *workspace.Workspace cv *configversion.ConfigurationVersion - want vcs.SetStatusOptions + // expect the given status options to be set. If nil then expect no + // status options to be set. + want *vcs.SetStatusOptions }{ { name: "set pending status", @@ -35,7 +37,7 @@ func TestReporter_HandleRun(t *testing.T) { Repo: "leg100/otf", }, }, - want: vcs.SetStatusOptions{ + want: &vcs.SetStatusOptions{ Workspace: "dev", Ref: "abc123", Repo: "leg100/otf", @@ -49,17 +51,17 @@ func TestReporter_HandleRun(t *testing.T) { cv: &configversion.ConfigurationVersion{ IngressAttributes: nil, }, - want: vcs.SetStatusOptions{}, + want: nil, }, { name: "skip UI-triggered run", run: &Run{ID: "run-123", Source: SourceUI}, - want: vcs.SetStatusOptions{}, + want: nil, }, { name: "skip API-triggered run", run: &Run{ID: "run-123", Source: SourceAPI}, - want: vcs.SetStatusOptions{}, + want: nil, }, } for _, tt := range tests { @@ -75,7 +77,11 @@ func TestReporter_HandleRun(t *testing.T) { err := reporter.handleRun(ctx, tt.run) require.NoError(t, err) - assert.Equal(t, tt.want, got) + if tt.want == nil { + assert.Equal(t, 0, len(got)) + } else { + assert.Equal(t, *tt.want, <-got) + } }) } } From 1fe8931dee915073719d7dffc48e3935056a99f1 Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Thu, 17 Oct 2024 15:23:34 +0100 Subject: [PATCH 4/6] wip --- internal/integration/connect_repo_e2e_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/integration/connect_repo_e2e_test.go b/internal/integration/connect_repo_e2e_test.go index 2a1e6e3de..aedda334a 100644 --- a/internal/integration/connect_repo_e2e_test.go +++ b/internal/integration/connect_repo_e2e_test.go @@ -59,10 +59,12 @@ func TestConnectRepoE2E(t *testing.T) { err = expect.Locator(page.Locator(`//div[@class='widget']//img[@id='run-trigger-github']`)).ToBeVisible() require.NoError(t, err) - // github should receive three pending status updates followed by a final - // update with details of planned resources - require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState()) - require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState()) + // GitHub should receive two pending status updates followed by a final + // update with details of planned resources. (The reason for two pending + // updates is that the run reporter de-dups status updates but only does + // so for OTF's abstract vcs.Status, which has a "running" value; but + // GitHub has no equivalent "running" value, so the Github client maps + // "running" to "pending"...). require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState()) require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState()) got := daemon.GetStatus(t, ctx) From 9be201c434ec10c91fe6eab493e0b9e7c4fd62ba Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Thu, 17 Oct 2024 17:42:03 +0100 Subject: [PATCH 5/6] wip --- internal/github/client.go | 7 +++---- internal/integration/connect_repo_e2e_test.go | 9 ++------- internal/run/reporter.go | 4 +--- internal/vcs/status.go | 1 - 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/internal/github/client.go b/internal/github/client.go index 656fff30c..ea6e7f814 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -2,14 +2,13 @@ package github import ( "context" - "sort" - "errors" "fmt" "net/http" "net/url" "os" "path" + "sort" "strconv" "strings" "time" @@ -190,7 +189,7 @@ func (g *Client) ListRepositories(ctx context.Context, opts vcs.ListRepositories // Apps.ListRepos endpoint does not support ordering on the server-side, // so instead we request *all* repos, page-by-page, and then sort // client-side. - var page = 1 + page := 1 for { result, resp, err := g.client.Apps.ListRepos(ctx, &github.ListOptions{ PerPage: opts.PageSize, @@ -448,7 +447,7 @@ func (g *Client) SetStatus(ctx context.Context, opts vcs.SetStatusOptions) error var status string switch opts.Status { - case vcs.PendingStatus, vcs.RunningStatus: + case vcs.PendingStatus: status = "pending" case vcs.SuccessStatus: status = "success" diff --git a/internal/integration/connect_repo_e2e_test.go b/internal/integration/connect_repo_e2e_test.go index aedda334a..866a6c7d9 100644 --- a/internal/integration/connect_repo_e2e_test.go +++ b/internal/integration/connect_repo_e2e_test.go @@ -59,13 +59,8 @@ func TestConnectRepoE2E(t *testing.T) { err = expect.Locator(page.Locator(`//div[@class='widget']//img[@id='run-trigger-github']`)).ToBeVisible() require.NoError(t, err) - // GitHub should receive two pending status updates followed by a final - // update with details of planned resources. (The reason for two pending - // updates is that the run reporter de-dups status updates but only does - // so for OTF's abstract vcs.Status, which has a "running" value; but - // GitHub has no equivalent "running" value, so the Github client maps - // "running" to "pending"...). - require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState()) + // GitHub should receive one pending status update followed by a final + // update with details of planned resources. require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState()) got := daemon.GetStatus(t, ctx) require.Equal(t, "success", got.GetState()) diff --git a/internal/run/reporter.go b/internal/run/reporter.go index 947027e64..0e4257eaa 100644 --- a/internal/run/reporter.go +++ b/internal/run/reporter.go @@ -109,10 +109,8 @@ func (r *Reporter) handleRun(ctx context.Context, run *Run) error { description string ) switch run.Status { - case RunPending, RunPlanQueued, RunApplyQueued: + case RunPending, RunPlanQueued, RunApplyQueued, RunPlanning, RunApplying, RunPlanned, RunConfirmed: status = vcs.PendingStatus - case RunPlanning, RunApplying, RunPlanned, RunConfirmed: - status = vcs.RunningStatus case RunPlannedAndFinished: status = vcs.SuccessStatus if run.Plan.ResourceReport != nil { diff --git a/internal/vcs/status.go b/internal/vcs/status.go index 3b326ded8..5b40f34bc 100644 --- a/internal/vcs/status.go +++ b/internal/vcs/status.go @@ -4,7 +4,6 @@ type Status string const ( PendingStatus Status = "pending" - RunningStatus Status = "running" SuccessStatus Status = "success" ErrorStatus Status = "error" FailureStatus Status = "failure" From d791cdde02e343b411b4865ac5fc7430c8f8cb5b Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Fri, 18 Oct 2024 15:09:15 +0100 Subject: [PATCH 6/6] wip --- internal/run/reporter.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/run/reporter.go b/internal/run/reporter.go index 0e4257eaa..f8641ab78 100644 --- a/internal/run/reporter.go +++ b/internal/run/reporter.go @@ -68,7 +68,10 @@ func (r *Reporter) Start(ctx context.Context) error { continue } if err := r.handleRun(ctx, event.Payload); err != nil { - return err + // any error is treated as non-fatal because reporting on runs is + // considered "best-effort" rather than an integral operation + r.Error(err, "reporting run vcs status", "run_id", event.Payload.ID) + return nil } } return pubsub.ErrSubscriptionTerminated