refactor: consolidate deprecated bool flag helpers into pkg/cli/flags.go #48680
There are no files selected for viewing
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,3 +300,68 @@ func TestEngineFlagUsageText(t *testing.T) { | |
| t.Errorf("Unexpected --engine filter usage text: %s", filterFlag.Usage) | ||
| } | ||
| } | ||
|
|
||
| func TestAddSecurityScannerFlag(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := &cobra.Command{Use: "test"} | ||
| addSecurityScannerFlag(cmd) | ||
|
|
||
| primary := cmd.Flags().Lookup("no-security-scanner") | ||
| if primary == nil { | ||
| t.Fatal("addSecurityScannerFlag should register --no-security-scanner") | ||
| } | ||
| if primary.Usage != "Skip security scanning of workflow markdown content" { | ||
| t.Errorf("Unexpected --no-security-scanner usage: %s", primary.Usage) | ||
| } | ||
|
|
||
| deprecated := cmd.Flags().Lookup("disable-security-scanner") | ||
| if deprecated == nil { | ||
| t.Fatal("addSecurityScannerFlag should register --disable-security-scanner as a deprecated alias") | ||
| } | ||
| if deprecated.Deprecated != "use --no-security-scanner instead" { | ||
| t.Errorf("Expected deprecation message 'use --no-security-scanner instead', got %q", deprecated.Deprecated) | ||
| } | ||
| } | ||
|
|
||
| func TestResolveDeprecatedBoolFlag(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| setup := func() *cobra.Command { | ||
| cmd := &cobra.Command{Use: "test"} | ||
| cmd.Flags().Bool("new-flag", false, "new flag") | ||
| cmd.Flags().Bool("old-flag", false, "old flag") | ||
| _ = cmd.Flags().MarkDeprecated("old-flag", "use --new-flag instead") | ||
| return cmd | ||
| } | ||
|
|
||
| t.Run("both false returns false", func(t *testing.T) { | ||
| t.Parallel() | ||
| cmd := setup() | ||
| if resolveDeprecatedBoolFlag(cmd, "new-flag", "old-flag") { | ||
| t.Error("expected false when both flags are unset") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("new flag true returns true", func(t *testing.T) { | ||
| t.Parallel() | ||
| cmd := setup() | ||
| if err := cmd.Flags().Set("new-flag", "true"); err != nil { | ||
| t.Fatalf("failed to set new-flag: %v", err) | ||
| } | ||
| if !resolveDeprecatedBoolFlag(cmd, "new-flag", "old-flag") { | ||
| t.Error("expected true when new flag is set") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("old flag true returns true", func(t *testing.T) { | ||
| t.Parallel() | ||
| cmd := setup() | ||
| if err := cmd.Flags().Set("old-flag", "true"); err != nil { | ||
| t.Fatalf("failed to set old-flag: %v", err) | ||
| } | ||
| if !resolveDeprecatedBoolFlag(cmd, "new-flag", "old-flag") { | ||
| t.Error("expected true when deprecated old flag is set") | ||
| } | ||
| }) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/tdd]
💡 SuggestionAdd a 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. |
||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,14 +78,10 @@ Note: In GitHub Enterprise repos, shorthand source specs resolve on your enterpr | |
| noStopAfter, _ := cmd.Flags().GetBool("no-stop-after") | ||
| stopAfter, _ := cmd.Flags().GetString("stop-after") | ||
| noMergeFlag, _ := cmd.Flags().GetBool("no-merge") | ||
| disableReleaseBump := resolveDeprecatedBoolFlag(cmd, "no-release-bump", "disable-release-bump") | ||
| noCompile, _ := cmd.Flags().GetBool("no-compile") | ||
| noRedirect, _ := cmd.Flags().GetBool("no-redirect") | ||
| disableSecurityScanner := resolveDeprecatedBoolFlag(cmd, "no-security-scanner", "disable-security-scanner") | ||
| approveFlag, _ := cmd.Flags().GetBool("approve") | ||
| createPRFlag, _ := cmd.Flags().GetBool("create-pull-request") | ||
| prFlagAlias, _ := cmd.Flags().GetBool("pr") | ||
|
|
@@ -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") | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] The 💡 SuggestionExtract an 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 @copilot please address this.
Contributor
There was a problem hiding this comment. The |
||
| addSecurityScannerFlag(cmd) | ||
| cmd.Flags().Bool("approve", false, "Approve all safe update changes. When strict mode is active (the default), the compiler emits warnings for new restricted secrets or unapproved action additions/removals not present in the existing gh-aw-manifest. Use this flag to approve and skip safe update enforcement") | ||
| cmd.Flags().Bool("no-compile", false, "Skip recompiling workflows during update (do not modify lock files)") | ||
| cmd.Flags().Bool("no-redirect", false, "Refuse updates when redirect frontmatter is present") | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
resolveDeprecatedBoolFlagsilently discards errors fromGetBool. If either flag name is misspelled at a call site, the function returnsfalsewithout 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:A panic at startup is far easier to diagnose than a silent
falsein 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.