|
| 1 | +# GitHub Copilot Code Review Instructions |
| 2 | + |
| 3 | +## Review Philosophy |
| 4 | + |
| 5 | +- Only comment when you have HIGH CONFIDENCE (>80%) that an issue exists |
| 6 | +- Be concise |
| 7 | +- Focus on actionable feedback, not observations |
| 8 | +- If you're uncertain, stay silent—false positives reduce trust |
| 9 | + |
| 10 | +## Project Context |
| 11 | + |
| 12 | +Kubernetes Operator for PostgreSQL (Operator SDK, controller-runtime). Go + YAML. Key paths: `internal/`, `percona/`, `pkg/apis` and `e2e-tests/`. |
| 13 | + |
| 14 | +## Priority Areas |
| 15 | + |
| 16 | +### Security |
| 17 | + |
| 18 | +- Hardcoded secrets, credentials, or API keys |
| 19 | +- SQL injection—use parameterized queries, never string concatenation |
| 20 | +- Missing or overly broad RBAC (`+kubebuilder:rbac` on reconcile functions) |
| 21 | +- Logging of secrets or sensitive data |
| 22 | +- Unvalidated user input before DB operations |
| 23 | + |
| 24 | +### Correctness |
| 25 | + |
| 26 | +- Logic errors that could cause panics or incorrect behavior |
| 27 | +- Race conditions, resource leaks (files, connections, memory) |
| 28 | +- Incorrect or missing error propagation |
| 29 | +- Error wrapping that doesn't add useful context |
| 30 | +- Redundant comments that restate what the code shows |
| 31 | + |
| 32 | +### Imports and Dependencies |
| 33 | + |
| 34 | +- Use standard import aliases: `corev1`, `appsv1`, `metav1`, `apierrors`, etc. (per `.golangci.yaml`) |
| 35 | +- Import order: standard, default, `github.com/percona` prefix |
| 36 | + |
| 37 | +### Controller / Reconcile Logic |
| 38 | + |
| 39 | +- Add `+kubebuilder:rbac` above reconcile functions that create/update K8s resources |
| 40 | +- Set controller/owner references for owned resources |
| 41 | +- Idempotent reconcile; handle `apierrors.IsConflict` with requeue |
| 42 | + |
| 43 | +### Logging |
| 44 | + |
| 45 | +- Prefer `logging.FromContext(ctx)` for loggers |
| 46 | +- Use structured fields: `log.Info("message", "key", value)` |
| 47 | +- Add logging for important operator actions (reconcile steps, errors, retries) |
| 48 | + |
| 49 | +### Testing |
| 50 | + |
| 51 | +- New features: expect unit tests and/or E2E (KUTTL) where appropriate |
| 52 | +- Unit tests should use `assert` and `require` from `github.com/stretchr/testify` wherever applicable |
| 53 | +- Utilize table driven tests when possible |
| 54 | +- Test names should describe the scenario |
| 55 | + |
| 56 | +## Response Format |
| 57 | + |
| 58 | +When you identify an issue: |
| 59 | + |
| 60 | +1. **Problem** (1 sentence) |
| 61 | +2. **Why it matters** (1 sentence, only if not obvious) |
| 62 | +3. **Fix** (concrete suggestion or code snippet) |
| 63 | + |
| 64 | +Example: |
| 65 | +``` |
| 66 | +1. **Problem**: This map access can panic if the map is nil. |
| 67 | +2. **Why it matters**: A panic can crash the operator and disrupt reconciliation. |
| 68 | +3. **Fix**: Initialize the map before use, e.g. `m := make(map[string]string)` before assigning or reading. |
| 69 | +``` |
| 70 | + |
| 71 | +## When to Stay Silent |
| 72 | + |
| 73 | +- You're uncertain whether something is an issue |
| 74 | +- The concern is stylistic and the code is acceptable |
| 75 | +- The fix would be a matter of preference, not correctness or security |
0 commit comments