fix(ansible): use default stdout callback and fix letsencrypt conversion#732
fix(ansible): use default stdout callback and fix letsencrypt conversion#732ktechmidas merged 2 commits intov1.0-devfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds comprehensive testnet infrastructure operations documentation, adjusts Ansible configuration for fact gathering and callback output formatting, and refactors the Let's Encrypt conversion script to use JSON file patching instead of direct shell commands. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
INFRA.MD (1)
283-293: Consider moving the node-status snapshot out of this evergreen guide.The dated status table will stale quickly and can mislead operators. Prefer linking to a live source (dashboard/runbook issue) instead of storing point-in-time state in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@INFRA.MD` around lines 283 - 293, The "## Current Node Status (as of 2026-02-26)" snapshot section contains a dated, point-in-time table (the hp-masternode rows) that will quickly become stale; remove or relocate this entire table and its heading from INFRA.MD and replace it with a single sentence pointing readers to a live source (e.g., the node status dashboard or a runbook issue URL). Ensure the replacement references the same concept (Current Node Status) and include the live link and brief access instructions so operators know where to find up-to-date node health information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/deploy.yml`:
- Line 338: The play currently sets gather_facts: true which conflicts with the
deploy guideline; update the play to set gather_facts: false, add the
dashmate_deploy tag to the play (so tasks can be targeted), and ensure strategy:
free is present to allow fast parallel deployments—locate the play definition in
ansible/deploy.yml and modify the gather_facts, tags, and strategy fields
accordingly.
In `@bin/convert-to-letsencrypt`:
- Around line 86-95: The current loop skips existing-but-empty SSL entries
because it checks "if ssl:"; instead ensure the nested dict path is initialized
unconditionally by using cfg.setdefault(...) to create platform -> gateway ->
ssl and assign it back to the local ssl variable, then always set
ssl['provider']='letsencrypt' and ensure ssl.setdefault('providerConfigs', {})
contains a 'letsencrypt' entry with an 'email' (use setdefault for the email).
Update the block that currently reads ssl = cfg.get('platform',
{}).get('gateway', {}).get('ssl', {}) and the subsequent conditional to always
initialize and patch the ssl dict (before the file write).
- Around line 82-97: The embedded Python currently injects a literal
'EMAIL_PLACEHOLDER' into the script, which is brittle for special characters;
change the inline Python to read the mail from an environment variable (e.g.,
os.environ.get('LE_EMAIL')) and set
ssl['providerConfigs']['letsencrypt']['email'] from that value, and ensure the
shell invocation exports or passes LE_EMAIL into the sudo/python3 execution
(instead of relying on later string substitution). Update the conversion
invocation that builds CONVERT_CMD to export/pass LE_EMAIL alongside CONFIG_FILE
so the python snippet in bin/convert-to-letsencrypt uses the real email safely
via the environment.
---
Nitpick comments:
In `@INFRA.MD`:
- Around line 283-293: The "## Current Node Status (as of 2026-02-26)" snapshot
section contains a dated, point-in-time table (the hp-masternode rows) that will
quickly become stale; remove or relocate this entire table and its heading from
INFRA.MD and replace it with a single sentence pointing readers to a live source
(e.g., the node status dashboard or a runbook issue URL). Ensure the replacement
references the same concept (Current Node Status) and include the live link and
brief access instructions so operators know where to find up-to-date node health
information.
ansible/deploy.yml
Outdated
| hosts: hp_masternodes | ||
| become: true | ||
| gather_facts: false | ||
| gather_facts: true |
There was a problem hiding this comment.
gather_facts setting conflicts with deploy guideline.
Line 338 sets gather_facts: true, but this playbook is required to keep gather_facts: false for fast parallel deploy behavior.
Suggested change
- gather_facts: true
+ gather_facts: falseAs per coding guidelines ansible/deploy.yml: "Add dashmate_deploy tag, set gather_facts: false, and use strategy: free in ansible/deploy.yml to enable fast, parallel deployments".
📝 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.
| gather_facts: true | |
| gather_facts: false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/deploy.yml` at line 338, The play currently sets gather_facts: true
which conflicts with the deploy guideline; update the play to set gather_facts:
false, add the dashmate_deploy tag to the play (so tasks can be targeted), and
ensure strategy: free is present to allow fast parallel deployments—locate the
play definition in ansible/deploy.yml and modify the gather_facts, tags, and
strategy fields accordingly.
| for name, cfg in c.get('configs', {}).items(): | ||
| ssl = cfg.get('platform', {}).get('gateway', {}).get('ssl', {}) | ||
| if ssl: | ||
| ssl['provider'] = 'letsencrypt' | ||
| pc = ssl.setdefault('providerConfigs', {}) | ||
| if 'letsencrypt' not in pc: | ||
| pc['letsencrypt'] = {'email': 'EMAIL_PLACEHOLDER'} | ||
| else: | ||
| pc['letsencrypt'].setdefault('email', 'EMAIL_PLACEHOLDER') | ||
| with open('$CONFIG_FILE', 'w') as f: |
There was a problem hiding this comment.
Do not skip SSL patching when the SSL object is empty.
Line 88 uses if ssl:; an existing-but-empty SSL map is skipped, so provider/providerConfigs are never set. Initialize the nested dict path and patch it regardless of emptiness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/convert-to-letsencrypt` around lines 86 - 95, The current loop skips
existing-but-empty SSL entries because it checks "if ssl:"; instead ensure the
nested dict path is initialized unconditionally by using cfg.setdefault(...) to
create platform -> gateway -> ssl and assign it back to the local ssl variable,
then always set ssl['provider']='letsencrypt' and ensure
ssl.setdefault('providerConfigs', {}) contains a 'letsencrypt' entry with an
'email' (use setdefault for the email). Update the block that currently reads
ssl = cfg.get('platform', {}).get('gateway', {}).get('ssl', {}) and the
subsequent conditional to always initialize and patch the ssl dict (before the
file write).
Switch ansible stdout_callback from yaml to default with result_format=yaml to fix output rendering issues. Update convert-to-letsencrypt script to directly patch config.json via python3 instead of using dashmate config set, which avoids issues when the letsencrypt providerConfig key doesn't exist yet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ution Replace brittle EMAIL_PLACEHOLDER string substitution with proper environment variable passing through SSH -> sudo -> python3. Safer for special characters in email addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ktechmidas
force-pushed
the
fix/ansible-cfg-and-letsencrypt
branch
from
2d4f0ea to
6a57734
Compare
2 participants
Summary
stdout_callbackfromyamltodefaultwithresult_format=yamlto fix output rendering issuesconvert-to-letsencryptscript to directly patchconfig.jsonvia python3 instead of usingdashmate config set, which avoids failures when theletsencryptproviderConfig key doesn't exist yetTest plan
convert-to-letsencrypton a node that has no existing letsencrypt config🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Chores