refactor: consolidate deprecated bool flag helpers into pkg/cli/flags.go - #48680
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48680 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (95 additions, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Centralizes deprecated boolean flag handling in pkg/cli/flags.go, reducing duplication while preserving CLI compatibility.
Changes:
- Adds shared security-scanner registration and deprecated-flag resolution helpers.
- Migrates five commands to the shared helpers.
- Consolidates duplicated tests into helper-focused coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/flags.go |
Adds shared flag helpers. |
pkg/cli/flags_test.go |
Tests registration and resolution helpers. |
pkg/cli/add_command.go |
Uses shared helpers. |
pkg/cli/add_command_test.go |
Removes duplicate alias test. |
pkg/cli/add_wizard_command.go |
Uses shared helpers. |
pkg/cli/add_wizard_command_test.go |
Removes duplicate alias test. |
pkg/cli/deploy_command.go |
Uses shared helpers. |
pkg/cli/deploy_command_test.go |
Removes duplicate alias test. |
pkg/cli/trial_command.go |
Consolidates deprecated boolean aliases. |
pkg/cli/trial_command_test.go |
Removes duplicate test and unused imports. |
pkg/cli/update_command.go |
Consolidates scanner and release-bump aliases. |
pkg/cli/update_command_test.go |
Removes superseded deprecation tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: Approve — clean, correct refactor
Mechanical dedup of the --no-X/--disable-X deprecated-flag pattern into two shared helpers; no correctness issues found across two independent passes (own review + grumpy-coder sub-agent).
Review notes
- Verified all 5 call sites (
add,add_wizard,deploy,trialx2,updatex2) passresolveDeprecatedBoolFlag(cmd, newName, oldName)with correct argument order matching prior behavior, including the less-obviousdelete-host-repo-before/force-delete-host-repo-beforepair intrial_command.go. addSecurityScannerFlagis applied consistently everywhere the old manual 3-line registration existed; no leftover duplicated registrations.- New helpers in
flags.goare simple, side-effect-free, and covered byTestAddSecurityScannerFlag/TestResolveDeprecatedBoolFlag(including both-false, new-set, old-set subtests). - Minor, non-blocking observation: deleting the 6 per-command deprecation tests in favor of centralized tests in
flags_test.gomeans a future copy-paste typo in one command's call site (wrong flag name string) would no longer be caught by that command's own test suite — only the shared helper's correctness is verified, not each call site's wiring. Not worth blocking on given current correctness, but worth keeping in mind if more commands adopt this pattern.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 31.8 AIC · ⌖ 4.31 AIC · ⊞ 7.4K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two gaps before merge.
📋 Key Themes & Highlights
Issues
-
Incomplete pattern —
--no-release-bump/--disable-release-bumpregistration inupdate_command.gois still inline (lines 171-173), while all security-scanner pairs were moved toaddSecurityScannerFlag. The PR's own goal of consistency calls for anaddReleaseBumpFlaghelper here. -
Coverage gap —
TestNewUpdateCommand_DeprecatesDisableReleaseBumpFlagwas deleted but no equivalent was added toflags_test.go. The deprecation message on--disable-release-bumpis now untested. -
Silent error swallowing —
resolveDeprecatedBoolFlagdiscardsGetBoolerrors with_. A misspelled flag name returnsfalsesilently; a panic on programmer error would surface this immediately.
Positive Highlights
- ✅ Clean elimination of 20+ lines of copy-pasted flag logic across 5 command files
- ✅
TestResolveDeprecatedBoolFlaghas well-structured parallel subtests covering both-false, new-set, and old-set cases - ✅
resolveDeprecatedBoolFlagis a genuinely general helper — good API surface - ✅ PR description accurately captures the scope and motivation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48 AIC · ⌖ 5.11 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -175,9 +171,7 @@ Note: In GitHub Enterprise repos, shorthand source specs resolve on your enterpr | |||
| cmd.Flags().Bool("no-release-bump", false, "Restrict automatic major version bumps to core actions/* only (non-core actions are left as-is)") | |||
| cmd.Flags().Bool("disable-release-bump", false, "Restrict automatic major version bumps to core actions/* only (non-core actions are left as-is)") | |||
| _ = cmd.Flags().MarkDeprecated("disable-release-bump", "use --no-release-bump instead") | |||
There was a problem hiding this comment.
[/codebase-design] The --no-release-bump/--disable-release-bump pair is still registered inline here, inconsistent with the addSecurityScannerFlag pattern introduced in this PR.
💡 Suggestion
Extract an addReleaseBumpFlag(cmd) helper in flags.go alongside addSecurityScannerFlag and replace these three lines:
func addReleaseBumpFlag(cmd *cobra.Command) {
cmd.Flags().Bool("no-release-bump", false, "Restrict automatic major version bumps...")
cmd.Flags().Bool("disable-release-bump", false, "Restrict automatic major version bumps...")
_ = cmd.Flags().MarkDeprecated("disable-release-bump", "use --no-release-bump instead")
}Then call addReleaseBumpFlag(cmd) at the registration site. All deprecated-pair registrations are then in one place, eliminating this last inline exception.
@copilot please address this.
| t.Error("expected true when deprecated old flag is set") | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] TestResolveDeprecatedBoolFlag and TestAddSecurityScannerFlag cover the security-scanner pair, but there is no test for the --no-release-bump/--disable-release-bump pair that was also migrated to resolveDeprecatedBoolFlag in this PR.
TestNewUpdateCommand_DeprecatesDisableReleaseBumpFlag was deleted from update_command_test.go without a corresponding canonical test in flags_test.go. The deprecation message on --disable-release-bump is now untested.
💡 Suggestion
Add a TestAddReleaseBumpFlag (or extend TestAddSecurityScannerFlag into a table-driven test) covering the --no-release-bump / --disable-release-bump pair:
func TestAddReleaseBumpFlag(t *testing.T) {
t.Parallel()
cmd := &cobra.Command{Use: "test"}
addReleaseBumpFlag(cmd)
deprecated := cmd.Flags().Lookup("disable-release-bump")
if deprecated == nil {
t.Fatal("addReleaseBumpFlag should register --disable-release-bump")
}
if deprecated.Deprecated != "use --no-release-bump instead" {
t.Errorf("unexpected deprecation message: %q", deprecated.Deprecated)
}
}@copilot please address this.
| // them into a single effective value. | ||
| func resolveDeprecatedBoolFlag(cmd *cobra.Command, newName, oldName string) bool { | ||
| newVal, _ := cmd.Flags().GetBool(newName) | ||
| oldVal, _ := cmd.Flags().GetBool(oldName) |
There was a problem hiding this comment.
[/codebase-design] resolveDeprecatedBoolFlag silently discards errors from GetBool. If either flag name is misspelled at a call site, the function returns false without any indication of the mistake — the call site will behave incorrectly and no test will catch it at registration time.
💡 Suggestion
Panic (or use must-style) on a programming error at startup, consistent with how cobra itself handles unknown-flag access in tests:
func resolveDeprecatedBoolFlag(cmd *cobra.Command, newName, oldName string) bool {
newVal, err := cmd.Flags().GetBool(newName)
if err != nil {
panic(fmt.Sprintf("resolveDeprecatedBoolFlag: flag %q not registered: %v", newName, err))
}
oldVal, err := cmd.Flags().GetBool(oldName)
if err != nil {
panic(fmt.Sprintf("resolveDeprecatedBoolFlag: flag %q not registered: %v", oldName, err))
}
return newVal || oldVal
}A panic at startup is far easier to diagnose than a silent false in production. Alternatively, return (bool, error) if callers can handle it, but a panic is idiomatic for programmer errors in cobra-based CLIs.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
ContextThis PR consolidates 6 per-command deprecated-flag tests (removed from Verdict
|
There was a problem hiding this comment.
Review: refactor consolidation is sound — one follow-up needed
The core refactor is clean and correct. addSecurityScannerFlag and resolveDeprecatedBoolFlag are well-named, well-tested, and properly applied across all five command files.
One inconsistency: the analogous no-release-bump/disable-release-bump pair in update_command.go had its resolution migrated to resolveDeprecatedBoolFlag but the 3-line registration block was left in place (not extracted to a helper). See the inline comment for the suggested fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19 AIC · ⌖ 5.3 AIC · ⊞ 5K
| @@ -175,9 +171,7 @@ Note: In GitHub Enterprise repos, shorthand source specs resolve on your enterpr | |||
| cmd.Flags().Bool("no-release-bump", false, "Restrict automatic major version bumps to core actions/* only (non-core actions are left as-is)") | |||
| cmd.Flags().Bool("disable-release-bump", false, "Restrict automatic major version bumps to core actions/* only (non-core actions are left as-is)") | |||
| _ = cmd.Flags().MarkDeprecated("disable-release-bump", "use --no-release-bump instead") | |||
There was a problem hiding this comment.
The no-release-bump/disable-release-bump registration block (lines 171–173) was not extracted into a helper the way the security-scanner pair was. For full consistency — and so that the deprecated-alias contract is tested centrally — consider adding an addReleaseBumpFlag(cmd) helper in flags.go (mirroring addSecurityScannerFlag) and a matching TestAddReleaseBumpFlag test in flags_test.go. The deleted TestNewUpdateCommand_DeprecatesDisableReleaseBumpFlag had value; removing it without a central replacement leaves the deprecation message untested. @copilot please address this.
The
--no-security-scanner/--disable-security-scannerregistration + resolution pattern was copy-pasted across 5 command files (~20 lines of identical logic, 5 near-duplicate tests). One test had already drifted in naming (HasDisableSecurityScannerFlagvsDeprecatesDisableSecurityScannerFlag), confirming the duplication was causing inconsistency.Changes
pkg/cli/flags.go— Two new helpers:addSecurityScannerFlag(cmd): registers--no-security-scanner+ deprecated--disable-security-scanneraliasresolveDeprecatedBoolFlag(cmd, newName, oldName string) bool: collapses a renamed flag pair into a single effective value5 command files — Replace copy-pasted 3-line registration and 3-line resolution blocks with the helpers.
resolveDeprecatedBoolFlagalso applied to the analogousno-release-bump/disable-release-bumppair inupdate_command.goanddelete-host-repo-before/force-delete-host-repo-beforeintrial_command.go.Tests — Remove 6 duplicated per-command deprecation tests; add
TestAddSecurityScannerFlagandTestResolveDeprecatedBoolFlag(subtests: both-false, new-set, old-set) inflags_test.goas the single authoritative location.