also looking at this
fix(gitlab): read a job trace as text, so ciLogs is not a lie
#1420
2 files
+56
-4
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -204,9 +204,9 @@ export class GitLabProvider implements GitProvider { | ||
| 204 | 204 | } |
| 205 | 205 | |
| 206 | 206 | /** A request whose 404 is an answer rather than an error. */ |
| 207 | private async optional<T>(method: string, path: string): Promise<T | null> { | |
| 207 | private async optional<T>(method: string, path: string, options: { raw?: boolean } = {}): Promise<T | null> { | |
| 208 | 208 | try { |
| 209 | return await this.request<T>(method, path) | |
| 209 | return await this.request<T>(method, path, undefined, options) | |
| 210 | 210 | } |
| 211 | 211 | catch (error) { |
| 212 | 212 | if (String(error).includes('404')) |
| @@ -651,8 +651,15 @@ export class GitLabProvider implements GitProvider { | ||
| 651 | 651 | } |
| 652 | 652 | |
| 653 | 653 | async getWorkflowRunLogs(runId: number): Promise<string | null> { |
| 654 | return this.optional<string>('GET', `/projects/${this.projectId}/jobs/${runId}/trace`) | |
| 655 | .catch(() => null) | |
| 654 | // `raw`, because a job trace is plain text. Without it the transport ran | |
| 655 | // `JSON.parse` over the log, threw on the first line that was not JSON — | |
| 656 | // which is every line — and the catch below reported that as "no logs". | |
| 657 | // GitLab declared `ciLogs: true` and could not return one. | |
| 658 | return this.optional<string>('GET', `/projects/${this.projectId}/jobs/${runId}/trace`, { raw: true }) | |
| 659 | .catch((error) => { | |
| 660 | this.logger.debug(`Could not read the trace for job ${runId}: ${formatError(error)}`) | |
| 661 | return null | |
| 662 | }) | |
| 656 | 663 | } |
| 657 | 664 | |
| 658 | 665 | /** |
| @@ -209,3 +209,48 @@ describe('re-running a failed run', () => { | ||
| 209 | 209 | expect(await gitlab().rerunWorkflowRun(77)).toBe(false) |
| 210 | 210 | }) |
| 211 | 211 | }) |
| 212 | ||
| 213 | /** | |
| 214 | * GitLab declared `ciLogs: true` and could not return a log. `optional` did | |
| 215 | * not pass `raw`, so the transport ran `JSON.parse` over a plain-text job | |
| 216 | * trace, threw, and the catch reported that as "no logs" — making `buddy | |
| 217 | * fix-ci` on GitLab answer "could not read the run logs" every single time. | |
| 218 | */ | |
| 219 | describe('reading a job log', () => { | |
| 220 | it('success case - gitlab returns the plain-text trace', async () => { | |
| 221 | const TRACE = `$ bun install\nerror: lockfile had changes, but lockfile is frozen\n` | |
| 222 | ||
| 223 | globalThis.fetch = (async () => new Response(TRACE, { | |
| 224 | status: 200, | |
| 225 | headers: { 'content-type': 'text/plain' }, | |
| 226 | })) as unknown as typeof fetch | |
| 227 | ||
| 228 | expect(await gitlab().getWorkflowRunLogs(77)).toBe(TRACE) | |
| 229 | }) | |
| 230 | ||
| 231 | it('success case - asks for the job trace, not the pipeline', async () => { | |
| 232 | const urls: string[] = [] | |
| 233 | globalThis.fetch = (async (input: string) => { | |
| 234 | urls.push(String(input)) | |
| 235 | return new Response('log text', { status: 200 }) | |
| 236 | }) as unknown as typeof fetch | |
| 237 | ||
| 238 | await gitlab().getWorkflowRunLogs(77) | |
| 239 | ||
| 240 | expect(urls[0]).toContain('/jobs/77/trace') | |
| 241 | }) | |
| 242 | ||
| 243 | it('edge case - a missing job reads as no log rather than an error', async () => { | |
| 244 | globalThis.fetch = (async () => new Response('', { status: 404 })) as unknown as typeof fetch | |
| 245 | ||
| 246 | expect(await gitlab().getWorkflowRunLogs(77)).toBeNull() | |
| 247 | }) | |
| 248 | ||
| 249 | it('success case - a trace that happens to be JSON is still returned verbatim', async () => { | |
| 250 | // The bug was parsing at all. A trace whose first line is JSON must not | |
| 251 | // come back as an object. | |
| 252 | globalThis.fetch = (async () => new Response('{"step":"install"}', { status: 200 })) as unknown as typeof fetch | |
| 253 | ||
| 254 | expect(await gitlab().getWorkflowRunLogs(77)).toBe('{"step":"install"}') | |
| 255 | }) | |
| 256 | }) | |