From c38ea2a6a457598778c84e1420490f69ac1d2ff5 Mon Sep 17 00:00:00 2001 From: Taras Pokornyy Date: Thu, 27 Aug 2026 14:54:10 +0300 Subject: [PATCH] [RAPTOR-19727] Don't suggest logs command that won't work --- cmd/artifact/build/create/cmd.go | 28 ++++++---- cmd/artifact/build/get/cmd.go | 16 +++--- internal/workload/build.go | 40 +++++++++++++- internal/workload/build_test.go | 90 +++++++++++++++++++++++++++++++ internal/workload/output.go | 14 +++++ internal/workload/output_test.go | 23 ++++++++ internal/workload/up/build.go | 8 +-- internal/workload/up/roll_test.go | 2 + internal/workload/up/run.go | 1 + internal/workload/up/run_test.go | 8 +++ 10 files changed, 205 insertions(+), 25 deletions(-) diff --git a/cmd/artifact/build/create/cmd.go b/cmd/artifact/build/create/cmd.go index aef78b143..6fdbefce2 100644 --- a/cmd/artifact/build/create/cmd.go +++ b/cmd/artifact/build/create/cmd.go @@ -142,22 +142,28 @@ func waitForAllBuilds( fmt.Fprintf(cmd.ErrOrStderr(), "Waiting for build %s...\n", buildID) build, werr := workload.WaitForBuild(artifactID, buildID, poll.Interval, poll.Timeout, nil) - if werr != nil && firstWaitErr == nil { - firstWaitErr = werr - } - if build == nil { - summaries = append(summaries, workload.BuildSummary{BuildID: buildID, Status: workload.BuildStatusCLIUnknown}) + if build != nil { + summary, serr := workload.BuildSummaryFor(build, workload.DefaultBuildLogTail) - continue - } + // Reuses summary.LogTail (just fetched above) to decide whether + // the logs hint is worth showing -- no extra call. + if workload.IsBuildErrorStatus(build.Status) { + werr = workload.BuildFailureMessage(artifactID, build.ID, build.Status, len(summary.LogTail) > 0) + } - summary, serr := workload.BuildSummaryFor(build, workload.DefaultBuildLogTail) - if serr != nil && firstWaitErr == nil { - firstWaitErr = serr + if serr != nil && firstWaitErr == nil { + firstWaitErr = serr + } + + summaries = append(summaries, summary) + } else { + summaries = append(summaries, workload.BuildSummary{BuildID: buildID, Status: workload.BuildStatusCLIUnknown}) } - summaries = append(summaries, summary) + if werr != nil && firstWaitErr == nil { + firstWaitErr = werr + } } return summaries, firstWaitErr diff --git a/cmd/artifact/build/get/cmd.go b/cmd/artifact/build/get/cmd.go index cd7ce96b4..a03a69d39 100644 --- a/cmd/artifact/build/get/cmd.go +++ b/cmd/artifact/build/get/cmd.go @@ -128,17 +128,15 @@ func runGet( return err } - if waitErr != nil { - return waitErr + // Covers both "already terminal-error on the first GET" and "failed + // during the wait". Reuses summary.LogTail (already fetched above) to + // decide whether the logs hint is worth showing -- no extra call. + if workload.IsBuildErrorStatus(build.Status) { + return workload.BuildFailureMessage(artifactID, build.ID, build.Status, len(summary.LogTail) > 0) } - // The build may have been already terminal-error on the first GET, in - // which case WaitForBuild was skipped and waitErr stays nil. Surface - // that explicitly so the process exits non-zero; hint at the logs - // command so the user has a one-step recovery to inspect what went - // wrong. - if workload.IsBuildErrorStatus(build.Status) { - return fmt.Errorf("build %s ended with status %s; run 'dr artifact build logs %s' to inspect", build.ID, build.Status, build.ID) + if waitErr != nil { + return waitErr } return nil diff --git a/internal/workload/build.go b/internal/workload/build.go index d53a3cc0b..74bc7a694 100644 --- a/internal/workload/build.go +++ b/internal/workload/build.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io" + "net/http" "strconv" "strings" "time" @@ -242,6 +243,11 @@ func ListArtifactBuilds(artifactID string, limit int) ([]Build, error) { // emits newline-delimited JSON; we tolerate malformed lines so a single bad // record cannot blank the whole tail. The original bytes for each line are // preserved in Raw so JSON output can pass them through unchanged. +// +// A 404 from the logs endpoint means no log resource exists for this build +// (e.g. it failed before the builder emitted anything) and is treated as a +// legitimate empty result rather than an error; other status codes still +// propagate as real errors. func GetArtifactBuildLogs(artifactID, buildID string) ([]BuildLogEntry, error) { url, err := config.GetEndpointURL("/api/v2/artifacts/" + escapeID(artifactID) + "/builds/" + escapeID(buildID) + "/logs") if err != nil { @@ -250,6 +256,11 @@ func GetArtifactBuildLogs(artifactID, buildID string) ([]BuildLogEntry, error) { resp, err := drapi.Get(url, "build logs") if err != nil { + var httpErr *drapi.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return nil, nil + } + return nil, err } @@ -316,7 +327,13 @@ func WaitForBuild( if IsTerminalBuildStatus(build.Status) { if IsBuildErrorStatus(build.Status) { - return build, fmt.Errorf("build %s ended with status %s; run 'dr artifact build logs %s' to inspect", buildID, build.Status, buildID) + // No 'dr artifact build logs' suggestion here: this shared + // primitive doesn't know whether logs exist, and callers + // that already fetch them (via BuildSummaryFor) shouldn't be + // forced into a second fetch just so this function can + // build a hint. Callers construct the final message + // themselves via BuildFailureMessage. + return build, fmt.Errorf("build %s ended with status %s", buildID, build.Status) } return build, nil @@ -330,6 +347,27 @@ func WaitForBuild( } } +// BuildLogsAvailable reports whether at least one log entry exists for the +// build. Used to decide whether it's worth pointing the user at +// 'dr artifact build logs' -- suggesting a command that 404s or prints +// nothing is worse than not suggesting anything. +func BuildLogsAvailable(artifactID, buildID string) bool { + entries, err := GetArtifactBuildLogs(artifactID, buildID) + + return err == nil && len(entries) > 0 +} + +// BuildFailureMessage formats the error for a build that ended in +// FAILED/CANCELLED, pointing at 'dr artifact build logs' only when +// logsAvailable is true. +func BuildFailureMessage(artifactID, buildID, status string, logsAvailable bool) error { + if !logsAvailable { + return fmt.Errorf("build %s ended with status %s; no logs were captured for this build", buildID, status) + } + + return fmt.Errorf("build %s ended with status %s; see 'dr artifact build logs %s %s'", buildID, status, artifactID, buildID) +} + // BuildSummaryFor composes the terminal-state summary RenderBuildSummary // renders. Duration comes from the Build timestamps; ImageURI is fetched // from the parent artifact's primary container only on COMPLETED (the diff --git a/internal/workload/build_test.go b/internal/workload/build_test.go index e78a65523..ac3d6f7aa 100644 --- a/internal/workload/build_test.go +++ b/internal/workload/build_test.go @@ -392,6 +392,96 @@ func TestGetArtifactBuildLogs_ParsesJSONL(t *testing.T) { assert.Equal(t, "line-2", entries[1].Message) } +// A 404 means no log resource exists for this build (e.g. it failed before +// the builder emitted anything) -- a legitimate empty result, not an error. +func TestGetArtifactBuildLogs_404IsEmpty(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + + defer srv.Close() + + installEndpoint(t, srv.URL) + + entries, err := GetArtifactBuildLogs("art-1", "b-1") + require.NoError(t, err) + assert.Nil(t, entries) +} + +func TestBuildLogsAvailable(t *testing.T) { + t.Run("true when entries exist", func(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"levelname":"INFO","message":"hi"}` + "\n")) + })) + + defer srv.Close() + + installEndpoint(t, srv.URL) + + assert.True(t, BuildLogsAvailable("art-1", "b-1")) + }) + + t.Run("false on 404", func(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + + defer srv.Close() + + installEndpoint(t, srv.URL) + + assert.False(t, BuildLogsAvailable("art-1", "b-1")) + }) + + t.Run("false on empty 200 body", func(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("")) + })) + + defer srv.Close() + + installEndpoint(t, srv.URL) + + assert.False(t, BuildLogsAvailable("art-1", "b-1")) + }) + + t.Run("false on server error", func(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + + defer srv.Close() + + installEndpoint(t, srv.URL) + + assert.False(t, BuildLogsAvailable("art-1", "b-1")) + }) +} + +func TestBuildFailureMessage(t *testing.T) { + t.Run("with logs available", func(t *testing.T) { + err := BuildFailureMessage("art-1", "b-1", BuildStatusFailed, true) + require.Error(t, err) + assert.EqualError(t, err, "build b-1 ended with status FAILED; see 'dr artifact build logs art-1 b-1'") + }) + + t.Run("without logs available", func(t *testing.T) { + err := BuildFailureMessage("art-1", "b-1", BuildStatusCancelled, false) + require.Error(t, err) + assert.EqualError(t, err, "build b-1 ended with status CANCELLED; no logs were captured for this build") + }) +} + func TestWaitForBuild_TerminalCompletedReturnsNil(t *testing.T) { installSkipAuth(t) diff --git a/internal/workload/output.go b/internal/workload/output.go index 1729164c3..886d3b777 100644 --- a/internal/workload/output.go +++ b/internal/workload/output.go @@ -222,11 +222,25 @@ func RenderBuildSummary(format outputformat.OutputFormat, summary BuildSummary) return nil } +// RenderBuildLogs prints a build's log entries: one formatted line each in +// text mode, or a JSON array (always [], never null, when empty). With no +// entries in text mode, "No logs found." goes to stderr so stdout stays log +// lines only and a `logs | grep`/pipe is not polluted by a status line. func RenderBuildLogs(format outputformat.OutputFormat, entries []BuildLogEntry) error { if format == outputformat.OutputFormatJSON { + if len(entries) == 0 { + entries = []BuildLogEntry{} + } + return printJSON(entries) } + if len(entries) == 0 { + fmt.Fprintln(os.Stderr, "No logs found.") + + return nil + } + for _, entry := range entries { fmt.Println(formatLogLine(entry)) } diff --git a/internal/workload/output_test.go b/internal/workload/output_test.go index 979125ef1..c878a65d9 100644 --- a/internal/workload/output_test.go +++ b/internal/workload/output_test.go @@ -452,6 +452,29 @@ func TestRenderBuildLogs_JSONPassthroughPreservesRaw(t *testing.T) { assert.Equal(t, "raw", got[0]["message"]) } +func TestRenderBuildLogs_TextEmpty(t *testing.T) { + var stderr string + + stdout := captureStdout(t, func() { + stderr = captureStderr(t, func() { + require.NoError(t, RenderBuildLogs(outputformat.OutputFormatText, nil)) + }) + }) + + // The hint goes to stderr so stdout stays log lines only (pipe/grep safe). + assert.Empty(t, stdout) + assert.Equal(t, "No logs found.\n", stderr) +} + +func TestRenderBuildLogs_JSONAlwaysArray(t *testing.T) { + output := captureStdout(t, func() { + require.NoError(t, RenderBuildLogs(outputformat.OutputFormatJSON, nil)) + }) + + // A regression that emits `null` instead of a JSON array must fail here. + assert.JSONEq(t, `[]`, output) +} + func TestFilterLogsByLevel(t *testing.T) { entries := []BuildLogEntry{ {Levelname: "DEBUG", Message: "d"}, diff --git a/internal/workload/up/build.go b/internal/workload/up/build.go index 2cf64ee8b..fe3d7c349 100644 --- a/internal/workload/up/build.go +++ b/internal/workload/up/build.go @@ -332,10 +332,10 @@ func buildImage(artifactID string, opts Options, report *reporter) (string, erro } if workload.IsBuildErrorStatus(built.Status) { - // Said here rather than left to the wait's own wording, because this - // is the only place that knows which artifact the build belongs to. - return built.ID, fmt.Errorf("build %s finished as %s; see 'dr artifact build logs %s %s'", - built.ID, built.Status, artifactID, built.ID) + // Precheck log availability before suggesting the logs command -- + // this is the only call site with no already-fetched log info, so + // it's the one place that needs its own extra fetch. + return built.ID, workload.BuildFailureMessage(artifactID, built.ID, built.Status, hasLogsFn(artifactID, built.ID)) } // A build still running when the wait expires keeps its id too: it is diff --git a/internal/workload/up/roll_test.go b/internal/workload/up/roll_test.go index 6b7bba3d3..bfdc2bdee 100644 --- a/internal/workload/up/roll_test.go +++ b/internal/workload/up/roll_test.go @@ -868,6 +868,8 @@ func TestRun_FailedBuildOnARollLeavesTheOldVersionServing(t *testing.T) { return &workload.Build{ID: id, Status: workload.BuildStatusFailed}, fmt.Errorf("build %s ended with status %s", id, workload.BuildStatusFailed) } + // Logs exist for this build, so the failure message should point at them. + f.hasLogs = func(string, string) bool { return true } install(t, f) diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index 0f714abf3..3a3779d9b 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -43,6 +43,7 @@ var ( lockArtifactFn = workload.LockArtifact triggerBuildFn = workload.TriggerArtifactBuild waitBuildFn = workload.WaitForBuild + hasLogsFn = workload.BuildLogsAvailable listBuildsFn = workload.ListArtifactBuilds getCredentialFn = workload.GetCredential findCredentialFn = workload.FindCredentialNamed diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index 39d976af2..b47df671f 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -159,6 +159,7 @@ type fakes struct { build func(string) (*workload.BuildTriggerResponse, error) waitBuild func(string, string, time.Duration, time.Duration, func(*workload.Build)) (*workload.Build, error) builds func(string, int) ([]workload.Build, error) + hasLogs func(string, string) bool // The roll track: refuse to queue a second swap, start one, follow it. guard func(string) error @@ -234,6 +235,11 @@ func install(t *testing.T, f fakes) { swap(t, &waitBuildFn, f.waitBuild) swap(t, &listBuildsFn, f.builds) + // Defaults to "no logs" so a test that does not care does not make a + // real network call by accident. + force(t, &hasLogsFn, func(string, string) bool { return false }) + swap(t, &hasLogsFn, f.hasLogs) + // Nothing stands in the way of a rollout unless a test says so, because // the quiet answer is the one every other roll test wants. force(t, &guardReplacementFn, func(string) error { return nil }) @@ -895,6 +901,8 @@ func TestRun_FailedBuildStopsAndNamesTheLogs(t *testing.T) { return &workload.Build{ID: id, Status: workload.BuildStatusFailed}, fmt.Errorf("build %s ended with status %s", id, workload.BuildStatusFailed) } + // Logs exist for this build, so the failure message should point at them. + f.hasLogs = func(string, string) bool { return true } install(t, f)