diff --git a/cmd/artifact/build/create/cmd.go b/cmd/artifact/build/create/cmd.go index 754f66b9d..cc85fadd2 100644 --- a/cmd/artifact/build/create/cmd.go +++ b/cmd/artifact/build/create/cmd.go @@ -144,22 +144,28 @@ func waitForAllBuilds( fmt.Fprintf(cmd.ErrOrStderr(), "Waiting for build %s...\n", buildID) build, werr := waitStreaming(cmd, artifactID, buildID, poll) - 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 50f76d99e..fc970791d 100644 --- a/internal/workload/build.go +++ b/internal/workload/build.go @@ -387,7 +387,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 @@ -401,6 +407,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 8a3865640..71d7405ad 100644 --- a/internal/workload/build_test.go +++ b/internal/workload/build_test.go @@ -544,6 +544,96 @@ func TestGetArtifactBuildLogs_ReadsTheOTELStream(t *testing.T) { assert.NotEmpty(t, entries[0].Raw, "OTEL record preserved for JSON passthrough") } +// An empty OTEL page (no records for this build's external_build_id) is a +// legitimate empty result, not an error. +func TestGetArtifactBuildLogs_EmptyPageIsEmpty(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(logsPage(""))) + })) + + defer srv.Close() + + installEndpoint(t, srv.URL) + + entries, err := GetArtifactBuildLogs("art-1", "b-1") + require.NoError(t, err) + assert.Empty(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(logsPage("", logEntryDoc("INFO", "hi")))) + })) + + 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 page", func(t *testing.T) { + installSkipAuth(t) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(logsPage(""))) + })) + + 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 9a0669c4e..55b4a3b82 100644 --- a/internal/workload/output.go +++ b/internal/workload/output.go @@ -254,11 +254,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 408a8be62..98d14eb1a 100644 --- a/internal/workload/output_test.go +++ b/internal/workload/output_test.go @@ -530,6 +530,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 0d1b33320..f88e2c4db 100644 --- a/internal/workload/up/build.go +++ b/internal/workload/up/build.go @@ -481,10 +481,10 @@ func buildImage(artifactID, attachTo string, opts Options, report *reporter) (st } 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 01e6c0202..d73c6acfe 100644 --- a/internal/workload/up/roll_test.go +++ b/internal/workload/up/roll_test.go @@ -1372,6 +1372,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 9147a08fd..42393e24c 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -46,6 +46,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 72149f25a..3f187c835 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -193,6 +193,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 // checkEndpoint is the one GET a deploy ends with. checkEndpoint func(string) (int, error) @@ -287,6 +288,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 }) @@ -1492,6 +1498,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)