Merged
Conversation
akila-i
force-pushed
the
build-logs-revamp
branch
from
17ad125 to
3238888
Compare
akila-i
requested review from
jhivandb,
kaviththiranga and
sameerajayasoma
as code owners
stefinie123
reviewed
Feb 6, 2026
Contributor
There was a problem hiding this comment.
We need to update lastSyncCommit, lastSyncDate and lastSyncDescription in `https://github.com/openchoreo/backstage-plugins/blob/main/packages/openchoreo-client-node/openapi-config.json since we have updated the open-api.yaml
Contributor
Author
The changes are from the PR: openchoreo/openchoreo#1747. Will update the sync details once it is merged. |
akila-i
force-pushed
the
build-logs-revamp
branch
2 times, most recently
from
cc2903b to
ec01125
Compare
Contributor
Author
This is done |
📝 WalkthroughWalkthroughAdds workflow-run status and logs: OpenAPI schema and endpoint additions, backend router and WorkflowService methods for status/logs retrieval, client API methods, BuildLogs UI refactor with per-step polling/accordion, style updates, and common type exports. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as BuildLogs UI
participant Client as OpenChoreoCiClient
participant Backend as CI Backend Router
participant Service as WorkflowService
participant API as OpenChoreo API
UI->>Client: fetchWorkflowRunStatus(build)
Client->>Backend: GET /workflow-run-status
Backend->>Service: getWorkflowRunStatus(...)
Service->>API: GET /namespaces/.../workflow-runs/{runName}/status
API-->>Service: ComponentWorkflowRunStatusResponse
Service-->>Backend: WorkflowRunStatusResponse
Backend-->>Client: status + logURL
Client-->>UI: status, steps, logURL
loop poll every 10s until terminal
UI->>Client: fetchWorkflowRunStatus(build)
end
UI->>Client: fetchWorkflowRunLogs(namespace,project,component,run,hasLiveObservability,opts)
Client->>Backend: GET /workflow-run-logs?step=...&sinceSeconds=...
Backend->>Service: fetchWorkflowRunLogs(...)
alt hasLiveObservability = true
Service->>API: GET /namespaces/.../workflow-runs/{runName}/logs?step=...&sinceSeconds=...
API-->>Service: ComponentWorkflowRunLogEntry[]
else observer path
Service->>API: resolve observer URL via main API
Service->>API: Observability client fetch logs
API-->>Service: ComponentWorkflowRunLogEntry[]
end
Service-->>Backend: LogEntry[]
Backend-->>Client: log entries
Client-->>UI: append logs to step
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
akila-i
force-pushed
the
build-logs-revamp
branch
from
ec01125 to
0274ed3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@plugins/openchoreo-ci-backend/src/router.ts`:
- Around line 131-134: The handler currently sets sinceSeconds using
Number.parseInt which can produce NaN or negative values; update the logic
around sinceSeconds (the parse and assignment shown) to validate the parsed
value is a finite non-negative integer and, if invalid, return a 400 response
with an explanatory message; specifically, after parsing
Number.parseInt(sinceSeconds, 10) ensure Number.isInteger(value) && value >= 0
before assigning, otherwise send a 400 error from the router handler.
- Around line 118-137: The endpoint router.get('/workflow-run-logs') currently
accepts an arbitrary absolute URL from the logUrl query and passes it into
workflowService.fetchWorkflowRunLogs, enabling SSRF; before calling
getUserTokenFromRequest / fetchWorkflowRunLogs validate logUrl: require it to be
either a relative path or an absolute URL whose hostname matches the trusted
OpenChoreo API domain or an allowlist of trusted hosts, and if it fails
validation throw an InputError (or return 400). Update the validation logic
around the logUrl extraction (the code using logUrl, step, sinceSeconds and
workflowService.fetchWorkflowRunLogs) to parse the URL, check hostname against
the configured TRUSTED_LOG_HOSTS or specific domain, and only pass the validated
URL to workflowService.fetchWorkflowRunLogs.
In `@plugins/openchoreo-ci/src/components/BuildLogs/BuildLogs.tsx`:
- Around line 166-171: Appending fetched entries when useSinceSeconds is true
can produce duplicate log lines; create a deduplication helper (e.g.,
deduplicateLogs) that filters by a stable key such as `${timestamp}-${log}` or a
unique id, then call it when updating state: inside the setLogsByStep update for
the activeStepName combine existing (prev[activeStepName] ?? []) and entries,
pass the combined array through deduplicateLogs, and set that result; reference
setLogsByStep, activeStepName, useSinceSeconds, and entries when making the
change.
- Around line 51-132: The polling closure in the useEffect (fetchStatus)
captures activeStepName once, causing stale reads and re-auto-selection after a
user changes steps; fix by introducing a mutable ref (e.g., activeStepNameRef)
that is kept in sync with activeStepName and read inside fetchStatus before
doing auto-selection (use setActiveStepName normally but also update
activeStepNameRef.current whenever activeStepName changes or right after calling
setActiveStepName), so the effect can remain with [client, build] deps and the
interval logic in useEffect/fetchStatus continues to use the up-to-date selected
step.
🧹 Nitpick comments (6)
packages/openchoreo-client-node/openapi/openchoreo-api.yaml (1)
822-848: Consider avoiding empty-string sentinels forlogURL.If logs aren’t available, consider making
logURLnullable or omitting it so clients don’t need to special-case empty strings.plugins/openchoreo-ci/src/api/OpenChoreoCiClient.ts (1)
174-204: Consider reusing theapiFetchhelper for consistency.This method duplicates fetch and error handling logic instead of leveraging the existing
apiFetchhelper. While functionally correct, usingapiFetchwould ensure consistent error messaging and reduce code duplication.♻️ Proposed refactor using apiFetch
async fetchWorkflowRunLogs( logUrl: string, options?: { step?: string; sinceSeconds?: number }, ): Promise<LogEntry[]> { if (!logUrl) { throw new Error('logUrl is required to fetch workflow run logs'); } - const baseUrl = await this.discovery.getBaseUrl('openchoreo-ci-backend'); - const url = new URL(`${baseUrl}/workflow-run-logs`); - - url.searchParams.set('logUrl', logUrl); - if (options?.step) { - url.searchParams.set('step', options.step); - } - if (typeof options?.sinceSeconds === 'number' && options.sinceSeconds > 0) { - url.searchParams.set('sinceSeconds', String(options.sinceSeconds)); - } - - const response = await this.fetchApi.fetch(url.toString()); - - if (!response.ok) { - const errorText = await response.text(); - throw new Error( - `Failed to fetch workflow run logs (${response.status}): ${errorText}`, - ); + const params: Record<string, string> = { logUrl }; + if (options?.step) { + params.step = options.step; + } + if (typeof options?.sinceSeconds === 'number' && options.sinceSeconds > 0) { + params.sinceSeconds = String(options.sinceSeconds); } - const entries = (await response.json()) as LogEntry[]; - return entries; + return this.apiFetch<LogEntry[]>('/workflow-run-logs', { params }); }plugins/openchoreo-ci-backend/src/services/WorkflowService.ts (2)
409-414: Consider adding a request timeout and simplifying the token check.The native
fetchcall has no timeout, which could cause requests to hang indefinitely if the server doesn't respond. Additionally,token && token.length > 0is redundant since an empty string is already falsy.♻️ Proposed fix with timeout and simplified check
- const response = await fetch(url.toString(), { - headers: - token && token.length > 0 - ? { Authorization: `Bearer ${token}` } - : undefined, - }); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30_000); + + try { + const response = await fetch(url.toString(), { + headers: token ? { Authorization: `Bearer ${token}` } : undefined, + signal: controller.signal, + }); + clearTimeout(timeoutId);Then wrap the rest of the response handling inside the try block and add a catch for
AbortError:} catch (err) { clearTimeout(timeoutId); if (err instanceof Error && err.name === 'AbortError') { throw new Error('Request timed out while fetching workflow run logs'); } throw err; }
427-443: Type assertion could be replaced with runtime validation.The inline type definition with
asassertion trusts the external API response shape. Consider validating the array structure before mapping, which you partially do on line 433-438 but only for the array type.plugins/openchoreo-ci/src/components/BuildLogs/BuildLogs.tsx (2)
42-48: UnnecessaryuseMemofor a constant array.The
terminalStatusesarray never changes between renders, so wrapping it inuseMemowith an empty dependency array adds overhead without benefit. Consider defining it as a module-level constant or inside the component without memoization.♻️ Proposed fix
+const TERMINAL_STATUSES = ['completed', 'failed', 'succeeded', 'error']; + +const isTerminalStatus = (status?: string) => + status ? TERMINAL_STATUSES.includes(status.toLowerCase()) : false; + export const LogsContent = ({ build }: LogsContentProps) => { const classes = useStyles(); const client = useApi(openChoreoCiClientApiRef); // ... state declarations ... - const terminalStatuses = useMemo( - () => ['completed', 'failed', 'succeeded', 'error'], - [], - ); - - const isTerminalStatus = (status?: string) => - status ? terminalStatuses.includes(status.toLowerCase()) : false;
322-327: Inline style creates a new object on every render.The inline
style={{ marginBottom: '4px' }}creates a new object reference on each render, which can impact performance in large log lists. Consider moving this to the stylesheet.♻️ Move style to CSS class
In
styles.ts, add:logEntry: { marginBottom: '4px', },Then update the component:
- <Box key={index} style={{ marginBottom: '4px' }}> + <Box key={index} className={classes.logEntry}>
Comment on lines
+131
to
+134
| sinceSeconds: | ||
| typeof sinceSeconds === 'string' | ||
| ? Number.parseInt(sinceSeconds, 10) | ||
| : undefined, |
There was a problem hiding this comment.
Reject non-numeric or negative sinceSeconds values.
parseInt can yield NaN; please validate and return a 400 for invalid input.
💡 Suggested validation
- const entries = await workflowService.fetchWorkflowRunLogs(
- logUrl,
- {
- step: typeof step === 'string' ? step : undefined,
- sinceSeconds:
- typeof sinceSeconds === 'string'
- ? Number.parseInt(sinceSeconds, 10)
- : undefined,
- },
- userToken,
- );
+ const parsedSinceSeconds =
+ typeof sinceSeconds === 'string'
+ ? Number.parseInt(sinceSeconds, 10)
+ : undefined;
+ if (
+ parsedSinceSeconds !== undefined &&
+ (Number.isNaN(parsedSinceSeconds) || parsedSinceSeconds < 0)
+ ) {
+ throw new InputError('sinceSeconds must be a non-negative integer');
+ }
+
+ const entries = await workflowService.fetchWorkflowRunLogs(
+ logUrl,
+ {
+ step: typeof step === 'string' ? step : undefined,
+ sinceSeconds: parsedSinceSeconds,
+ },
+ userToken,
+ );🤖 Prompt for AI Agents
In `@plugins/openchoreo-ci-backend/src/router.ts` around lines 131 - 134, The
handler currently sets sinceSeconds using Number.parseInt which can produce NaN
or negative values; update the logic around sinceSeconds (the parse and
assignment shown) to validate the parsed value is a finite non-negative integer
and, if invalid, return a 400 response with an explanatory message;
specifically, after parsing Number.parseInt(sinceSeconds, 10) ensure
Number.isInteger(value) && value >= 0 before assigning, otherwise send a 400
error from the router handler.
0274ed3 to
ab43699
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/openchoreo-client-node/openapi/openchoreo-observability-api.yaml`:
- Around line 111-118: The OpenAPI parameter example for namespaceName is
incorrect (it shows a workflow run ID); update the example value for the
parameter named "namespaceName" in openchoreo-observability-api.yaml to a
realistic namespace string (e.g., a service or team name) rather than a run ID
so consumers see a proper namespace example—locate the parameter block for
"namespaceName" and replace the example value accordingly.
- Around line 1116-1129: The ComponentWorkflowRunLogEntry schema declares logs
include a timestamp but does not require the timestamp property; update the
schema so it matches the endpoint contract by adding "timestamp" to the required
array of ComponentWorkflowRunLogEntry (ensuring the timestamp property remains
type: string, format: date-time) OR change the description text on
ComponentWorkflowRunLogEntry.timestamp to indicate timestamps may be absent;
modify the required array or the timestamp description accordingly to keep the
OpenAPI definition consistent with the endpoint documentation.
In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts`:
- Around line 371-378: The fetchWorkflowRunLogs function can throw if callers
pass undefined for the options parameter; change the function to default options
to an empty object (e.g., options = {}) in the signature or add an early guard
that assigns options = {} at the top of fetchWorkflowRunLogs so accesses like
options.step or options.sinceSeconds are safe; update any related uses in the
function (references to options.step and options.sinceSeconds) accordingly.
In `@plugins/openchoreo-ci/src/api/OpenChoreoCiClientApi.ts`:
- Around line 64-79: The docstring for fetchWorkflowRunLogs is inaccurate: it
mentions a "provided log URL" but the method signature accepts run identifiers
(namespaceName, projectName, componentName, runName, hasLiveObservability,
options) rather than a URL; update the docstring to describe that the client
fetches logs for a workflow run by resolving or invoking the appropriate log
endpoint using those identifiers (or alternatively change the API to accept a
logUrl parameter if you intend callers to pass a URL), and ensure the updated
comment on fetchWorkflowRunLogs clearly lists the parameters used to locate the
logs and the behavior when hasLiveObservability is true.
In `@plugins/openchoreo-ci/src/components/BuildLogs/BuildLogs.tsx`:
- Around line 228-233: The useEffect that fetches logs (referenced by the
useEffect block using client, statusState?.hasLiveObservability, activeStepName,
statusState?.status) reads build.namespaceName, build.projectName,
build.componentName, and build.name but does not include build in the dependency
array; update the dependency array to include the build object (or the specific
build properties: build.namespaceName, build.projectName, build.componentName,
build.name) so the effect re-runs whenever the build prop changes and prevents
showing stale logs.
🧹 Nitpick comments (6)
plugins/openchoreo-ci/src/api/OpenChoreoCiClientApi.ts (1)
50-53: Add a proper@deprecatedJSDoc instead of a TODO.
Line 51: consumers won’t see deprecation warnings in TS tooling with a TODO.Proposed tweak
- // TODO: Deprecate this method and use the new method fetchWorkflowRunLogs instead + /** `@deprecated` Use fetchWorkflowRunLogs instead. */ fetchBuildLogsForBuild(build: ModelsBuild): Promise<RuntimeLogsResponse>;plugins/openchoreo-ci/src/api/OpenChoreoCiClient.ts (1)
174-216: Consider reusingapiFetchfor consistent error handling and headers.
Lines 174–216: this duplicates base URL building and error handling, which can drift.Suggested refactor
- const baseUrl = await this.discovery.getBaseUrl('openchoreo-ci-backend'); - const url = new URL(`${baseUrl}/workflow-run-logs`); - - url.searchParams.set('namespaceName', namespaceName); - url.searchParams.set('projectName', projectName); - url.searchParams.set('componentName', componentName); - url.searchParams.set('runName', runName); - url.searchParams.set( - 'hasLiveObservability', - hasLiveObservability.toString(), - ); - if (options?.step) { - url.searchParams.set('step', options.step); - } - if (typeof options?.sinceSeconds === 'number' && options.sinceSeconds > 0) { - url.searchParams.set('sinceSeconds', String(options.sinceSeconds)); - } - - const response = await this.fetchApi.fetch(url.toString()); - - if (!response.ok) { - const errorText = await response.text(); - throw new Error( - `Failed to fetch workflow run logs (${response.status}): ${errorText}`, - ); - } - - const entries = (await response.json()) as LogEntry[]; - return entries; + const params: Record<string, string> = { + namespaceName, + projectName, + componentName, + runName, + hasLiveObservability: String(hasLiveObservability), + }; + if (options?.step) params.step = options.step; + if (typeof options?.sinceSeconds === 'number' && options.sinceSeconds > 0) { + params.sinceSeconds = String(options.sinceSeconds); + } + + return this.apiFetch<LogEntry[]>('/workflow-run-logs', { params });plugins/openchoreo-ci/src/components/BuildLogs/BuildLogs.tsx (3)
42-48: Consider moving constant array outside the component.
terminalStatusesis a static array that never changes. UsinguseMemowith an empty dependency array works but adds unnecessary overhead. Defining it as a module-level constant is cleaner and avoids the hook call entirely.♻️ Suggested refactor
+const TERMINAL_STATUSES = ['completed', 'failed', 'succeeded', 'error']; + +const isTerminalStatus = (status?: string) => + status ? TERMINAL_STATUSES.includes(status.toLowerCase()) : false; + export const LogsContent = ({ build }: LogsContentProps) => { const classes = useStyles(); const client = useApi(openChoreoCiClientApiRef); // ... state declarations ... - - const terminalStatuses = useMemo( - () => ['completed', 'failed', 'succeeded', 'error'], - [], - ); - - const isTerminalStatus = (status?: string) => - status ? terminalStatuses.includes(status.toLowerCase()) : false;
323-329: Consider a stable key for log entries if logs could be deduplicated.Using array index as key (
key={index}) works here since logs only append. However, if the duplicate log entries issue is addressed with deduplication (which may filter entries), index-based keys could cause unnecessary re-renders. Using a composite key from log data would be more robust.♻️ Suggested improvement
- {activeLogs.map((logEntry, index) => ( - <Box key={index} style={{ marginBottom: '4px' }}> + {activeLogs.map((logEntry, index) => ( + <Box key={`${logEntry.timestamp ?? index}-${index}`} style={{ marginBottom: '4px' }}>
331-335: Dead code: else branch is never visible.When
activeStepName !== step.name, the Accordion is collapsed (expanded={false}on line 287), so the AccordionDetails content is hidden. The "Expand this step to view logs" message will never be seen by users.♻️ Suggested simplification
<AccordionDetails> - {activeStepName === step.name ? ( <Box className={classes.logsContainer}> {logsLoading && activeLogs.length === 0 && ( <Box className={classes.inlineLoadingContainer}> <CircularProgress size={18} /> <Typography variant="body2" color="textSecondary"> Loading logs... </Typography> </Box> )} {!logsLoading && activeLogs.length === 0 && ( <Typography variant="body2" className={classes.noLogsText}> No logs available for this step </Typography> )} {activeLogs.map((logEntry, index) => ( <Box key={index} style={{ marginBottom: '4px' }}> <Typography variant="body2" className={classes.logText}> {logEntry.log} </Typography> </Box> ))} </Box> - ) : ( - <Typography variant="body2" className={classes.noLogsText}> - Expand this step to view logs. - </Typography> - )} </AccordionDetails>packages/openchoreo-client-node/openapi/openchoreo-observability-api.yaml (1)
104-155: Clarify streaming semantics and cap response size.The summary says “stream” but the response is a JSON array. If this isn’t SSE/NDJSON, drop “stream”; if it is, define the proper content type. Also consider adding
limitandmaxItemsto avoid unbounded payloads (CKV_OPENAPI_21).♻️ Suggested spec tweaks
- summary: Get workflow run logs (stream) + summary: Get workflow run logs ... - Logs are returned as a JSON array of log entries with timestamps, optionally filtered by step. + Logs are returned as a JSON array of log entries with timestamps, optionally filtered by step. + Use `limit` to cap the number of entries returned. ... - name: step in: query required: false description: Filter logs by a specific workflow step name schema: type: string example: build-step + - name: limit + in: query + required: false + description: Maximum number of log entries to return + schema: + type: integer + minimum: 1 + maximum: 10000 + default: 100 + example: 100 ... schema: type: array description: Array of log entries with timestamps + maxItems: 10000 items: $ref: '#/components/schemas/ComponentWorkflowRunLogEntry'
Comment on lines
+111
to
+118
| - name: namespaceName | ||
| in: path | ||
| required: true | ||
| description: The namespace name | ||
| schema: | ||
| type: string | ||
| example: 'reading-list-service-workflow-290506e5' | ||
| - name: projectName |
There was a problem hiding this comment.
Namespace example value looks copied from runName.
This example should be a namespace, not the workflow run ID.
📝 Example fix
- example: 'reading-list-service-workflow-290506e5'
+ example: 'default'🤖 Prompt for AI Agents
In `@packages/openchoreo-client-node/openapi/openchoreo-observability-api.yaml`
around lines 111 - 118, The OpenAPI parameter example for namespaceName is
incorrect (it shows a workflow run ID); update the example value for the
parameter named "namespaceName" in openchoreo-observability-api.yaml to a
realistic namespace string (e.g., a service or team name) rather than a run ID
so consumers see a proper namespace example—locate the parameter block for
"namespaceName" and replace the example value accordingly.
Comment on lines
+1116
to
+1129
| ComponentWorkflowRunLogEntry: | ||
| type: object | ||
| description: A single log entry from a component workflow run | ||
| required: | ||
| - log | ||
| properties: | ||
| timestamp: | ||
| type: string | ||
| format: date-time | ||
| description: Timestamp when the log entry was generated (RFC3339 format) | ||
| example: '2025-01-06T10:00:00.123Z' | ||
| log: | ||
| type: string | ||
| description: The log message content |
There was a problem hiding this comment.
Timestamp should be required (or description updated).
Endpoint text says logs are returned “with timestamps,” but the schema doesn’t require timestamp. Align the schema or revise the description.
📝 Suggested schema alignment
ComponentWorkflowRunLogEntry:
type: object
description: A single log entry from a component workflow run
required:
- - log
+ - log
+ - timestamp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ComponentWorkflowRunLogEntry: | |
| type: object | |
| description: A single log entry from a component workflow run | |
| required: | |
| - log | |
| properties: | |
| timestamp: | |
| type: string | |
| format: date-time | |
| description: Timestamp when the log entry was generated (RFC3339 format) | |
| example: '2025-01-06T10:00:00.123Z' | |
| log: | |
| type: string | |
| description: The log message content | |
| ComponentWorkflowRunLogEntry: | |
| type: object | |
| description: A single log entry from a component workflow run | |
| required: | |
| - log | |
| - timestamp | |
| properties: | |
| timestamp: | |
| type: string | |
| format: date-time | |
| description: Timestamp when the log entry was generated (RFC3339 format) | |
| example: '2025-01-06T10:00:00.123Z' | |
| log: | |
| type: string | |
| description: The log message content |
🤖 Prompt for AI Agents
In `@packages/openchoreo-client-node/openapi/openchoreo-observability-api.yaml`
around lines 1116 - 1129, The ComponentWorkflowRunLogEntry schema declares logs
include a timestamp but does not require the timestamp property; update the
schema so it matches the endpoint contract by adding "timestamp" to the required
array of ComponentWorkflowRunLogEntry (ensuring the timestamp property remains
type: string, format: date-time) OR change the description text on
ComponentWorkflowRunLogEntry.timestamp to indicate timestamps may be absent;
modify the required array or the timestamp description accordingly to keep the
OpenAPI definition consistent with the endpoint documentation.
- Introduced new API endpoints to retrieve the status and logs of component workflow runs. - Added schemas for `ComponentWorkflowRunStatusResponse` and `WorkflowStepStatus` to define the structure of the responses. - Updated OpenAPI specification to include detailed descriptions and examples for the new endpoints. Signed-off-by: Akila-I <akila.99g@gmail.com>
- Added new methods `fetchWorkflowRunStatus` and `fetchWorkflowRunLogs` to the OpenChoreoCiClient for retrieving workflow run status and logs. - Updated the OpenChoreoCiClientApi interface to include the new methods. - Enhanced the BuildLogs component to utilize the new API methods for improved log fetching and status display. - Introduced necessary styles for better UI presentation of workflow steps and logs. Signed-off-by: Akila-I <akila.99g@gmail.com>
- Enhanced the BuildStatusChip component to include a new case for 'succeeded' status. - Added a Chip element with a success icon and label for better visual representation of successful builds. Signed-off-by: Akila-I <akila.99g@gmail.com>
… run status and logs endpoints Signed-off-by: Akila-I <akila.99g@gmail.com>
akila-i
force-pushed
the
build-logs-revamp
branch
from
ab43699 to
99e3d89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts`:
- Around line 494-509: The error handling in the WorkflowService fetch for
workflow run logs calls await response.text(), which can be empty or throw
because the openapi-fetch client may have already consumed the body; replace
that call and use the existing error object instead (the same pattern used in
fetchBuildLogs). In the error branch of the method (the block checking if (error
|| !response.ok)), remove await response.text() and derive details from the
error variable (e.g., error?.message or JSON.stringify(error)) when logging and
when constructing the thrown Error, while keeping the 404 branch and
ObservableNotConfiguredError behavior unchanged.
🧹 Nitpick comments (3)
plugins/openchoreo-ci/src/components/BuildStatusChip/BuildStatusChip.tsx (1)
12-29: Consider consolidating duplicate switch cases.The
'completed'and'succeeded'cases render identical JSX. You can use fall-through to reduce duplication.♻️ Suggested consolidation
switch (statusType) { case 'completed': + case 'succeeded': return ( <Chip icon={<CheckCircleIcon style={{ color: '#2e7d32' }} />} label={status} size="small" className={`${classes.statusChip} ${classes.successChip}`} /> ); - case 'succeeded': - return ( - <Chip - icon={<CheckCircleIcon style={{ color: '#2e7d32' }} />} - label={status} - size="small" - className={`${classes.statusChip} ${classes.successChip}`} - /> - ); case 'failed':plugins/openchoreo-ci/src/api/OpenChoreoCiClient.ts (1)
174-217: Consider usingapiFetchfor consistency.This method manually constructs the URL and handles the fetch, while other methods use the
apiFetchhelper. This inconsistency makes the code harder to maintain.♻️ Suggested refactor using apiFetch
async fetchWorkflowRunLogs( namespaceName: string, projectName: string, componentName: string, runName: string, hasLiveObservability: boolean, options?: { step?: string; sinceSeconds?: number }, ): Promise<LogEntry[]> { if (!namespaceName || !projectName || !componentName || !runName) { throw new Error( 'namespaceName, projectName, componentName and runName are required fields for fetching workflow run logs', ); } - const baseUrl = await this.discovery.getBaseUrl('openchoreo-ci-backend'); - const url = new URL(`${baseUrl}/workflow-run-logs`); - - url.searchParams.set('namespaceName', namespaceName); - url.searchParams.set('projectName', projectName); - url.searchParams.set('componentName', componentName); - url.searchParams.set('runName', runName); - url.searchParams.set( - 'hasLiveObservability', - hasLiveObservability.toString(), - ); - if (options?.step) { - url.searchParams.set('step', options.step); - } - if (typeof options?.sinceSeconds === 'number' && options.sinceSeconds > 0) { - url.searchParams.set('sinceSeconds', String(options.sinceSeconds)); - } - - const response = await this.fetchApi.fetch(url.toString()); - - if (!response.ok) { - const errorText = await response.text(); - throw new Error( - `Failed to fetch workflow run logs (${response.status}): ${errorText}`, - ); - } - - const entries = (await response.json()) as LogEntry[]; - return entries; + const params: Record<string, string> = { + namespaceName, + projectName, + componentName, + runName, + hasLiveObservability: hasLiveObservability.toString(), + }; + if (options?.step) { + params.step = options.step; + } + if (typeof options?.sinceSeconds === 'number' && options.sinceSeconds > 0) { + params.sinceSeconds = String(options.sinceSeconds); + } + + return this.apiFetch<LogEntry[]>('/workflow-run-logs', { params }); }packages/openchoreo-client-node/openapi/openchoreo-observability-api.yaml (1)
146-164: Consider addingmaxItemsto the response array schema.The response array schema lacks a
maxItemsconstraint. While not always necessary, adding a reasonable limit can help document expected behavior and may be flagged by security-focused OpenAPI linters.Suggested fix
content: application/json: schema: type: array description: Array of log entries with timestamps + maxItems: 10000 items: $ref: '#/components/schemas/ComponentWorkflowRunLogEntry'
Comment on lines
+494
to
+509
| if (error || !response.ok) { | ||
| if (response.status === 404) { | ||
| this.logger.info( | ||
| `Workflow run logs endpoint not available (404). The observability service may not support workflow run logs yet.`, | ||
| ); | ||
| throw new ObservabilityNotConfiguredError(componentName); | ||
| } | ||
| const errorText = await response.text(); | ||
| this.logger.error( | ||
| `Failed to fetch workflow run logs for component ${componentName}: ${response.status} ${response.statusText}`, | ||
| { error: errorText }, | ||
| ); | ||
| throw new Error( | ||
| `Failed to fetch workflow run logs: ${response.status} ${response.statusText}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential issue: response.text() may fail or return empty after error parsing.
When the openapi-fetch client encounters an error response, it typically consumes the response body to populate the error object. Calling response.text() afterwards may return an empty string or throw, resulting in an unhelpful error message.
Consider using the error object directly for error details, similar to how fetchBuildLogs handles this at line 334-338:
Suggested approach
if (error || !response.ok) {
if (response.status === 404) {
this.logger.info(
`Workflow run logs endpoint not available (404). The observability service may not support workflow run logs yet.`,
);
throw new ObservabilityNotConfiguredError(componentName);
}
- const errorText = await response.text();
this.logger.error(
`Failed to fetch workflow run logs for component ${componentName}: ${response.status} ${response.statusText}`,
- { error: errorText },
+ { error },
);
throw new Error(
`Failed to fetch workflow run logs: ${response.status} ${response.statusText}`,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (error || !response.ok) { | |
| if (response.status === 404) { | |
| this.logger.info( | |
| `Workflow run logs endpoint not available (404). The observability service may not support workflow run logs yet.`, | |
| ); | |
| throw new ObservabilityNotConfiguredError(componentName); | |
| } | |
| const errorText = await response.text(); | |
| this.logger.error( | |
| `Failed to fetch workflow run logs for component ${componentName}: ${response.status} ${response.statusText}`, | |
| { error: errorText }, | |
| ); | |
| throw new Error( | |
| `Failed to fetch workflow run logs: ${response.status} ${response.statusText}`, | |
| ); | |
| } | |
| if (error || !response.ok) { | |
| if (response.status === 404) { | |
| this.logger.info( | |
| `Workflow run logs endpoint not available (404). The observability service may not support workflow run logs yet.`, | |
| ); | |
| throw new ObservabilityNotConfiguredError(componentName); | |
| } | |
| this.logger.error( | |
| `Failed to fetch workflow run logs for component ${componentName}: ${response.status} ${response.statusText}`, | |
| { error }, | |
| ); | |
| throw new Error( | |
| `Failed to fetch workflow run logs: ${response.status} ${response.statusText}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts` around lines
494 - 509, The error handling in the WorkflowService fetch for workflow run logs
calls await response.text(), which can be empty or throw because the
openapi-fetch client may have already consumed the body; replace that call and
use the existing error object instead (the same pattern used in fetchBuildLogs).
In the error branch of the method (the block checking if (error ||
!response.ok)), remove await response.text() and derive details from the error
variable (e.g., error?.message or JSON.stringify(error)) when logging and when
constructing the thrown Error, while keeping the 404 branch and
ObservableNotConfiguredError behavior unchanged.
stefinie123
approved these changes
Feb 10, 2026
stefinie123
approved these changes
Feb 10, 2026
This was referenced Feb 12, 2026
This was referenced Mar 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR implements frontend changes to retrieve component workflow run status and logs.
Related: openchoreo/openchoreo#1474
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
openchoreo/openchoreo#1747
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Chores