diff --git a/provider_buildkite.go b/provider_buildkite.go --- a/provider_buildkite.go +++ b/provider_buildkite.go @@ -259,10 +259,16 @@ "org", org, ) pipelineURI := pipelineATURI(knot, pipelineRkey) + // Persist the org we actually used. cfg.Org (not the resolved + // `org`) is intentional: empty means "fall back to defaultOrg + // at read time", which keeps the row meaningful even if the + // provider's defaultOrg changes later — and matches what older + // rows scan as before this column existed. if err := p.st.InsertBuildkiteBuild(ctx, BuildkiteBuildRef{ BuildUUID: build.ID, BuildNumber: build.Number, PipelineSlug: cfg.Pipeline, + Org: cfg.Org, Knot: knot, PipelineRkey: pipelineRkey, Workflow: wf.Name, @@ -377,13 +383,16 @@ if ref == nil { return nil, ErrLogsNotFound } - // Resolve the org against which we should pull jobs/logs. We - // don't persist it on BuildkiteBuildRef today (the slug + token - // have always been enough); fall back to the provider default, - // which is correct for the common single-org install. A - // future migration can add a column when multi-org installs - // need it. - org := p.defaultOrg + // Resolve the org against which we should pull jobs/logs. The + // workflow YAML can override the spindle's default at Spawn + // time (tack.buildkite.org), so we honour whatever the row + // recorded; an empty value means the workflow didn't override, + // which is the same as "use defaultOrg" — including for rows + // written before the org column was added. + org := ref.Org + if org == "" { + org = p.defaultOrg + } build, err := p.client.GetBuild(ctx, org, ref.PipelineSlug, ref.BuildNumber) if err != nil { diff --git a/provider_buildkite_test.go b/provider_buildkite_test.go --- a/provider_buildkite_test.go +++ b/provider_buildkite_test.go @@ -171,7 +171,7 @@ gotCh <- captured{path: r.URL.Path, body: body} w.WriteHeader(http.StatusCreated) _ = json.NewEncoder(w).Encode(buildkite.Build{ID: "uuid-9", Number: 9}) }) - p, _, _, _ := newBuildkiteTestProvider(t, buildkite.WebhookModeToken, "s", bk) + p, st, _, _ := newBuildkiteTestProvider(t, buildkite.WebhookModeToken, "s", bk) raw := strings.Join([]string{ "tack:", @@ -222,6 +222,34 @@ got.body.PullRequestBaseBranch) } case <-time.After(2 * time.Second): t.Fatal("CreateBuild not called") + } + + // The workflow's `org` override has to round-trip through + // the store: /logs (and any future post-creation API call) + // looks the row up by tuple and re-issues calls against the + // org we recorded here. If we drop it, log retrieval falls + // back to defaultOrg and 404s for cross-org workflows. + deadline := time.Now().Add(2 * time.Second) + var ref *BuildkiteBuildRef + for time.Now().Before(deadline) { + var err error + ref, err = st.LookupBuildkiteBuildByTuple( + context.Background(), + "knot.example.com", "rkey-x", "ci.yml", + ) + if err != nil { + t.Fatalf("lookup: %v", err) + } + if ref != nil { + break + } + time.Sleep(20 * time.Millisecond) + } + if ref == nil { + t.Fatal("buildkite build row not persisted within deadline") + } + if ref.Org != "workflow-org" { + t.Fatalf("ref.Org = %q; want %q", ref.Org, "workflow-org") } } diff --git a/store.go b/store.go --- a/store.go +++ b/store.go @@ -329,13 +329,21 @@ // places: the webhook handler (by build UUID) when an event arrives, // and the /logs handler (by knot+rkey+workflow) when an appview // client asks for output. type BuildkiteBuildRef struct { - BuildUUID string - BuildNumber int64 - PipelineSlug string - Knot string - PipelineRkey string - Workflow string - PipelineURI string + BuildUUID string + BuildNumber int64 + PipelineSlug string + // Org is the Buildkite organisation slug the build was created + // against. Persisted at Spawn time so /logs and any other + // post-creation API call can target the same org the workflow + // originally chose — see the workflow YAML `tack.buildkite.org` + // override. Empty means "use the provider's default org", which + // is what every row written before the org column existed will + // scan as. + Org string + Knot string + PipelineRkey string + Workflow string + PipelineURI string } // InsertBuildkiteBuild records that a Buildkite build was created on @@ -346,19 +354,20 @@ // instead of failing. func (s *store) InsertBuildkiteBuild(ctx context.Context, ref BuildkiteBuildRef) error { _, err := s.db.ExecContext(ctx, `INSERT INTO buildkite_builds ( - build_uuid, build_number, pipeline_slug, + build_uuid, build_number, pipeline_slug, org, knot, pipeline_rkey, workflow, pipeline_uri, created_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(build_uuid) DO UPDATE SET build_number = excluded.build_number, pipeline_slug = excluded.pipeline_slug, + org = excluded.org, knot = excluded.knot, pipeline_rkey = excluded.pipeline_rkey, workflow = excluded.workflow, pipeline_uri = excluded.pipeline_uri, created_at = excluded.created_at`, - ref.BuildUUID, ref.BuildNumber, ref.PipelineSlug, + ref.BuildUUID, ref.BuildNumber, ref.PipelineSlug, ref.Org, ref.Knot, ref.PipelineRkey, ref.Workflow, ref.PipelineURI, time.Now().UTC().Format(time.RFC3339Nano), ) @@ -376,12 +385,12 @@ // nil check. func (s *store) LookupBuildkiteBuildByUUID(ctx context.Context, buildUUID string) (*BuildkiteBuildRef, error) { var ref BuildkiteBuildRef err := s.db.QueryRowContext(ctx, - `SELECT build_uuid, build_number, pipeline_slug, + `SELECT build_uuid, build_number, pipeline_slug, org, knot, pipeline_rkey, workflow, pipeline_uri FROM buildkite_builds WHERE build_uuid = ?`, buildUUID, ).Scan( - &ref.BuildUUID, &ref.BuildNumber, &ref.PipelineSlug, + &ref.BuildUUID, &ref.BuildNumber, &ref.PipelineSlug, &ref.Org, &ref.Knot, &ref.PipelineRkey, &ref.Workflow, &ref.PipelineURI, ) if errors.Is(err, sql.ErrNoRows) { @@ -405,7 +414,7 @@ // if anyone ever wants that. func (s *store) LookupBuildkiteBuildByTuple(ctx context.Context, knot, pipelineRkey, workflow string) (*BuildkiteBuildRef, error) { var ref BuildkiteBuildRef err := s.db.QueryRowContext(ctx, - `SELECT build_uuid, build_number, pipeline_slug, + `SELECT build_uuid, build_number, pipeline_slug, org, knot, pipeline_rkey, workflow, pipeline_uri FROM buildkite_builds WHERE knot = ? AND pipeline_rkey = ? AND workflow = ? @@ -413,7 +422,7 @@ ORDER BY created_at DESC LIMIT 1`, knot, pipelineRkey, workflow, ).Scan( - &ref.BuildUUID, &ref.BuildNumber, &ref.PipelineSlug, + &ref.BuildUUID, &ref.BuildNumber, &ref.PipelineSlug, &ref.Org, &ref.Knot, &ref.PipelineRkey, &ref.Workflow, &ref.PipelineURI, ) if errors.Is(err, sql.ErrNoRows) { diff --git a/store_migrate.go b/store_migrate.go --- a/store_migrate.go +++ b/store_migrate.go @@ -7,6 +7,7 @@ import ( "context" "fmt" + "strings" ) // schema is the full set of CREATE statements applied at startup. It is @@ -92,10 +93,18 @@ -- the webhook is the hot path for status fan-out. -- -- The (knot, pipeline_rkey, workflow) index supports the /logs -- handler, which only knows that tuple at request time. +-- org is the Buildkite organisation slug the build was created +-- against. A workflow's YAML can override the spindle's default +-- org via tack.buildkite.org, so we persist whatever was used at +-- Spawn time and read it back when fetching jobs/logs. The empty +-- string means "use the provider's defaultOrg" — that's both the +-- usual single-org case and what every row written before this +-- column existed will scan as. CREATE TABLE IF NOT EXISTS buildkite_builds ( build_uuid TEXT PRIMARY KEY, build_number INTEGER NOT NULL, pipeline_slug TEXT NOT NULL, + org TEXT NOT NULL DEFAULT '', knot TEXT NOT NULL, pipeline_rkey TEXT NOT NULL, workflow TEXT NOT NULL, @@ -107,9 +116,30 @@ ON buildkite_builds (knot, pipeline_rkey, workflow); ` // migrate applies the schema. Safe to call repeatedly. +// +// CREATE TABLE IF NOT EXISTS is enough for fresh databases, but it +// won't widen an already-existing table. Columns added after the +// initial release therefore need a parallel ALTER TABLE step here; +// SQLite has no `ADD COLUMN IF NOT EXISTS`, so we run the ALTER and +// swallow the "duplicate column" error that fires on subsequent +// startups. Anything else is fatal. func (s *store) migrate(ctx context.Context) error { if _, err := s.db.ExecContext(ctx, schema); err != nil { return fmt.Errorf("apply schema: %w", err) + } + for _, alter := range []string{ + // Persist the Buildkite org each build was created + // against so /logs can target the same org the workflow + // chose. Pre-existing rows scan as empty string, which + // the provider treats as "use defaultOrg". + `ALTER TABLE buildkite_builds ADD COLUMN org TEXT NOT NULL DEFAULT ''`, + } { + if _, err := s.db.ExecContext(ctx, alter); err != nil { + if strings.Contains(err.Error(), "duplicate column name") { + continue + } + return fmt.Errorf("apply alter %q: %w", alter, err) + } } return nil }