refactor: high cohesion low coupling - interface segregation, registry, structured errors#3
Merged
Conversation
…y, structured errors, retry/hooks
BREAKING CHANGES:
- Provider constructors (NewGitHubProvider, etc.) are now unexported
- Use NewProvider(Config{}) factory instead
- ENCRYPTION_KEY env var is now required for credential encryption
Architecture improvements:
- Split 48-method Provider interface into 8 focused sub-interfaces:
RepoManager, ChangeRequestManager, WebhookManager, BranchManager,
DiffManager, CommitManager, FileManager, ReleaseManager
- Registry pattern: providers self-register via init(), factory uses registry
- Structured error types: ProviderError, NotFoundError, AuthError, RateLimitError
- Logger interface with no-op default
- HTTP retry with exponential backoff and jitter
- Request/Response lifecycle hooks middleware
Code quality:
- Extract common utilities: pagination, diff counting, state mapping, FullName parsing
- Remove ekit dependency, replace with local pkg/encoding
- Remove base64 wrapper functions in gitee.go
- Connect Config.Logger/RetryConfig/Hooks to all provider constructors
- Unexport BaseOption/BaseProviderConfig (internal only)
Complete implementations:
- Gitee provider: full 48-method implementation (was only TestConnection)
- GitCode provider: implement CreateDiscussion, CreateReview, ParseWebhookEvent, ValidateWebhookSignature
Security:
- Remove hardcoded encryption key fallback in credential/encrypt.go
- Require ENCRYPTION_KEY env var (32 bytes)
Tests:
- Add credential/encrypt_test.go (encrypt/decrypt, BuildAuthURL, BuildSSHCommand)
- Add pkg/encoding/base64_test.go
There was a problem hiding this comment.
Pull request overview
This PR refactors the provider package toward a higher-cohesion / lower-coupling architecture by splitting the monolithic Provider interface into focused sub-interfaces, introducing a registry-based provider factory, and adding shared utilities (pagination/state/diff/convert), structured errors, retry logic, and request/response hooks. It also removes the external ekit dependency in favor of local pkg/encoding, expands provider implementations (notably Gitee), and tightens credential encryption requirements.
Changes:
- Split
Providerinto 8 focused interfaces and migrate constructors to a registry-drivenNewProvider(Config)factory. - Add shared utilities (pagination/state/diff/convert), request lifecycle hooks, logging abstraction, and HTTP retry support in the base provider.
- Replace
ekit/pkg/encodingwithpkg/encoding, implement/complete multiple providers (notably Gitee), and harden credential encryption to requireENCRYPTION_KEY.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| provider/util.go | Removes local total-count parsing helper and keeps SHA validation helper. |
| provider/tencent_code.go | Migrates Tencent Code provider to registry + shared utils (pagination/diff/state/convert). |
| provider/tencent_code_test.go | Updates tests to use NewProvider(Config{...}). |
| provider/stateutil.go | Adds shared CR state mapping helpers. |
| provider/retry.go | Introduces retry config + backoff/jitter logic for HTTP requests. |
| provider/registry.go | Adds provider constructor registry + registration helpers. |
| provider/provider.go | Splits Provider into composed sub-interfaces and adds/updates type docs. |
| provider/pagination.go | Adds paging defaults + shared total-count header parsing. |
| provider/middleware.go | Adds request/response lifecycle hook types and execution helpers. |
| provider/logger.go | Adds minimal Logger interface + no-op implementation. |
| provider/iface_webhooks.go | New focused interface for webhook operations. |
| provider/iface_repos.go | New focused interface for repo operations. |
| provider/iface_releases.go | New focused interface for releases/tags/archives. |
| provider/iface_files.go | New focused interface for file operations. |
| provider/iface_diffs.go | New focused interface for diff/review/discussion operations. |
| provider/iface_commits.go | New focused interface for commit operations. |
| provider/iface_changes.go | New focused interface for change request lifecycle. |
| provider/iface_branches.go | New focused interface for branch operations. |
| provider/gitlab.go | Migrates GitLab provider to registry-based construction + logger. |
| provider/github.go | Migrates GitHub provider to registry-based construction + logger. |
| provider/gitee.go | Implements full Gitee provider functionality and integrates shared utilities. |
| provider/gitea.go | Migrates Gitea provider to registry-based construction + logger. |
| provider/gitcode.go | Migrates GitCode provider to registry-based construction + webhook/review/discussion completion. |
| provider/forgejo.go | Migrates Forgejo provider to registry-based construction + logger. |
| provider/factory.go | Replaces switch-based factory with registry-based NewProvider. |
| provider/errors.go | Adds structured/sentinel provider errors + helpers. |
| provider/diffutil.go | Adds shared diff stats helpers and raw diff builder. |
| provider/convertutil.go | Adds shared owner/repo parsing and EventRepo builder. |
| provider/base.go | Adds logger/retry/hooks support, structured HTTP errors, and response body restoration helper. |
| pkg/encoding/base64.go | Adds local Base64 helpers (replacing ekit). |
| pkg/encoding/base64_test.go | Adds tests for local Base64 helpers. |
| go.sum | Removes ekit sums. |
| go.mod | Removes ekit dependency. |
| credential/encrypt.go | Removes hardcoded fallback key; requires ENCRYPTION_KEY and switches to local encoding. |
| credential/encrypt_test.go | Adds tests for encryption/decryption and related credential manager helpers. |
Comments suppressed due to low confidence (1)
provider/tencent_code_test.go:36
- This test ignores the error return from NewProvider; if registration fails or the constructor returns an error, p will be nil and the test will panic on p.Platform().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
16
to
20
| var ( | ||
| key []byte | ||
| keyOnce sync.Once | ||
| ) | ||
|
|
Comment on lines
+126
to
+128
| // Execute request hooks | ||
| ctx = b.hooks.executeRequestHooks(ctx, req) | ||
|
|
Comment on lines
+168
to
+170
| // Execute request hooks | ||
| ctx = b.hooks.executeRequestHooks(ctx, req) | ||
|
|
Comment on lines
152
to
154
| if resp.StatusCode >= 400 { | ||
| return nil, fmt.Errorf("%s API %s %s returned %d: %s", b.errPrefix, method, path, resp.StatusCode, string(respBody)) | ||
| return nil, NewProviderError(Platform(b.errPrefix), fmt.Sprintf("%s %s", method, path), resp.StatusCode, string(respBody)) | ||
| } |
Comment on lines
194
to
196
| if resp.StatusCode >= 400 { | ||
| return nil, fmt.Errorf("%s API %s %s returned %d: %s", b.errPrefix, method, path, resp.StatusCode, string(body[:min(len(body), 200)])) | ||
| return nil, NewProviderError(Platform(b.errPrefix), fmt.Sprintf("%s %s", method, path), resp.StatusCode, string(body[:min(len(body), 200)])) | ||
| } |
Comment on lines
+24
to
+28
| Register(PlatformGitLab, func(cfg Config) (Provider, error) { | ||
| return newGitLabProvider(cfg), nil | ||
| }) | ||
| } | ||
|
|
Comment on lines
+26
to
+30
| func init() { | ||
| Register(PlatformGitea, func(cfg Config) (Provider, error) { | ||
| return newGiteaProvider(cfg), nil | ||
| }) | ||
| } |
Comment on lines
+32
to
+36
| func newGiteaProvider(cfg Config) *giteaProvider { | ||
| logger := cfg.Logger | ||
| if logger == nil { | ||
| logger = NewNoopLogger() | ||
| } |
Comment on lines
+26
to
+30
| func init() { | ||
| Register(PlatformForgejo, func(cfg Config) (Provider, error) { | ||
| return newForgejoProvider(cfg), nil | ||
| }) | ||
| } |
Comment on lines
+32
to
+36
| func newForgejoProvider(cfg Config) *forgejoProvider { | ||
| logger := cfg.Logger | ||
| if logger == nil { | ||
| logger = NewNoopLogger() | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
改动总结
架构改进
代码质量
补全实现
安全修复
测试
Breaking Changes