docs: add VCS abstraction design doc
Outlines phased approach for GitHub support: - Phase 1: Port github/ package from strat fork - Phase 2: Add vcs/ interface with runtime detection - Phase 3: Wire up cmd/review-bot Issue: #76
This commit is contained in:
Rodin
@@ -0,0 +1,210 @@
|
|||||||
|
# VCS Abstraction Layer Design
|
||||||
|
|
||||||
|
Issue: #76
|
||||||
|
|
||||||
|
## Problem
|
||||||
|
|
||||||
|
review-bot currently only works with Gitea. We want it to work on GitHub (github.com and GHE) without maintaining two forks.
|
||||||
|
|
||||||
|
## Constraints
|
||||||
|
|
||||||
|
- Must be backward compatible — existing Gitea usage unchanged
|
||||||
|
- Runtime detection — no config flag needed
|
||||||
|
- Same types — gitea/ and github/ packages already use identical structs
|
||||||
|
- Minimal diff — don't rewrite, just abstract
|
||||||
|
|
||||||
|
## Current State
|
||||||
|
|
||||||
|
Two packages exist:
|
||||||
|
- `gitea/` in rodin/review-bot — Gitea API
|
||||||
|
- `github/` in strat/review-bot — GitHub API (forked from gitea/, adjusted paths)
|
||||||
|
|
||||||
|
Method signatures are identical. Types are identical. Only API endpoints differ.
|
||||||
|
|
||||||
|
## Proposed Approach
|
||||||
|
|
||||||
|
### Phase 1: Port github/ package
|
||||||
|
|
||||||
|
Copy strat/review-bot's `github/` package into rodin/review-bot.
|
||||||
|
|
||||||
|
Files:
|
||||||
|
- `github/client.go`
|
||||||
|
- `github/client_test.go`
|
||||||
|
- `github/diff.go`
|
||||||
|
|
||||||
|
No changes to gitea/ package.
|
||||||
|
|
||||||
|
### Phase 2: Add vcs/ abstraction
|
||||||
|
|
||||||
|
```
|
||||||
|
vcs/
|
||||||
|
interface.go # Client interface
|
||||||
|
client.go # NewClient() with detection
|
||||||
|
doc.go # package doc
|
||||||
|
```
|
||||||
|
|
||||||
|
**interface.go:**
|
||||||
|
```go
|
||||||
|
package vcs
|
||||||
|
|
||||||
|
import "context"
|
||||||
|
|
||||||
|
// Client abstracts VCS operations. Implemented by gitea.Client and github.Client.
|
||||||
|
type Client interface {
|
||||||
|
GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error)
|
||||||
|
GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
|
||||||
|
GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error)
|
||||||
|
GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error)
|
||||||
|
GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
|
||||||
|
GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error)
|
||||||
|
PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error)
|
||||||
|
ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
|
||||||
|
GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error)
|
||||||
|
ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
|
||||||
|
DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
|
||||||
|
GetAuthenticatedUser(ctx context.Context) (string, error)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Types re-exported from gitea package (canonical source)
|
||||||
|
type (
|
||||||
|
PullRequest = gitea.PullRequest
|
||||||
|
ChangedFile = gitea.ChangedFile
|
||||||
|
CommitStatus = gitea.CommitStatus
|
||||||
|
ReviewComment = gitea.ReviewComment
|
||||||
|
ContentEntry = gitea.ContentEntry
|
||||||
|
Review = gitea.Review
|
||||||
|
)
|
||||||
|
```
|
||||||
|
|
||||||
|
**client.go:**
|
||||||
|
```go
|
||||||
|
package vcs
|
||||||
|
|
||||||
|
import (
|
||||||
|
"strings"
|
||||||
|
|
||||||
|
"gitea.weiker.me/rodin/review-bot/gitea"
|
||||||
|
"gitea.weiker.me/rodin/review-bot/github"
|
||||||
|
)
|
||||||
|
|
||||||
|
// NewClient creates a VCS client based on the server URL.
|
||||||
|
// GitHub detection: github.com, api.github.com, or "github" in domain (GHE).
|
||||||
|
func NewClient(baseURL, token string) Client {
|
||||||
|
if isGitHub(baseURL) {
|
||||||
|
return github.NewClient(baseURL, token)
|
||||||
|
}
|
||||||
|
return gitea.NewClient(baseURL, token)
|
||||||
|
}
|
||||||
|
|
||||||
|
func isGitHub(baseURL string) bool {
|
||||||
|
lower := strings.ToLower(baseURL)
|
||||||
|
if strings.Contains(lower, "github.com") {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
// GHE: github.mycompany.com but NOT gitea.mycompany.com
|
||||||
|
if strings.Contains(lower, "github") && !strings.Contains(lower, "gitea") {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### Phase 3: Update cmd/review-bot
|
||||||
|
|
||||||
|
Change from:
|
||||||
|
```go
|
||||||
|
client := gitea.NewClient(baseURL, token)
|
||||||
|
```
|
||||||
|
|
||||||
|
To:
|
||||||
|
```go
|
||||||
|
client := vcs.NewClient(baseURL, token)
|
||||||
|
```
|
||||||
|
|
||||||
|
All callers already use the interface implicitly (same method names). This is a one-line change.
|
||||||
|
|
||||||
|
## API Differences Handled in github/
|
||||||
|
|
||||||
|
| Operation | Gitea | GitHub |
|
||||||
|
|-----------|-------|--------|
|
||||||
|
| API prefix | `/api/v1/repos/` | `/repos/` (github.com uses api.github.com) |
|
||||||
|
| Get diff | `GET /pulls/{n}.diff` | `GET /pulls/{n}` + `Accept: application/vnd.github.diff` |
|
||||||
|
| Auth header | `Authorization: token X` | `Authorization: Bearer X` |
|
||||||
|
|
||||||
|
The github/ package already handles these. No new code needed.
|
||||||
|
|
||||||
|
## Methods NOT in interface (Gitea-only)
|
||||||
|
|
||||||
|
These methods exist in gitea/ but aren't needed for core review flow:
|
||||||
|
- `GetTimelineReviewCommentID` — Gitea-specific timeline API
|
||||||
|
- `GetTimelineReviewCommentIDForReview` — Gitea-specific
|
||||||
|
- `EditComment` — supersede flow (Gitea-specific)
|
||||||
|
- `ResolveComment` — Gitea-specific
|
||||||
|
|
||||||
|
If a caller needs these, they can type-assert: `if gc, ok := client.(*gitea.Client); ok { ... }`
|
||||||
|
|
||||||
|
## Testing Strategy
|
||||||
|
|
||||||
|
1. Unit tests for `isGitHub()` detection
|
||||||
|
2. Existing gitea/ tests unchanged
|
||||||
|
3. Existing github/ tests from strat fork
|
||||||
|
4. Integration test: mock server, verify correct client selected
|
||||||
|
|
||||||
|
## Phases
|
||||||
|
|
||||||
|
| PR | Content | Exit Criteria |
|
||||||
|
|----|---------|---------------|
|
||||||
|
| 1 | Port github/ package | `go test ./github/...` passes |
|
||||||
|
| 2 | Add vcs/ interface | `go build ./...` compiles |
|
||||||
|
| 3 | Update cmd/review-bot | All existing tests pass, GitHub detection works |
|
||||||
|
|
||||||
|
## Open Questions
|
||||||
|
|
||||||
|
1. **Type re-export vs duplicate** — Using type aliases (`type PullRequest = gitea.PullRequest`) means callers import vcs and get gitea types. Alternative: duplicate types in vcs/. Preference?
|
||||||
|
|
||||||
|
2. **github.com API base URL** — For github.com, API is at `api.github.com`, not `github.com/api`. The github/ package needs to handle this. Does strat's version already?
|
||||||
|
|
||||||
|
3. **GHE API path** — GHE uses `/api/v3/repos/`. Verify strat's github/ package handles this.
|
||||||
|
|
||||||
|
## Answers to Open Questions
|
||||||
|
|
||||||
|
### Q1: Type re-export vs duplicate
|
||||||
|
|
||||||
|
**Answer: Type aliases.**
|
||||||
|
- Keeps one source of truth (gitea/ package)
|
||||||
|
- If types ever diverge, we'll know at compile time
|
||||||
|
- Callers importing `vcs` get clean namespace
|
||||||
|
|
||||||
|
### Q2 & Q3: API URL handling
|
||||||
|
|
||||||
|
**Answer: NewClient needs URL transformation.**
|
||||||
|
|
||||||
|
The github/ package expects the API root:
|
||||||
|
- github.com → `https://api.github.com`
|
||||||
|
- GHE → `https://github.concur.com/api/v3`
|
||||||
|
|
||||||
|
But callers pass server URLs (from `github.server_url` context):
|
||||||
|
- github.com → `https://github.com`
|
||||||
|
- GHE → `https://github.concur.com`
|
||||||
|
|
||||||
|
**Updated client.go:**
|
||||||
|
```go
|
||||||
|
func NewClient(serverURL, token string) Client {
|
||||||
|
if isGitHub(serverURL) {
|
||||||
|
apiURL := toGitHubAPIURL(serverURL)
|
||||||
|
return github.NewClient(apiURL, token)
|
||||||
|
}
|
||||||
|
return gitea.NewClient(serverURL, token)
|
||||||
|
}
|
||||||
|
|
||||||
|
func toGitHubAPIURL(serverURL string) string {
|
||||||
|
lower := strings.ToLower(serverURL)
|
||||||
|
if strings.Contains(lower, "github.com") && !strings.Contains(lower, "api.github.com") {
|
||||||
|
return "https://api.github.com"
|
||||||
|
}
|
||||||
|
// GHE: append /api/v3
|
||||||
|
return strings.TrimRight(serverURL, "/") + "/api/v3"
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
This keeps the github/ package unchanged (it already handles the API URL format).
|
||||||
Reference in New Issue
Block a user