ankn
a5f8fb49ab
Bahn: aisupport, Analyse-O2C-C2S, awesome-bahn-mcp-servers, beam-mcp,
Confluence_Bot, db-planet-mcp-server, O2C-Harness, project-audit,
Projekt-KIQ-HP, teamlandkarte-mcp
Dhive: Jury-Voting
Privat: CV, NoteGraph (NOTE: NoteGraph needs complete redo after consolidation)
Shared: AI-Orchestrator, OrgMyLife, power_skills_and_more
Shared/references: symphony (read-only)
Bahn repos remain available as independent remotes - this monorepo
pulls them in via subtree, the originals are untouched.
106 lines
3.6 KiB
Markdown
106 lines
3.6 KiB
Markdown
# Code-Review (Self-Review mit GitLab Discussions)
|
||
|
||
**WICHTIG:** Code-Review erst durchführen wenn die MR-Pipeline GRÜN ist.
|
||
Reihenfolge: Push → MR erstellen → Pipeline abwarten → Pipeline grün → Code-Review → Findings fixen → Merge.
|
||
Nicht reviewen solange Pipeline noch läuft oder failed ist – erst Pipeline fixen!
|
||
|
||
## Ablauf nach MR-Erstellung (ERST wenn MR-Pipeline grün!)
|
||
|
||
1. **Code lokal reviewen** (nicht nur Diff, sondern gesamten Code)
|
||
2. **Findings als GitLab Discussions anlegen**
|
||
3. **Findings selbst fixen**
|
||
4. **Threads resolven**
|
||
5. **Erst mergen wenn alle Threads resolved**
|
||
|
||
## Review-Kriterien
|
||
|
||
### Clean Code
|
||
- Single Responsibility Principle
|
||
- Sprechende Namen (Variablen, Methoden, Klassen)
|
||
- Kleine Methoden (max 20 Zeilen)
|
||
- Kein toter Code, keine auskommentierten Blöcke
|
||
- DRY (Don't Repeat Yourself)
|
||
|
||
### Separation of Concerns
|
||
- Controller nur für HTTP-Handling (kein Business-Logic)
|
||
- Service-Layer für Business-Logik
|
||
- Repository nur für Datenzugriff
|
||
- DTOs für API-Kommunikation (nicht Entity direkt exponieren)
|
||
|
||
### Best Practices
|
||
- Input-Validierung (@Valid, Bean Validation)
|
||
- Fehlerbehandlung (Exception Handler, sinnvolle HTTP Status Codes)
|
||
- Logging an kritischen Stellen
|
||
- Keine Secrets/Credentials im Code
|
||
- Immutable wo möglich
|
||
|
||
### Tests
|
||
- Alle öffentlichen Methoden getestet
|
||
- Edge Cases abgedeckt (null, leer, ungültig)
|
||
- Given-When-Then Struktur
|
||
- Mocks nur wo nötig (keine Over-Mocking)
|
||
|
||
### Security
|
||
- Keine SQL Injection (Parameterized Queries / JPA)
|
||
- Keine XSS (Output Encoding)
|
||
- Keine Secrets im Code
|
||
- Dependencies ohne bekannte CVEs
|
||
|
||
## GitLab Discussion anlegen
|
||
|
||
```bash
|
||
# Discussion an einer bestimmten Datei/Zeile
|
||
glab api -X POST "projects/:id/merge_requests/{mr_iid}/discussions" \
|
||
-f "body=**Clean Code:** Diese Methode hat zu viele Verantwortlichkeiten. Bitte in separate Methoden aufteilen." \
|
||
-f "position[base_sha]=$(git merge-base main HEAD)" \
|
||
-f "position[start_sha]=$(git merge-base main HEAD)" \
|
||
-f "position[head_sha]=$(git rev-parse HEAD)" \
|
||
-f "position[position_type]=text" \
|
||
-f "position[new_path]=src/main/java/com/example/controller/AddressController.java" \
|
||
-f "position[new_line]=42"
|
||
|
||
# Allgemeine Discussion (nicht an Datei gebunden)
|
||
glab api -X POST "projects/:id/merge_requests/{mr_iid}/discussions" \
|
||
-f "body=**Architektur:** Service-Layer fehlt. Controller greift direkt auf Repository zu. Bitte AddressService einführen."
|
||
```
|
||
|
||
## Findings fixen und Thread resolven
|
||
|
||
```bash
|
||
# Fix implementieren
|
||
# ... Code ändern ...
|
||
git add -A && git commit -m "fix(review): Service-Layer einführen (#issue)"
|
||
git push
|
||
|
||
# Thread resolven
|
||
glab api -X PUT "projects/:id/merge_requests/{mr_iid}/discussions/{discussion_id}" \
|
||
-f "resolved=true"
|
||
```
|
||
|
||
## Merge-Bedingung
|
||
|
||
```bash
|
||
# Prüfen ob alle Threads resolved sind
|
||
UNRESOLVED=$(glab api "projects/:id/merge_requests/{mr_iid}/discussions" | python3 -c "
|
||
import sys, json
|
||
discussions = json.load(sys.stdin)
|
||
unresolved = [d for d in discussions if any(n.get('resolvable') and not n.get('resolved') for n in d.get('notes',[]))]
|
||
print(len(unresolved))
|
||
")
|
||
|
||
if [ "$UNRESOLVED" = "0" ]; then
|
||
echo "Alle Threads resolved - merge möglich"
|
||
glab mr merge --when-pipeline-succeeds
|
||
else
|
||
echo "Noch $UNRESOLVED offene Threads - erst fixen"
|
||
fi
|
||
```
|
||
|
||
## Wichtig
|
||
|
||
- Review den GESAMTEN Code, nicht nur den Diff
|
||
- Mindestens 3 Review-Punkte prüfen (auch wenn alles gut aussieht → "LGTM" als Discussion)
|
||
- Findings mit Kategorie-Prefix: **Clean Code:**, **Security:**, **Architektur:**, **Test:**
|
||
- Jedes Finding muss actionable sein (konkreter Verbesserungsvorschlag)
|
||
- Erst mergen wenn 0 unresolved Threads
|