Skip to content

Conversation

dguido
Copy link
Member

@dguido dguido commented Sep 15, 2025

Summary

This PR fixes critical deployment failures caused by double-templating issues and improves code consistency by resolving all Jinja2 spacing issues throughout the codebase.

Problem

  1. Issue Template string unable to convert #14835: Ansible 12.0.0 introduced stricter template validation that breaks deployments with double-templating patterns like {{ lookup('file', '{{ var }}') }}
  2. Numerous Jinja2 spacing inconsistencies flagged by ansible-lint
  3. Test suite was incorrectly scanning external dependencies, causing false positives

Solution

1. Fixed Double-Templating (Critical Bug Fix)

  • 7 instances across 6 files affecting multiple cloud providers
  • Pattern fixed: '{{ var }}'var inside lookup() calls
  • Affects: Azure, DigitalOcean, GCE, Linode, cloud-init, and IPsec configurations

2. Fixed Jinja2 Spacing Issues

  • 33+ spacing issues resolved across all cloud provider configurations
  • Patterns fixed:
    • }} {%}}{% (removed spaces between blocks)
    • int -1int - 1 (proper operator spacing)
    • |b64encode| b64encode (filter spacing)
    • - 1 ]['field']- 1]['field'] (array access)
    • Multiline expressions consolidated to single lines

3. Enhanced Test Suite

  • Fixed test_comprehensive_boolean_scan.py to only scan Algo code
  • Excludes external dependencies (.env/, ansible_collections/)
  • Excludes CloudFormation templates (which require string booleans)
  • Added test_double_templating.py to prevent regression
  • All tests verified with mutation testing

Testing

All 87 unit tests pass

$ uv run pytest tests/unit/
======================= 87 passed, 14 warnings in 4.08s ========================

0 Jinja2 spacing issues remaining

$ ansible-lint --profile=production 2>&1 | grep -c "jinja\[spacing\]"
0

Ansible syntax checks pass

$ ansible-playbook main.yml --syntax-check
playbook: main.yml

$ ansible-playbook users.yml --syntax-check
playbook: users.yml

Mutation testing verified

  • Test catches bare false in else clauses
  • Test catches string boolean assignments
  • Test catches double-templating patterns

Files Changed

  • 21 Ansible/YAML files: Fixed templating and spacing
  • 1 test file enhanced: Better scope and documentation
  • 1 test file added: Double-templating detection
  • 1 test file removed: Unrelated BSD IPv6 test

Impact

  • Fixes critical deployment failures for Azure, DigitalOcean, GCE, Linode
  • Improves code quality with consistent Jinja2 formatting
  • Prevents future regressions with comprehensive tests
  • Maintains full compatibility with Ansible 12.0.0+

Fixes #14835

Related

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

This PR fixes critical deployment issues and improves code consistency for Ansible 12 compatibility.

## Fixed Issues

### 1. Double-templating bug (Issue #14835)
Fixed 7 instances of invalid double-templating that breaks deployments:
- Changed `{{ lookup('file', '{{ var }}') }}` to `{{ lookup('file', var) }}`
- Affects Azure, DigitalOcean, GCE, Linode, and IPsec configurations
- Added comprehensive test to prevent regression

### 2. Jinja2 spacing inconsistencies
Fixed 33+ spacing issues for better code quality:
- Removed spaces between Jinja2 blocks: `}} {%` → `}}{%`
- Fixed operator spacing: `int -1` → `int - 1`
- Fixed filter spacing: `|b64encode` → `| b64encode`
- Consolidated multiline expressions to single lines

### 3. Test suite improvements
Enhanced boolean type checking test to be more targeted:
- Excludes external dependencies and CloudFormation templates
- Only tests Algo's actual codebase
- Verified with mutation testing
- Added comprehensive documentation

## Testing
- All 87 unit tests pass
- 0 Jinja2 spacing issues remaining (verified by ansible-lint)
- Ansible syntax checks pass for all playbooks
- Mutation testing confirms tests catch real issues

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@dguido dguido requested a review from jackivanov as a code owner September 15, 2025 13:48
Copy link
Contributor

claude bot commented Sep 15, 2025

Claude finished @dguido's task —— View job


PR Review - Comprehensive Analysis

I've thoroughly reviewed this PR and am impressed with the quality and comprehensiveness of the fixes. This addresses critical deployment failures caused by Ansible 12's stricter template validation.

Excellent Work - Strong Approve

Double-Templating Fixes (Critical Bug Fixes)

  • Correctly Fixed: All 7 double-templating instances across 6 files
  • Pattern Analysis: The fixes properly changed '{{ var }}'var inside lookup() calls
  • Examples reviewed:
    • roles/cloud-azure/tasks/main.yml:25: "{{ lookup('file', SSH_keys.public) }}"
    • roles/cloud-digitalocean/tasks/main.yml:9: "{{ lookup('file', SSH_keys.public) }}"
    • roles/strongswan/tasks/client_configs.yml:16: "{{ lookup('file', ipsec_pki_path + '/cacert.pem') | b64encode }}"
    • files/cloud-init/base.yml:33: "{{ lookup('file', SSH_keys.public) }}"

These fixes resolve the core Ansible 12 deployment failures without changing functionality.

Jinja2 Spacing Corrections

  • Comprehensive: Fixed 33+ spacing issues across all cloud provider files
  • Consistent patterns properly addressed:
    • }} {%}}{% (block separation)
    • int -1int - 1 (operator spacing)
    • |b64encode| b64encode (filter spacing)
    • Array access spacing normalized
  • Quality: All changes maintain readability while fixing lint violations

Test Suite Enhancements

New test_double_templating.py:

  • Excellent coverage: Tests all 3 problematic patterns (lookup embedded, nested braces, quoted strings)
  • Mutation testing verified: Confirms tests catch actual bugs
  • Positive tests: Ensures valid patterns aren't flagged
  • Specific bug detection: Tests exact patterns from issue Template string unable to convert #14835

Enhanced test_comprehensive_boolean_scan.py:

  • Improved scope: Now correctly excludes external dependencies (.env/, ansible_collections/)
  • Better documentation: Comprehensive docstring explains purpose and mutation testing
  • Focused testing: Only scans Algo's actual code, not CloudFormation templates
  • Comprehensive patterns: Catches string booleans, bare true/false, and string comparisons

Security Analysis

No security concerns identified:

  • Template fixes: Don't expose additional attack surface
  • File permissions: Maintained properly (600 for sensitive configs)
  • Input validation: No changes to validation logic
  • Key handling: SSH key lookup patterns remain secure

Security improvements:

  • More robust template processing reduces parsing errors that could lead to misconfigurations
  • Better test coverage prevents regressions that could create security gaps

Performance Considerations

Minimal performance impact:

  • Template processing: Cleaner Jinja2 expressions may marginally improve parsing speed
  • Lookup operations: Same number of file lookups, just corrected syntax
  • Test execution: New tests add <1s to test suite runtime
  • Memory usage: No significant changes to memory footprint

Code Quality Assessment

Follows repository best practices:

  • Adheres to CLAUDE.md: Maintains security-first approach and conservative changes
  • Consistent style: All Jinja2 spacing now follows ansible-lint standards
  • Test coverage: Comprehensive mutation-tested coverage prevents regressions
  • Documentation: Clear commit messages and PR description
  • Atomic changes: Each fix addresses specific, well-defined issues

Potential Issues ⚠️

Minor observations (not blocking):

  1. Test file scope: The comprehensive boolean scan could potentially miss edge cases in excluded directories, but this is by design and appropriate.

  2. Regex complexity: The double-templating detection uses multiple complex regex patterns. Consider adding inline comments for future maintainability:

    # Pattern 1: lookup() with embedded {{ }} 
    pattern1 = r"lookup\s*\([^)]*['\"]{{[^}]*}}['\"][^)]*\)"
  3. Error messages: The double-templating test provides excellent error formatting, but could benefit from links to documentation for fixing the issues.

Recommendations

  1. Merge immediately: These are critical fixes for Ansible 12 compatibility
  2. Consider backporting: If maintaining older branches, these fixes may need backporting
  3. Monitor closely: Watch for any unexpected template behavior in production deployments
  4. Documentation update: Consider updating deployment docs to mention Ansible 12 compatibility

Final Assessment

This PR demonstrates exceptional attention to detail and thorough testing practices. The combination of critical bug fixes, comprehensive test coverage with mutation testing verification, and adherence to coding standards makes this a model example of quality maintenance work.

Impact: Fixes critical deployment failures affecting multiple cloud providers
Risk: Very low - changes are surgical and well-tested
Quality: Excellent - comprehensive coverage with mutation-tested validation

Strongly recommend immediate merge 🚢

- Remove unnecessary f-string prefix where no placeholders are used
- Fixes ruff F541 error
- Break long lines to stay within 120 character limit
- Extract variables for better readability
- Fixes ruff E501 errors
@dguido dguido merged commit 4bb13a5 into master Sep 15, 2025
25 checks passed
@dguido dguido deleted the fix/ansible-12-double-templating branch September 15, 2025 13:54
@dguido dguido mentioned this pull request Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template string unable to convert
1 participant