Refactor DownloadWorkflowLogs to LogsDownloadOptions and migrate all callsites#31336
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
DownloadWorkflowLogs to LogsDownloadOptions and migrate all callsites
There was a problem hiding this comment.
Pull request overview
Refactors the DownloadWorkflowLogs API away from a large positional parameter list to a typed LogsDownloadOptions struct, and migrates all in-repo callsites (production + tests) to the new options-based invocation.
Changes:
- Introduced
LogsDownloadOptionsand updatedDownloadWorkflowLogs(ctx, opts)signature in the logs orchestrator. - Migrated the
logscommand production callsite to use a named struct literal. - Migrated all test callsites to use
LogsDownloadOptions(setting only the fields needed).
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_orchestrator.go | Adds LogsDownloadOptions and refactors DownloadWorkflowLogs to accept it. |
| pkg/cli/logs_command.go | Updates the production callsite to pass a LogsDownloadOptions struct. |
| pkg/cli/logs_json_stderr_order_test.go | Migrates tests to call DownloadWorkflowLogs with LogsDownloadOptions. |
| pkg/cli/logs_ci_scenario_test.go | Migrates CI-scenario test to the options-based API. |
| pkg/cli/logs_download_test.go | Migrates download tests to the options-based API. |
| pkg/cli/context_cancellation_test.go | Migrates cancellation/timeout tests to the options-based API. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
| NoFirewall bool | ||
| Parse bool | ||
| JSONOutput bool | ||
| Timeout int |
| WorkflowName: "nonexistent-workflow-12345", | ||
| Count: 100, | ||
| OutputDir: "/tmp/test-logs", | ||
| Timeout: 1, |
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (130 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in 🔍 What the gate inferred from this diff
References:
|
🧪 Test Quality Sentinel ReportTest Quality Score: 93/100✅ Excellent test quality
Test Classification Details
Nature of Changes
No flagged tests — all existing behavioral invariants are preserved. Build Tag CheckAll 4 modified test files have the required Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 93/100. Test quality is excellent — 0% of modified tests are implementation tests (threshold: 30%). All 7 test updates are clean call-site refactoring that preserves existing behavioral contracts. No coding-guideline violations detected.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — this is a pure refactor that reshapes a public interface across multiple callsites.
Key Theme
- Partial struct adoption: The options struct was introduced at the call boundary, but the function body immediately shadows every field with a local variable (lines 68–92). This re-creates the flat parameter surface inside the implementation, reducing the locality benefit that motivated the refactor. Prefer
opts.Xreferences throughout the body.
Positive Highlights
- ✅ The motivating problem (26-arg positional signature) is real and the struct approach is exactly right
- ✅ All 8 callsites migrated cleanly; test call-sites now only set the fields they care about — this is a meaningful readability win
- ✅ Zero behavioural change; the orchestration logic is preserved intact
- ✅ PR description is clear and includes a concrete before/after example
Verdict
Approving — the core API improvement is solid and the existing reviewer comments cover the Timeout unit ambiguity well. The unpack block is an improvement opportunity, not a blocker.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.8M
| train := opts.Train | ||
| format := opts.Format | ||
| artifactSets := opts.ArtifactSets | ||
| after := opts.After |
There was a problem hiding this comment.
[/improve-codebase-architecture] The DownloadWorkflowLogs body immediately unpacks all 25 struct fields into local variables, re-creating the same flat parameter surface inside the implementation. Per the "locality" principle, the benefit of introducing LogsDownloadOptions is that readers can refer to opts.Engine, opts.JSONOutput, etc. directly — there is no need to shadow every field.
Consider removing the unpack block and using opts.X throughout the function body. This shortens the function, makes the struct's role clear, and keeps a single source of truth.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the review items in
Targeted tests for the touched areas pass. |
Uh oh!
There was an error while loading. Please reload this page.