From 5f7ffab487054e7a438e4eb4e1012d53267987f4 Mon Sep 17 00:00:00 2001 From: Rodin Date: Thu, 14 May 2026 05:26:05 +0000 Subject: [PATCH] feat(#125): rename GITEA_URL to VCS_URL with deprecated fallback - Add --vcs-url flag as primary (reads VCS_URL env var) - Keep --gitea-url and GITEA_URL as deprecated fallbacks with warnings - Update action.yml: rename gitea-url input to vcs-url, pass VCS_URL to binary - Update ci.yml: use VCS_URL env var in Run review step - Update integration tests: INTEGRATION_GITEA_URL -> INTEGRATION_VCS_URL - Update README: --vcs-url / VCS_URL with fallback note in env var table Backward compat: existing GITEA_URL users get a deprecation warning and continue to work unchanged until they migrate to VCS_URL. --- .gitea/actions/review/action.yml | 18 +-- .gitea/workflows/ci.yml | 2 +- PLAN-125.md | 175 +++++++++++++++++++++++++++++ README.md | 2 +- cmd/review-bot/integration_test.go | 6 +- cmd/review-bot/main.go | 23 +++- integration_test.go | 17 +-- 7 files changed, 216 insertions(+), 27 deletions(-) create mode 100644 PLAN-125.md diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index 8934e40..1045427 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -6,8 +6,8 @@ name: 'AI Code Review' description: 'Run AI-powered code review on a pull request using review-bot' inputs: - gitea-url: - description: 'Gitea instance URL (defaults to server_url)' + vcs-url: + description: 'VCS server URL (defaults to server_url)' required: false default: '' repo: @@ -112,11 +112,11 @@ runs: id: version shell: bash run: | - GITEA_URL="${{ inputs.gitea-url || github.server_url }}" + BASE_URL="${{ inputs.vcs-url || github.server_url }}" REPO="${{ inputs.repo || 'rodin/review-bot' }}" if [ "${{ inputs.version }}" = "latest" ]; then - VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \ - | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')") + VERSION=$(curl -sSf "${BASE_URL}/api/v1/repos/${REPO}/releases?limit=1" \ + | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')") if [ -z "$VERSION" ]; then echo "Failed to determine latest version" >&2 exit 1 @@ -161,14 +161,14 @@ runs: if: steps.cache.outputs.cache-hit != 'true' shell: bash run: | - GITEA_URL="${{ inputs.gitea-url || github.server_url }}" + BASE_URL="${{ inputs.vcs-url || github.server_url }}" REPO="${{ inputs.repo || 'rodin/review-bot' }}" VERSION="${{ steps.version.outputs.version }}" BINARY="review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}" - curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \ + curl -sSfL "${BASE_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \ -o "${{ runner.temp }}/review-bot" - curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \ + curl -sSfL "${BASE_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \ -o "${{ runner.temp }}/checksums.txt" # Verify SHA-256 checksum @@ -198,7 +198,7 @@ runs: - name: Run review shell: bash env: - GITEA_URL: ${{ inputs.gitea-url || github.server_url }} + VCS_URL: ${{ inputs.vcs-url || github.server_url }} GITEA_REPO: ${{ inputs.repo || github.repository }} PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }} REVIEWER_TOKEN: ${{ inputs.reviewer-token }} diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index d6f2299..716f86d 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -49,7 +49,7 @@ jobs: - run: go build -o review-bot ./cmd/review-bot - name: Run ${{ matrix.name }} review env: - GITEA_URL: ${{ github.server_url }} + VCS_URL: ${{ github.server_url }} GITEA_REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }} diff --git a/PLAN-125.md b/PLAN-125.md new file mode 100644 index 0000000..29ac82c --- /dev/null +++ b/PLAN-125.md @@ -0,0 +1,175 @@ +# Plan: Issue #125 — Rename GITEA_URL → VCS_URL + +## Problem + +The `GITEA_URL` environment variable (and `--gitea-url` flag) implies the binary only works with Gitea. +Now that review-bot supports both Gitea and GitHub/GHES, this name is misleading. +Renaming to `VCS_URL` makes the binary platform-agnostic in its interface. + +## Constraints + +- Must not break existing users who already use `GITEA_URL` — need a fallback +- The CLI flag `--gitea-url` should also be updated to `--vcs-url` for consistency +- `INTEGRATION_GITEA_URL` in integration tests is a test-only env var, not the binary's interface; but should be updated for clarity +- The action YAML uses `GITEA_URL` as an internal shell variable in bash scripts — distinct from the env var passed to the binary +- All changes must compile and pass existing tests + +## Files Affected + +### Binary / Go source +| File | Change | +|------|--------| +| `cmd/review-bot/main.go` | Rename `--gitea-url` → `--vcs-url`, add `VCS_URL` as primary, keep `GITEA_URL` fallback | +| `cmd/review-bot/integration_test.go` | Rename `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` (test-only, no external compat concern) | +| `integration_test.go` | Same — rename `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` | + +### Action YAML +| File | Change | +|------|--------| +| `.gitea/actions/review/action.yml` | Rename input `gitea-url` → `vcs-url`; update env var passed to binary: `VCS_URL` instead of `GITEA_URL`; keep internal bash var as `GITEA_URL` (only used for release download, not passed to binary) | +| `.gitea/workflows/ci.yml` | Rename `GITEA_URL` env var to `VCS_URL` in Run review step | + +### Documentation +| File | Change | +|------|--------| +| `README.md` | Update CLI example, env var table entry | + +## Proposed Approach + +### 1. Backward-compatible env var lookup in main.go + +Replace: +```go +giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL") +``` + +With: +```go +giteaURL := flag.String("vcs-url", envOrDefaultFallback("VCS_URL", "GITEA_URL", ""), "VCS server URL (e.g. https://gitea.example.com)") +``` + +Add a helper: +```go +// envOrDefaultFallback reads primary env var; if empty, falls back to deprecated env var. +func envOrDefaultFallback(primary, deprecated, defaultVal string) string { + if v := os.Getenv(primary); v != "" { + return v + } + if v := os.Getenv(deprecated); v != "" { + slog.Warn("deprecated env var in use; rename to " + primary, "old", deprecated, "new", primary) + return v + } + return defaultVal +} +``` + +**Note:** This must be called AFTER `setupLogger` conceptually, but the flag default is evaluated at flag registration time. Since `setupLogger` runs before `flag.Parse()`, the slog.Warn will print correctly at runtime. We use `log.Printf` as a fallback if this proves problematic. + +Actually — flag defaults are evaluated at registration (line 57), before `setupLogger`. The warning won't go through slog. Two options: +- Use `log.Printf` for the deprecation warning (always visible) +- Move the fallback lookup to after `flag.Parse()`, checking if the parsed value is still empty + +**Decision:** Move fallback to a post-parse check. This is cleaner: +```go +vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL") +flag.Parse() +// Backward compat: fall back to deprecated GITEA_URL +if *vcsURL == "" { + if v := os.Getenv("GITEA_URL"); v != "" { + slog.Warn("GITEA_URL is deprecated; use VCS_URL instead") + *vcsURL = v + } +} +``` + +This is clean, idiomatic, and the warning goes through slog correctly. + +### 2. Keep `--gitea-url` as deprecated alias + +Add a hidden flag for backward compat: +```go +giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url") +``` + +Post-parse: +```go +if *vcsURL == "" && *giteaURLAlias != "" { + slog.Warn("--gitea-url is deprecated; use --vcs-url instead") + *vcsURL = *giteaURLAlias +} +``` + +### 3. Internal variable rename + +Rename `giteaURL` local variable → `vcsURL` throughout `main.go` for consistency. + +### 4. Error message update + +```go +fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n") +``` + +### 5. Action YAML changes + +In `.gitea/actions/review/action.yml`: +- Input `gitea-url` → `vcs-url` (with same description, `required: false`, `default: ''`) +- Line 172: `GITEA_URL: ${{ inputs.gitea-url || github.server_url }}` → `VCS_URL: ${{ inputs.vcs-url || github.server_url }}` +- Lines 115, 140: internal bash vars `GITEA_URL=` are used for downloading binaries — NOT passed to the review-bot binary. Leave them as internal bash vars (they're scope-local in bash). These could be renamed to `SERVER_URL` or `BASE_URL` for local clarity, but renaming them isn't strictly required. + +In `.gitea/workflows/ci.yml`: +- Line 52: `GITEA_URL: ${{ github.server_url }}` → `VCS_URL: ${{ github.server_url }}` + +### 6. Integration test updates + +`INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` in both test files. + +### 7. README + +- CLI example: `--gitea-url` → `--vcs-url` +- Env var table: `GITEA_URL` → `VCS_URL`, add note about `GITEA_URL` fallback + +## Backward Compatibility Summary + +| Old | New | Fallback? | +|-----|-----|-----------| +| `GITEA_URL` env var | `VCS_URL` | ✅ with deprecation warning | +| `--gitea-url` flag | `--vcs-url` | ✅ with deprecation warning | +| `gitea-url` action input | `vcs-url` | ⚠️ No (action version bump handles this) | +| `INTEGRATION_GITEA_URL` | `INTEGRATION_VCS_URL` | N/A (test-only) | + +## Error Cases + +- Both `VCS_URL` and `GITEA_URL` set: `VCS_URL` wins (primary takes precedence) +- Both `--vcs-url` and `--gitea-url` provided: `--vcs-url` wins +- Neither set: existing "missing required flags" error unchanged + +## Edge Cases + +- `os.Getenv` returns "" for unset AND set-to-empty — consistent with existing behavior +- The `envOrDefault` helper is unchanged; we add `envOrDefaultFallback` for the one renamed var + +## Testing Strategy + +- Existing unit tests pass unchanged (they don't test env var parsing directly) +- Integration tests updated to use new env var name +- Manual: `GITEA_URL=https://example.com ./review-bot --repo x --pr 1 ...` should print deprecation warning and proceed +- Manual: `VCS_URL=https://example.com ./review-bot ...` should work silently + +## Completion Checklist + +1. `VCS_URL` is read first; `GITEA_URL` is fallback with deprecation warning +2. `--vcs-url` flag is primary; `--gitea-url` is deprecated alias with warning +3. Error message references `--vcs-url` not `--gitea-url` +4. `action.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary +5. `ci.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary +6. README updated in CLI example and env var table +7. Integration tests use `INTEGRATION_VCS_URL` +8. `go test ./...` passes +9. `go vet ./...` passes +10. `go build ./cmd/review-bot` succeeds + +## Open Questions + +- Should the CLI flag `--gitea-url` be completely hidden from `--help` or just deprecated with a note? The issue doesn't specify. Decision: keep it visible but add "(deprecated: use --vcs-url)" to the description. +- Should action.yml also add `gitea-url` as a deprecated input alias? The issue says "Update the action to pass the new env var name" — no mention of backward compat for the action input. Decision: rename only, no alias (action users pin a version anyway). +- The bash-internal `GITEA_URL` variable in action.yml scripts (used for release download, not passed to binary) — rename for clarity? Decision: yes, rename to `BASE_URL` to avoid confusion with the env var. diff --git a/README.md b/README.md index cd9fc03..34685b0 100644 --- a/README.md +++ b/README.md @@ -299,7 +299,7 @@ All flags have environment variable equivalents: | Flag | Env Var | |------|---------| -| `--gitea-url` | `GITEA_URL` | +| `--vcs-url` | `VCS_URL` (fallback: `GITEA_URL`) | | `--repo` | `GITEA_REPO` | | `--pr` | `PR_NUMBER` | | `--reviewer-token` | `REVIEWER_TOKEN` | diff --git a/cmd/review-bot/integration_test.go b/cmd/review-bot/integration_test.go index a19aa1f..e357624 100644 --- a/cmd/review-bot/integration_test.go +++ b/cmd/review-bot/integration_test.go @@ -17,7 +17,7 @@ import ( // Integration test requires a running Gitea instance and LLM endpoint. // Set environment variables: // -// INTEGRATION_GITEA_URL - Gitea base URL +// INTEGRATION_VCS_URL - VCS base URL // INTEGRATION_GITEA_TOKEN - Gitea API token with repo access // INTEGRATION_GITEA_REPO - owner/repo with an open PR // INTEGRATION_PR_NUMBER - PR number to test against @@ -25,7 +25,7 @@ import ( // INTEGRATION_LLM_API_KEY - LLM API key // INTEGRATION_LLM_MODEL - Model name func TestIntegration_FullReviewFlow(t *testing.T) { - giteaURL := os.Getenv("INTEGRATION_GITEA_URL") + giteaURL := os.Getenv("INTEGRATION_VCS_URL") giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN") giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO") prNumStr := os.Getenv("INTEGRATION_PR_NUMBER") @@ -104,7 +104,7 @@ func TestIntegration_FullReviewFlow(t *testing.T) { } func TestIntegration_PostAndCleanup(t *testing.T) { - giteaURL := os.Getenv("INTEGRATION_GITEA_URL") + giteaURL := os.Getenv("INTEGRATION_VCS_URL") giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN") giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO") prNumStr := os.Getenv("INTEGRATION_PR_NUMBER") diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 9a9da91..157980f 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -54,7 +54,8 @@ func main() { logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json") verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error") // CLI flags - giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL") + vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL (e.g. https://gitea.example.com)") + giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url") repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)") prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number") reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name") @@ -91,12 +92,24 @@ func main() { slog.Info("review-bot starting", "version", version) + // Backward compatibility: fall back to deprecated env var / flag if VCS_URL / --vcs-url not set. + if *vcsURL == "" { + if v := os.Getenv("GITEA_URL"); v != "" { + slog.Warn("GITEA_URL is deprecated; rename the environment variable to VCS_URL") + *vcsURL = v + } + } + if *vcsURL == "" && *giteaURLAlias != "" { + slog.Warn("--gitea-url is deprecated; use --vcs-url instead") + *vcsURL = *giteaURLAlias + } + // Validate required fields // For aicore provider, llm-base-url and llm-api-key are not required isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore - if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" { + if *vcsURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" { fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n") - fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n") + fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n") os.Exit(1) } if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") { @@ -139,7 +152,7 @@ func main() { } // Initialize clients - giteaClient := gitea.NewClient(*giteaURL, *reviewerToken) + giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel) if *llmTemp < 0 || *llmTemp > 2 { slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2") @@ -453,7 +466,7 @@ func main() { // Supersede all old reviews with link to the new one if len(oldReviews) > 0 { - newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID) + newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*vcsURL, "/"), owner, repoName, prNumber, posted.ID) for _, oldReview := range oldReviews { cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID) if err != nil { diff --git a/integration_test.go b/integration_test.go index d3ccce2..314e434 100644 --- a/integration_test.go +++ b/integration_test.go @@ -16,16 +16,17 @@ import ( // Integration test requires a running Gitea instance and LLM endpoint. // Set environment variables: -// INTEGRATION_GITEA_URL - Gitea base URL -// INTEGRATION_GITEA_TOKEN - Gitea API token with repo access -// INTEGRATION_GITEA_REPO - owner/repo with an open PR -// INTEGRATION_PR_NUMBER - PR number to test against -// INTEGRATION_LLM_BASE_URL - LLM API base URL -// INTEGRATION_LLM_API_KEY - LLM API key -// INTEGRATION_LLM_MODEL - Model name +// +// INTEGRATION_VCS_URL - VCS base URL +// INTEGRATION_GITEA_TOKEN - Gitea API token with repo access +// INTEGRATION_GITEA_REPO - owner/repo with an open PR +// INTEGRATION_PR_NUMBER - PR number to test against +// INTEGRATION_LLM_BASE_URL - LLM API base URL +// INTEGRATION_LLM_API_KEY - LLM API key +// INTEGRATION_LLM_MODEL - Model name func TestIntegration_FullReviewFlow(t *testing.T) { - giteaURL := os.Getenv("INTEGRATION_GITEA_URL") + giteaURL := os.Getenv("INTEGRATION_VCS_URL") giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN") giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO") prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")