Refactor buildCustomJobs into focused helpers to reduce complexity#36694
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
buildCustomJobs into focused helpers to reduce complexity
There was a problem hiding this comment.
Pull request overview
Refactors Compiler.buildCustomJobs in pkg/workflow/compiler_jobs.go by decomposing a large custom-job compilation routine into smaller helpers, aiming to reduce cyclomatic complexity while preserving the emitted workflow YAML shape and custom-job execution behavior.
Changes:
- Split custom job orchestration into
buildCustomJobs+ focused helpers for skip logic, dependency computation, and job construction. - Extracted job property parsing into dedicated helpers (e.g., runs-on, permissions, strategy, timeout, concurrency, env, container, services, outputs).
- Introduced
formatIndentedYAMLFieldto centralize YAML marshal + indentation formatting for multi-line job fields.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_jobs.go | Breaks down buildCustomJobs into cohesive helpers and adds a shared YAML field formatter to reduce repetition. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The only changed file is pkg/workflow/compiler_jobs.go (production code only). Test Quality Sentinel skipped. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (455 new lines in 📄 Draft ADR committed:
📋 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
|
![github-actions[bot]](https://github.com/_gh/avatars/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- API design:
formatIndentedYAMLField's booleantrimTrailingNewlineflag is a classic flag-argument smell — consider two named helpers or letting callers handle the trim explicitly. - Thin wrapper:
extractCustomJobPropertiesadds an extra indirection level for only two lines of delegation; collapsing it intobuildCustomJobwould flatten the call graph without losing clarity. - Predicate side effects:
shouldSkipCustomJoblogs as a side effect of what reads as a pure boolean query — separating those concerns makes it safer to test in isolation. - Silent fallthrough:
configureCustomJobExecutionsilently treats a non-stringusesvalue as a step-based job; a warning log would make misconfiguration visible. - Magic constant: the 6-space indent in
formatIndentedYAMLFieldis now centralised (✅ good!) but would benefit from a named constant to document its intent.
Positive Highlights
- ✅
buildCustomJobsorchestrator is now crystal-clear at under 30 lines - ✅
formatIndentedYAMLFieldcorrectly eliminates ~10 copy-paste marshal/indent blocks - ✅ Receiver vs. package-level function split is consistent and principled (methods only where
cfields are needed) - ✅ Execution-mode boundary (reusable vs. step-based) is now explicit and well-named
- ✅ Faithful extraction — no behavioral drift observed
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M
| if trimTrailingNewline { | ||
| return strings.TrimSuffix(formatted, "\n"), nil | ||
| } | ||
| return formatted, nil |
There was a problem hiding this comment.
[/improve-codebase-architecture] formatIndentedYAMLField's boolean trimTrailingNewline parameter is a flag argument — a known design smell that forces callers to supply a magic true/false whose meaning is invisible at the call site.
💡 Suggested approach
Two options:
Option A — two named helpers:
func formatIndentedYAMLField(fieldName string, value any) (string, error) { ... }
func formatIndentedYAMLFieldTrimmed(fieldName string, value any) (string, error) {
s, err := formatIndentedYAMLField(fieldName, value)
return strings.TrimSuffix(s, "\n"), err
}Option B — keep one helper, but have the callers call strings.TrimSuffix explicitly so the intent is self-documenting:
formatted, err := formatIndentedYAMLField("environment", v)
if err != nil { ... }
job.Environment = strings.TrimSuffix(formatted, "\n")Either approach makes the trim intent clear without a boolean flag.
| if err := c.extractCustomJobRunsOn(job, jobName, configMap); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
[/zoom-out] extractCustomJobProperties is a two-line delegation wrapper that adds an extra call stack layer without carrying semantic weight. Callers of buildCustomJob already trust it to orchestrate; moving the two calls up to buildCustomJob directly would make the decomposition one level shallower and easier to navigate.
💡 Suggestion
In buildCustomJob, replace:
if err := c.extractCustomJobProperties(job, jobName, configMap); err != nil {
return nil, err
}with the two direct calls:
if err := c.extractCustomJobCoreProperties(job, jobName, configMap); err != nil {
return nil, err
}
extractCustomJobOutputs(job, jobName, configMap)This removes the intermediate extractCustomJobProperties function entirely and keeps buildCustomJob as the single orchestration point.
| // Built-in jobs are already created before buildCustomJobs; treat jobs.<builtin> | ||
| // entries as customization-only and do not create duplicate jobs. | ||
| if _, exists := c.jobManager.GetJob(jobName); exists { | ||
| compilerJobsLog.Printf("Skipping jobs.%s (built-in job already exists)", jobName) |
There was a problem hiding this comment.
[/zoom-out] shouldSkipCustomJob is named as a pure predicate (should...) but has a side effect: it logs when returning true. This violates the principle of least surprise — a reader calling this function wouldn't expect it to emit log output.
💡 Suggestion
Either rename to skipCustomJob and document the side effect, or move the log statements to the call site in buildCustomJobs:
if c.shouldSkipCustomJob(jobName) {
compilerJobsLog.Printf("Skipping jobs.%s ...", jobName)
continue
}The latter keeps shouldSkipCustomJob a pure predicate that's easier to test and reason about.
| if hasUses { | ||
| if usesStr, ok := uses.(string); ok { | ||
| return configureCustomReusableWorkflow(job, jobName, usesStr, configMap) | ||
| } |
There was a problem hiding this comment.
[/zoom-out] configureCustomJobExecution silently falls through to step-based configuration when uses is present but its value is not a string. This was also the original behavior, but now it's a named entry point and the silent path could confuse future maintainers.
💡 Suggestion
Add a warning log for the non-string uses case so misconfigurations are surfaced:
func (c *Compiler) configureCustomJobExecution(job *Job, jobName string, configMap map[string]any, data *WorkflowData) error {
if uses, hasUses := configMap["uses"]; hasUses {
if usesStr, ok := uses.(string); ok {
return configureCustomReusableWorkflow(job, jobName, usesStr, configMap)
}
compilerJobsLog.Printf("Warning: job '%s' has a non-string 'uses' value (%T); treating as step-based job", jobName, uses)
}
return c.configureCustomJobSteps(job, jobName, configMap, data)
}This preserves backward-compatibility while making the footgun visible.
| } | ||
|
|
||
| lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") | ||
| var b strings.Builder |
There was a problem hiding this comment.
[/improve-codebase-architecture] The 6-space indentation " " is a magic constant repeated across all the helper functions replaced by formatIndentedYAMLField. Centralising the formatter was the right call, but the hardcoded string literal inside it still requires a reader to count spaces to understand the intent.
💡 Suggestion
const jobYAMLIndent = " " // 6 spaces — matches GitHub Actions job-level YAML indentation
func formatIndentedYAMLField(fieldName string, value any, trimTrailingNewline bool) (string, error) {
...
b.WriteString(jobYAMLIndent + line + "\n")
...
}This is a minor readability win, but the constant name makes the intent explicit for anyone who later needs to adjust indentation.
buildCustomJobsinpkg/workflow/compiler_jobs.gowas a 365-line complexity hotspot. This PR decomposes it into cohesive helpers, bringing the orchestrator under 150 lines while preserving existing custom-job behavior.Refactor scope: orchestration vs. implementation
buildCustomJobsas the high-level flow (iterate jobs, skip built-ins/pre-activation, build job, add to manager).Dependency and activation handling extracted
needson.needsjobs.Job property extraction split by concern
Execution mode handling isolated
uses,with,secrets) from standard step-based configuration (pre-steps/steps+ GH_HOST step injection).