Stitch any CI into Tangled
buildkite: persist resolved org on build mapping
Spawn previously stored cfg.Org on the buildkite_builds row, leaving
empty when the workflow YAML didn't set tack.buildkite.org and
relying on the read path to fall back to the provider's defaultOrg.
That coupling let historical lookups drift: if defaultOrg ever
changed, log fetches and webhook joins for older builds would silently
target the wrong organisation.
authored by
Mitchell Hashimoto
and committed by
Tangled
ab5b79b1
fdeea446
+107
-12
+14
-12
provider_buildkite.go
+14
-12
provider_buildkite.go
···
259
259
)
260
260
261
261
pipelineURI := pipelineATURI(knot, pipelineRkey)
262
-
// Persist the org we actually used. cfg.Org (not the resolved
263
-
// `org`) is intentional: empty means "fall back to defaultOrg
264
-
// at read time", which keeps the row meaningful even if the
265
-
// provider's defaultOrg changes later — and matches what older
266
-
// rows scan as before this column existed.
262
+
// Persist the *resolved* org — the one we actually issued the
263
+
// CreateBuild against — rather than cfg.Org. If we stored only
264
+
// cfg.Org, a later change to the provider's defaultOrg would
265
+
// silently retarget historical lookups (logs, webhook joins) at
266
+
// the wrong organisation. Legacy rows written before this fix
267
+
// may still have an empty Org; the read path keeps the
268
+
// defaultOrg fallback for those (see Logs).
267
269
if err := p.st.InsertBuildkiteBuild(ctx, BuildkiteBuildRef{
268
270
BuildUUID: build.ID,
269
271
BuildNumber: build.Number,
270
272
PipelineSlug: cfg.Pipeline,
271
-
Org: cfg.Org,
273
+
Org: org,
272
274
Knot: knot,
273
275
PipelineRkey: pipelineRkey,
274
276
Workflow: wf.Name,
···
383
385
return nil, ErrLogsNotFound
384
386
}
385
387
386
-
// Resolve the org against which we should pull jobs/logs. The
387
-
// workflow YAML can override the spindle's default at Spawn
388
-
// time (tack.buildkite.org), so we honour whatever the row
389
-
// recorded; an empty value means the workflow didn't override,
390
-
// which is the same as "use defaultOrg" — including for rows
391
-
// written before the org column was added.
388
+
// Resolve the org against which we should pull jobs/logs.
389
+
// Spawn now persists the *resolved* org used at create time, so
390
+
// for any row written by current code ref.Org is authoritative
391
+
// and we use it verbatim. The empty-string fallback to
392
+
// defaultOrg only exists for legacy rows on disk that predate
393
+
// persisting the resolved org; new rows should never hit it.
392
394
org := ref.Org
393
395
if org == "" {
394
396
org = p.defaultOrg
+93
provider_buildkite_test.go
+93
provider_buildkite_test.go
···
253
253
}
254
254
}
255
255
256
+
// TestBuildkiteSpawnPersistsResolvedOrg pins that Spawn writes the
257
+
// *resolved* org to the buildkite_builds row when the workflow YAML
258
+
// omits `tack.buildkite.org`. Storing cfg.Org (empty) here would let
259
+
// historical lookups silently follow a later change to defaultOrg
260
+
// into the wrong organisation; storing the value we actually issued
261
+
// the CreateBuild against keeps each row self-describing.
262
+
func TestBuildkiteSpawnPersistsResolvedOrg(t *testing.T) {
263
+
bk := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
264
+
// The default org in newBuildkiteTestProvider is "myorg";
265
+
// the URL must reflect the resolved fallback so we know
266
+
// what value the row is expected to capture.
267
+
if !strings.Contains(r.URL.Path, "/organizations/myorg/") {
268
+
t.Errorf("CreateBuild path = %q; want /organizations/myorg/", r.URL.Path)
269
+
}
270
+
w.WriteHeader(http.StatusCreated)
271
+
_ = json.NewEncoder(w).Encode(buildkite.Build{ID: "uuid-r", Number: 3})
272
+
})
273
+
p, st, _, _ := newBuildkiteTestProvider(t, buildkite.WebhookModeToken, "s", bk)
274
+
275
+
p.Spawn(context.Background(), "knot.example.com", "rkey-r",
276
+
&tangled.Pipeline_TriggerMetadata{
277
+
Push: &tangled.Pipeline_PushTriggerData{
278
+
NewSha: "abc", Ref: "refs/heads/main",
279
+
},
280
+
},
281
+
// No `org:` under tack.buildkite — must fall back to the
282
+
// provider's defaultOrg AND persist that resolved value.
283
+
[]*tangled.Pipeline_Workflow{
284
+
{Name: "ci.yml", Raw: "tack:\n buildkite:\n pipeline: mypipe\n"},
285
+
},
286
+
)
287
+
288
+
deadline := time.Now().Add(2 * time.Second)
289
+
var ref *BuildkiteBuildRef
290
+
for time.Now().Before(deadline) {
291
+
var err error
292
+
ref, err = st.LookupBuildkiteBuildByUUID(context.Background(), "uuid-r")
293
+
if err != nil {
294
+
t.Fatalf("lookup: %v", err)
295
+
}
296
+
if ref != nil {
297
+
break
298
+
}
299
+
time.Sleep(20 * time.Millisecond)
300
+
}
301
+
if ref == nil {
302
+
t.Fatal("buildkite build row not persisted within deadline")
303
+
}
304
+
// The whole point of the change: ref.Org records the org the
305
+
// build was created against, not "" (which would hand off to
306
+
// whatever defaultOrg happens to be at read time).
307
+
if ref.Org != "myorg" {
308
+
t.Fatalf("ref.Org = %q; want %q (resolved defaultOrg)",
309
+
ref.Org, "myorg")
310
+
}
311
+
312
+
// Belt-and-braces: simulate defaultOrg drifting after the row
313
+
// was written. Logs() should still target the org persisted on
314
+
// the row, not the new default. We swap defaultOrg out and
315
+
// stand up a sibling httptest server that fails the test if
316
+
// anything but /organizations/myorg/ is requested.
317
+
gotPath := make(chan string, 1)
318
+
logSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
319
+
gotPath <- r.URL.Path
320
+
w.WriteHeader(http.StatusOK)
321
+
_ = json.NewEncoder(w).Encode(buildkite.Build{
322
+
ID: "uuid-r", Number: 3, Jobs: nil,
323
+
})
324
+
}))
325
+
t.Cleanup(logSrv.Close)
326
+
prev := buildkite.APIBase
327
+
buildkite.APIBase = logSrv.URL
328
+
t.Cleanup(func() { buildkite.APIBase = prev })
329
+
330
+
p.defaultOrg = "newdefault"
331
+
ch, err := p.Logs(context.Background(), "knot.example.com", "rkey-r", "ci.yml")
332
+
if err != nil {
333
+
t.Fatalf("Logs: %v", err)
334
+
}
335
+
// Drain so the goroutine completes; we only care about the
336
+
// initial GetBuild path.
337
+
for range ch {
338
+
}
339
+
select {
340
+
case path := <-gotPath:
341
+
if !strings.Contains(path, "/organizations/myorg/") {
342
+
t.Fatalf("Logs hit %q; want path containing /organizations/myorg/", path)
343
+
}
344
+
case <-time.After(2 * time.Second):
345
+
t.Fatal("Logs did not call GetBuild")
346
+
}
347
+
}
348
+
256
349
// TestBuildkiteSpawnInvalidYAML proves a workflow without the
257
350
// required `tack.buildkite.pipeline` field is skipped — no API
258
351
// call, no DB row, no status. A misconfigured workflow shouldn't