feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106
@@ -59,7 +59,7 @@ func main() {
|
||||
provider := flag.String("provider", envOrDefault("VCS_PROVIDER", "gitea"), "VCS provider: gitea or github")
|
||||
baseURL := flag.String("base-url", envOrDefault("VCS_BASE_URL", ""), "VCS API base URL (for github provider; defaults to https://api.github.com)")
|
||||
vcsURL := flag.String("vcs-url", envOrDefault("VCS_URL", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", ""))), "VCS instance URL (Gitea) [deprecated alias: --gitea-url]")
|
||||
// Keep --gitea-url as hidden alias (flag package doesn't support aliases natively, handle below)
|
||||
// Keep --gitea-url as backward-compatible alias (flag package doesn't support aliases natively, handle below)
|
||||
repo := flag.String("repo", envOrDefault("VCS_REPO", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", ""))), "Repository (owner/name)")
|
||||
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
|
||||
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
|
||||
|
|
||||
@@ -84,7 +84,9 @@ func main() {
|
||||
aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
|
||||
sonnet-review-bot
commented
[MINOR] The --gitea-url alias registration uses **[MINOR]** The --gitea-url alias registration uses `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` after vcsURL has already been parsed from env vars. This means the default for the alias is whatever vcsURL resolved to at registration time, not a live binding. In practice this works fine because flag.Parse() hasn't run yet, but it's subtly fragile: if the env-var chain for vcsURL ever produces an empty string, the alias and the primary flag start with different defaults. A comment explaining this would help future maintainers.
gpt-review-bot
commented
[NIT] The comment says the --gitea-url flag is a hidden alias, but it is registered normally and will appear in usage. Either adjust wording to 'deprecated alias' or consider not exposing it in help if truly intended to be hidden. **[NIT]** The comment says the --gitea-url flag is a hidden alias, but it is registered normally and will appear in usage. Either adjust wording to 'deprecated alias' or consider not exposing it in help if truly intended to be hidden.
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)`. This dereferences `*vcsURL` at flag registration time to capture the current default, which is correct. However, the comment says 'whichever flag is set last by flag.Parse wins' — this is accurate but the behavior may surprise users if both `--vcs-url` and `--gitea-url` are passed simultaneously (last one wins rather than an error). This is a known limitation of the `flag` package alias approach and should ideally be documented in the flag usage or a warning emitted if both are set.
rodin
commented
Added a comment explaining the shared Added a comment explaining the shared `*string` pointer mechanism. `flag.StringVar(vcsURL, ...)` binds `--gitea-url` to the same underlying `*string` as `--vcs-url`, so there's no divergence risk — both flags always resolve to the same value after `flag.Parse()`. Comment added in 7d6fe27.
rodin
commented
Updated the inline comment from "hidden alias" to "backward-compatible alias" in Updated the inline comment from "hidden alias" to "backward-compatible alias" in 7d6fe27. The flag does appear in `--help` output (Go's `flag` package doesn't support hiding flags), so "hidden" was inaccurate. "Backward-compatible" better describes the intent.
sonnet-review-bot
commented
[NIT] The comment **[NIT]** The comment `// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).` is accurate but slightly misleading: with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` the last *parsed* flag wins at parse time. If both `--vcs-url` and `--gitea-url` are supplied, the rightmost one on the command line takes effect. This is the correct and intended behaviour, but a small clarification in the comment ("last supplied on the command line wins") would help future readers.
|
||||
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
|
||||
|
rodin marked this conversation as resolved
sonnet-review-bot
commented
[NIT] The **[NIT]** The `--gitea-url` alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: `flag.StringVar shares the pointer, so whichever flag is set last wins.` The current comment is accurate but its length makes it easy to miss the ORDERING constraint.
|
||||
|
||||
// Register --gitea-url as a deprecated alias for --vcs-url
|
||||
// Register --gitea-url as a backward-compatible alias for --vcs-url.
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `--gitea-url` alias registration reads `*vcsURL` at registration time (before `flag.Parse()`). At that point `*vcsURL` holds the default value from `envOrDefault(...)`, so the alias default is correct. However, if a user passes `--vcs-url foo` on the command line, the alias's default is stale — only the variable is shared, not a live default. This is acceptable because the flag package updates the pointed-to variable via `flag.StringVar`, so any value written by either `--vcs-url` or `--gitea-url` goes to the same `*string`. But the help text for `--gitea-url` will show the env-var default rather than reflecting `--vcs-url`'s current value, which is mildly confusing. Consider a brief comment explaining the aliasing semantics.
sonnet-review-bot
commented
[NIT] The comment on the **[NIT]** The comment on the `--gitea-url` alias registration says "The *vcsURL dereference captures the env-var-resolved default at registration time". This is accurate but the behaviour when both `--vcs-url` and `--gitea-url` are passed (last one wins) is subtle enough that a brief integration test would provide more confidence than a comment alone.
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` where `*vcsURL` is the default value captured at the moment of the call — before `flag.Parse()`. This is correct as written because `*vcsURL` is still the default string at that point, but it creates a subtle ordering dependency: if `vcsURL` ever acquired a non-default value between its declaration and this `flag.StringVar` call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (`Must stay after vcsURL declaration and before flag.Parse()`), but the fragility is worth noting. A cleaner pattern would be `flag.StringVar(vcsURL, "gitea-url", "", "...")` or extracting the default value to a named constant.
|
||||
// StringVar shares the *string pointer with vcsURL, so whichever flag is
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `--gitea-url` alias registration uses `*vcsURL` as the default value at registration time. This works correctly because flag parsing hasn't happened yet, so `*vcsURL` holds the env-var-resolved default. However this is subtle — if the evaluation order ever changed (e.g., someone moves `flag.Parse()` above this line), the alias would silently use an empty string default instead. A brief comment noting this dependency on registration-before-parse order would help future readers.
|
||||
// set last by flag.Parse wins — both point to the same underlying value.
|
||||
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
|
||||
|
||||
flag.Parse()
|
||||
@@ -535,7 +537,7 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] supersedeOldReviews uses a string-based provider switch ('github' / else-Gitea) rather than a typed constant or enum. The provider value is passed as a plain string through multiple function calls. If a third provider is added later, it's easy to miss this function. Consider using the same switch structure as the client factory (case 'gitea': / case 'github': / default: return fmt.Errorf) to make exhaustiveness explicit. **[MINOR]** supersedeOldReviews uses a string-based provider switch ('github' / else-Gitea) rather than a typed constant or enum. The provider value is passed as a plain string through multiple function calls. If a third provider is added later, it's easy to miss this function. Consider using the same switch structure as the client factory (case 'gitea': / case 'github': / default: return fmt.Errorf) to make exhaustiveness explicit.
|
||||
return nil
|
||||
case "gitea":
|
||||
|
rodin marked this conversation as resolved
Outdated
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `supersedeOldReviews` function has a duplicate doc comment: the first line says 'supersedeOldReviews marks old reviews as superseded.' and the second line repeats 'supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.' This is a copy-paste artifact that should be cleaned up.
|
||||
// Fall through to Gitea-specific logic below the switch.
|
||||
// Continue to Gitea-specific logic below the switch.
|
||||
default:
|
||||
return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider)
|
||||
}
|
||||
|
||||
@@ -80,7 +80,8 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
|
||||
Body: comment.Body,
|
||||
}
|
||||
payload.Comments = append(payload.Comments, rc)
|
||||
// Use CommitID from the first comment that has one
|
||||
// Use CommitID from the first comment that has one.
|
||||
// All comments in a single review are expected to reference the same commit.
|
||||
if payload.CommitID == "" && comment.CommitID != "" {
|
||||
payload.CommitID = comment.CommitID
|
||||
}
|
||||
|
||||
[NIT] The
--gitea-urlalias registration comment says 'If a user passes both --vcs-url and --gitea-url, the last one on the command line takes effect'. However, since both--vcs-urland--gitea-urlshare the same pointer viaStringVar, only the last flag parsed wins. This is the documented behavior but may be surprising to users who setGITEA_URLenv var and also pass--vcs-urlon the command line — the flag will win. This is standard flag package behavior but worth a brief note in the help text.