chore(ci): fail CI when defaults/main.yml drifts from argument_specs.yml - #186
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds an Ansible argument-specification generator and a validator for drift between role defaults and specifications. Full-repository linting runs the validator. Elasticsearch maintenance options and certificate-passphrase handling are updated. Full-stack matrix concurrency is reduced. ChangesArgument specification validation
Full-stack CI execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Defaults as defaults/main.yml
participant Generator as gen_argspecs.py
participant Specs as argument_specs.yml
participant CI as GitHub Actions
participant Checker as check_argspecs.py
CLI->>Generator: generate role specifications
Generator->>Defaults: parse defaults and descriptions
Defaults-->>Generator: return variable entries
Generator->>Specs: write argument_specs.yml
CI->>Checker: run repository validation
Checker->>Defaults: load default variables
Checker->>Specs: load main options
Checker-->>CI: return validation status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/gen_argspecs.py`:
- Line 57: Rename the comprehension variable l in the pending_desc assignment to
a descriptive name such as line, updating the join expression accordingly while
preserving the existing filtering and stripping behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 291c565a-efaf-4b71-893f-a72895df37bb
📒 Files selected for processing (4)
.github/workflows/test_linting.ymlroles/elasticstack/defaults/main.ymlscripts/check_argspecs.pyscripts/gen_argspecs.py
| break | ||
| desc_lines.append(lines[i].lstrip("# ").rstrip()) | ||
| i += 1 | ||
| pending_desc = " ".join(l for l in desc_lines if l).strip() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the ambiguous comprehension variable.
Line 57 uses l, which Ruff reports as E741. Use a descriptive name.
Proposed fix
- pending_desc = " ".join(l for l in desc_lines if l).strip()
+ pending_desc = " ".join(desc_line for desc_line in desc_lines if desc_line).strip()📝 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
| pending_desc = " ".join(l for l in desc_lines if l).strip() | |
| pending_desc = " ".join(desc_line for desc_line in desc_lines if desc_line).strip() |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 57-57: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gen_argspecs.py` at line 57, Rename the comprehension variable l in
the pending_desc assignment to a descriptive name such as line, updating the
join expression accordingly while preserving the existing filtering and
stripping behavior.
Source: Linters/SAST tools
Oddly
force-pushed
the
chore/ci-argument-specs-drift-check
branch
from
August 8, 2026 09:12
6047513 to
6fc2034
Compare
Oddly
force-pushed
the
chore/ci-argument-specs-drift-check
branch
from
August 10, 2026 11:36
6fc2034 to
bfb0533
Compare
Adds scripts/check_argspecs.py — for every role that ships a meta/argument_specs.yml, compare the set of top-level (non-underscored) variables in defaults/main.yml against the options declared in the spec. Any mismatch either way (var in defaults but not spec, option in spec but no longer in defaults) exits non-zero with the offending names and a pointer to scripts/gen_argspecs.py for regeneration. Wired into test_linting.yml as the last step of the whole-collection lint job (skipped for the per-role invocation so we do not run five duplicate checks per PR). Also promotes gen_argspecs.py from /tmp/ into scripts/ so the fix path is self-contained, and uncomments elasticstack_cert_pass in defaults (one commented line the initial argument_specs PR left as spec-only drift — flipping to elasticstack_cert_pass: "" matches the actual role behaviour and clears the check).
Oddly
force-pushed
the
chore/ci-argument-specs-drift-check
branch
from
August 11, 2026 07:04
bfb0533 to
dd5070d
Compare
…ce defaults Two follow-ups the #190 merge surfaced against #186's own drift check: scripts/check_argspecs.py: only compare the role's 'main' entry point options against defaults/main.yml. Task-file entry points (node_maintenance_start / _end) take per-invocation parameters like elasticsearch_maintenance_password and _api_url that don't need role-wide defaults — they're inputs to a specific action, not defaults. The wider check was flagging those as spurious drift. roles/elasticsearch/meta/argument_specs.yml: the six role-wide maintenance defaults (elasticsearch_maintenance_wait_status, _health_retries, _health_delay, _wait_health, _require_green, elasticsearch_drain_cluster_settings) were declared under the two task-file entry points but not under 'main'. defaults/main.yml lists them, so the (correctly narrower) drift check now catches that gap and this backfill closes it. Also drops the pre-existing 'role_prefix | Verb' name[casing] warnings that were tripping lint_full for any PR whose diff pulled in the cert_detect_content_mode tasks. The convention is intentional (it matches the commit-message style), so it's moved from warn_list to skip_list rather than papered over with a noqa on every task.
The 'Set common password for common certificates' set_fact gated on elasticstack_cert_pass being defined, which was safe as long as the role's own default kept the variable commented out. When the drift check work uncommented it to a default of "" (so the arg spec and defaults stay in sync), the gate started firing and blanked elasticsearch_tls_key_passphrase. That empty string cascaded into elasticsearch-keystore.yml's http.ssl.keystore.secure_password Set step, which pipes it as stdin to `elasticsearch-keystore add` — and that binary rejects empty passphrases with a non-zero exit. Reproduced as: elasticsearch_custom passing on main (var commented, gate false) but failing on the drift branch (var defined empty, gate true, empty stdin) even though no keystore code changed between them. The docstring already promised 'Leave empty to use the per-role passphrases', so this changes the gate from 'is defined' to a non-empty check, which matches the documented behaviour and keeps downstream unaffected whether the default line is present or absent.
…narios room Committed memory with 6 concurrent slots landed close to 100 GB out of ~120 GB usable on incus-ci. That was fine while the memory-gate timeout was 15 min, but repeated real-world runs with the 45-min timeout still expose the 20 GB elasticstack_default and 13.8 GB es_kibana scenarios to steady starvation: they lose the retry race against smaller peers that release+reacquire faster. Dropping to 3 slots keeps peak committed memory well within reach and lets the heavy jobs actually acquire on the first attempt. Wall clock roughly doubles for the full matrix but every scenario finishes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test_full_stack.yml:
- Line 105: Replace the inline Molecule matrix job in the full-stack workflow
with a call to the reusable molecule.yml workflow, passing the matrix
configuration and max-parallel value through its supported inputs. Remove the
duplicated inline Molecule setup while preserving the existing test matrix
behavior and concurrency of 3.
- Line 104: Correct the wall-clock estimate comment near the matrix
configuration: for pull_request and merge_group, state that the eight-job matrix
runs in three ideal waves at max-parallel 3 (about 1.5×), rather than roughly
doubling. Scope the estimate to the default 48-job matrix or explicitly describe
the event-specific impact.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f64dc5c-bfa5-4048-b4a4-05b5486606e5
📒 Files selected for processing (1)
.github/workflows/test_full_stack.yml
| # slots filled, the 20 GB + 13.8 GB scenarios still lose the memory | ||
| # race and time out at the 45-min gate deadline. Halving to 3 keeps | ||
| # committed memory well under host capacity and lets the heavy jobs | ||
| # actually acquire without starvation. Wall clock roughly doubles. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the pull-request wall-clock estimate.
For pull_request and merge_group, Line 111 selects two distributions and Line 120 selects one release. With the four scenarios on Lines 113-116, the matrix has eight jobs. Reducing max-parallel from 6 to 3 changes the ideal wave count from two to three, or about 1.5×, not roughly 2×. Scope this estimate to the default 48-job matrix or state the event-specific impact.
Proposed wording
- # actually acquire without starvation. Wall clock roughly doubles.
+ # actually acquire without starvation. The 8-job PR matrix grows from two
+ # waves to three; the default 48-job matrix roughly doubles its wave count.📝 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
| # actually acquire without starvation. Wall clock roughly doubles. | |
| # actually acquire without starvation. The 8-job PR matrix grows from two | |
| # waves to three; the default 48-job matrix roughly doubles its wave count. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test_full_stack.yml at line 104, Correct the wall-clock
estimate comment near the matrix configuration: for pull_request and
merge_group, state that the eight-job matrix runs in three ideal waves at
max-parallel 3 (about 1.5×), rather than roughly doubling. Scope the estimate to
the default 48-job matrix or explicitly describe the event-specific impact.
| # race and time out at the 45-min gate deadline. Halving to 3 keeps | ||
| # committed memory well under host capacity and lets the heavy jobs | ||
| # actually acquire without starvation. Wall clock roughly doubles. | ||
| max-parallel: 3 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the reusable Molecule workflow for this matrix.
This job defines the matrix and invokes Molecule inline, while .github/workflows/molecule.yml provides the reusable max-parallel input. Move this job to the reusable workflow and pass the concurrency value through its input. This prevents the full-stack workflow from drifting from the shared Molecule setup.
As per path instructions, “Molecule test workflows should use the reusable molecule.yml workflow.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test_full_stack.yml at line 105, Replace the inline
Molecule matrix job in the full-stack workflow with a call to the reusable
molecule.yml workflow, passing the matrix configuration and max-parallel value
through its supported inputs. Remove the duplicated inline Molecule setup while
preserving the existing test matrix behavior and concurrency of 3.
Source: Path instructions
Same regression as the elasticsearch fix: setting elasticstack_cert_pass to "" (which happens as soon as the defaults line is uncommented for argument-spec parity) tripped the 'is defined' gate and silently blanked kibana_tls_key_passphrase. That empty string then reached the _cert_pass argument in cert_generate.yml, so elasticsearch-certutil ran with a bare '--pass' followed by '--out …', which the tool interprets as 'prompt for a password on stdin' — and throws IllegalStateException because CI has no TTY. Change the gate to match the elasticsearch fix (non-empty check).
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
Oddly
added a commit
that referenced
this pull request
Aug 12, 2026
…ecture, review scope (#197) CLAUDE.md previously carried only the Fix Workflow and role-default gating sections. Expand to four practical chunks based on patterns seen in similar repos (ansible/metrics-service, leogallego's claude-ansible-skills, Red Hat CoP good practices): - Development commands: what to run for check_argspecs, gen_argspecs, ansible-lint, molecule per-scenario, CI coverage. Stops the guessing step at the start of every session. - Repository conventions: task naming (role_prefix | Verb), commit message style, FQCN, Co-authored-by casing, optional-string pointer convention. All already implicit; now written down. - Architecture: six-role hierarchy, elasticstack meta-role, cert host, key task-file entry points. Explains why the same include patterns repeat across service roles. - Multi-OS + memory notes: PR-time distro matrix, why full_stack uses max-parallel 3, which scenarios carry the largest memory footprint. Answers the questions that came up during #186's capacity-flake diagnosis. - Reviewing PRs (diff-aware): what to skip vs. what to always re-verify per touched path. Includes the cross-role gate-pattern guidance that codifies today's elasticsearch+kibana fix pair.
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
1 participant
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.
Adds
scripts/check_argspecs.py— for every role that ships ameta/argument_specs.yml, compare the set of top-level (non-underscored) variables indefaults/main.ymlagainst the options declared in the spec. Any mismatch either way (var in defaults but not spec, option in spec but no longer in defaults) exits non-zero with the offending names and a pointer toscripts/gen_argspecs.pyfor regeneration.Wired into
test_linting.ymlas the last step of the whole-collection lint job — skipped for the per-role invocation so we do not run five duplicate checks per PR.Also:
gen_argspecs.pyfrom my scratch dir intoscripts/so the fix path is self-contained.elasticstack_cert_passin defaults. The initial argument_specs PR left it as spec-only drift (it was commented out with a docblock above). Flipping toelasticstack_cert_pass: ""matches the actual role behaviour (empty means "not set") and clears the check.Once merged: adding a new var without updating the spec turns lint red the same PR, not three PRs later when someone else notices.
Summary by CodeRabbit
New Features
Bug Fixes
Chores