diff --git a/.claude/skills/pr-review/README.md b/.claude/skills/pr-review/README.md new file mode 100644 index 00000000..94cdeed7 --- /dev/null +++ b/.claude/skills/pr-review/README.md @@ -0,0 +1,145 @@ +# PR Review Skill + +Comprehensive PR review workflow for ansible.platform collection maintainers. + +## Structure + +``` +pr-review/ +├── skill.md # Main skill file (ENTRY POINT) +├── README.md # This file +└── references/ + ├── feature-review.md # Feature PR detailed checklist + ├── bugfix-review.md # Bugfix PR detailed checklist + ├── ci-workflow-review.md # CI/workflow PR checklist + └── connection-manager-review.md # Core infrastructure (CRITICAL) +``` + +## Usage + +```bash +/pr-review +``` + +## Workflow Overview + +The main skill file (`skill.md`) routes to appropriate reference guides: + +### Step 1: Pre-Merge CI Checks (BEFORE safe-to-test) +- Collection completeness test +- Unit tests +- Sanity tests +- Changelog verification + +### Step 2: Route to Specific Review +- `feat:` → `references/feature-review.md` +- `fix:` → `references/bugfix-review.md` +- `ci:` → `references/ci-workflow-review.md` + +### Step 3: Safe-to-Test Readiness +- Determine if ready for `safe to test` label + +### Step 4: Post-Label Monitoring +- Watch integration test results +- Re-run transient failures + +## Reference Guides + +### connection-manager-review.md ⚠️ CRITICAL +**Use for:** Changes to connection plugin, manager process, RPC layer, base clients + +**ALWAYS check if PR touches these files:** +- `plugins/connection/http.py` +- `plugins/plugin_utils/manager/*` +- `plugins/plugin_utils/platform/{base_client,direct_client,config,registry}.py` + +**Covers:** +- Backwards compatibility (affects ALL modules) +- Fork safety (macOS + Python 3.12) +- Connection modes (persistent vs direct) +- Subprocess spawning (vault credentials, security) +- Socket management (cleanup, permissions) +- RPC protocol changes +- HTTP session management +- Multi-service support (Gateway/Controller/EDA/Hub) +- Security (credential handling, subprocess safety) +- Performance (manager lifecycle, idle timeout) +- Testing requirements (MUST test multiple modules) + +**Why critical:** These changes affect every module in the collection. Bugs here are hard to debug and impact all users. + +### feature-review.md +**Use for:** New modules, new features, refactoring + +**Covers:** +- Seven-file pattern validation +- Architecture compliance (Ansible Model vs API Model) +- Endpoint path verification (service prefixes) +- Transform mixin review (name→ID resolution) +- Test coverage requirements +- meta/runtime.yml registration + +### bugfix-review.md +**Use for:** Bug fixes, regressions + +**Covers:** +- Jira issue verification +- Bug description adequacy +- Regression test requirements +- Fix validation (addresses root cause) +- Similar bug scanning +- Backwards compatibility + +### ci-workflow-review.md +**Use for:** CI, GitHub Actions, workflow changes + +**Covers:** +- New workflow detection and justification +- Workflow security review +- Secret protection (label gates, author checks) +- Trigger condition validation +- Permission review (least-privilege) +- Test infrastructure changes +- Dependency updates +- Fork testing requirements + +## Key Principles + +1. **Collection completeness test runs BEFORE safe-to-test** + - Must pass before label can be applied + - Ensures meta/runtime.yml registration + +2. **Pre-merge vs Post-label checks** + - Pre-merge: completeness, unit, sanity, changelog + - Post-label: integration tests (live AAP) + +3. **Review routing** + - Skill determines PR type + - Routes to appropriate checklist + - Prevents missing critical checks + +## Review Output + +Each review produces structured output: +- Summary +- Pre-merge CI status +- Architecture review (features) +- Blockers +- Safe-to-test readiness +- Final verdict + +## Maintenance + +Update these files when: +- Collection architecture changes +- New CI checks added +- Review requirements change +- New patterns discovered + +## Based On + +- PR #227 review experience (saved in `.claude/PR_REVIEW_CONTEXT_227.md`) +- ansible-community/ai-forge pr-review skill (base structure) +- ansible.platform collection standards + +**Last Updated:** 2026-09-11 diff --git a/.claude/skills/pr-review/references/bugfix-review.md b/.claude/skills/pr-review/references/bugfix-review.md new file mode 100644 index 00000000..80e408d2 --- /dev/null +++ b/.claude/skills/pr-review/references/bugfix-review.md @@ -0,0 +1,598 @@ +# Bugfix PR Review Guide + +Detailed checklist for reviewing bugfix PRs. + +## When to Use + +- PR title starts with `fix:` or `bug:` +- PR references a Jira issue (AAP-XXXXX) +- PR fixes a regression or defect + +--- + +## ⚠️ CRITICAL: Check for Core Infrastructure Changes + +**Before proceeding, check if bugfix touches connection/manager files:** + +```bash +git diff origin/devel --name-only | grep -E \ + 'plugins/connection/|plugins/plugin_utils/manager/|plugins/plugin_utils/platform/(base_client|direct_client|config|registry)' +``` + +**If ANY match found:** +→ **STOP:** Read `connection-manager-review.md` FIRST before continuing with this checklist + +**Why:** Bugs in connection/manager code affect ALL modules. Extra checks needed: +- Test fix works for ALL modules (not just one) +- Verify both connection modes (persistent, direct) +- Check fork safety not broken +- Ensure no new security issues + +--- + +## Jira Issue Verification + +### 1. Check Jira Reference in PR Title + +**Required format:** +``` +[AAP-12345] Fix description +``` + +or + +``` +AAP-12345: Fix description +``` + +**Validation:** +```bash +# Extract Jira issue from PR title +gh pr view --repo ansible/ansible.platform --json title \ + --jq '.title' | grep -oE 'AAP-[0-9]+' +``` + +**If missing:** +```markdown +❌ **Blocker:** Missing Jira issue reference in PR title + +Please update the PR title to include the Jira issue: +[AAP-XXXXX] +``` + +### 2. Verify Jira Issue Details (If Available) + +**Optional checks if you have Jira access:** + +- Issue exists and is valid +- Issue describes the bug being fixed +- Issue severity/priority set +- Issue is in correct status (In Progress, In Review) + +--- + +## Bug Description Review + +### 1. Read PR Description + +**Required information:** + +- [ ] **What was broken:** Clear description of the bug +- [ ] **How to reproduce:** Steps to trigger the bug +- [ ] **Root cause:** Why the bug occurred +- [ ] **What changed:** The fix applied +- [ ] **How verified:** Testing approach + +**Example good PR description:** + +```markdown +## Bug Description + +**Issue:** Vaulted aap_username/aap_password cause subprocess.Popen to fail + +**Reproduce:** +1. Encrypt credentials with ansible-vault +2. Run any platform module +3. Error: TypeError: expected str, not AnsibleVaultEncryptedUnicode + +**Root Cause:** +process_manager.py passes vaulted credentials directly to subprocess.Popen +without converting to strings first. + +**Fix:** +Convert gateway_config credentials to str() before passing to Popen. + +**Verification:** +- Added unit test with MockAnsibleVaultEncryptedUnicode +- Test verifies str() conversion happens +- Manual test with vaulted group_vars +``` + +### 2. Validate Bug Exists + +**Check if you can understand the bug from description:** + +- [ ] Bug scenario is clear +- [ ] Root cause makes sense +- [ ] Fix addresses root cause (not just symptoms) + +**If unclear:** +```markdown +⚠️ **Question:** Can you provide more details on how to reproduce this bug? + +Current description doesn't include clear reproduction steps. This makes it +difficult to verify the fix addresses the root cause. +``` + +--- + +## Regression Test Verification + +**Critical requirement:** Bugfix MUST include a test that would have caught the bug. + +### 1. Check Test Type + +**Three options (in order of preference):** + +#### Option 1: Unit Test (Preferred) + +**When:** Bug is in testable code path (transform logic, utilities, etc.) + +**Location:** `tests/unit/` + +**Example:** +```python +# tests/unit/plugins/plugin_utils/manager/test_vault_credentials.py + +def test_vault_credentials_converted_to_strings(): + """Test that vaulted credentials are converted to str before Popen.""" + + # Create vaulted credentials + vaulted_username = MockAnsibleVaultEncryptedUnicode("admin") + vaulted_password = MockAnsibleVaultEncryptedUnicode("secret") + + # ... test that str() is called before Popen +``` + +**Checklist:** + +- [ ] Test file name clearly indicates what it tests +- [ ] Test name starts with `test_` and describes bug scenario +- [ ] Test uses mock/fixture to reproduce bug conditions +- [ ] Test would FAIL on code before fix +- [ ] Test PASSES on code after fix +- [ ] Docstring explains the bug being tested + +#### Option 2: Molecule Test Scenario + +**When:** Bug requires full module execution with mock server + +**Location:** `extensions/molecule/_mock/converge.yml` + +**Example:** +```yaml +# New scenario for vault credentials bug +- name: Test with vaulted credentials (regression test for AAP-XXXXX) + ansible.platform.organization: + aap_username: "{{ lookup('ansible.builtin.vault', 'admin') }}" + aap_password: "{{ lookup('ansible.builtin.vault', 'secret') }}" + name: "Test Org" + state: present + register: result + +- name: Verify vault credentials work + assert: + that: + - not result.failed +``` + +**Checklist:** + +- [ ] Test scenario added to existing molecule suite +- [ ] Scenario name references Jira issue or bug +- [ ] Test reproduces bug conditions +- [ ] Test passes after fix + +#### Option 3: Integration Test + +**When:** Bug only appears with live AAP instance + +**Location:** `tests/integration/targets/s_test/tasks/` + +**Example:** +```yaml +# Add to existing integration test +- name: Test specific bug scenario (AAP-XXXXX regression) + ansible.platform.foo: + # ... parameters that trigger the bug + register: result + +- name: Verify bug is fixed + assert: + that: + - result is success + - result.changed +``` + +**Checklist:** + +- [ ] Integration test can be deferred to follow-up PR +- [ ] If added, references Jira issue +- [ ] Tests specific bug condition + +### 2. Verify Test Adequacy + +**Questions to ask:** + +- [ ] Would this test have FAILED before the fix? +- [ ] Does the test cover the exact bug scenario? +- [ ] Is the test stable (not flaky)? +- [ ] Does the test assertion verify the fix, not just "didn't crash"? + +**Red flags:** + +```python +# ❌ BAD: Test just checks it doesn't fail +def test_vault_fix(): + result = do_something() + assert result is not None # Too vague! + +# ✅ GOOD: Test verifies specific behavior +def test_vault_credentials_converted_to_strings(): + # Verify str() conversion happens + assert isinstance(cmd[5], str) + assert type(cmd[5]) == str # Not just str subclass + assert cmd[5] == "admin" # Value is correct +``` + +--- + +## Code Review: The Fix + +### 1. Verify Fix Location + +**Check if fix is in correct place:** + +- [ ] Fix is in the file identified as root cause +- [ ] Fix is minimal (doesn't refactor unrelated code) +- [ ] Fix doesn't introduce new complexity + +**Example review:** + +```python +# BEFORE (buggy) +cmd = [ + sys.executable, + gateway_config.username, # ❌ Vault object +] + +# AFTER (fixed) +cmd = [ + sys.executable, + str(gateway_config.username), # ✅ Converted to str +] +``` + +**Checklist:** + +- [ ] Fix addresses root cause +- [ ] No unnecessary changes +- [ ] No commented-out debug code left behind + +### 2. Check for Similar Bugs + +**Scan for same pattern elsewhere:** + +```bash +# Example: If bug was missing str() conversion, search for similar +grep -r "subprocess.Popen" plugins/ | grep -v "str(" + +# Example: If bug was missing null check, search for similar +grep -r "\.get(" plugins/ | grep -v "if .* is not None" +``` + +**Ask:** +- [ ] Could this bug exist in other modules? +- [ ] Should we fix them in this PR or create follow-up issues? + +**If found:** + +```markdown +⚠️ **Note:** Similar pattern found in other files + +Found similar subprocess.Popen usage without str() conversion in: +- `plugins/foo/bar.py:123` +- `plugins/baz/qux.py:456` + +**Recommendation:** +- Fix in this PR (same root cause) OR +- Create follow-up Jira issue to track +``` + +### 3. Backwards Compatibility + +**Ensure fix doesn't break existing behavior:** + +- [ ] Fix handles both old and new scenarios +- [ ] No breaking changes to public API +- [ ] Existing tests still pass + +**Example check:** + +```python +# ✅ GOOD: Handles both vaulted and non-vaulted +str(gateway_config.username) # Works for both str and VaultString + +# ❌ BAD: Breaks existing behavior +if isinstance(gateway_config.username, AnsibleVaultEncryptedUnicode): + username = str(gateway_config.username) +else: + username = gateway_config.username # Doesn't handle None! +``` + +--- + +## Documentation Updates + +### 1. Changelog Fragment + +**Required format:** + +```yaml +# changelogs/fragments/-.yml +bugfixes: + - "Fix description of what was broken (ansible/ansible.platform#)." +``` + +**Example:** + +```yaml +# changelogs/fragments/250-vault-credentials.yml +bugfixes: + - "Fix subprocess spawn failure when aap_username or aap_password are vaulted (ansible/ansible.platform#250)." +``` + +**Checklist:** + +- [ ] Fragment file exists +- [ ] Uses `bugfixes:` category (not `minor_changes:`) +- [ ] Description is clear and user-facing +- [ ] PR number referenced +- [ ] Past tense ("Fix", not "Fixes") + +### 2. Known Issues / Release Notes + +**If bug was in a released version:** + +```markdown +⚠️ **Note:** This bug affects released versions + +**Affected versions:** 2.5.0, 2.6.0 +**Workaround:** Use non-vaulted credentials or `| string` filter + +**Recommendation:** Add note to release notes for backport PR +``` + +### 3. Documentation Clarification + +**If bug revealed unclear docs:** + +```markdown +💡 **Suggestion:** Update documentation + +This bug suggests the documentation about vault support could be clearer. + +**Consider adding to docs:** +- Vault is supported for all authentication parameters +- No special configuration needed (fixed in 2.7.0+) +``` + +--- + +## Testing Strategy + +### 1. Manual Testing (If Applicable) + +**For complex bugs, verify manual testing was done:** + +```markdown +**Manual testing performed:** +- [ ] Created vaulted group_vars/all.yml +- [ ] Ran playbook with vaulted credentials +- [ ] Verified module execution succeeds +- [ ] Verified no error in logs +``` + +### 2. CI Test Execution + +**Ensure tests pass:** + +```bash +# Run unit tests +pytest tests/unit -v -k + +# Run molecule tests +molecule test -s + +# Run sanity +ansible-test sanity --docker +``` + +--- + +## Review Output Template + +```markdown +## Bugfix Review: # + +### Jira Reference + +- ✅ Jira issue: AAP-12345 +- ✅ Issue in PR title + +### Bug Description + +**What was broken:** Vaulted credentials cause subprocess failure + +**Root cause:** Missing str() conversion before Popen + +**Fix applied:** Convert credentials to str() + +✅ Bug description is clear and complete + +### Regression Test + +- ✅ **Test type:** Unit test +- ✅ **Location:** `tests/unit/plugins/plugin_utils/manager/test_vault_credentials.py` +- ✅ **Test coverage:** + - Vaulted username + - Vaulted password + - Vaulted base_url + - None values +- ✅ **Test quality:** Would have caught the bug + +### Code Review + +- ✅ Fix in correct location: `process_manager.py:293-306` +- ✅ Minimal change (only affected lines) +- ✅ No similar bugs found elsewhere +- ✅ Backwards compatible + +### Documentation + +- ✅ Changelog fragment: `changelogs/fragments/250-vault-fix.yml` +- ✅ Correct category: `bugfixes` +- ✅ Clear description + +### Blockers + +None + +### Recommendations + +1. ✅ All requirements met +2. Consider adding note to docs about vault support + +### Verdict + +✅ **APPROVE** - Ready for `safe to test` label + +Well-documented fix with comprehensive regression test. Addresses root cause without introducing new issues. +``` + +--- + +## Common Bugfix Patterns + +### Pattern 1: Missing Null/None Check + +**Bug:** +```python +# Crashes when value is None +result = some_value.upper() +``` + +**Fix:** +```python +# Safe +result = some_value.upper() if some_value else None +``` + +**Required test:** +```python +def test_handles_none_value(): + assert handle_value(None) is None # Doesn't crash +``` + +### Pattern 2: Type Conversion Missing + +**Bug:** +```python +# Fails with AnsibleVaultEncryptedUnicode +subprocess.Popen([sys.executable, password]) +``` + +**Fix:** +```python +# Convert to str first +subprocess.Popen([sys.executable, str(password)]) +``` + +**Required test:** +```python +def test_converts_vault_to_string(): + vault_str = MockAnsibleVaultEncryptedUnicode("secret") + # Verify str() is called +``` + +### Pattern 3: Incorrect Conditional Logic + +**Bug:** +```python +# Wrong operator +if resource.state == "present" or "enforced": # Always True! +``` + +**Fix:** +```python +# Correct +if resource.state in ("present", "enforced"): +``` + +**Required test:** +```python +def test_state_conditional(): + assert check_state("absent") is False + assert check_state("present") is True + assert check_state("enforced") is True +``` + +### Pattern 4: Missing Error Handling + +**Bug:** +```python +# Doesn't handle API errors +result = api_client.get(url) +return result["data"] # KeyError if error response +``` + +**Fix:** +```python +# Handle errors +try: + result = api_client.get(url) + return result.get("data", []) +except APIError as e: + module.fail_json(msg=f"API error: {e}") +``` + +**Required test:** +```python +def test_handles_api_error(): + with pytest.raises(AnsibleFailJson): + handle_api_response({"error": "Not found"}) +``` + +--- + +## Red Flags + +**Request changes if:** + +- ❌ No regression test included +- ❌ Fix doesn't address root cause (just hides symptoms) +- ❌ Fix introduces breaking changes +- ❌ Similar bugs found but not fixed +- ❌ Test wouldn't have caught the bug +- ❌ Missing Jira reference +- ❌ No changelog fragment + +**Approve with recommendations if:** + +- ✅ All requirements met +- ⚠️ Could add more test coverage (but basic covered) +- ⚠️ Documentation could be clearer (but not wrong) + +--- + +**Last Updated:** 2026-09-11 diff --git a/.claude/skills/pr-review/references/ci-workflow-review.md b/.claude/skills/pr-review/references/ci-workflow-review.md new file mode 100644 index 00000000..a255f154 --- /dev/null +++ b/.claude/skills/pr-review/references/ci-workflow-review.md @@ -0,0 +1,790 @@ +# CI/Workflow PR Review Guide + +Review checklist for CI, GitHub Actions, and workflow changes. + +## When to Use + +- PR title starts with `ci:`, `chore:`, or `build:` +- PR modifies `.github/workflows/` +- PR modifies CI configuration files +- PR modifies test infrastructure + +--- + +## Critical Checks (High Priority) + +**For NEW workflows:** +1. ✅ **Purpose justified** - Why new vs modifying existing? +2. ✅ **Tested in fork** - MUST be tested before merge +3. ✅ **Secret protection** - If secrets used, proper gates in place + +**For ALL workflow changes:** +1. ✅ **Security review** - No injection vulnerabilities +2. ✅ **Secret protection** - Secrets not exposed to untrusted code +3. ✅ **Permissions** - Follow least-privilege principle + +**Quick secret protection check:** +```bash +# Does workflow use secrets AND run on pull_request? +grep -l "secrets\." .github/workflows/*.yml | \ + xargs grep -l "on: pull_request" && \ + echo "⚠️ DANGER: Secrets exposed to fork PRs!" + +# Should use pull_request_target + label gate instead +grep -l "safe to test" .github/workflows/*.yml +``` + +--- + +## Workflow File Changes + +### 1. GitHub Actions Workflow Review + +**Files to check:** +``` +.github/workflows/ +├── ci.yml # Main CI pipeline +├── integration.yml # Integration tests (safe to test) +├── release.yml # Release automation +└── ... +``` + +**First, check if this is a NEW workflow:** + +```bash +# List all workflows before PR +git diff origin/devel --name-only | grep '.github/workflows/' + +# Check if file is new (Added) +git diff origin/devel --name-status .github/workflows/ | grep '^A' +``` + +**If NEW workflow detected, additional checks required:** + +- [ ] **Purpose justified** - Why is a new workflow needed vs modifying existing? +- [ ] **Name follows convention** - Descriptive, matches collection patterns +- [ ] **No duplication** - Doesn't replicate existing workflow functionality +- [ ] **Documented in PR** - Clear explanation of what it does and why +- [ ] **Minimal scope** - Does one thing well, not overloaded +- [ ] **Tested in fork** - MUST be tested before merge (see Fork Testing section) + +**For each modified or new workflow:** + +#### 1.1 Syntax and Structure + +```bash +# Validate workflow YAML syntax +yamllint .github/workflows/.yml + +# Check GitHub Actions syntax +gh workflow view --repo ansible/ansible.platform +``` + +**Checklist:** + +- [ ] Valid YAML syntax +- [ ] Workflow name is clear and descriptive +- [ ] Trigger conditions are appropriate +- [ ] Jobs are properly defined +- [ ] Steps are in logical order + +#### 1.2 Trigger Conditions + +**Common triggers:** + +```yaml +# ✅ GOOD: Specific, intentional triggers +on: + pull_request: + types: [opened, synchronize, reopened] + paths: + - 'plugins/**' + - 'tests/**' + push: + branches: + - devel + - stable-* + +# ❌ BAD: Too broad, wastes CI resources +on: [push, pull_request] # Runs on every push to any branch +``` + +**Checklist:** + +- [ ] Triggers are specific (not too broad) +- [ ] Path filters used when appropriate +- [ ] Branch filters protect main branches +- [ ] No unnecessary workflow runs + +#### 1.3 Permissions + +```yaml +# ✅ GOOD: Minimal required permissions +permissions: + contents: read + pull-requests: write # Only if needed + +# ❌ BAD: Excessive permissions +permissions: write-all # Never use this +``` + +**Checklist:** + +- [ ] Permissions follow least-privilege principle +- [ ] Only grants what's needed for the workflow +- [ ] No `write-all` permission + +#### 1.4 Security Review + +**Check for security issues:** + +```yaml +# ❌ DANGEROUS: Untrusted input in shell +- name: Run command + run: echo "${{ github.event.issue.title }}" # Injection risk! + +# ✅ SAFE: Use environment variables +- name: Run command + env: + TITLE: ${{ github.event.issue.title }} + run: echo "$TITLE" + +# ❌ DANGEROUS: pull_request_target with untrusted code +on: pull_request_target # Runs with repo secrets +# Then: +- uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} # Untrusted code! + +# ✅ SAFE: Use pull_request or validate first +on: pull_request # No repo secrets exposed +``` + +**Security checklist:** + +- [ ] No direct use of `github.event.*` in shell commands +- [ ] `pull_request_target` used carefully (or not at all) +- [ ] Secrets not logged or exposed +- [ ] Third-party actions pinned to SHA (not `@main`) +- [ ] No `curl | sh` or similar dangerous patterns + +#### 1.5 Secret Protection + +**Critical: Verify secrets are protected from untrusted code** + +**Rule 1: Never expose secrets to PRs from forks** + +```yaml +# ❌ DANGEROUS: Secrets available to fork PRs +on: pull_request # From any fork! +jobs: + build: + steps: + - uses: actions/checkout@v4 + - run: | + echo "${{ secrets.AAP_PASSWORD }}" # LEAKED to fork! + +# ✅ SAFE: Use pull_request_target with label gate +on: + pull_request: + types: [labeled] + +jobs: + integration: + if: | + github.event.label.name == 'safe to test' && + github.event.pull_request.author_association == 'MEMBER' + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - run: | + echo "Secrets only after manual approval" + env: + AAP_PASSWORD: ${{ secrets.AAP_PASSWORD }} +``` + +**Rule 2: Check secret visibility in workflow runs** + +```bash +# Verify secrets are masked in logs +# Check recent workflow runs +gh run view --repo ansible/ansible.platform --log | grep -i password +# Should show: *** (masked) +# Should NOT show: actual password value +``` + +**Rule 3: Validate secret references** + +```yaml +# ✅ GOOD: Secrets in env vars, not inline +- name: Deploy + env: + API_TOKEN: ${{ secrets.GALAXY_API_TOKEN }} + run: ansible-galaxy collection publish --token "$API_TOKEN" + +# ❌ BAD: Secret in command (shows in process list) +- name: Deploy + run: ansible-galaxy collection publish --token ${{ secrets.GALAXY_API_TOKEN }} +``` + +**Secret protection checklist:** + +- [ ] **No secrets on pull_request trigger** (use `pull_request_target` + label gate) +- [ ] **Label gate implemented** - `safe to test` or similar for secret access +- [ ] **Author association check** - Verify `MEMBER`, `COLLABORATOR`, or `OWNER` +- [ ] **Secrets in env vars** - Not directly in shell commands +- [ ] **No secret logging** - Verify secrets are masked in test runs +- [ ] **No secret in artifacts** - Don't upload logs/files containing secrets +- [ ] **Secret rotation documented** - If secrets compromised, how to rotate + +**Example: Safe secret usage pattern** + +```yaml +name: Integration Tests (Safe to Test) + +on: + pull_request: + types: [labeled] + +jobs: + check-access: + # First job: verify access + if: | + github.event.label.name == 'safe to test' && + (github.event.pull_request.author_association == 'MEMBER' || + github.event.pull_request.author_association == 'OWNER') + runs-on: ubuntu-latest + outputs: + approved: ${{ steps.check.outputs.approved }} + steps: + - id: check + run: echo "approved=true" >> $GITHUB_OUTPUT + + integration: + needs: check-access + if: needs.check-access.outputs.approved == 'true' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + + - name: Run tests with secrets + env: + AAP_HOSTNAME: ${{ secrets.AAP_HOSTNAME }} + AAP_USERNAME: ${{ secrets.AAP_USERNAME }} + AAP_PASSWORD: ${{ secrets.AAP_PASSWORD }} + run: | + # Secrets are now available, but only after manual approval + ansible-playbook tests/integration/playbook.yml +``` + +**Verify secret protection:** + +```bash +# 1. Check if workflow runs on pull_request (dangerous) +grep -n "on: pull_request" .github/workflows/*.yml + +# 2. Check if secrets used without protection +grep -A 5 "secrets\." .github/workflows/*.yml | grep -v "pull_request_target" + +# 3. Check for label gates +grep -n "safe to test" .github/workflows/*.yml +``` + +#### 1.6 Dependencies and Actions + +```yaml +# ✅ GOOD: Pinned to specific version +- uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2 + +# ⚠️ ACCEPTABLE: Pinned to major version (with auto-updates) +- uses: actions/checkout@v4 + +# ❌ BAD: Unpinned, can break anytime +- uses: actions/checkout@main +``` + +**Checklist:** + +- [ ] Actions pinned to SHA or major version +- [ ] No deprecated actions +- [ ] Dependencies are maintained/trustworthy +- [ ] Version comments included for pinned SHAs + +--- + +### 2. Test Infrastructure Changes + +**Files:** +``` +tests/ +├── test_completeness.py # Collection completeness test +├── unit/ # Unit test framework +├── integration/ # Integration test framework +└── ... +``` + +#### 2.1 Test Script Changes + +**For `tests/test_completeness.py` or similar:** + +**Checklist:** + +- [ ] Changes maintain test intent +- [ ] New checks are justified and documented +- [ ] Test still catches real issues (not just passes blindly) +- [ ] Error messages are clear and actionable + +**Example review:** + +```python +# ❌ BAD: Makes test too permissive +if module_name.startswith("_"): + continue # Skip without justification + +# ✅ GOOD: Clear intent, documented exception +if module_name in EXEMPTED_MODULES: + # These modules are exempt because [reason] + continue +``` + +#### 2.2 Molecule Test Infrastructure + +**Files:** +``` +extensions/molecule/ +├── default/ # Shared molecule config +│ └── molecule.yml +└── _mock/ # Per-module scenarios +``` + +**Checklist:** + +- [ ] `molecule.yml` syntax valid +- [ ] Platform/driver configuration appropriate +- [ ] Dependency declarations correct +- [ ] No hardcoded secrets or credentials + +#### 2.3 Integration Test Infrastructure + +**Files:** +``` +tests/integration/ +├── integration_config.yml # Test configuration +└── targets/ +``` + +**Checklist:** + +- [ ] Changes don't break existing tests +- [ ] New configuration is documented +- [ ] Secrets handling is secure + +--- + +### 3. CI Configuration Files + +#### 3.1 Requirements Files + +**Files:** +``` +requirements.txt # Python dependencies +meta/ee-requirements.txt # Execution environment deps +test-requirements.txt # Test dependencies +``` + +**Checklist:** + +- [ ] Version pins are intentional (not accidental) +- [ ] Dependencies are needed (no unused deps) +- [ ] Licenses are compatible +- [ ] Security vulnerabilities addressed + +**Version pinning review:** + +```txt +# ✅ GOOD: Pinned with reason +requests==2.28.1 # Later versions break X + +# ⚠️ ACCEPTABLE: Minimum version +requests>=2.28.0 # Need feature X from this version + +# ❌ BAD: Unintentionally too restrictive +requests==2.28.1 # Why this exact version? +``` + +#### 3.2 Ansible Configuration + +**Files:** +``` +ansible.cfg +.ansible-lint +pyproject.toml +``` + +**Checklist:** + +- [ ] Changes are documented +- [ ] Doesn't break existing functionality +- [ ] Compatible with supported Ansible versions + +--- + +### 4. Documentation for CI Changes + +**Ensure these are updated if needed:** + +- [ ] `CONTRIBUTING.md` - If workflow process changed +- [ ] `README.md` - If badges/CI status affected +- [ ] Inline comments - If workflow logic is complex +- [ ] Changelog fragment - For user-visible CI changes + +**Example good documentation:** + +```yaml +# This workflow runs on PRs labeled 'safe to test' to avoid +# exposing secrets to untrusted code. It runs integration tests +# against a live AAP instance provisioned in CI. +name: Integration Tests (Safe to Test) + +on: + pull_request: + types: [labeled] + +jobs: + check-label: + if: github.event.label.name == 'safe to test' + # ... rest of workflow +``` + +--- + +## Review Checklist by Change Type + +### Change Type: New Workflow Added + +- [ ] Workflow name is descriptive +- [ ] Purpose documented (inline or in PR) +- [ ] Justified (doesn't duplicate existing workflow) +- [ ] Triggers are appropriate +- [ ] Permissions follow least-privilege +- [ ] **No security issues** (injection, untrusted input) +- [ ] **Secret protection implemented** (if secrets used) + - [ ] No secrets on `pull_request` trigger + - [ ] Label gate (`safe to test`) for secret access + - [ ] Author association check + - [ ] Secrets in env vars, not inline +- [ ] Tested in fork or with dry-run +- [ ] Monitoring plan for first few runs + +### Change Type: Workflow Modified + +- [ ] Change is minimal (focused) +- [ ] Doesn't break existing functionality +- [ ] Backwards compatible (or breaking change justified) +- [ ] Comments explain complex logic + +### Change Type: Workflow Removed + +- [ ] Removal is justified +- [ ] No dependencies on removed workflow +- [ ] Documented in changelog if user-visible + +### Change Type: Test Infrastructure + +- [ ] Tests still pass +- [ ] Test coverage maintained or improved +- [ ] No tests disabled without justification +- [ ] Test matrix appropriate (not too broad/narrow) + +### Change Type: Dependency Update + +- [ ] Security vulnerability fixed OR +- [ ] Feature needed OR +- [ ] Bug fixed +- [ ] Breaking changes documented +- [ ] Tests verify compatibility + +--- + +## Common CI/Workflow Patterns + +### Pattern 1: Safe to Test Workflow + +**Purpose:** Run integration tests only after manual approval + +**Key elements:** + +```yaml +on: + pull_request: + types: [labeled] + +jobs: + integration: + if: github.event.label.name == 'safe to test' + runs-on: ubuntu-latest + steps: + # ... +``` + +**Review checklist:** + +- [ ] Label name is correct +- [ ] Only runs when label present +- [ ] Removes label after run (or on failure) + +### Pattern 2: Matrix Testing + +**Purpose:** Test across multiple Python/Ansible versions + +```yaml +strategy: + matrix: + python-version: ['3.9', '3.10', '3.11'] + ansible-version: ['2.14', '2.15', '2.16'] +``` + +**Review checklist:** + +- [ ] Matrix is necessary (not excessive) +- [ ] Versions are supported combinations +- [ ] Fast-fail appropriate (`fail-fast: false` if needed) + +### Pattern 3: Conditional Steps + +```yaml +- name: Run only on devel + if: github.ref == 'refs/heads/devel' + run: echo "Devel only" + +- name: Run only on PR + if: github.event_name == 'pull_request' + run: echo "PR only" +``` + +**Review checklist:** + +- [ ] Conditions are correct +- [ ] Logic is clear and documented +- [ ] No unintended side effects + +--- + +## Testing CI Changes + +### 1. Fork Testing + +**Before approving:** + +```markdown +**Testing request:** + +Please test this workflow in your fork: + +1. Push this branch to your fork +2. Create a PR in your fork +3. Verify workflow runs as expected +4. Share workflow run URL +``` + +### 2. Dry-Run Testing + +```bash +# For workflow changes, can use act locally +act pull_request -W .github/workflows/.yml + +# Or review with GitHub CLI +gh workflow view --repo ansible/ansible.platform +``` + +### 3. Incremental Rollout + +**For risky changes:** + +```markdown +**Recommendation:** Incremental rollout + +1. Merge this PR to enable workflow +2. Monitor first few runs +3. If issues, can quickly disable with follow-up PR +4. Add monitoring/alerting if critical workflow +``` + +--- + +## Review Output Template + +```markdown +## CI/Workflow Review: # + +### Change Summary + +**Type:** [New Workflow | Workflow Modification | Dependency Update | Test Infrastructure] + +**Files Modified:** +- `.github/workflows/ci.yml` - Added Python 3.12 to matrix +- `requirements.txt` - Updated requests to 2.31.0 + +### Security Review + +- ✅ No untrusted input in shell commands +- ✅ Permissions follow least-privilege +- ✅ Actions pinned to versions +- ✅ No secret exposure risk + +### Functionality Review + +- ✅ Workflow triggers appropriate +- ✅ Test matrix reasonable (Python 3.9-3.12) +- ✅ Backwards compatible +- ✅ Error handling adequate + +### Testing + +- ✅ Tested in fork: https://github.com/user/repo/actions/runs/123 +- ✅ Workflow syntax valid +- ⚠️ Recommend monitoring first few runs after merge + +### Documentation + +- ✅ Inline comments explain complex logic +- ⚠️ Consider updating CONTRIBUTING.md with new Python version + +### Blockers + +None + +### Recommendations + +1. ✅ Changes look good +2. ⚠️ Monitor first runs after merge +3. 💡 Consider adding workflow_dispatch for manual testing + +### Verdict + +✅ **APPROVE** - Safe to merge + +Security reviewed, tested in fork, no issues found. +``` + +--- + +## Red Flags + +**Request changes if:** + +- ❌ Security issues (untrusted input, excessive permissions) +- ❌ Workflow will waste CI resources (too broad triggers) +- ❌ Breaking change without justification +- ❌ Not tested in fork +- ❌ Removes critical tests without replacement + +**Approve with warnings if:** + +- ✅ Functionally correct +- ⚠️ Could be more efficient (but works) +- ⚠️ Documentation could be better (but not wrong) +- ⚠️ Needs monitoring (but safe to try) + +--- + +## Post-Merge Monitoring + +**For approved CI changes:** + +```markdown +## Post-Merge Checklist + +After this PR merges, monitor: + +1. **First workflow run:** Check logs for unexpected errors +2. **CI dashboard:** Ensure no increase in failure rate +3. **Performance:** Check if CI time increased significantly +4. **Costs:** Monitor GitHub Actions minutes usage + +**If issues found:** +- Revert quickly if critical +- File follow-up issue if minor +- Adjust workflow if efficiency problem +``` + +--- + +## Quick Reference: Review Commands + +### Check for New Workflows + +```bash +# List new workflow files in PR +git diff origin/devel --name-status .github/workflows/ | grep '^A' + +# Show new workflow content +git diff origin/devel .github/workflows/.yml +``` + +### Secret Protection Audit + +```bash +# CRITICAL: Find workflows with secrets on pull_request trigger +echo "=== DANGEROUS: Secrets exposed to pull_request ===" +for f in .github/workflows/*.yml; do + if grep -q "on: pull_request" "$f" && grep -q "secrets\." "$f"; then + echo "❌ $f - Secrets exposed to fork PRs!" + fi +done + +# Find workflows with safe to test label gate +echo "=== SAFE: Label-gated workflows ===" +grep -l "safe to test" .github/workflows/*.yml + +# Check for author association checks +echo "=== Author association protection ===" +grep -l "author_association" .github/workflows/*.yml + +# Verify secrets are in env vars, not inline +echo "=== Secrets in commands (UNSAFE) ===" +grep -n '\${{ secrets\.' .github/workflows/*.yml | grep -v 'env:' +``` + +### Workflow Validation + +```bash +# Validate YAML syntax +yamllint .github/workflows/.yml + +# Check GitHub Actions syntax (requires gh CLI) +gh workflow view --repo ansible/ansible.platform + +# Test locally with act (if available) +act pull_request -W .github/workflows/.yml --dryrun +``` + +### Permission Review + +```bash +# Find workflows with write-all (DANGEROUS) +grep -n "write-all" .github/workflows/*.yml + +# List all permission declarations +grep -A 5 "permissions:" .github/workflows/*.yml +``` + +### Trigger Review + +```bash +# Find all pull_request triggers (check if secrets used) +grep -n "on: pull_request" .github/workflows/*.yml + +# Find all pull_request_target triggers (verify safety) +grep -n "pull_request_target" .github/workflows/*.yml + +# Check for workflow_dispatch (manual triggers) +grep -n "workflow_dispatch" .github/workflows/*.yml +``` + +--- + +**Last Updated:** 2026-09-11 diff --git a/.claude/skills/pr-review/references/connection-manager-review.md b/.claude/skills/pr-review/references/connection-manager-review.md new file mode 100644 index 00000000..19fa33b0 --- /dev/null +++ b/.claude/skills/pr-review/references/connection-manager-review.md @@ -0,0 +1,890 @@ +# Connection Plugin & Manager Process Review Guide + +Critical review checklist for changes to core infrastructure: connection plugin, manager process, RPC layer, and base clients. + +## When to Use + +**CRITICAL: Use this guide when PR changes ANY of these files:** + +### Core Infrastructure Files + +``` +plugins/connection/ +├── http.py # Connection plugin (persistent vs direct routing) + +plugins/plugin_utils/manager/ +├── process_manager.py # Subprocess spawning, socket management +├── platform_manager.py # PlatformService + PlatformManager (in subprocess) +├── manager_process.py # Subprocess entry point (main()) +├── rpc_client.py # Client-side RPC communication + +plugins/plugin_utils/platform/ +├── base_client.py # Base API client (shared logic) +├── direct_client.py # Direct connection mode implementation +├── config.py # GatewayConfig, authentication +└── registry.py # Module registry, API version detection +``` + +**Why this matters:** +- Changes affect **ALL modules** in the collection +- Breaking changes impact **ALL users** +- Bugs here are **hard to debug** (cross-module impact) +- Security issues affect **entire authentication flow** +- Performance issues affect **all playbook runs** + +--- + +## Critical Checks (Do These First) + +### 1. Backwards Compatibility Impact + +**Question:** Does this change break existing modules? + +```bash +# Test ALL modules, not just one +pytest tests/unit/plugins/plugin_utils/ -v + +# Run molecule tests for multiple modules +molecule test -s organization_mock +molecule test -s team_mock +molecule test -s user_mock + +# Check if existing playbooks still work +ansible-playbook tests/integration/playbook.yml +``` + +**Checklist:** + +- [ ] **No breaking changes to public APIs** (RPC protocol, config, etc.) +- [ ] **Existing modules still work** (test at least 3 different modules) +- [ ] **Both connection modes work** (persistent and direct) +- [ ] **All authentication methods work** (username/password, token, OAuth) +- [ ] **Deprecation warnings** if behavior changes + +**Example breaking change:** + +```python +# ❌ BREAKING: Changed RPC method signature +# BEFORE +def execute(self, operation, ansible_instance): + pass + +# AFTER +def execute(self, operation, ansible_instance, extra_param): # Breaks all callers! + pass + +# ✅ SAFE: Added optional parameter +def execute(self, operation, ansible_instance, extra_param=None): + pass +``` + +### 2. Connection Mode Verification + +**Both modes must work:** + +- **Persistent mode** - Manager subprocess lives across tasks +- **Direct mode** - Ephemeral, new process per task + +```python +# Test both modes +# Persistent (default) +ansible-playbook test.yml # Uses persistent connection + +# Direct (fallback) +AAP_CONNECTION_MODE=direct ansible-playbook test.yml +``` + +**Checklist:** + +- [ ] Change works in **persistent mode** +- [ ] Change works in **direct mode** +- [ ] Mode detection still works (persistent preferred, direct fallback) +- [ ] Connection manager cleanup still works +- [ ] Socket permissions correct (Unix domain socket) + +### 3. Fork Safety (macOS + Python 3.12) + +**Critical:** Manager subprocess must be fork-safe + +```python +# ❌ DANGEROUS: HTTP session created before fork +session = requests.Session() # Created in main process + +# Later +subprocess.Popen(...) # Fork happens +# Now session is broken on macOS + Python 3.12! + +# ✅ SAFE: Create session AFTER fork in subprocess +def run_in_subprocess(): + session = requests.Session() # Created in subprocess +``` + +**Why this matters:** +- Python 3.12 changed fork behavior on macOS +- HTTP sessions break across fork +- This is WHY we have a manager subprocess pattern + +**Checklist:** + +- [ ] No HTTP sessions/connections created before subprocess spawn +- [ ] No shared state between parent and subprocess +- [ ] Manager process creates own HTTP session +- [ ] Tested on macOS + Python 3.12 (if possible) + +--- + +## File-Specific Checks + +### plugins/connection/http.py + +**Purpose:** Routes execution to persistent or direct connection mode + +#### Changes to Check + +**1. Connection Mode Routing** + +```python +# Verify routing logic still works +def run(self, cmd, in_data=None, sudoable=True): + # Should route to persistent or direct based on mode + if self._use_persistent_connection(): + return self._run_persistent(cmd, in_data) + else: + return self._run_direct(cmd, in_data) +``` + +**Checklist:** + +- [ ] Persistent mode detection still works +- [ ] Direct mode fallback still works +- [ ] Mode switching doesn't break mid-playbook +- [ ] Connection errors handled gracefully + +**2. Error Handling** + +```python +# ✅ GOOD: Graceful fallback +try: + return self._run_persistent(cmd, in_data) +except ConnectionError as e: + self._display.vvv(f"Persistent connection failed: {e}, falling back to direct") + return self._run_direct(cmd, in_data) + +# ❌ BAD: Crashes entire playbook +return self._run_persistent(cmd, in_data) # No fallback! +``` + +**Checklist:** + +- [ ] Connection errors don't crash playbook +- [ ] Falls back to direct mode on persistent failure +- [ ] Error messages are clear and actionable +- [ ] Logging at appropriate verbosity level + +**3. State Management** + +**Checklist:** + +- [ ] No shared state between tasks (unless intended) +- [ ] Connection cleanup happens on close +- [ ] No resource leaks (sockets, file descriptors) + +--- + +### plugins/plugin_utils/manager/process_manager.py + +**Purpose:** Spawns and manages the subprocess, handles socket communication + +#### Critical Areas + +**1. Subprocess Spawning** + +```python +@staticmethod +def spawn_manager_process(script_path, socket_path, gateway_config, ...): + # CRITICAL: All arguments must be subprocess.Popen compatible + cmd = [ + sys.executable, + str(script_path), # ✅ Must be str or Path + socket_path, + str(gateway_config.base_url), # ✅ Convert vault strings + str(gateway_config.username) if gateway_config.username else "", + str(gateway_config.password) if gateway_config.password else "", + ] + + process = subprocess.Popen(cmd, ...) +``` + +**Checklist:** + +- [ ] **All cmd arguments are str/bytes/Path** (not custom objects) +- [ ] **Vault credentials converted to str()** (AnsibleVaultEncryptedUnicode) +- [ ] **None values handled** (convert to "" or omit) +- [ ] **No shell=True** (security risk) +- [ ] **Process cleanup on error** + +**Common bugs:** + +```python +# ❌ BUG: Vault credentials not converted +cmd = [sys.executable, gateway_config.password] # TypeError if vaulted! + +# ✅ FIX: Convert to string +cmd = [sys.executable, str(gateway_config.password) if gateway_config.password else ""] +``` + +**2. Socket Management** + +```python +# Socket creation and cleanup +socket_path = "/tmp/ansible-platform-.sock" +``` + +**Checklist:** + +- [ ] **Socket path unique per manager** (identifier) +- [ ] **Socket removed on cleanup** (not orphaned) +- [ ] **Socket permissions secure** (0600, owner-only) +- [ ] **Socket directory exists and writable** +- [ ] **Old sockets cleaned up** (stale socket detection) +- [ ] **Socket path length < 104 chars** (Unix socket limit) + +**3. Process Lifecycle** + +**Checklist:** + +- [ ] **Process spawned correctly** +- [ ] **Process PID tracked** +- [ ] **Process terminated on cleanup** +- [ ] **Zombie processes prevented** (wait/reap) +- [ ] **Idle timeout works** (manager exits when idle) +- [ ] **Orphan prevention** (subprocess doesn't outlive parent) + +**4. Error Handling** + +```python +# ✅ GOOD: Handle spawn failures +try: + process = subprocess.Popen(cmd, ...) +except OSError as e: + raise AnsibleConnectionFailure(f"Failed to spawn manager: {e}") +finally: + # Cleanup resources + if socket_path and os.path.exists(socket_path): + os.unlink(socket_path) +``` + +**Checklist:** + +- [ ] Spawn failures don't leave orphaned sockets +- [ ] Spawn failures don't leave zombie processes +- [ ] Clear error messages for common failures +- [ ] Resource cleanup in finally blocks + +--- + +### plugins/plugin_utils/manager/platform_manager.py + +**Purpose:** PlatformService (runs in subprocess), handles RPC requests + +#### Critical Areas + +**1. RPC Request Handling** + +```python +class PlatformService: + def execute(self, operation, ansible_instance, ...): + # This runs in SUBPROCESS + # Handles requests from action plugins +``` + +**Checklist:** + +- [ ] **RPC protocol unchanged** (or versioned) +- [ ] **All parameters serializable** (pickle/JSON) +- [ ] **Return values serializable** +- [ ] **Exceptions properly caught and returned** +- [ ] **No blocking operations** (with timeout) + +**2. HTTP Session Management** + +```python +# ✅ GOOD: Session created in subprocess +class PlatformService: + def __init__(self, gateway_config): + self.session = self._create_session() # Created AFTER fork + + def _create_session(self): + session = requests.Session() + session.verify = self.gateway_config.verify_ssl + return session +``` + +**Checklist:** + +- [ ] Session created in subprocess (not before fork) +- [ ] Session reused across requests (not recreated) +- [ ] Session properly authenticated +- [ ] SSL verification configurable +- [ ] Session cleanup on exit + +**3. API Version Detection** + +```python +# Detect Gateway API version +version = self._detect_api_version() +self.registry = Registry(version) +``` + +**Checklist:** + +- [ ] Version detection still works +- [ ] Falls back gracefully if detection fails +- [ ] Works with new Gateway versions +- [ ] Cached (not detected every request) + +**4. Error Handling** + +**Checklist:** + +- [ ] HTTP errors properly caught +- [ ] Errors serialized back to client +- [ ] Stack traces included for debugging +- [ ] No secrets in error messages + +--- + +### plugins/plugin_utils/manager/rpc_client.py + +**Purpose:** Client-side RPC communication (action plugin → manager subprocess) + +#### Critical Areas + +**1. Request/Response Protocol** + +```python +# Send request to subprocess +request = { + "operation": "execute", + "ansible_instance": ansible_instance, # Must be serializable! +} + +response = self._send_request(request) +``` + +**Checklist:** + +- [ ] **Request format unchanged** (or versioned) +- [ ] **All request data serializable** +- [ ] **Response format unchanged** +- [ ] **Timeouts implemented** (don't hang forever) +- [ ] **Large responses handled** (memory limits) + +**2. Socket Communication** + +**Checklist:** + +- [ ] Socket connection retries (handle transient failures) +- [ ] Socket timeout configured +- [ ] Connection pooling/reuse (if applicable) +- [ ] Proper socket close on error + +**3. Error Propagation** + +```python +# ✅ GOOD: Preserve original error +if response.get("error"): + raise AnsibleError(response["error"]["message"]) + +# ❌ BAD: Loses error context +if response.get("error"): + raise AnsibleError("Something went wrong") +``` + +**Checklist:** + +- [ ] Errors from subprocess properly raised +- [ ] Error context preserved (stack trace, type) +- [ ] User-friendly error messages +- [ ] No swallowed exceptions + +--- + +### plugins/plugin_utils/platform/base_client.py & direct_client.py + +**Purpose:** API client logic (HTTP requests, response handling) + +#### Critical Areas + +**1. HTTP Request Construction** + +```python +def _make_request(self, method, path, data=None): + url = urljoin(self.base_url, path) + response = self.session.request(method, url, json=data) + return response.json() +``` + +**Checklist:** + +- [ ] **URL construction correct** (no double slashes, path joining) +- [ ] **HTTP methods handled** (GET, POST, PATCH, DELETE) +- [ ] **Request data serialization** (JSON encoding) +- [ ] **Headers set correctly** (Content-Type, Accept) +- [ ] **Authentication headers included** + +**2. Response Handling** + +**Checklist:** + +- [ ] HTTP status codes handled (200, 201, 204, 400, 404, 500) +- [ ] JSON parsing errors handled +- [ ] Empty responses handled (204 No Content) +- [ ] Pagination handled (if applicable) +- [ ] Rate limiting handled + +**3. Authentication Flow** + +```python +# Authenticate with Gateway +def authenticate(self): + if self.oauth_token: + self.session.headers["Authorization"] = f"Bearer {self.oauth_token}" + else: + self.session.auth = (self.username, self.password) +``` + +**Checklist:** + +- [ ] Username/password auth works +- [ ] OAuth token auth works +- [ ] Token refresh handled (if applicable) +- [ ] Auth failures provide clear messages +- [ ] No credentials in logs + +**4. Multi-Service Support** + +**Checklist:** + +- [ ] Gateway endpoints work (`/api/gateway/v1/`) +- [ ] Controller endpoints work (`/api/controller/v2/`) +- [ ] EDA endpoints work (`/api/eda/v1/`) +- [ ] Hub endpoints work (`/api/hub/v3/`) +- [ ] Service routing logic correct + +**5. Name → ID Lookup** + +```python +def lookup_resource_id(self, resource_type, name, endpoint): + # Critical: Used by ALL modules for reference resolution + result = self._make_request("GET", f"{endpoint}?name={name}") + return result["results"][0]["id"] +``` + +**Checklist:** + +- [ ] Lookup works for all resource types +- [ ] Handles not found (404) gracefully +- [ ] Handles multiple matches (ambiguous name) +- [ ] Handles special characters in names +- [ ] Caching works (if implemented) + +--- + +### plugins/plugin_utils/platform/registry.py + +**Purpose:** Module registry, API version detection, transform mixin loading + +#### Critical Areas + +**1. Module Registration** + +```python +# Modules auto-register on import +REGISTRY.register("organization", OrganizationTransformMixin_v1) +``` + +**Checklist:** + +- [ ] Registration mechanism still works +- [ ] All modules still registered +- [ ] No duplicate registrations +- [ ] Registration happens before use + +**2. API Version Detection** + +```python +def detect_api_version(self): + # Detect Gateway API version + response = self.client.get("/api/gateway/") + return response["version"] +``` + +**Checklist:** + +- [ ] Version detection for Gateway works +- [ ] Falls back if version endpoint missing +- [ ] Caches version (not detected every time) +- [ ] Works with future versions + +**3. Transform Mixin Loading** + +```python +def get_mixin(self, module_name, api_version): + # Load version-specific mixin + return self.mixins[module_name][api_version] +``` + +**Checklist:** + +- [ ] Loads correct mixin for API version +- [ ] Falls back if version not found +- [ ] Error message clear if module not found + +--- + +## Testing Requirements (CRITICAL) + +**Changes to connection/manager MUST have these tests:** + +### 1. Unit Tests (Required) + +```python +# tests/unit/plugins/plugin_utils/manager/test_.py + +def test_backwards_compatibility(): + """Ensure existing behavior still works.""" + # Test old code path + +def test_new_functionality(): + """Test the new change.""" + # Test new code path + +def test_error_handling(): + """Test error conditions.""" + # Test failures, edge cases +``` + +**Minimum coverage:** + +- [ ] Test changed code path +- [ ] Test unchanged code path (regression) +- [ ] Test error conditions +- [ ] Test both connection modes (if applicable) +- [ ] Mock external dependencies (HTTP, subprocess) + +### 2. Integration Tests (Required) + +**CRITICAL: Test with MULTIPLE modules** + +```yaml +# Test at least 3 different modules +- name: Test organization module + ansible.platform.organization: + name: "Test Org" + register: org_result + +- name: Test team module + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + register: team_result + +- name: Test user module + ansible.platform.user: + username: "testuser" + register: user_result + +# Verify all work +- assert: + that: + - org_result is success + - team_result is success + - user_result is success +``` + +**Why multiple modules:** +- Ensures change doesn't break specific module patterns +- Tests different transform mixin behaviors +- Catches edge cases + +### 3. Connection Mode Testing + +```bash +# Test persistent mode (default) +ansible-playbook tests/integration/test.yml + +# Test direct mode +AAP_CONNECTION_MODE=direct ansible-playbook tests/integration/test.yml + +# Both should work! +``` + +### 4. Vault Credentials Testing + +```yaml +# Test with vaulted credentials (common pattern) +# group_vars/all.yml +aap_username: !vault | + $ANSIBLE_VAULT;1.1;AES256 + ... + +# Should still work +``` + +### 5. Manual Testing Checklist + +**Test these scenarios manually:** + +- [ ] Fresh manager spawn (first task) +- [ ] Manager reuse (second task, same play) +- [ ] Manager cleanup (end of play) +- [ ] Connection failure recovery +- [ ] Invalid credentials handling +- [ ] Network timeout handling +- [ ] Concurrent playbook runs (multiple manager processes) + +--- + +## Performance Considerations + +### 1. Connection Overhead + +**Question:** Does this change add latency? + +```python +# ❌ BAD: API call for every field +for field in fields: + lookup_id(field) # N API calls! + +# ✅ GOOD: Batch lookups +lookup_ids(fields) # 1 API call +``` + +**Checklist:** + +- [ ] No unnecessary API calls added +- [ ] Batching opportunities taken +- [ ] Caching used where appropriate +- [ ] No blocking operations without timeout + +### 2. Memory Usage + +**Checklist:** + +- [ ] No memory leaks (resources freed) +- [ ] Large responses handled (streaming if needed) +- [ ] Session pool doesn't grow unbounded +- [ ] Manager subprocess memory stable + +### 3. Idle Timeout + +**Check idle timeout still works:** + +```python +# Manager should exit after idle timeout +# Default: 30 seconds +``` + +**Checklist:** + +- [ ] Idle timer resets on activity +- [ ] Manager exits cleanly when idle +- [ ] Socket removed on exit +- [ ] New manager spawns on next task + +--- + +## Security Review + +### 1. Credential Handling + +**CRITICAL: Ensure credentials never logged/exposed** + +```python +# ❌ DANGEROUS: Credentials in logs +logger.debug(f"Auth with {username}:{password}") # LEAKED! + +# ✅ SAFE: Mask credentials +logger.debug(f"Auth with {username}:***") + +# ✅ SAFE: Use display.vvvv for secrets (hidden by default) +self._display.vvvv(f"Password: {password}") # Only with -vvvv +``` + +**Checklist:** + +- [ ] No credentials in logs (debug/info/warning) +- [ ] No credentials in error messages +- [ ] No credentials in subprocess arguments (visible in ps) +- [ ] Credentials converted from vault properly +- [ ] Credentials cleared from memory when done + +### 2. Subprocess Security + +**Checklist:** + +- [ ] No shell=True (command injection risk) +- [ ] Arguments properly escaped +- [ ] Environment variables sanitized +- [ ] Working directory secure +- [ ] File permissions correct (sockets, temp files) + +### 3. Socket Security + +**Checklist:** + +- [ ] Socket permissions 0600 (owner-only) +- [ ] Socket in secure directory (/tmp with unique name) +- [ ] No symlink attacks (verify socket is real) +- [ ] Socket removed on cleanup + +--- + +## Documentation Requirements + +**For connection/manager changes, update these docs:** + +- [ ] `docs/03-sdk-architecture.md` - If architecture changes +- [ ] `docs/11-persistent-manager-idle-timeout.md` - If timeout logic changes +- [ ] CHANGELOG - User-visible changes +- [ ] Inline code comments - Complex logic + +**Example good documentation:** + +```python +def spawn_manager_process(self, ...): + """ + Spawn manager subprocess with fork-safe HTTP session. + + CRITICAL: All arguments must be str/bytes/Path for subprocess.Popen. + Vault credentials (AnsibleVaultEncryptedUnicode) must be converted + to str() before passing, or subprocess will fail with TypeError. + + This method spawns the manager in a separate process to work around + macOS + Python 3.12 fork safety issues with HTTP sessions. The + session is created AFTER the fork, inside the subprocess. + + Args: + gateway_config: May contain vaulted credentials + ... + + Raises: + AnsibleConnectionFailure: If subprocess spawn fails + """ +``` + +--- + +## Review Output Template + +```markdown +## Connection/Manager Review: # + +### Change Summary + +**Files Changed:** +- `plugins/connection/http.py` - Connection mode routing +- `plugins/plugin_utils/manager/process_manager.py` - Subprocess spawning + +**Change Type:** [Bugfix | Feature | Performance | Refactor] + +### Critical Checks + +#### Backwards Compatibility +- ✅ Tested with organization, team, user modules +- ✅ Both connection modes work (persistent, direct) +- ✅ All auth methods work (username/password, token) +- ⚠️ Deprecation warning added for old behavior + +#### Fork Safety +- ✅ No HTTP session created before fork +- ✅ All resources created in subprocess +- ✅ Tested on macOS + Python 3.12 (if possible) + +#### Connection Modes +- ✅ Persistent mode works +- ✅ Direct mode works +- ✅ Fallback logic intact +- ✅ Mode detection unchanged + +#### Security +- ✅ No credentials in logs +- ✅ No shell=True usage +- ✅ Socket permissions correct (0600) +- ✅ Vault credentials converted to str() + +### Testing + +#### Unit Tests +- ✅ `tests/unit/plugins/plugin_utils/manager/test_process_manager.py` - Added +- ✅ Test coverage: 95% of changed code +- ✅ Tests both old and new behavior + +#### Integration Tests +- ✅ Tested with 3+ modules (organization, team, user) +- ✅ Both connection modes tested +- ✅ Vault credentials tested +- ✅ All tests passing + +#### Manual Testing +- ✅ Fresh manager spawn works +- ✅ Manager reuse works +- ✅ Idle timeout still works +- ✅ Connection failure recovery works + +### Performance +- ✅ No additional API calls +- ✅ No memory leaks detected +- ✅ Manager subprocess memory stable + +### Documentation +- ✅ Inline comments added for complex logic +- ⚠️ Consider updating `docs/03-sdk-architecture.md` +- ✅ Changelog fragment present + +### Blockers + +None + +### Recommendations + +1. ✅ All critical checks passed +2. ⚠️ Consider adding example to architecture docs +3. 💡 Monitor manager subprocess memory in production + +### Verdict + +✅ **APPROVE** - Ready for `safe to test` + +Well-tested change with comprehensive coverage. Backwards compatible, fork-safe, and maintains security guarantees. + +**Extra validation:** Will monitor first few integration test runs closely. +``` + +--- + +## Red Flags (Request Changes If Found) + +**Immediate blockers:** + +- ❌ Breaking change without deprecation +- ❌ Not tested with multiple modules +- ❌ Credentials logged or exposed +- ❌ shell=True usage +- ❌ HTTP session created before fork +- ❌ No error handling for subprocess spawn +- ❌ Socket cleanup missing +- ❌ Both connection modes not tested + +**Request changes with explanation:** + +- ⚠️ Complex logic without comments +- ⚠️ Performance regression (more API calls) +- ⚠️ Missing unit tests for critical path +- ⚠️ Unclear error messages +- ⚠️ Documentation not updated + +--- + +**Last Updated:** 2026-09-11 diff --git a/.claude/skills/pr-review/references/feature-review.md b/.claude/skills/pr-review/references/feature-review.md new file mode 100644 index 00000000..6f0bc6c1 --- /dev/null +++ b/.claude/skills/pr-review/references/feature-review.md @@ -0,0 +1,659 @@ +# Feature PR Review Guide + +Detailed checklist for reviewing feature PRs (new modules, new functionality). + +## When to Use + +- PR adds a new module +- PR adds new functionality to existing module +- PR refactors code significantly +- PR title starts with `feat:` or `feature:` + +--- + +## ⚠️ CRITICAL: Check for Core Infrastructure Changes + +**Before proceeding, check if PR changes connection/manager files:** + +```bash +git diff origin/devel --name-only | grep -E \ + 'plugins/connection/|plugins/plugin_utils/manager/|plugins/plugin_utils/platform/(base_client|direct_client|config|registry)' +``` + +**If ANY match found:** +→ **STOP:** Read `connection-manager-review.md` FIRST before continuing with this checklist + +**Why:** Connection/manager changes affect ALL modules. They require extra scrutiny for: +- Backwards compatibility +- Fork safety (macOS + Python 3.12) +- Both connection modes (persistent vs direct) +- Security (credential handling, subprocess spawning) + +--- + +## Seven-File Pattern Validation + +**For new modules, verify all required files are present:** + +| # | File | Required? | What to Check | +|---|------|-----------|---------------| +| 1 | `plugins/modules/.py` | ✅ Always | Module stub with DOCUMENTATION + EXAMPLES | +| 2 | `plugins/action/.py` | ✅ Always | ActionModule class (Pattern A/B/C) | +| 3 | `plugins/plugin_utils/ansible_models/.py` | ✅ Always | AnsibleFoo dataclass with stable fields | +| 4 | `plugins/plugin_utils/api/v1/.py` | ✅ Always | APIFoo_v1 + transform mixin | +| 5 | `tests/unit/` | ✅ Always | pytest tests for transform logic | +| 6 | `extensions/molecule/_mock/` | ✅ Recommended | Mock server tests | +| 7 | `tests/integration/targets/s_test/` | ⚠️ Can defer | Live AAP tests (can add later) | + +**Commands to verify:** + +```bash +# Check module stub +test -f plugins/modules/.py && echo "✅ Module stub" || echo "❌ Missing" + +# Check action plugin +test -f plugins/action/.py && echo "✅ Action plugin" || echo "❌ Missing" + +# Check Ansible model +test -f plugins/plugin_utils/ansible_models/.py && echo "✅ Ansible model" || echo "❌ Missing" + +# Check API model + mixin +test -f plugins/plugin_utils/api/v1/.py && echo "✅ API model" || echo "❌ Missing" + +# Check unit tests +find tests/unit -name "**" -type f | head -1 + +# Check molecule tests +test -d extensions/molecule/_mock && echo "✅ Molecule tests" || echo "⚠️ Missing" + +# Check integration tests +test -d tests/integration/targets/s_test && echo "✅ Integration" || echo "⚠️ Can add later" +``` + +--- + +## Architecture Compliance + +### 1. Ansible Model Review + +**File:** `plugins/plugin_utils/ansible_models/.py` + +**Checklist:** + +- [ ] Uses `@dataclass` decorator +- [ ] Class name is `Ansible` (PascalCase) +- [ ] Required fields first, optional fields with defaults after +- [ ] Reference fields use **string names**, NOT integer IDs +- [ ] Read-only fields (id, created, modified, url) marked Optional +- [ ] `state` field defaults to `"present"` +- [ ] No API-specific fields (those belong in API model) + +**Example validation:** + +```python +@dataclass +class AnsibleFoo: + # ✅ CORRECT: String names for references + name: str + organization: str # ✅ String, not int + + # ✅ CORRECT: Optional fields with defaults + description: Optional[str] = None + state: str = "present" + + # ✅ CORRECT: Read-only fields + id: Optional[int] = None + created: Optional[str] = None + modified: Optional[str] = None + url: Optional[str] = None +``` + +**Common mistakes:** + +```python +# ❌ WRONG: Using integer IDs in Ansible model +organization: int # Should be: organization: str + +# ❌ WRONG: API-specific fields +organization_id: int # This belongs in API model only + +# ❌ WRONG: Required field after optional +name: str # ✅ OK +description: Optional[str] = None # ✅ OK +organization: str # ❌ WRONG - required after optional +``` + +--- + +### 2. API Model Review + +**File:** `plugins/plugin_utils/api/v1/.py` + +**Checklist:** + +- [ ] Uses `@dataclass` decorator +- [ ] Class name is `API_v1` +- [ ] All fields are Optional (API model is wire format) +- [ ] Reference fields use **integer IDs** +- [ ] Matches Gateway/Controller/EDA/Hub API response structure +- [ ] Read-only fields included + +**Example validation:** + +```python +@dataclass +class APIFoo_v1: + # ✅ CORRECT: All fields Optional + name: Optional[str] = None + + # ✅ CORRECT: Integer IDs for references + organization: Optional[int] = None # ID, not name + + # ✅ CORRECT: Optional fields + description: Optional[str] = None + + # ✅ CORRECT: Read-only fields included + id: Optional[int] = None + created: Optional[str] = None + modified: Optional[str] = None + url: Optional[str] = None +``` + +**Common mistakes:** + +```python +# ❌ WRONG: Using string names in API model +organization: str # Should be: organization: Optional[int] = None + +# ❌ WRONG: Required fields (all should be Optional) +name: str # Should be: name: Optional[str] = None +``` + +--- + +### 3. Transform Mixin Review + +**File:** `plugins/plugin_utils/api/v1/.py` (same file as API model) + +**Critical checks:** + +#### 3.1 Endpoint Path Declaration + +```python +def get_endpoint_operations(self): + return EndpointOperation( + path="/api///s/", # ← CHECK THIS + list_path="/api///s/", + detail_path="/api///s/{id}/", + lookup_field="name", + ) +``` + +**Verify correct service prefix:** + +| Service | Correct Prefix | Example | +|---------|---------------|---------| +| Gateway | `/api/gateway/v1/` | `/api/gateway/v1/teams/` | +| Controller | `/api/controller/v2/` | `/api/controller/v2/ad_hoc_commands/` | +| EDA | `/api/eda/v1/` | `/api/eda/v1/projects/` | +| Hub | `/api/hub/v3/` | `/api/hub/v3/namespaces/` | + +**Common mistake:** +```python +# ❌ WRONG: EDA resource with Gateway prefix +path="/api/gateway/v1/projects/" # Should be: /api/eda/v1/projects/ +``` + +#### 3.2 Name → ID Resolution + +**For reference fields, verify resolution logic:** + +```python +@classmethod +def from_ansible_data(cls, ansible_instance, context): + api_data = {} + + # ✅ CORRECT: Resolve organization name → ID + if ansible_instance.organization: + org_id = context.manager.lookup_resource_id( + "organization", # Resource type + ansible_instance.organization, # Name + endpoint="/api/gateway/v1/organizations/" # Lookup endpoint + ) + api_data["organization"] = org_id + + # ✅ CORRECT: Pass through non-reference fields + if ansible_instance.name: + api_data["name"] = ansible_instance.name + + return APIFoo_v1(**api_data) +``` + +**Checklist:** + +- [ ] All reference fields (org, team, credential, etc.) use `lookup_resource_id()` +- [ ] Correct resource type passed to lookup +- [ ] Correct endpoint path for lookup +- [ ] Non-reference fields pass through directly +- [ ] Write-only fields (passwords) excluded from read-back + +#### 3.3 Reverse Transform (from_api) + +```python +@classmethod +def from_api(cls, api_data, context): + # ✅ CORRECT: Map API data back to Ansible model + return AnsibleFoo( + id=api_data.get("id"), + name=api_data.get("name"), + organization=api_data.get("organization"), # ID in API, name in Ansible + description=api_data.get("description"), + ) +``` + +**Common mistake:** +```python +# ❌ WRONG: Forgetting to map field in reverse transform +# If you add opa_query_path to from_ansible_data() but forget from_api(), +# idempotency breaks (second run always shows changed=true) +``` + +--- + +### 4. Module Documentation Review + +**File:** `plugins/modules/.py` + +**Checklist:** + +- [ ] `DOCUMENTATION` block present and valid YAML +- [ ] `module: ` matches filename +- [ ] `short_description` clear and concise +- [ ] All parameters documented with: + - [ ] `description` + - [ ] `type` + - [ ] `required` or `default` +- [ ] `extends_documentation_fragment: ansible.platform.auth` present +- [ ] `EXAMPLES` block shows realistic use cases +- [ ] `RETURN` block documents all returned values +- [ ] `version_added` set correctly + +**Validation command:** + +```bash +# Check DOCUMENTATION is valid YAML +python3 -c " +import yaml +with open('plugins/modules/.py') as f: + content = f.read() + doc_start = content.find('DOCUMENTATION = \"\"\"') + len('DOCUMENTATION = \"\"\"') + doc_end = content.find('\"\"\"', doc_start) + yaml.safe_load(content[doc_start:doc_end]) +print('✅ DOCUMENTATION valid') +" +``` + +**Common issues:** + +```yaml +# ❌ WRONG: Missing type +organization: + description: The organization name + # Missing: type: str + +# ❌ WRONG: Missing extends_documentation_fragment +# Should have: +extends_documentation_fragment: + - ansible.platform.auth + +# ❌ WRONG: Vague description +name: + description: The name + # Should be: "The unique name of the foo resource." +``` + +--- + +### 5. Test Coverage Review + +#### 5.1 Unit Tests + +**Location:** `tests/unit/plugins/plugin_utils/api/v1/test_.py` + +**Required tests:** + +```python +def test_from_ansible_data_basic(): + """Test basic field mapping.""" + ansible_foo = AnsibleFoo(name="test", organization="Red Hat") + # ... verify transformation + +def test_from_ansible_data_with_optional_fields(): + """Test optional fields handled correctly.""" + # ... test with and without optional fields + +def test_from_ansible_data_resolves_organization(): + """Test that organization name resolves to ID.""" + # Mock lookup_resource_id + # Verify it's called correctly + # Verify ID is in API data + +def test_from_api_maps_all_fields(): + """Test reverse transformation.""" + api_data = {"id": 1, "name": "test", "organization": 42} + # Verify all fields mapped back + +def test_endpoint_operations_returns_correct_path(): + """Test endpoint path is correct.""" + # Verify service prefix correct +``` + +**Checklist:** + +- [ ] Tests cover `from_ansible_data()` transformation +- [ ] Tests cover `from_api()` reverse transformation +- [ ] Tests cover name→ID resolution for reference fields +- [ ] Tests cover optional field handling (None values) +- [ ] Tests verify endpoint path is correct +- [ ] All tests pass: `pytest tests/unit -v -k ` + +#### 5.2 Molecule Tests + +**Location:** `extensions/molecule/_mock/converge.yml` + +**Required scenarios:** + +```yaml +# 1. Create (state: present) +- name: Create foo + ansible.platform.foo: + name: "Test Foo" + organization: "Test Org" + state: present + register: result + +- name: Verify created + assert: + that: + - result.changed + - result.id is defined + +# 2. Idempotency (run again, no change) +- name: Create foo again (idempotent) + ansible.platform.foo: + name: "Test Foo" + organization: "Test Org" + state: present + register: result + +- name: Verify no change + assert: + that: + - not result.changed + +# 3. Update (modify field) +- name: Update foo description + ansible.platform.foo: + name: "Test Foo" + description: "Updated" + state: present + register: result + +- name: Verify updated + assert: + that: + - result.changed + - result.description == "Updated" + +# 4. Delete (state: absent) +- name: Delete foo + ansible.platform.foo: + name: "Test Foo" + state: absent + register: result + +- name: Verify deleted + assert: + that: + - result.changed +``` + +**Checklist:** + +- [ ] Create scenario present +- [ ] Idempotency test present (run twice, second returns changed: false) +- [ ] Update scenario present +- [ ] Delete scenario present +- [ ] Mock server handles all endpoints (check `mock/prepare.yml`) +- [ ] Tests pass: `molecule test -s _mock` + +#### 5.3 Integration Tests (Can Defer) + +**Location:** `tests/integration/targets/s_test/` + +**Note:** Can be added later, not required for initial merge if Molecule tests are comprehensive. + +**If present, verify:** + +``` +tests/integration/targets/s_test/ +├── tasks/ +│ └── main.yml # Test playbook +├── aliases # CI target groups +└── meta/ + └── main.yml # Test dependencies +``` + +--- + +## meta/runtime.yml Registration + +**Must be updated for new modules:** + +```yaml +# meta/runtime.yml +action_groups: + gateway: + - ad_hoc_command + - application + - # ← ADD THIS +``` + +**Validation:** + +```bash +# Check if module is in runtime.yml +grep "" meta/runtime.yml || echo "❌ Missing from runtime.yml" +``` + +--- + +## Action Plugin Pattern Detection + +**File:** `plugins/action/.py` + +**Three patterns:** + +### Pattern A: Declarative (Simplest) + +```python +class ActionModule(ActionPlatformGenericResource): + module_name = "foo" + ansible_model_class = AnsibleFoo +``` + +**Only 3 lines. Use when:** +- Standard CRUD operations only +- No custom logic needed +- Fields map 1:1 to API + +**Example:** `organization.py` + +### Pattern B: Hook-Based + +```python +class ActionModule(ActionPlatformGenericResource): + module_name = "foo" + ansible_model_class = AnsibleFoo + + def _pre_create(self, ansible_instance, api_instance): + # Custom logic before create + pass + + def _post_update(self, result, ansible_instance): + # Custom logic after update + pass +``` + +**Use when:** +- Need side effects (create related resources) +- Need validation beyond API +- Need to modify result + +### Pattern C: Fully Custom + +```python +class ActionModule(ActionPlatformGenericResource): + def execute(self, ansible_instance): + # Completely custom implementation + pass +``` + +**Use when:** +- Non-standard workflow (e.g., launch and wait) +- Multiple endpoint orchestration +- Write-only fields need special handling + +**Example:** `user.py` (has password write-only field) + +**Review checklist:** + +- [ ] Pattern choice is appropriate for complexity +- [ ] If Pattern C, justify why A/B won't work +- [ ] Custom logic is tested + +--- + +## Multi-Endpoint Pattern + +**When needed:** Resource uses different endpoints for different operations. + +**Example:** `opa_query_path` field only on Controller API, not Gateway API + +```python +def get_endpoint_operations(self): + # Route based on which fields are present + if self.opa_query_path is not None: + # Use Controller API when opa_query_path present + return EndpointOperation( + path="/api/controller/v2/organizations/", + lookup_field="name", + ) + + # Default: Gateway API + return EndpointOperation( + path="/api/gateway/v1/organizations/", + lookup_field="name", + ) +``` + +**Review checklist:** + +- [ ] Routing logic is clear and documented +- [ ] Both endpoints return compatible data +- [ ] Tests cover both code paths + +--- + +## Documentation Updates + +**Check if these need updating:** + +- [ ] `docs/07-adding-resources.md` - If new pattern introduced +- [ ] `docs/10-case-study-aap-platform.md` - If new resource category +- [ ] README examples - If user-facing feature + +--- + +## CasC Team Notification + +**Notify CasC team if PR affects:** + +- Return value structure changes +- New module (they may need to add support) +- Authentication parameter changes +- Breaking changes to existing modules + +**How to notify:** + +```bash +# Add comment to PR +gh pr comment --repo ansible/ansible.platform \ + --body "@ansible/casc-team FYI - this PR adds a new module: " +``` + +**Or add label:** +```bash +gh pr edit --repo ansible/ansible.platform --add-label "casc-review-needed" +``` + +--- + +## Review Output Template + +```markdown +## Feature Review: # + +### Seven-File Pattern + +| File | Status | Notes | +|------|--------|-------| +| Module stub | ✅ | Complete | +| Action plugin | ✅ | Pattern A (appropriate) | +| Ansible model | ✅ | Uses string names | +| API model | ✅ | Uses integer IDs | +| Transform mixin | ✅ | Correct endpoint path | +| Unit tests | ✅ | 95% coverage | +| Molecule tests | ✅ | All scenarios covered | +| Integration tests | ⚠️ | Can add later | + +### Architecture Compliance + +- ✅ Ansible Model uses string names for references +- ✅ API Model uses integer IDs +- ✅ Transform mixin resolves names → IDs correctly +- ✅ Endpoint path has correct service prefix: `/api/gateway/v1/` +- ✅ meta/runtime.yml updated + +### Test Coverage + +- ✅ Unit tests: 12 tests, all passing +- ✅ Molecule tests: Create, update, delete, idempotency - all passing +- ⚠️ Integration tests: Can add later (molecule tests are comprehensive) + +### Documentation + +- ✅ DOCUMENTATION complete and valid +- ✅ EXAMPLES show realistic use cases +- ✅ RETURN values documented +- ✅ Changelog fragment present + +### Blockers + +None + +### Recommendations + +1. Consider adding integration tests in follow-up PR +2. CasC team notification recommended (new module) + +### Verdict + +✅ **APPROVE** - Ready for `safe to test` label + +All pre-merge checks passed. Architecture compliant. Comprehensive test coverage. +``` + +--- + +**Last Updated:** 2026-09-11 diff --git a/.claude/skills/pr-review/skill.md b/.claude/skills/pr-review/skill.md new file mode 100644 index 00000000..cbd409e4 --- /dev/null +++ b/.claude/skills/pr-review/skill.md @@ -0,0 +1,338 @@ +--- +name: pr-review +description: >- + Reviews pull requests in ansible.platform collection as a maintainer. + Checks CI failures, architecture compliance, and routes to appropriate + review workflow (feature, bugfix, or CI/workflow). +user-invocable: true +--- + +# ansible.platform PR Review + +Reviews PRs as a collection maintainer following ansible.platform standards. + +## Usage + +```bash +/pr-review +``` + +## Overview + +This skill performs a structured PR review with these priorities: + +1. **Pre-merge CI checks** (must pass BEFORE `safe to test`) + - Collection completeness test + - Unit tests + - Sanity tests + +2. **Route to specific review type:** + - Feature PR → `references/feature-review.md` + - Bugfix PR → `references/bugfix-review.md` + - CI/workflow PR → `references/ci-workflow-review.md` + +3. **Safe-to-test readiness** (after fixes) + +4. **Post-label CI monitoring** (integration tests) + +--- + +## Workflow + +### Step 1: Fetch PR Information + +```bash +gh pr view --repo ansible/ansible.platform \ + --json title,body,author,files,statusCheckRollup,labels +``` + +**Extract:** +- PR type from title: `feat:`, `fix:`, `ci:`, `refactor:`, `docs:` +- Files changed count +- CI check status +- Jira issue reference + +--- + +### Step 2: Pre-Merge CI Checks (BEFORE safe-to-test) + +**These must pass before applying `safe to test` label:** + +#### 2.1 Collection Completeness Test + +**Purpose:** Ensures new modules are registered in `meta/runtime.yml` + +```bash +# If failing, get error details +gh run view --repo ansible/ansible.platform --log | \ + grep -A 10 "collection completeness" +``` + +**Common failure:** +``` +The following items should be added to meta/runtime.yml action-groups.gateway: + +``` + +**Fix:** +```diff +# meta/runtime.yml +action_groups: + gateway: ++ - + - application +``` + +**Why it matters:** Enables `module_defaults` for `group/ansible.platform.gateway` + +#### 2.2 Unit Tests + +```bash +# Check unit test failures +gh run view --repo ansible/ansible.platform --log | \ + grep -A 20 "pytest" +``` + +**Review:** +- Assertion failures +- Import errors +- Test coverage for changed code + +#### 2.3 Sanity Tests + +```bash +# Check sanity test failures +gh run view --repo ansible/ansible.platform --log | \ + grep -A 20 "ansible-test sanity" +``` + +**Common issues:** +- Documentation validation (malformed DOCUMENTATION) +- Import validation +- PEP8 violations + +#### 2.4 DVCS Integration (Non-blocking) + +**Check:** Jira issue reference in PR title + +**Format:** `[AAP-XXXXX]` or `AAP-XXXXX` + +**If missing:** Request Jira reference (but not a blocker) + +--- + +### Step 3: Changelog Verification + +**Check if changelog needed:** + +```bash +# Changed files that require changelog +- plugins/**/*.py → YES +- tests/**/*.py → YES +- docs/**/*.md → NO (docs-only) +- .github/**/*.yml → NO (CI-only) +``` + +**Verify changelog exists:** +```bash +ls changelogs/fragments/ | grep -E "|" +``` + +**Validate format:** +```yaml +# For features +minor_changes: + - "Short description (ansible/ansible.platform#)." + +# For bugfixes +bugfixes: + - "Fix description (ansible/ansible.platform#)." +``` + +--- + +### Step 4: Route to Specific Review + +**FIRST: Check if PR touches core infrastructure (connection/manager):** + +```bash +# Check for connection or manager changes +git diff origin/devel --name-only | grep -E \ + 'plugins/connection/|plugins/plugin_utils/manager/|plugins/plugin_utils/platform/(base_client|direct_client|config|registry)' +``` + +**If connection/manager files changed:** +→ **CRITICAL:** Read `references/connection-manager-review.md` FIRST (regardless of PR type) + +**Then, based on PR type, read appropriate reference:** + +| PR Type | Reference File | When to Use | +|---------|---------------|-------------| +| `feat:` | `references/feature-review.md` | New module, new feature | +| `fix:` | `references/bugfix-review.md` | Bug fix, regression fix | +| `ci:` | `references/ci-workflow-review.md` | CI, GitHub Actions, workflow changes | +| `refactor:` | `references/feature-review.md` | Code refactoring (use feature checklist) | +| `docs:` | Skip to Step 6 | Documentation only | +| **Connection/Manager** | `references/connection-manager-review.md` | Core infrastructure changes | + +**Read the reference file and follow its checklist.** + +--- + +### Step 5: Determine Safe-to-Test Readiness + +**Prerequisites for `safe to test` label:** + +- ✅ Collection completeness test passing +- ✅ Unit tests passing +- ✅ Sanity tests passing +- ✅ Changelog fragment present (if code changes) +- ✅ Jira issue referenced (recommended) +- ✅ No obvious security issues + +**Decision:** + +| Status | Action | +|--------|--------| +| All prerequisites met | ✅ **Ready for `safe to test`** | +| Any pre-merge check failing | ❌ **Request fixes first** | +| Docs-only PR | ✅ **Can merge without label** | + +--- + +### Step 6: Post-Label Monitoring (After safe-to-test applied) + +**Once `safe to test` label is applied, integration tests run:** + +```bash +# Monitor CI +gh pr checks --repo ansible/ansible.platform --watch +``` + +**Integration test failures:** +- AAP connectivity issues (transient → re-run) +- Resource creation failures (check required fields) +- Timeout errors (check wait/polling logic) +- 404 errors (wrong endpoint path in mixin) + +**Re-run transient failures:** +```bash +gh run rerun --repo ansible/ansible.platform --failed +``` + +--- + +### Step 7: Post Review + +**Use template based on findings:** + +```markdown +## PR Review: # + +**Type:** [Feature|Bugfix|CI/Workflow|Docs] +**Files Changed:** X files +**CI Status:** [✅ All Green|❌ N Failing|⏳ Pending] + +### Pre-Merge CI Checks + +- [x] Collection completeness: ✅ Passing +- [ ] Unit tests: ❌ 2 failures (see details below) +- [x] Sanity tests: ✅ Passing +- [x] Changelog: ✅ Present + +### Architecture Review (Features Only) + +[See feature-review.md checklist results] + +### Blockers + +1. **Unit test failures** + - File: `tests/unit/plugins/plugin_utils/api/v1/test_foo.py:42` + - Issue: Assertion failed - expected 'bar', got 'baz' + - Fix: Update test expectation to match implementation + +2. **Missing meta/runtime.yml entry** + ```diff + action_groups: + gateway: + + - foo + ``` + +### Safe-to-Test Status + +**Status:** ❌ **NOT READY** + +**Reason:** Unit tests failing + +**Next Steps:** +1. Fix unit test failures +2. Push changes +3. Re-review +4. Apply `safe to test` label + +### Final Verdict + +**⚠️ REQUEST CHANGES** + +Please address the blockers above. Once fixed, I'll re-review and we can proceed with integration testing. +``` + +**Post review:** +```bash +# Request changes +gh pr review --repo ansible/ansible.platform \ + --request-changes --body "$(cat review.md)" + +# Approve (after all checks pass) +gh pr review --repo ansible/ansible.platform \ + --approve --body "LGTM! All checks passing." +``` + +--- + +## Quick Reference + +### CI Check Priority + +1. **Pre-merge (MUST pass before `safe to test`):** + - Collection completeness ← **Run FIRST** + - Unit tests + - Sanity tests + - Changelog verification + +2. **Post-label (triggered BY `safe to test`):** + - Integration tests (live AAP) + +### PR Type Detection + +``` +feat: → Feature review +fix: → Bugfix review +ci: → CI/workflow review +docs: → Skip to safe-to-test check +refactor: → Feature review (architecture check) +``` + +### Common Fixes + +**Collection completeness failure:** +→ Add module to `meta/runtime.yml` + +**Missing changelog:** +→ Create `changelogs/fragments/-.yml` + +**Integration test failures:** +→ Check if transient, re-run if needed + +--- + +## Reference Files + +- `references/feature-review.md` - Seven-file pattern, architecture compliance +- `references/bugfix-review.md` - Regression test requirements +- `references/ci-workflow-review.md` - CI/workflow specific checks + +--- + +**Last Updated:** 2026-09-11 +**Based on:** PR #227 review experience diff --git a/docs/12-pr-review-guidelines.md b/docs/12-pr-review-guidelines.md new file mode 100644 index 00000000..12987679 --- /dev/null +++ b/docs/12-pr-review-guidelines.md @@ -0,0 +1,2131 @@ +# PR Review Guidelines + +Comprehensive guidelines for reviewing pull requests in the ansible.platform collection. + +**Audience:** Maintainers, reviewers, and contributors + +**Last Updated:** 2026-09-11 + +--- + +## Table of Contents + +1. [Review Process Overview](#review-process-overview) +2. [Pre-Merge CI Checks](#pre-merge-ci-checks) +3. [Feature PR Review](#feature-pr-review) +4. [Bugfix PR Review](#bugfix-pr-review) +5. [CI/Workflow PR Review](#ciworkflow-pr-review) +6. [Connection/Manager PR Review](#connectionmanager-pr-review-critical) +7. [Architecture Principles](#architecture-principles) +8. [Code Quality Standards](#code-quality-standards) +9. [Testing Requirements](#testing-requirements) +10. [Common Issues and Fixes](#common-issues-and-fixes) +11. [Getting PRs Merged Faster](#getting-prs-merged-faster) + +--- + +## Review Process Overview + +### Workflow Steps + +1. **Pre-Merge CI Checks** - Must pass BEFORE `safe to test` label + - Collection completeness test + - Unit tests + - Sanity tests + - Changelog verification + +2. **Code Review** - Route to appropriate checklist: + - Feature → Seven-file pattern + architecture + - Bugfix → Regression test + root cause + - CI/Workflow → Security + secret protection + - Connection/Manager → Cross-module impact + fork safety + +3. **Safe to Test** - Apply label after pre-merge checks pass + +4. **Integration Tests** - Triggered by `safe to test` label + +5. **Final Review** - 2+ approvals, all checks green + +6. **Merge** - Squash and merge to devel + +### Priority Checks + +| Priority | Check | Blocker? | +|----------|-------|----------| +| 🔴 Critical | Collection completeness test | YES | +| 🔴 Critical | Unit tests | YES | +| 🔴 Critical | Sanity tests | YES | +| 🔴 Critical | Connection/manager changes (if applicable) | YES | +| 🟡 Important | Changelog fragment | YES (if code changes) | +| 🟡 Important | Jira reference | YES (if bugfix) | +| 🟢 Optional | Integration tests | NO (can defer) | + +--- + +## Pre-Merge CI Checks + +**These checks run automatically and MUST pass before `safe to test` label.** + +### 1. Collection Completeness Test + +**What it checks:** All modules extending `ansible.platform.auth` are registered in `meta/runtime.yml` + +**Purpose:** Enables `module_defaults` for `group/ansible.platform.gateway` + +**Test location:** `tests/test_completeness.py` + +**Common failure:** +``` +The following items should be added to meta/runtime.yml action-groups.gateway: + ad_hoc_command +``` + +**Fix:** +```yaml +# meta/runtime.yml +action_groups: + gateway: + - ad_hoc_command # ← ADD THIS + - application + - authenticator +``` + +**Why it matters:** +- Ensures `module_defaults` work for all modules +- Enforces consistency across collection +- Required CI check (blocks merge) + +### 2. Unit Tests + +**What they check:** Transform logic, utilities, framework code + +**Run locally:** +```bash +pytest tests/unit/ -v + +# Run specific test +pytest tests/unit/plugins/plugin_utils/api/v1/test_team.py -v +``` + +**Common failures:** +- Assertion errors (expected vs actual mismatch) +- Import errors (missing dependencies) +- Mock/fixture issues + +### 3. Sanity Tests + +**What they check:** +- Documentation format (DOCUMENTATION, EXAMPLES, RETURN) +- Import validation +- PEP8 compliance +- Ansible-specific rules + +**Run locally:** +```bash +ansible-test sanity --docker + +# Run specific test +ansible-test sanity plugins/modules/organization.py --docker +``` + +**Common failures:** +- Malformed DOCUMENTATION YAML +- Missing required doc fields +- Import violations +- Formatting issues + +### 4. Changelog Fragment + +**Required if PR changes:** +- `plugins/**/*.py` → YES +- `tests/**/*.py` → YES +- `docs/**/*.md` → NO +- `.github/**/*.yml` → NO (unless user-visible) + +**Location:** `changelogs/fragments/-.yml` + +**Formats:** + +```yaml +# For features +minor_changes: + - "Add foo module for managing Foo resources (ansible/ansible.platform#123)." + +# For bugfixes +bugfixes: + - "Fix vault credential handling in manager subprocess (ansible/ansible.platform#124)." + +# For breaking changes +breaking_changes: + - "Remove deprecated bar parameter from baz module. Use qux instead (ansible/ansible.platform#125)." + +# For deprecations +deprecated_features: + - "The old_param parameter in foo module is deprecated and will be removed in 3.0.0. Use new_param instead (ansible/ansible.platform#126)." +``` + +**Validation:** +```bash +# Check if fragment exists +ls changelogs/fragments/ | grep -E "|" + +# Validate YAML syntax +ansible-doc-extractor --validate changelogs/fragments/.yml +``` + +--- + +## Feature PR Review + +**Use for:** New modules, new features, significant refactoring + +### Seven-File Pattern Checklist + +**For new modules, all files must be present:** + +| # | File | Required? | What to Check | +|---|------|-----------|---------------| +| 1 | `plugins/modules/.py` | ✅ Always | Module stub with DOCUMENTATION + EXAMPLES | +| 2 | `plugins/action/.py` | ✅ Always | ActionModule class (Pattern A/B/C) | +| 3 | `plugins/plugin_utils/ansible_models/.py` | ✅ Always | AnsibleFoo dataclass, stable fields | +| 4 | `plugins/plugin_utils/api/v1/.py` | ✅ Always | APIFoo_v1 + transform mixin | +| 5 | `tests/unit/` | ✅ Always | pytest tests for transform logic | +| 6 | `extensions/molecule/_mock/` | ✅ Recommended | Mock server tests, idempotency | +| 7 | `tests/integration/targets/s_test/` | ⚠️ Can defer | Live AAP tests (can add later) | + +**Verification commands:** + +```bash +# Check all files +for file in \ + "plugins/modules/.py" \ + "plugins/action/.py" \ + "plugins/plugin_utils/ansible_models/.py" \ + "plugins/plugin_utils/api/v1/.py"; do + test -f "$file" && echo "✅ $file" || echo "❌ MISSING: $file" +done + +# Check tests +find tests/unit -name "**" -type f +test -d extensions/molecule/_mock && echo "✅ Molecule" || echo "⚠️ Molecule missing" +test -d tests/integration/targets/s_test && echo "✅ Integration" || echo "⚠️ Can add later" +``` + +### Architecture Compliance + +#### 1. Ansible Model Review (`plugins/plugin_utils/ansible_models/.py`) + +**✅ Correct pattern:** + +```python +@dataclass +class AnsibleTeam: + # Required fields first + name: str + organization: str # ✅ String name, NOT integer ID + + # Optional fields with defaults + description: Optional[str] = None + state: str = "present" + + # Read-only fields (from API) + id: Optional[int] = None + created: Optional[str] = None + modified: Optional[str] = None + url: Optional[str] = None +``` + +**❌ Common mistakes:** + +```python +@dataclass +class AnsibleTeam: + organization: int # ❌ WRONG - Should be str + organization_id: int # ❌ WRONG - API-specific field + name: str # ❌ WRONG - Required after optional + description: Optional[str] = None +``` + +**Checklist:** + +- [ ] Uses `@dataclass` decorator +- [ ] Class name is `Ansible` (PascalCase) +- [ ] Required fields first, optional with defaults after +- [ ] Reference fields use **string names** (not IDs) +- [ ] Read-only fields marked `Optional` +- [ ] `state` field defaults to `"present"` +- [ ] No API-specific fields + +#### 2. API Model Review (`plugins/plugin_utils/api/v1/.py`) + +**✅ Correct pattern:** + +```python +@dataclass +class APITeam_v1: + # All fields Optional (wire format) + name: Optional[str] = None + organization: Optional[int] = None # ✅ Integer ID + description: Optional[str] = None + + # Read-only fields + id: Optional[int] = None + created: Optional[str] = None + modified: Optional[str] = None + url: Optional[str] = None +``` + +**❌ Common mistakes:** + +```python +@dataclass +class APITeam_v1: + name: str # ❌ WRONG - Should be Optional[str] = None + organization: str # ❌ WRONG - Should be Optional[int] (ID) +``` + +**Checklist:** + +- [ ] Uses `@dataclass` decorator +- [ ] Class name is `API_v1` +- [ ] **All fields are Optional** (wire format) +- [ ] Reference fields use **integer IDs** (not names) +- [ ] Matches Gateway/Controller/EDA/Hub API response +- [ ] Read-only fields included + +#### 3. Transform Mixin Review (same file as API model) + +**✅ Correct endpoint path declaration:** + +```python +class TeamTransformMixin_v1: + def get_endpoint_operations(self): + return EndpointOperation( + path="/api/gateway/v1/teams/", # ✅ Full path with service prefix + list_path="/api/gateway/v1/teams/", + detail_path="/api/gateway/v1/teams/{id}/", + lookup_field="name", + ) +``` + +**Service prefix table:** + +| Service | Prefix | Example | +|---------|--------|---------| +| Gateway | `/api/gateway/v1/` | `/api/gateway/v1/teams/` | +| Controller | `/api/controller/v2/` | `/api/controller/v2/job_templates/` | +| EDA | `/api/eda/v1/` | `/api/eda/v1/projects/` | +| Hub | `/api/hub/v3/` | `/api/hub/v3/namespaces/` | + +**❌ Common mistake:** + +```python +# ❌ WRONG: EDA resource with Gateway prefix +path="/api/gateway/v1/projects/" # Should be: /api/eda/v1/projects/ +``` + +**✅ Correct name→ID resolution:** + +```python +@classmethod +def from_ansible_data(cls, ansible_instance, context): + api_data = {} + + # Resolve organization name → ID + if ansible_instance.organization: + org_id = context.manager.lookup_resource_id( + "organization", # Resource type + ansible_instance.organization, # Name from user + endpoint="/api/gateway/v1/organizations/" # Lookup endpoint + ) + api_data["organization"] = org_id # ID in API + + # Pass through non-reference fields + if ansible_instance.name: + api_data["name"] = ansible_instance.name + + return APITeam_v1(**api_data) +``` + +**✅ Correct reverse transform:** + +```python +@classmethod +def from_api(cls, api_data, context): + """Map API response back to Ansible model.""" + return AnsibleTeam( + id=api_data.get("id"), + name=api_data.get("name"), + organization=api_data.get("organization"), # ID→name conversion handled elsewhere + description=api_data.get("description"), + # ⚠️ DON'T FORGET: If you add field to from_ansible_data(), + # also add to from_api() or idempotency breaks! + ) +``` + +**Checklist:** + +- [ ] Endpoint path has correct service prefix +- [ ] All reference fields use `lookup_resource_id()` +- [ ] Non-reference fields pass through directly +- [ ] Write-only fields (passwords) excluded from read +- [ ] Reverse transform (`from_api()`) maps all fields back +- [ ] Multi-endpoint pattern used if fields split across APIs + +#### 4. Module Documentation Review (`plugins/modules/.py`) + +**✅ Correct DOCUMENTATION:** + +```python +DOCUMENTATION = """ +module: team +short_description: Manage teams in AAP +description: + - Create, update, or delete teams in Ansible Automation Platform. + - Teams are collections of users with shared permissions. +version_added: "2.7.0" +extends_documentation_fragment: + - ansible.platform.auth +options: + name: + description: + - The unique name of the team. + type: str + required: true + organization: + description: + - The organization name or ID that the team belongs to. + - Accepts either the organization name (looked up automatically) or integer ID. + type: str + required: true + description: + description: + - Optional description of the team. + type: str + required: false + state: + description: + - Desired state of the team. + type: str + choices: ['present', 'absent', 'exists'] + default: present +author: + - "Your Name (@github_username)" +""" + +EXAMPLES = """ +- name: Create engineering team + ansible.platform.team: + name: "Engineering" + organization: "Red Hat" + description: "Engineering team" + state: present + +- name: Delete team + ansible.platform.team: + name: "Engineering" + organization: "Red Hat" + state: absent + +- name: Check if team exists + ansible.platform.team: + name: "Engineering" + organization: "Red Hat" + state: exists + register: result +""" + +RETURN = """ +id: + description: The ID of the team + returned: success + type: int + sample: 42 +name: + description: The name of the team + returned: success + type: str + sample: "Engineering" +organization: + description: The organization ID + returned: success + type: int + sample: 1 +""" +``` + +**Checklist:** + +- [ ] `module:` matches filename +- [ ] `short_description` clear and concise +- [ ] All parameters have `description`, `type`, `required`/`default` +- [ ] `extends_documentation_fragment: ansible.platform.auth` present +- [ ] `EXAMPLES` show realistic use cases +- [ ] `RETURN` documents all returned values +- [ ] `version_added` set correctly +- [ ] Valid YAML (test with `python -c "import yaml; yaml.safe_load(DOCUMENTATION)"`) + +#### 5. meta/runtime.yml Registration + +**Required for all new modules:** + +```yaml +# meta/runtime.yml +action_groups: + gateway: + - ad_hoc_command + - application + - authenticator + - # ← ADD THIS + - organization + - team +``` + +**Verification:** + +```bash +grep "" meta/runtime.yml || echo "❌ Missing from runtime.yml" +``` + +### Test Coverage Requirements + +#### Unit Tests (Required) + +**Location:** `tests/unit/plugins/plugin_utils/api/v1/test_.py` + +**Minimum tests:** + +```python +def test_from_ansible_data_basic(): + """Test basic field mapping.""" + ansible_team = AnsibleTeam(name="test", organization="Red Hat") + context = create_mock_context() + + mixin = TeamTransformMixin_v1() + api_team = mixin.from_ansible_data(ansible_team, context) + + assert api_team.name == "test" + # ... verify all fields + +def test_from_ansible_data_resolves_organization(): + """Test organization name→ID resolution.""" + ansible_team = AnsibleTeam(name="test", organization="Red Hat") + context = create_mock_context() + context.manager.lookup_resource_id = Mock(return_value=42) + + mixin = TeamTransformMixin_v1() + api_team = mixin.from_ansible_data(ansible_team, context) + + assert api_team.organization == 42 + context.manager.lookup_resource_id.assert_called_once_with( + "organization", "Red Hat", endpoint="/api/gateway/v1/organizations/" + ) + +def test_from_api_maps_all_fields(): + """Test reverse transformation.""" + api_data = {"id": 1, "name": "test", "organization": 42} + + mixin = TeamTransformMixin_v1() + ansible_team = mixin.from_api(api_data, context) + + assert ansible_team.id == 1 + assert ansible_team.name == "test" + +def test_endpoint_operations_correct_path(): + """Test endpoint path.""" + mixin = TeamTransformMixin_v1() + ops = mixin.get_endpoint_operations() + + assert ops.path == "/api/gateway/v1/teams/" +``` + +**Run:** +```bash +pytest tests/unit/plugins/plugin_utils/api/v1/test_team.py -v +``` + +#### Molecule Tests (Recommended) + +**Location:** `extensions/molecule/_mock/converge.yml` + +**Required scenarios:** + +```yaml +--- +# 1. CREATE +- name: Create team + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + state: present + register: result + +- name: Verify created + assert: + that: + - result.changed + - result.id is defined + +# 2. IDEMPOTENCY +- name: Create team again (idempotent) + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + state: present + register: result + +- name: Verify no change + assert: + that: + - not result.changed # ← Idempotent! + +# 3. UPDATE +- name: Update team description + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + description: "Updated" + state: present + register: result + +- name: Verify updated + assert: + that: + - result.changed + - result.description == "Updated" + +# 4. DELETE +- name: Delete team + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + state: absent + register: result + +- name: Verify deleted + assert: + that: + - result.changed +``` + +**Run:** +```bash +molecule test -s team_mock +``` + +#### Integration Tests (Can Defer) + +**Location:** `tests/integration/targets/s_test/` + +**Can add in follow-up PR if:** +- Unit tests comprehensive +- Molecule tests comprehensive +- Follow-up Jira ticket created + +--- + +## Bugfix PR Review + +**Use for:** Bug fixes, regressions, defects + +### Jira Issue Verification + +**Required format in PR title:** +``` +[AAP-12345] Fix description +``` + +**Validation:** +```bash +gh pr view --json title --jq '.title' | grep -oE 'AAP-[0-9]+' +``` + +**If missing:** +```markdown +❌ **Blocker:** Missing Jira issue reference in PR title + +Please update: [AAP-XXXXX] +``` + +### Bug Description Requirements + +**PR description must include:** + +1. **What was broken:** Clear description +2. **How to reproduce:** Step-by-step +3. **Root cause:** Why the bug occurred +4. **What changed:** The fix applied +5. **How verified:** Testing approach + +**Example:** + +```markdown +## Bug Description + +**Issue:** Vaulted aap_username/aap_password cause subprocess.Popen to fail + +**Reproduce:** +1. Encrypt credentials with ansible-vault +2. Run any platform module +3. Error: TypeError: expected str, not AnsibleVaultEncryptedUnicode + +**Root Cause:** +process_manager.py passes vaulted credentials directly to subprocess.Popen +without converting to strings first. + +**Fix:** +Convert gateway_config credentials to str() before passing to Popen. + +**Verification:** +- Added unit test with MockAnsibleVaultEncryptedUnicode +- Test verifies str() conversion happens +- Manual test with vaulted group_vars +``` + +### Regression Test Requirements (CRITICAL) + +**Bugfix MUST include test that would have caught the bug.** + +**Three options (in order of preference):** + +#### Option 1: Unit Test (Preferred) + +```python +# tests/unit/plugins/plugin_utils/manager/test_vault_credentials.py + +def test_vault_credentials_converted_to_strings(): + """ + Test that vaulted credentials are converted to str before Popen. + + Regression test for AAP-XXXXX where vaulted credentials caused: + TypeError: expected str, not AnsibleVaultEncryptedUnicode + """ + # Create vaulted credentials + vaulted_username = MockAnsibleVaultEncryptedUnicode("admin") + vaulted_password = MockAnsibleVaultEncryptedUnicode("secret") + + # Verify type + assert type(vaulted_username).__name__ != "str" + + # Create config with vaulted creds + gateway_config = GatewayConfig( + base_url="https://gateway.example.com", + username=vaulted_username, + password=vaulted_password, + ) + + # Mock subprocess.Popen + with patch("subprocess.Popen") as mock_popen: + mock_popen.return_value.pid = 12345 + + ProcessManager.spawn_manager_process( + script_path=Path("manager_process.py"), + socket_path="/tmp/test.sock", + gateway_config=gateway_config, + ) + + # Verify Popen was called + assert mock_popen.called + + # Get cmd argument + cmd = mock_popen.call_args[0][0] + + # ✅ CRITICAL: Verify arguments are plain str, not Vault objects + for i, arg in enumerate(cmd): + assert isinstance(arg, (str, bytes)) or hasattr(arg, "__fspath__"), \ + f"cmd[{i}] = {arg!r} is not valid for subprocess" + + # Verify credentials converted + username_arg = cmd[5] + password_arg = cmd[6] + + assert isinstance(username_arg, str) and type(username_arg) == str + assert isinstance(password_arg, str) and type(password_arg) == str + assert username_arg == "admin" + assert password_arg == "secret" +``` + +**Checklist:** + +- [ ] Test reproduces bug conditions +- [ ] Test would FAIL before fix +- [ ] Test PASSES after fix +- [ ] Docstring explains bug (Jira reference) +- [ ] Assertions verify fix, not just "didn't crash" + +#### Option 2: Molecule Test + +```yaml +# extensions/molecule/organization_mock/converge.yml + +- name: Test with vaulted credentials (AAP-XXXXX regression) + ansible.platform.organization: + aap_username: "{{ lookup('ansible.builtin.vault', encrypted_username) }}" + aap_password: "{{ lookup('ansible.builtin.vault', encrypted_password) }}" + name: "Test Org" + state: present + register: result + +- name: Verify vault credentials work + assert: + that: + - not result.failed + - result.changed +``` + +#### Option 3: Integration Test + +```yaml +# tests/integration/targets/organizations_test/tasks/main.yml + +- name: Test specific bug scenario (AAP-XXXXX) + ansible.platform.organization: + # ... parameters that trigger bug + register: result + +- assert: + that: + - result is success +``` + +### Code Review: The Fix + +**✅ Good fix pattern:** + +```python +# BEFORE (buggy) +cmd = [ + sys.executable, + gateway_config.username, # ❌ Vault object + gateway_config.password, # ❌ Vault object +] + +# AFTER (fixed) +cmd = [ + sys.executable, + str(gateway_config.username) if gateway_config.username else "", # ✅ Converted + str(gateway_config.password) if gateway_config.password else "", # ✅ Converted +] +``` + +**Checklist:** + +- [ ] Fix addresses root cause (not just symptoms) +- [ ] Fix is minimal (no unrelated refactoring) +- [ ] No commented-out debug code +- [ ] Backwards compatible +- [ ] No breaking changes + +### Check for Similar Bugs + +**Scan codebase for same pattern:** + +```bash +# Example: If bug was missing str() conversion +grep -r "subprocess.Popen" plugins/ | grep -v "str(" + +# Example: If bug was missing null check +grep -r "\.get(" plugins/ | grep -v "if .* is not None" +``` + +**If found:** +```markdown +⚠️ **Note:** Similar pattern found + +Found similar usage in: +- plugins/foo/bar.py:123 + +**Recommendation:** +- Fix in this PR (same root cause) OR +- Create follow-up Jira ticket +``` + +--- + +## CI/Workflow PR Review + +**Use for:** GitHub Actions workflows, CI configuration, test infrastructure + +### Critical Checks for New Workflows + +**First, detect if workflow is NEW:** + +```bash +# List new workflows +git diff origin/devel --name-status .github/workflows/ | grep '^A' +``` + +**If NEW workflow, additional checks:** + +- [ ] **Purpose justified** - Why new vs modifying existing? +- [ ] **Name descriptive** - Clear what it does +- [ ] **No duplication** - Doesn't replicate existing workflow +- [ ] **Documented in PR** - Explanation of purpose +- [ ] **Minimal scope** - Does one thing well +- [ ] **Tested in fork** - REQUIRED before merge + +### Security Review (CRITICAL) + +#### 1. Check for Injection Vulnerabilities + +**❌ DANGEROUS:** + +```yaml +- name: Run command + run: echo "${{ github.event.issue.title }}" # Injection risk! +``` + +**✅ SAFE:** + +```yaml +- name: Run command + env: + TITLE: ${{ github.event.issue.title }} + run: echo "$TITLE" +``` + +#### 2. Secret Protection (CRITICAL) + +**Rule 1: Never expose secrets to fork PRs** + +**❌ DANGEROUS:** + +```yaml +on: pull_request # Runs for ANY fork! +jobs: + build: + steps: + - run: echo "${{ secrets.AAP_PASSWORD }}" # LEAKED to fork! +``` + +**✅ SAFE:** + +```yaml +on: + pull_request: + types: [labeled] + +jobs: + integration: + # Only run after manual approval + if: | + github.event.label.name == 'safe to test' && + github.event.pull_request.author_association == 'MEMBER' + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - env: + AAP_PASSWORD: ${{ secrets.AAP_PASSWORD }} + run: ansible-playbook tests/integration/ +``` + +**Rule 2: Secrets in env vars, not inline** + +**❌ BAD:** + +```yaml +- run: ansible-galaxy publish --token ${{ secrets.GALAXY_TOKEN }} +``` + +**✅ GOOD:** + +```yaml +- env: + GALAXY_TOKEN: ${{ secrets.GALAXY_TOKEN }} + run: ansible-galaxy publish --token "$GALAXY_TOKEN" +``` + +**Secret protection checklist:** + +- [ ] No secrets on `pull_request` trigger +- [ ] Label gate (`safe to test`) for secret access +- [ ] Author association check (`MEMBER`, `OWNER`) +- [ ] Secrets in env vars, not inline +- [ ] No secret logging +- [ ] No secrets in artifacts +- [ ] Secrets masked in workflow runs + +**Verification commands:** + +```bash +# CRITICAL: Find workflows leaking secrets to fork PRs +for f in .github/workflows/*.yml; do + if grep -q "on: pull_request" "$f" && grep -q "secrets\." "$f"; then + echo "❌ DANGER: $f exposes secrets to fork PRs!" + fi +done + +# Find safe to test label gates +grep -l "safe to test" .github/workflows/*.yml + +# Find author association checks +grep -l "author_association" .github/workflows/*.yml + +# Find unsafe inline secret usage +grep -n '\${{ secrets\.' .github/workflows/*.yml | grep -v 'env:' +``` + +#### 3. Permissions Review + +**✅ GOOD:** + +```yaml +permissions: + contents: read + pull-requests: write # Only if needed +``` + +**❌ BAD:** + +```yaml +permissions: write-all # Never use! +``` + +**Checklist:** + +- [ ] Permissions follow least-privilege +- [ ] Only grants what's needed +- [ ] No `write-all` permission + +#### 4. Trigger Conditions + +**✅ GOOD:** + +```yaml +on: + pull_request: + types: [opened, synchronize, reopened] + paths: + - 'plugins/**' + - 'tests/**' + push: + branches: + - devel + - stable-* +``` + +**❌ BAD:** + +```yaml +on: [push, pull_request] # Too broad, wastes CI +``` + +### Fork Testing Requirement + +**For new workflows, MUST test in fork:** + +```markdown +**Testing request:** + +Please test this workflow in your fork: + +1. Push this branch to your fork +2. Create PR in your fork +3. Verify workflow runs as expected +4. Share workflow run URL +``` + +### Quick Reference: Review Commands + +```bash +# Check for new workflows +git diff origin/devel --name-status .github/workflows/ | grep '^A' + +# Validate YAML syntax +yamllint .github/workflows/.yml + +# Check GitHub Actions syntax +gh workflow view --repo ansible/ansible.platform + +# Find workflows with write-all (DANGEROUS) +grep -n "write-all" .github/workflows/*.yml + +# List all permission declarations +grep -A 5 "permissions:" .github/workflows/*.yml + +# Find all pull_request triggers +grep -n "on: pull_request" .github/workflows/*.yml + +# Find pull_request_target (verify safety) +grep -n "pull_request_target" .github/workflows/*.yml +``` + +--- + +## Connection/Manager PR Review (CRITICAL) + +**⚠️ Use when PR changes core infrastructure files:** + +``` +plugins/connection/http.py +plugins/plugin_utils/manager/* +plugins/plugin_utils/platform/{base_client,direct_client,config,registry}.py +``` + +**Why critical:** These changes affect **ALL modules** in the collection. + +### Detection + +```bash +# Auto-detect connection/manager changes +git diff origin/devel --name-only | grep -E \ + 'plugins/connection/|plugins/plugin_utils/manager/|plugins/plugin_utils/platform/(base_client|direct_client|config|registry)' +``` + +**If ANY match → Extra scrutiny required** + +### Critical Checks + +#### 1. Backwards Compatibility (MUST TEST) + +**Test with MULTIPLE modules (minimum 3):** + +```yaml +# Test organization, team, AND user modules +- name: Test organization + ansible.platform.organization: + name: "Test Org" + register: org_result + +- name: Test team + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + register: team_result + +- name: Test user + ansible.platform.user: + username: "testuser" + register: user_result + +- assert: + that: + - org_result is success + - team_result is success + - user_result is success +``` + +**Why:** Ensures change doesn't break specific module patterns. + +**Checklist:** + +- [ ] No breaking changes to RPC protocol +- [ ] All authentication methods work (username/password, token, OAuth) +- [ ] Existing modules still work (test 3+ different modules) +- [ ] Both connection modes work (persistent, direct) + +#### 2. Fork Safety (macOS + Python 3.12) + +**Python 3.12 on macOS changed fork behavior - HTTP sessions break.** + +**❌ WRONG:** + +```python +# Session created BEFORE fork +session = requests.Session() + +subprocess.Popen(...) # Fork happens +# Session is now broken on macOS! +``` + +**✅ CORRECT:** + +```python +# Session created AFTER fork in subprocess +def run_in_subprocess(): + session = requests.Session() # Created in subprocess +``` + +**Checklist:** + +- [ ] No HTTP sessions created before subprocess spawn +- [ ] No shared state between parent and subprocess +- [ ] Manager process creates own HTTP session +- [ ] Tested on macOS + Python 3.12 (if possible) + +#### 3. Connection Mode Testing + +**Both modes MUST work:** + +```bash +# Test persistent mode (default) +ansible-playbook tests/integration/test.yml + +# Test direct mode (fallback) +AAP_CONNECTION_MODE=direct ansible-playbook tests/integration/test.yml +``` + +**Checklist:** + +- [ ] Persistent mode works +- [ ] Direct mode works +- [ ] Fallback logic intact +- [ ] Mode detection unchanged + +#### 4. Subprocess Spawning Security + +**CRITICAL: Vault credentials must be converted to str()** + +**❌ BUG:** + +```python +cmd = [ + sys.executable, + gateway_config.password, # TypeError if vaulted! +] +``` + +**✅ FIX:** + +```python +cmd = [ + sys.executable, + str(gateway_config.password) if gateway_config.password else "", +] +``` + +**Checklist:** + +- [ ] All cmd arguments are str/bytes/Path +- [ ] Vault credentials → str() +- [ ] None values handled +- [ ] No shell=True (injection risk) +- [ ] Process cleanup on error + +#### 5. Socket Management + +**Checklist:** + +- [ ] Socket path unique per manager (identifier) +- [ ] Socket removed on cleanup +- [ ] Socket permissions secure (0600) +- [ ] Socket directory exists and writable +- [ ] Stale socket detection +- [ ] Socket path < 104 chars (Unix limit) + +#### 6. Security Review + +**❌ DANGEROUS:** + +```python +logger.debug(f"Auth with {username}:{password}") # LEAKED! +``` + +**✅ SAFE:** + +```python +logger.debug(f"Auth with {username}:***") +``` + +**Checklist:** + +- [ ] No credentials in logs/errors +- [ ] No credentials in subprocess args (visible in `ps`) +- [ ] Credentials converted from vault +- [ ] Socket permissions correct +- [ ] No shell=True + +#### 7. Multi-Service Support + +**Verify ALL services work:** + +```python +# Gateway +path="/api/gateway/v1/teams/" + +# Controller +path="/api/controller/v2/job_templates/" + +# EDA +path="/api/eda/v1/projects/" + +# Hub +path="/api/hub/v3/namespaces/" +``` + +### Testing Requirements (MANDATORY) + +#### Unit Tests + +```python +def test_backwards_compatibility(): + """Ensure existing behavior still works.""" + +def test_new_functionality(): + """Test the change.""" + +def test_vault_credentials(): + """Test vault string conversion.""" +``` + +#### Integration Tests (Multiple Modules!) + +**MUST test 3+ modules:** + +```bash +pytest tests/integration/targets/organizations_test/ -v +pytest tests/integration/targets/teams_test/ -v +pytest tests/integration/targets/users_test/ -v +``` + +#### Connection Mode Testing + +```bash +# Both must work +ansible-playbook test.yml +AAP_CONNECTION_MODE=direct ansible-playbook test.yml +``` + +#### Vault Testing + +```yaml +# group_vars/all.yml +aap_username: !vault | + $ANSIBLE_VAULT;1.1;AES256 + ... + +# Must work with vaulted creds +``` + +--- + +## Architecture Principles + +### Three-Tier Data Model + +``` +Playbook Parameters (user input) + ↓ +Ansible Model (stable, user-facing, string names) + ↓ +Transform Mixin (name→ID resolution, business logic) + ↓ +API Model (version-specific, wire format, integer IDs) + ↓ +HTTP Request to AAP Gateway +``` + +**Purpose:** + +- **Ansible Model** never changes → User-facing stability +- **API Model** changes with API versions → Isolated in `api/v1/`, `api/v2/` +- **Transform Mixin** adapts between stable and version-specific + +**Example:** + +```python +# User writes (Ansible Model) +- ansible.platform.team: + organization: "Red Hat" # String name + +# Transform Mixin resolves +org_id = manager.lookup_resource_id("organization", "Red Hat") +# Returns: 42 + +# API Model (wire format) +{"organization": 42} # Integer ID + +# HTTP Request +POST /api/gateway/v1/teams/ +{"name": "Engineering", "organization": 42} +``` + +### Endpoint Path Versioning + +**CRITICAL:** Folder versioning ≠ Service API versioning + +**The `api/v1/`, `api/v2/` folders = collection transform versions** + +**Example:** + +```python +# File: plugins/plugin_utils/api/v1/organization.py +# This is collection transform v1, NOT service API v1! + +# Can contain transforms for: +# - Gateway API v1: /api/gateway/v1/organizations/ +# - Controller API v2: /api/controller/v2/organizations/ ← Controller v2 in api/v1/ folder! +``` + +**Endpoint paths declared in mixin:** + +```python +def get_endpoint_operations(self): + return EndpointOperation( + path="/api/controller/v2/organizations/", # Full path + lookup_field="name", + ) +``` + +### Service Prefix Table + +| Service | Prefix | Example Endpoint | +|---------|--------|-----------------| +| Gateway | `/api/gateway/v1/` | `/api/gateway/v1/teams/` | +| Controller | `/api/controller/v2/` | `/api/controller/v2/job_templates/` | +| EDA | `/api/eda/v1/` | `/api/eda/v1/projects/` | +| Hub | `/api/hub/v3/` | `/api/hub/v3/namespaces/` | + +**Mismatched prefix = 404 errors!** + +### Single Gateway Authentication + +**One set of credentials for ALL services:** + +```yaml +# Works for Gateway, Controller, EDA, Hub +aap_hostname: "{{ gateway_url }}" +aap_username: "{{ username }}" +aap_password: "{{ password }}" +``` + +**Gateway routes internally - no separate credentials needed.** + +--- + +## Code Quality Standards + +### Idempotency + +**Requirement:** Second run with same parameters returns `changed: false` + +**Test pattern:** + +```yaml +# Run 1: Should change +- ansible.platform.team: + name: "Engineering" + state: present + register: result1 + +# Run 2: Should NOT change +- ansible.platform.team: + name: "Engineering" + state: present + register: result2 + +- assert: + that: + - result1.changed + - not result2.changed # Idempotent! +``` + +### Error Handling + +**✅ GOOD:** + +```python +try: + org_id = manager.lookup_resource_id("organization", org_name) +except ResourceNotFound: + module.fail_json( + msg=f"Organization '{org_name}' not found. " + f"Create it first or check spelling." + ) +``` + +**❌ BAD:** + +```python +# No error handling, crashes with unclear error +org_id = manager.lookup_resource_id("organization", org_name) +``` + +### Security + +**Sensitive parameters:** + +```python +module = AnsibleModule( + argument_spec=dict( + password=dict(type='str', no_log=True), # ✅ Masked + token=dict(type='str', no_log=True), + api_key=dict(type='str', no_log=True), + ) +) +``` + +**No credentials in logs:** + +```python +# ✅ SAFE +self._display.vvvv(f"Password: {password}") # Only with -vvvv + +# ❌ DANGEROUS +logger.debug(f"Password: {password}") # Logged! +``` + +--- + +## Testing Requirements + +### Test Pyramid + +``` + Integration (optional initially) + /\ + / \ + Molecule (recommended) + / \ + Unit (required) +``` + +### Unit Tests (Required) + +**What to test:** + +- Transform logic +- Name→ID resolution +- Optional field handling +- Endpoint path correctness + +**Run:** + +```bash +pytest tests/unit/ -v +pytest tests/unit/plugins/plugin_utils/api/v1/test_team.py -v +``` + +### Molecule Tests (Recommended) + +**Scenarios:** + +1. Create (state: present) +2. Idempotency (second run) +3. Update (change field) +4. Delete (state: absent) + +**Run:** + +```bash +molecule test -s team_mock +``` + +### Integration Tests (Can Defer) + +**Can add in follow-up PR with Jira ticket** + +**Run:** + +```bash +# Requires live AAP instance + safe to test label +pytest tests/integration/targets/teams_test/ -v +``` + +--- + +## Common Issues and Fixes + +### 1. Collection Completeness Test Failure + +**Error:** + +``` +The following items should be added to meta/runtime.yml action-groups.gateway: + ad_hoc_command +``` + +**Fix:** + +```yaml +# meta/runtime.yml +action_groups: + gateway: + - ad_hoc_command +``` + +### 2. Wrong Service Prefix (404 Errors) + +**Symptom:** Integration tests fail with 404 + +**Cause:** + +```python +# ❌ EDA resource with Gateway prefix +path="/api/gateway/v1/projects/" +``` + +**Fix:** + +```python +# ✅ Correct EDA prefix +path="/api/eda/v1/projects/" +``` + +### 3. Vault Credentials Not Converted + +**Error:** + +``` +TypeError: expected str, bytes or os.PathLike object, not AnsibleVaultEncryptedUnicode +``` + +**Fix:** + +```python +# Convert to string +str(gateway_config.password) if gateway_config.password else "" +``` + +### 4. Idempotency Broken + +**Symptom:** Second run always shows `changed: true` + +**Cause:** Field missing in `from_api()` reverse transform + +**Fix:** + +```python +@classmethod +def from_api(cls, api_data, context): + return AnsibleFoo( + opa_query_path=api_data.get("opa_query_path"), # ← Add this! + ) +``` + +### 5. Missing Changelog Fragment + +**Error:** PR checks fail with "No changelog fragment found" + +**Fix:** + +```bash +# Create fragment +cat > changelogs/fragments/123-add-foo.yml <_mock + +# Check collection completeness +python tests/test_completeness.py +``` + +2. **Add changelog fragment** (if code changes) + +3. **Update meta/runtime.yml** (if new module) + +4. **Include Jira reference** (if bugfix) + +5. **Test in fork** (if workflow changes) + +### During Review + +1. **Respond promptly** (within 3-5 business days) + +2. **Mark conversations resolved** after addressing + +3. **Re-request review** after pushing fixes + +4. **Don't force-push** after review (breaks diff viewing) + +### Common Delays + +| Issue | Typical Delay | Prevention | +|-------|--------------|------------| +| Missing changelog | 2-3 days | Add before submitting | +| Missing meta/runtime.yml | 1 day | Run completeness test locally | +| CI failures | 1-3 days | Run tests locally first | +| Unresolved comments | 3-7 days | Respond within 3 days | +| Missing Jira reference | 1 day | Add to title before submitting | + +--- + +## Review Checklist Summary + +### All PRs + +- [ ] Pre-merge CI passing (completeness, unit, sanity) +- [ ] Changelog fragment present (if code changes) +- [ ] Jira reference in title (if bugfix) +- [ ] Documentation complete +- [ ] No security issues + +### Feature PRs + +- [ ] Seven-file pattern complete +- [ ] Ansible Model uses string names +- [ ] API Model uses integer IDs +- [ ] Transform mixin correct +- [ ] Endpoint prefix correct +- [ ] meta/runtime.yml updated +- [ ] Test coverage adequate + +### Bugfix PRs + +- [ ] Jira referenced +- [ ] Bug description clear +- [ ] Regression test added +- [ ] Root cause addressed +- [ ] Backwards compatible + +### CI/Workflow PRs + +- [ ] New workflow justified +- [ ] Security reviewed +- [ ] Secret protection (if secrets) +- [ ] Tested in fork +- [ ] Minimal scope + +### Connection/Manager PRs + +- [ ] Tested with 3+ modules +- [ ] Both connection modes work +- [ ] Fork-safe +- [ ] Backwards compatible +- [ ] Vault credentials handled +- [ ] No credential leaks + +--- + +## Contact + +**Questions about PR review?** + +- Slack: `#aap-platform-collection` (Red Hat internal) +- GitHub: Comment on PR or open discussion +- Documentation: `docs/` folder + +**Report CI issues:** + +- GitHub Issues: `ansible/ansible.platform` +- Include: PR number, CI run ID, error logs + +--- + +## Collection-Specific Expectations + +### API/Completeness Parity + +**Requirement:** New modules should provide comprehensive AAP API coverage. + +**When adding a new module:** + +1. **Check API capabilities:** + - Does the API support all CRUD operations? + - Are there fields we can't expose (write-only, deprecated)? + - Are there multi-endpoint patterns needed? + +2. **Document limitations:** + ```python + # In module DOCUMENTATION + notes: + - This module requires AAP 2.5 or later + - The opa_query_path field is read-only via Controller API + - Organization creation requires Gateway API (AAP 2.5+) + ``` + +3. **API version requirements:** + ```python + # In transform mixin + def get_endpoint_operations(self): + # Document minimum AAP version + # AAP 2.5+ required for /api/gateway/v1/ + return EndpointOperation(path="/api/gateway/v1/organizations/") + ``` + +**Completeness expectations:** + +| Coverage | Acceptable? | Notes | +|----------|------------|-------| +| 100% field parity | ✅ Ideal | All API fields exposed | +| 90-99% field parity | ✅ Acceptable | Document missing fields | +| 80-89% field parity | ⚠️ Review needed | Justify missing fields | +| <80% field parity | ❌ Incomplete | Add more fields or explain | + +**Fields that can be skipped:** + +- Internal API fields (`related`, `summary_fields`) +- Deprecated fields (document in module notes) +- Write-only fields not useful for Ansible (explain why) + +### Stable vs Devel/Backport Considerations + +**Branch strategy:** + +``` +devel (main development) + ↓ +stable-2.7 (AAP 2.7 compatible) + ↓ +stable-2.6 (AAP 2.6 compatible) +``` + +#### Rules for Devel Branch + +**New features:** +- ✅ Go to `devel` first +- Must be AAP version-compatible (document minimum version) +- Can use latest API features + +**Breaking changes:** +- ⚠️ Allowed in devel (major version bumps) +- Must document migration path +- Add to `breaking_changes:` in changelog + +**Example:** +```yaml +# changelogs/fragments/130-breaking-org-field.yml +breaking_changes: + - "organization module - removed deprecated max_hosts field. Use organization_settings module instead (ansible/ansible.platform#130)." +``` + +#### Rules for Stable Branch Backports + +**What can be backported:** + +- ✅ **Bugfixes** - Always backport critical bugs +- ✅ **Security fixes** - Always backport +- ✅ **Minor enhancements** - If backwards compatible +- ❌ **New modules** - Stay in devel +- ❌ **Breaking changes** - Never backport +- ❌ **New required fields** - Breaking, stay in devel + +**Backport process:** + +1. Merge to `devel` first +2. Create backport PR to `stable-2.X` +3. Label PR with `backport:stable-2.X` +4. Changelog fragment goes in both branches + +**Example backport PR title:** +``` +[Backport stable-2.7] Fix vault credential handling (AAP-12345) +``` + +**Backport changelog:** +```yaml +# In devel AND stable-2.7 +bugfixes: + - "Fix vault credential handling in manager subprocess (ansible/ansible.platform#124)." +``` + +#### Version Compatibility Matrix + +| AAP Version | Collection Branch | Python | Ansible Core | +|-------------|------------------|--------|--------------| +| AAP 2.7 | stable-2.7, devel | 3.9+ | 2.15+ | +| AAP 2.6 | stable-2.6 | 3.9+ | 2.14+ | +| AAP 2.5 | stable-2.5 | 3.9+ | 2.14+ | + +**Version-specific features:** + +```python +# In module DOCUMENTATION +requirements: + - ansible.platform >= 2.7.0 # For new gateway features + - AAP >= 2.5.0 # For gateway API support +``` + +### Changelog/CasC Expectations + +#### Changelog Fragment Requirements + +**When required:** + +- ✅ New module → `minor_changes:` +- ✅ New feature → `minor_changes:` +- ✅ Bugfix → `bugfixes:` +- ✅ Breaking change → `breaking_changes:` +- ✅ Deprecation → `deprecated_features:` +- ❌ Docs-only → No changelog +- ❌ CI-only (internal) → No changelog +- ⚠️ Test changes → Only if user-visible + +**Fragment naming:** + +```bash +# Format: -.yml +changelogs/fragments/123-add-foo-module.yml +changelogs/fragments/124-fix-vault-creds.yml +changelogs/fragments/125-deprecate-bar.yml +``` + +**Quality standards:** + +```yaml +# ✅ GOOD: Clear, user-facing, past tense +minor_changes: + - "Added foo module for managing Foo resources in AAP (ansible/ansible.platform#123)." + +# ❌ BAD: Technical jargon, present tense +minor_changes: + - "Implements FooTransformMixin_v1 for foo resource endpoint mapping" + +# ✅ GOOD: Actionable bugfix description +bugfixes: + - "Fixed subprocess spawn failure when aap_username or aap_password are vaulted (ansible/ansible.platform#124)." + +# ❌ BAD: Vague, no context +bugfixes: + - "Fixed bug in manager process" +``` + +#### CasC (Configuration as Code) Team Notification + +**When to notify CasC team:** + +The CasC team maintains `casc_instance` facts and role integration. Notify them when PR affects: + +1. **Return value structure changes:** + ```python + # BEFORE + return {"id": 1, "name": "foo"} + + # AFTER (BREAKING - notify CasC!) + return {"resource": {"id": 1, "name": "foo"}} + ``` + +2. **New modules:** + - CasC may need to add support + - Notify so they can plan integration + +3. **Authentication parameter changes:** + - New auth methods + - Changed parameter names + - OAuth flow changes + +4. **State behavior changes:** + - New states added (`enforced`, `exists`) + - State semantics changed + +**How to notify:** + +```bash +# Option 1: GitHub comment +gh pr comment --repo ansible/ansible.platform \ + --body "@ansible/casc-team FYI - New module added: foo. May need casc_instance fact support." + +# Option 2: Add label +gh pr edit --repo ansible/ansible.platform \ + --add-label "casc-review-needed" + +# Option 3: Slack (if urgent) +# Post in #aap-casc channel +``` + +**CasC review checklist:** + +- [ ] Return values documented in RETURN block +- [ ] New modules added to CasC module list +- [ ] Auth changes documented +- [ ] Breaking changes have migration guide + +### CI/Test Coverage Expectations + +#### Test Coverage Requirements + +**Minimum coverage by PR type:** + +| PR Type | Unit | Molecule | Integration | +|---------|------|----------|-------------| +| New module | ✅ Required | ✅ Required | ⚠️ Can defer | +| New feature | ✅ Required | ✅ Recommended | ⚠️ Can defer | +| Bugfix | ✅ Required | ⚠️ If applicable | ⚠️ If applicable | +| Refactor | ✅ Required | ✅ Required | ❌ Not needed | +| Docs only | ❌ Not needed | ❌ Not needed | ❌ Not needed | +| CI/workflow | ⚠️ If testing code | ❌ Not needed | ❌ Not needed | + +#### Unit Test Coverage Targets + +**Target:** 80%+ coverage for new code + +**Check coverage:** + +```bash +# Run with coverage +pytest tests/unit/ --cov=plugins/plugin_utils --cov-report=html + +# View report +open htmlcov/index.html +``` + +**Coverage expectations:** + +- **Transform mixins:** 90%+ coverage (critical path) +- **Utilities:** 80%+ coverage +- **Action plugins:** 70%+ coverage (harder to test) + +**Coverage exemptions:** + +- Exception handling for "impossible" cases +- Defensive code for future compatibility +- Debug logging statements + +#### Molecule Test Scenarios + +**Required scenarios for new modules:** + +1. ✅ **Create** - `state: present` +2. ✅ **Idempotency** - Run twice, second returns `changed: false` +3. ✅ **Update** - Modify field +4. ✅ **Delete** - `state: absent` + +**Optional but recommended:** + +5. ⚠️ **Exists** - `state: exists` check +6. ⚠️ **Enforced** - `state: enforced` if supported +7. ⚠️ **Error handling** - Invalid parameters + +**Molecule test quality:** + +```yaml +# ✅ GOOD: Comprehensive assertions +- name: Create team + ansible.platform.team: + name: "Test Team" + organization: "Test Org" + state: present + register: result + +- name: Verify all fields + assert: + that: + - result.changed + - result.id is defined + - result.name == "Test Team" + - result.organization is defined + +# ❌ BAD: Minimal assertions +- assert: + that: + - result.changed # Not checking actual result! +``` + +#### Integration Test Deferral Policy + +**Can defer integration tests if:** + +1. ✅ Unit tests cover transform logic +2. ✅ Molecule tests cover all scenarios +3. ✅ Follow-up Jira ticket created +4. ✅ Documented in PR comments + +**Cannot defer if:** + +1. ❌ Bug found in production (regression test needed) +2. ❌ Multi-endpoint pattern (needs live API testing) +3. ❌ Complex state machine (molecule mock insufficient) + +**Deferral template:** + +```markdown +## Integration Test Deferral + +**Reason:** Comprehensive unit and molecule coverage + +**Follow-up:** Created AAP-XXXXX to track integration tests + +**Coverage:** +- ✅ Unit tests: 95% coverage of transform logic +- ✅ Molecule tests: All CRUD scenarios covered +- ⏳ Integration tests: Deferred to AAP-XXXXX +``` + +#### CI Pipeline Stages + +**Pre-merge (runs on all PRs):** + +1. **Sanity** - Documentation, imports, PEP8 +2. **Unit** - pytest tests +3. **Completeness** - meta/runtime.yml check +4. **Linting** - black, isort, flake8 + +**Post-label (triggered by `safe to test`):** + +5. **Molecule** - Mock server tests +6. **Integration** - Live AAP tests + +**Expected run times:** + +- Pre-merge: ~5-10 minutes +- Molecule: ~10-15 minutes +- Integration: ~15-30 minutes +- **Total: ~30-55 minutes** + +#### CI Failure Investigation + +**When CI fails:** + +1. **Check logs:** + ```bash + gh run view --repo ansible/ansible.platform --log + ``` + +2. **Categorize failure:** + - ❌ Real failure → Fix required + - ⚠️ Transient failure → Re-run + - ⚠️ Infrastructure issue → Report to maintainers + +3. **Common transient failures:** + - AAP instance connectivity + - Container pull failures + - Network timeouts + +4. **Re-run transient failures:** + ```bash + gh run rerun --repo ansible/ansible.platform --failed + ``` + +**Failure ownership:** + +| Failure Type | Owner | Action | +|-------------|-------|---------| +| Unit test | PR author | Fix code | +| Sanity | PR author | Fix format/docs | +| Completeness | PR author | Update meta/runtime.yml | +| Integration (real) | PR author | Fix code | +| Integration (transient) | Anyone | Re-run | +| CI infrastructure | Maintainers | Report issue | + +--- + +## Related Documentation + +- [docs/07-adding-resources.md](07-adding-resources.md) - Seven-file pattern +- [docs/04-data-model-transformation.md](04-data-model-transformation.md) - Three-tier architecture +- [docs/03-sdk-architecture.md](03-sdk-architecture.md) - Manager subprocess +- [CONTRIBUTING.md](../CONTRIBUTING.md) - Contributor guide +- `.claude/skills/pr-review/` - Automated review skill (maintainers) + +--- + +**Last Updated:** 2026-09-11 diff --git a/pyproject.toml b/pyproject.toml index 5f441b31..62dce078 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,6 +14,8 @@ exclude = [ "services", "aap_gateway_api/migrations", "django-ansible-base", + ".claude", + "docs", ] [tool.ruff.lint]