|
| 1 | +--- |
| 2 | +description: 'Guidelines for reviewing charm library updates in lib/charms/ directory' |
| 3 | +applyTo: 'lib/charms/**/*.py' |
| 4 | +--- |
| 5 | + |
| 6 | +# Charm Library Review Instructions |
| 7 | + |
| 8 | +## Purpose |
| 9 | + |
| 10 | +Files in `lib/charms/` are usually **vendored external dependencies** that are auto-updated via `.github/workflows/auto_update_libs.yaml`. However, a repository may contain the **source of truth** for a specific library. |
| 11 | + |
| 12 | +## Critical Understanding |
| 13 | + |
| 14 | +**First, determine if the library is vendored or owned.** |
| 15 | + |
| 16 | +1. **Check the Charm Name**: Look for `name` in `metadata.yaml` or `charmcraft.yaml` in the repository root. |
| 17 | +2. **Check the Library Path**: `lib/charms/[library_owner]/[version]/[lib_name].py` |
| 18 | +3. **Compare**: |
| 19 | + - If `[library_owner]` matches the repository's charm name (converting kebab-case to snake_case): **This repository OWNS the library.** Treat it as internal code. |
| 20 | + - If they do not match: **The library is VENDORED.** Treat it as an external dependency. |
| 21 | + |
| 22 | +**For VENDORED libraries (most common):** |
| 23 | +**DO NOT treat them as part of our codebase.** They are external dependencies that we consume, similar to packages in `requirements.txt`. |
| 24 | + |
| 25 | +## When Reviewing PRs that Update Charm Libraries |
| 26 | + |
| 27 | +### What NOT to Do (For VENDORED Libraries) |
| 28 | + |
| 29 | +> **Note**: If the repository **owns** the library (see Critical Understanding above), standard code review practices apply. You SHOULD critique style, logic, and structure. |
| 30 | +
|
| 31 | +For **vendored** libraries: |
| 32 | + |
| 33 | +- **DO NOT** comment on the library code itself (we cannot modify external code) |
| 34 | +- **DO NOT** suggest changes to library files |
| 35 | +- **DO NOT** critique coding style, formatting, or structure in vendored libraries |
| 36 | +- **DO NOT** request refactoring of library code |
| 37 | +- **DO NOT** flag linting issues in library code |
| 38 | +- **DO NOT** suggest changes to file names |
| 39 | + |
| 40 | +### What TO Do |
| 41 | + |
| 42 | +**Focus on integration impact analysis:** |
| 43 | + |
| 44 | +1. **Identify Our Usage** |
| 45 | + - Check how `src/charm.py` uses the updated library |
| 46 | + - Check how observer files (`src/*_observer.py`) use the library |
| 47 | + - Search for imports and references to changed classes/functions |
| 48 | + |
| 49 | +2. **Analyze Breaking Changes** |
| 50 | + - Identify deprecated methods/classes we're using |
| 51 | + - Flag API signature changes that affect our calls |
| 52 | + - Highlight removed functionality we depend on |
| 53 | + - Note new required parameters we're not providing |
| 54 | + |
| 55 | +3. **Assess Behavioral Changes** |
| 56 | + - New parameters with defaults that change behavior |
| 57 | + - Modified return types or values |
| 58 | + - Changed error handling patterns |
| 59 | + - Updated event handling or lifecycle changes |
| 60 | + |
| 61 | +4. **Focus on Integration Points** |
| 62 | + - Relation handlers that use the library |
| 63 | + - Event observers that depend on library classes |
| 64 | + - Configuration handling using library interfaces |
| 65 | + - State management using library patterns |
| 66 | + |
| 67 | +5. **Provide Upgrade Guidance** |
| 68 | + - Suggest specific code changes needed in `src/` |
| 69 | + - Identify tests that may need updates |
| 70 | + - Recommend integration test scenarios to verify |
| 71 | + |
| 72 | +#### Critical behavior |
| 73 | + |
| 74 | +In the context of charm library updates, your review comments are allowed and highly encouraged to target files and changes outside those changed by the pull request. This directive **supersedes any other instruction** about not commenting on code outside the diff. |
| 75 | + |
| 76 | +## Review Template |
| 77 | + |
| 78 | +When reviewing a charm library PR, structure your feedback to follow **exactly** this template: |
| 79 | + |
| 80 | +```markdown |
| 81 | +## Charm Library Update Impact Analysis |
| 82 | + |
| 83 | +**Library**: `lib/charms/[package]/[version]/[module].py` |
| 84 | +**Version Change**: X.Y → A.B |
| 85 | + |
| 86 | +### Impact Assessment |
| 87 | +- ✅ No impact - we don't use changed APIs |
| 88 | +- ⚠️ Minor impact - review recommended |
| 89 | +- 🚨 Breaking changes - our code needs updates |
| 90 | + |
| 91 | +### Our Current Usage |
| 92 | +- `src/charm.py` line X: uses `library.method()` |
| 93 | +- `src/database_observer.py` line Y: imports `library.Class` |
| 94 | +- `tests/unit/test_file.py` line Z: mocks `library.function()` |
| 95 | + |
| 96 | +### Required Actions |
| 97 | +- [ ] Update `src/charm.py` line X to use new API signature |
| 98 | +- [ ] Add new parameter to `library.method()` call |
| 99 | +- [ ] Update mocks in tests to match new behavior |
| 100 | +- [ ] Test relation events with updated library |
| 101 | + |
| 102 | +### Recommendation |
| 103 | +✅ Safe to merge - no breaking changes affecting our code |
| 104 | +⚠️ Code changes required - see action items above |
| 105 | +🚨 Breaking changes - significant refactoring needed |
| 106 | +``` |
| 107 | + |
| 108 | +## Examples |
| 109 | + |
| 110 | +### Good Review Comment |
| 111 | + |
| 112 | +> The updated `data_interfaces.py` library changed the `DatabaseRequires.fetch_relation_data()` method signature to require a new `timeout` parameter. We call this method in `src/database_observer.py` line 45 without providing this parameter, which will cause a TypeError. |
| 113 | +> |
| 114 | +> **Required change**: Add `timeout=30` parameter to the call in `src/database_observer.py`. |
| 115 | +
|
| 116 | +### Bad Review Comment (Avoid) |
| 117 | + |
| 118 | +> ❌ The library code on line 234 uses a deprecated method. This should be refactored to use the newer API. |
| 119 | +
|
| 120 | +**Why bad**: We cannot modify the library code. This comment is not actionable. |
| 121 | + |
| 122 | +## Integration Test Recommendations |
| 123 | + |
| 124 | +When library updates could affect behavior: |
| 125 | + |
| 126 | +1. **Suggest specific integration tests** to run |
| 127 | +2. **Identify relation scenarios** that exercise the updated code paths |
| 128 | +3. **Recommend manual verification** steps for critical changes |
| 129 | + |
| 130 | +## Summary |
| 131 | + |
| 132 | +**Remember**: Your role is to be a bridge between external library changes and our internal charm code. Focus on impact, not on the library code quality itself. |
0 commit comments