Reduce CircleCI boilerplate #663
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Defs as .circleci/defs.m4
participant JobMacro as Job macro (e.g., quick_job)
participant Generated as Generated CircleCI config
Note over Defs: New reusable boilerplate macro
Defs->>JobMacro: define [circleci_boilerplate]
JobMacro->>Generated: emit image line
JobMacro->>Generated: invoke circleci_boilerplate
Note right of Generated: job now contains image + expanded boilerplate (resource_class, env, restore_cache, checkout, save_cache)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.circleci/defs.m4 (2)
9-23: Quote CIRCLE_COMPARE_URL and sanity-check pipeline. availability*
- Quote the URL to avoid YAML parsing surprises and accidental colon interpretation.
- Please confirm that pipeline.project.git_url and pipeline.git.base_revision are always present in your pipelines (non‑PR builds may not set base_revision). If absent, this will render an unusable URL or fail interpolation.
Apply this diff:
- CIRCLE_COMPARE_URL: << pipeline.project.git_url >>/compare/<< pipeline.git.base_revision >>..<<pipeline.git.revision>> + CIRCLE_COMPARE_URL: '<< pipeline.project.git_url >>/compare/<< pipeline.git.base_revision >>..<< pipeline.git.revision >>'
14-23: Re-evaluate caching .git (size/immutability trade-offs)Caching the entire .git directory can be large and churn per-commit. Consider a lighter strategy (e.g., rely on checkout only, or cache a shallow clone dir keyed by branch prefix). If you keep this, ensure cache quotas are monitored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.circleci/defs.m4(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
🔇 Additional comments (1)
.circleci/defs.m4 (1)
25-31: LGTM on macro placement and YAML structureThe boilerplate call sits at the correct level (sibling of docker), and subsequent run steps remain within steps. Indentation looks consistent across all jobs.
Please validate the rendered config to be safe:
- Generate: m4 -P .circleci/defs.m4 > /tmp/config.yml
- Validate: circleci config validate /tmp/config.yml
Also applies to: 37-39, 45-46, 52-55, 61-65, 71-75, 80-87, 105-107, 113-114, 120-123, 129-133, 139-143, 148-155
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.circleci/defs.m4 (2)
14-23: Reassess caching of.gitCaching the entire Git metadata can grow large and compete with dependency caches; CircleCI already optimizes checkout. Ensure wins outweigh cache I/O.
If this cache isn’t materially reducing checkout time, drop it:
- - restore_cache: - keys: - - source-v1-{{ .Branch }}-{{ .Revision }} - - source-v1-{{ .Branch }}- - - source-v1- - - checkout - - save_cache: - key: source-v1-{{ .Branch }}-{{ .Revision }} - paths: - - ".git" + - checkoutOtherwise, consider limiting cache size via shallow fetch (in checkout) and rotating the cache key prefix (e.g.,
source-v2-) if formats change.
67-75: Remove- run: envto avoid secrets leakagePrinting the full environment can expose tokens/keys in logs. Prefer a whitelisted context print if needed.
- - run: env + - run: + name: Show build context (safe) + command: | + echo "BRANCH=$CIRCLE_BRANCH" + echo "SHA1=$CIRCLE_SHA1"
♻️ Duplicate comments (2)
.circleci/defs.m4 (2)
9-23: Parameterize resource_class (default to large)Avoid hard-coding; let callers override when needed (e.g., xlarge). Also switch call sites to
circleci_boilerplate()to ensure arg parsing.-define([circleci_boilerplate], [dnl - resource_class: large +define([circleci_boilerplate], [dnl + resource_class: ifelse($1,[],[large],[$1]) environment:Update invocations below (examples shown elsewhere in this file):
-circleci_boilerplate +circleci_boilerplate()
25-31: Usecircleci_boilerplate()consistently after parameterizationOnce
circleci_boilerplateaccepts args, ensure it’s invoked with parentheses to enable$1resolution and maintain correct expansion.-circleci_boilerplate +circleci_boilerplate()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.circleci/Makefile(1 hunks).circleci/defs.m4(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
🔇 Additional comments (2)
.circleci/defs.m4 (2)
11-13: Verify CIRCLE_COMPARE_URL construction and runtime variables
- Confirm runtime availability of pipeline.project.git_url, pipeline.git.base_revision, and pipeline.git.revision in your CircleCI org — presence in the repo (.circleci/config.yml) doesn't guarantee runtime exposure.
- For GitHub compare links prefer three dots (
base...head) — replace..with...if you intend GitHub compare URLs.- Avoid shadowing CircleCI's built-in CIRCLE_COMPARE_URL unless intentional.
Locations: .circleci/defs.m4 (lines 11–13); rendered occurrences present throughout .circleci/config.yml.
Proposed tweak (if targeting GitHub):
- CIRCLE_COMPARE_URL: << pipeline.project.git_url >>/compare/<< pipeline.git.base_revision >>..<<pipeline.git.revision>> + CIRCLE_COMPARE_URL: << pipeline.project.git_url >>/compare/<< pipeline.git.base_revision >>...<<pipeline.git.revision >>
89-101: Validatejob_dependencesYAML shaping — verifiedGenerated .circleci/config.yml validates; non-canary jobs render as “- job:” with nested
requires, and the canary (jdk25) entries appear as plain “- job” (no colon).
| circleci_boilerplate | ||
| - run: ./scripts/test-nonquick-txt-diff.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the macro call fix across all job macros
Update every occurrence to circleci_boilerplate(); keep indentation as-is so YAML remains valid under the job key.
-circleci_boilerplate
+circleci_boilerplate()Also applies to: 45-46, 52-55, 61-65, 71-75, 80-87
🤖 Prompt for AI Agents
In .circleci/defs.m4 around lines 37-38 (and also apply the same change at
45-46, 52-55, 61-65, 71-75, 80-87), the macro calls use bare
"circleci_boilerplate" which must be changed to "circleci_boilerplate()" to be a
proper macro invocation; update each occurrence to add the parentheses exactly
where the name appears and preserve the existing indentation so the generated
YAML remains valid under the job key.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
.circleci/defs.m4 (6)
57-65: Apply the same invocation pattern here- kvasir-$1-jdk$2: + kvasir-$1-jdk$2: docker: - image: mdernst/daikon-$1-jdk$2-plus -circleci_boilerplate +circleci_boilerplate([source-cache-kvasir-$1-jdk$2])
67-75: Apply the same invocation pattern here- typecheck-latest-$1-jdk$2: + typecheck-latest-$1-jdk$2: docker: - image: mdernst/daikon-$1-jdk$2 -circleci_boilerplate +circleci_boilerplate([source-cache-typecheck-latest-$1-jdk$2])
76-87: Apply the same invocation pattern here- typecheck-bundled-$1-jdk$2: + typecheck-bundled-$1-jdk$2: docker: - image: mdernst/daikon-$1-jdk$2 -circleci_boilerplate +circleci_boilerplate([source-cache-typecheck-bundled-$1-jdk$2])
33-39: Apply the same invocation pattern here- nonquick-txt-diff-$1-jdk$2: + nonquick-txt-diff-$1-jdk$2: docker: - image: mdernst/daikon-$1-jdk$2 -circleci_boilerplate +circleci_boilerplate([source-cache-nonquick-txt-diff-$1-jdk$2])
41-46: Apply the same invocation pattern here- non-txt-diff-$1-jdk$2: + non-txt-diff-$1-jdk$2: docker: - image: mdernst/daikon-$1-jdk$2 -circleci_boilerplate +circleci_boilerplate([source-cache-non-txt-diff-$1-jdk$2])
48-55: Apply the same invocation pattern here- misc-$1-jdk$2: + misc-$1-jdk$2: docker: - image: mdernst/daikon-$1-jdk$2-plus -circleci_boilerplate +circleci_boilerplate([source-cache-misc-$1-jdk$2]).circleci/Makefile (1)
7-8: Make validate phony and avoid cd; validate the built artifact directlyRun CircleCI validator against the generated config; mark phony to prevent name collisions.
+.PHONY: validate clean -validate: config.yml - cd .. && circleci config validate +validate: config.yml + circleci config validate -c config.yml
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.circleci/Makefile(1 hunks).circleci/config.yml(68 hunks).circleci/defs.m4(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
🔇 Additional comments (1)
.circleci/config.yml (1)
25-31: Resolved — .circleci/config.yml validates; no duplicate-anchor issue foundcircleci config validate -c .circleci/config.yml returned "Config file at .circleci/config.yml is valid." No changes required.
| define([circleci_boilerplate], [dnl | ||
| resource_class: large | ||
| environment: | ||
| CIRCLE_COMPARE_URL: << pipeline.project.git_url >>/compare/<< pipeline.git.base_revision >>..<<pipeline.git.revision>> | ||
| steps: | ||
| - restore_cache: | ||
| keys: | ||
| - source-v1-{{ .Branch }}-{{ .Revision }} | ||
| - &source-cache source-v1-{{ .Branch }}-{{ .Revision }} | ||
| - source-v1-{{ .Branch }}- | ||
| - source-v1- | ||
| - checkout | ||
| - save_cache: | ||
| key: source-v1-{{ .Branch }}-{{ .Revision }} | ||
| key: *source-cache | ||
| paths: | ||
| - ".git" | ||
| - ".git"])dnl |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parametrize boilerplate: unique cache anchor per job; configurable resource_class
Prevents duplicate anchor names and lets you tune resources without editing the macro.
-define([circleci_boilerplate], [dnl
- resource_class: large
+define([circleci_boilerplate], [dnl
+ resource_class: ifelse($2,[],[large],[$2])
environment:
CIRCLE_COMPARE_URL: << pipeline.project.git_url >>/compare/<< pipeline.git.base_revision >>..<<pipeline.git.revision>>
steps:
- restore_cache:
keys:
- - &source-cache source-v1-{{ .Branch }}-{{ .Revision }}
+ - &$1 source-v1-{{ .Branch }}-{{ .Revision }}
- source-v1-{{ .Branch }}-
- source-v1-
- checkout
- save_cache:
- key: *source-cache
+ key: *$1
paths:
- ".git"])dnlCommittable suggestion skipped: line range outside the PR's diff.
| define([quick_job], [dnl | ||
| quick-txt-diff-$1-jdk$2: | ||
| docker: | ||
| - image: mdernst/daikon-$1-jdk$2 | ||
| circleci_boilerplate | ||
| - run: ./scripts/test-quick-txt-diff.sh | ||
| ])dnl |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass unique anchor names when invoking boilerplate
Ensures YAML validity and prepares for optional resource_class overrides.
-quick-txt-diff-$1-jdk$2:
+quick-txt-diff-$1-jdk$2:
docker:
- image: mdernst/daikon-$1-jdk$2
-circleci_boilerplate
+circleci_boilerplate([source-cache-quick-txt-diff-$1-jdk$2])📝 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.
| define([quick_job], [dnl | |
| quick-txt-diff-$1-jdk$2: | |
| docker: | |
| - image: mdernst/daikon-$1-jdk$2 | |
| circleci_boilerplate | |
| - run: ./scripts/test-quick-txt-diff.sh | |
| ])dnl | |
| define([quick_job], [dnl | |
| quick-txt-diff-$1-jdk$2: | |
| docker: | |
| - image: mdernst/daikon-$1-jdk$2 | |
| circleci_boilerplate([source-cache-quick-txt-diff-$1-jdk$2]) | |
| - run: ./scripts/test-quick-txt-diff.sh | |
| ])dnl |
🤖 Prompt for AI Agents
In .circleci/defs.m4 around lines 25 to 31, the macro invocation calls
circleci_boilerplate without supplying a unique anchor name which can produce
invalid YAML and prevents per-job resource_class overrides; update the macro
invocation to pass a unique anchor identifier (for example derived from the
quick job name and jdk version) as an argument to circleci_boilerplate and
thread that argument through the boilerplate macro so the generated YAML uses
distinct anchors per job and accepts an optional resource_class override.
| ifelse($2,canary_version,,[dnl | ||
| - $3-$1-jdk[]canary_version | ||
| ])dnl | ||
| ifelse($1,canary_os,,[ - $3-canary_os[]-jdk$2 | ||
| ifelse($1,canary_os,,[dnl | ||
| - $3-canary_os[]-jdk$2 | ||
| ])dnl |
There was a problem hiding this comment.
🧹 Nitpick
Clarify intent of canary gating; current m4 emits dependencies despite dnl
Using dnl inside the ifelse else-branch doesn’t disable it; the generated config still adds “require jdk25” and “require ubuntu” edges (as seen in config.yml). If that’s intentional, remove dnl for clarity; if not, drop these else branches.
Option A (keep gating; remove misleading dnl):
-ifelse($2,canary_version,,[dnl
- - $3-$1-jdk[]canary_version
-])
+ifelse($2,canary_version,,[
+ - $3-$1-jdk[]canary_version
+])Option B (disable gating by version/OS):
-ifelse($2,canary_version,,[
- - $3-$1-jdk[]canary_version
-])
-ifelse($1,canary_os,,[
- - $3-canary_os[]-jdk$2
-])
+ dnl version/OS gating disabled📝 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.
| ifelse($2,canary_version,,[dnl | |
| - $3-$1-jdk[]canary_version | |
| ])dnl | |
| ifelse($1,canary_os,,[ - $3-canary_os[]-jdk$2 | |
| ifelse($1,canary_os,,[dnl | |
| - $3-canary_os[]-jdk$2 | |
| ])dnl | |
| ifelse($2,canary_version,,[ | |
| - $3-$1-jdk[]canary_version | |
| ]) | |
| ifelse($1,canary_os,,[ | |
| - $3-canary_os[]-jdk$2 | |
| ]) |
| ifelse($2,canary_version,,[dnl | |
| - $3-$1-jdk[]canary_version | |
| ])dnl | |
| ifelse($1,canary_os,,[ - $3-canary_os[]-jdk$2 | |
| ifelse($1,canary_os,,[dnl | |
| - $3-canary_os[]-jdk$2 | |
| ])dnl | |
| dnl version/OS gating disabled |
🤖 Prompt for AI Agents
In .circleci/defs.m4 around lines 94-99 the use of dnl inside the ifelse
else-branches is misleading because dnl does not prevent the macro from emitting
the dependency lines; decide whether canary gating is intended and fix
accordingly: Option A (keep gating): remove the dnl tokens from the
else-branches so the emitted lines intentionally include the canary
suffix/prefix and make the intent explicit; Option B (disable gating): delete
the else-branch blocks entirely (the whole second argument to those ifelse
calls) so no canary-specific dependency lines are emitted. Ensure the chosen
change is applied consistently for both the version and OS checks and update
comments to reflect the behavior.
1 participant
Summary by CodeRabbit
Refactor
Chores