[check-licenses] Refactor policy to nested subcommands Refactor PolicyCommand into a nested sub-commander that delegates to PolicyAddCommand. This moves the mandatory -bug and optional -desc flags to the add sub-command, aligning with standard CLI syntax (policy add -bug ...). Implemented UX error catching to detect flags placed after positional arguments and print helpful instructions. Updated usage strings and unit tests. Change-Id: I4a595781daf58d3a9c5dc89cf3f17b60e26c6603 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1602049 Reviewed-by: Jiaming Li <lijiaming@google.com> Commit-Queue: Jerry Belton <jcecil@google.com>
diff --git a/tools/check-licenses/cmd/policy.go b/tools/check-licenses/cmd/policy.go index 9bb5667..041509a 100644 --- a/tools/check-licenses/cmd/policy.go +++ b/tools/check-licenses/cmd/policy.go
@@ -19,15 +19,44 @@ ) type PolicyCommand struct { - fuchsiaDir string - bug string - description string + fuchsiaDir string } func (*PolicyCommand) Name() string { return "policy" } func (*PolicyCommand) Synopsis() string { return "Manage policy exceptions." } func (*PolicyCommand) Usage() string { - return `policy add <CheckName> <targetPath> -bug <BugID> [-desc <Description>]: + return `policy <subcommand> [options]: + Manage policy exceptions. + + Subcommands: + add Add a new policy exception. +` +} + +func (p *PolicyCommand) SetFlags(f *flag.FlagSet) { + f.StringVar(&p.fuchsiaDir, "fuchsia_dir", os.Getenv("FUCHSIA_DIR"), "Location of the fuchsia root directory.") +} + +func (p *PolicyCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus { + subFlags := flag.NewFlagSet("policy", flag.ContinueOnError) + if err := subFlags.Parse(f.Args()); err != nil { + return subcommands.ExitUsageError + } + subCommander := subcommands.NewCommander(subFlags, "policy") + subCommander.Register(&PolicyAddCommand{fuchsiaDir: p.fuchsiaDir}, "") + return subCommander.Execute(ctx) +} + +type PolicyAddCommand struct { + fuchsiaDir string + bug string + description string +} + +func (*PolicyAddCommand) Name() string { return "add" } +func (*PolicyAddCommand) Synopsis() string { return "Add a policy exception." } +func (*PolicyAddCommand) Usage() string { + return `add -bug <BugID> [-desc <Description>] <CheckName> <targetPath>: Adds a policy exception for the given project or file path. Flags: @@ -35,21 +64,89 @@ -desc Optional description for this exception. Examples: - fx check-licenses policy -bug b/123 add AllProjectsMustHaveALicense vendor/foo - fx check-licenses policy -bug b/456 -desc "Custom exception" add AllLicenseTextsMustBeRecognized third_party/bar/LICENSE - + fx check-licenses policy add -bug b/123 AllProjectsMustHaveALicense vendor/foo + fx check-licenses policy add -bug b/456 -desc "Custom exception" AllLicenseTextsMustBeRecognized third_party/bar/LICENSE ` } -func (p *PolicyCommand) SetFlags(f *flag.FlagSet) { - f.StringVar(&p.fuchsiaDir, "fuchsia_dir", os.Getenv("FUCHSIA_DIR"), "Location of the fuchsia root directory.") +func (p *PolicyAddCommand) SetFlags(f *flag.FlagSet) { f.StringVar(&p.bug, "bug", "", "Bug ID tracking this exception (Mandatory).") f.StringVar(&p.description, "desc", "Auto-generated exception", "Optional description for this exception.") } -func (p *PolicyCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus { - if f.NArg() != 3 || f.Arg(0) != "add" { - fmt.Fprintln(os.Stderr, "Usage: fx check-licenses policy -bug <BugID> [-desc <Description>] add <CheckName> <targetPath>") +func (p *PolicyAddCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus { + // UX Check: Detect misplaced flags + misplacedFlags := false + for _, arg := range f.Args() { + if strings.HasPrefix(arg, "-") { + misplacedFlags = true + break + } + } + + if misplacedFlags || f.NArg() != 2 { + var bugVal string + var descVal string + var positionals []string + + args := f.Args() + for i := 0; i < len(args); i++ { + arg := args[i] + if arg == "-bug" || arg == "--bug" { + if i+1 < len(args) { + bugVal = args[i+1] + i++ + } + } else if arg == "-desc" || arg == "--desc" { + if i+1 < len(args) { + descVal = args[i+1] + i++ + } + } else if strings.HasPrefix(arg, "-") { + // skip unknown flags + } else { + positionals = append(positionals, arg) + } + } + + if p.bug != "" && bugVal == "" { + bugVal = p.bug + } + if p.description != "Auto-generated exception" && descVal == "" { + descVal = p.description + } + + var cmdBuilder strings.Builder + cmdBuilder.WriteString("fx check-licenses policy add") + if bugVal != "" { + cmdBuilder.WriteString(fmt.Sprintf(" -bug %s", bugVal)) + } else { + cmdBuilder.WriteString(" -bug <BugID>") + } + if descVal != "" && descVal != "Auto-generated exception" { + cmdBuilder.WriteString(fmt.Sprintf(" -desc %q", descVal)) + } + + if len(positionals) > 0 { + cmdBuilder.WriteString(fmt.Sprintf(" %s", positionals[0])) + } else { + cmdBuilder.WriteString(" <CheckName>") + } + if len(positionals) > 1 { + cmdBuilder.WriteString(fmt.Sprintf(" %s", positionals[1])) + } else { + cmdBuilder.WriteString(" <targetPath>") + } + for _, extra := range positionals[2:] { + cmdBuilder.WriteString(fmt.Sprintf(" %s", extra)) + } + + if misplacedFlags { + fmt.Fprintln(os.Stderr, "❌ Error: Flags (like -bug or -desc) must be placed BEFORE positional arguments.") + } else { + fmt.Fprintln(os.Stderr, "❌ Error: Invalid number of arguments.") + } + fmt.Fprintf(os.Stderr, "Try running this copy-pasteable command instead:\n %s\n\n", cmdBuilder.String()) return subcommands.ExitUsageError } @@ -58,7 +155,7 @@ return subcommands.ExitUsageError } - checkName := f.Arg(1) + checkName := f.Arg(0) if !v2config.ValidPolicyChecks[checkName] { var validChecks []string for k := range v2config.ValidPolicyChecks { @@ -67,7 +164,7 @@ fmt.Fprintf(os.Stderr, "Error: invalid check name %q. Must be one of: %s\n", checkName, strings.Join(validChecks, ", ")) return subcommands.ExitUsageError } - targetPath := filepath.Clean(f.Arg(2)) + targetPath := filepath.Clean(f.Arg(1)) if err := AddPolicyException(p.fuchsiaDir, checkName, targetPath, p.bug, p.description); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err)
diff --git a/tools/check-licenses/cmd/policy_test.go b/tools/check-licenses/cmd/policy_test.go index bfe60a2..4c19b24 100644 --- a/tools/check-licenses/cmd/policy_test.go +++ b/tools/check-licenses/cmd/policy_test.go
@@ -52,7 +52,7 @@ // Test 2: Public project f2 := flag.NewFlagSet("test2", flag.ContinueOnError) cmd.SetFlags(f2) - f2.Parse([]string{"-bug", "b/123", "add", "AllProjectsMustHaveALicense", "src/foo/bar"}) + f2.Parse([]string{"add", "-bug", "b/123", "AllProjectsMustHaveALicense", "src/foo/bar"}) if status := cmd.Execute(ctx, f2); status != subcommands.ExitSuccess { t.Errorf("Expected ExitSuccess for public project, got %v", status) } @@ -77,7 +77,7 @@ // Test 3: Private project (vendor/...) f3 := flag.NewFlagSet("test3", flag.ContinueOnError) cmd.SetFlags(f3) - f3.Parse([]string{"-bug", "b/123", "add", "AllProjectsMustHaveALicense", "vendor/my_private_proj"}) + f3.Parse([]string{"add", "-bug", "b/123", "AllProjectsMustHaveALicense", "vendor/my_private_proj"}) if status := cmd.Execute(ctx, f3); status != subcommands.ExitSuccess { t.Errorf("Expected ExitSuccess for private project, got %v", status) } @@ -90,7 +90,7 @@ // Test 4: Already exists (should not fail, should exit success) f4 := flag.NewFlagSet("test4", flag.ContinueOnError) cmd.SetFlags(f4) - f4.Parse([]string{"-bug", "b/123", "add", "AllProjectsMustHaveALicense", "src/foo/bar"}) + f4.Parse([]string{"add", "-bug", "b/123", "AllProjectsMustHaveALicense", "src/foo/bar"}) if status := cmd.Execute(ctx, f4); status != subcommands.ExitSuccess { t.Errorf("Expected ExitSuccess when exception already exists, got %v", status) } @@ -98,7 +98,7 @@ // Test 5: Third party file grouping (should group by project name from manifest) f5 := flag.NewFlagSet("test5", flag.ContinueOnError) cmd.SetFlags(f5) - f5.Parse([]string{"-bug", "b/123", "add", "AllLicenseTextsMustBeRecognized", "vendor/my_private_proj/LICENSE"}) + f5.Parse([]string{"add", "-bug", "b/123", "AllLicenseTextsMustBeRecognized", "vendor/my_private_proj/LICENSE"}) if status := cmd.Execute(ctx, f5); status != subcommands.ExitSuccess { t.Errorf("Expected ExitSuccess for third party file, got %v", status) } @@ -115,6 +115,14 @@ if status := cmd.Execute(ctx, f6); status != subcommands.ExitUsageError { t.Errorf("Expected ExitUsageError for missing -bug flag, got %v", status) } + + // Test 7: Misplaced flags (UX Check, should fail) + f7 := flag.NewFlagSet("test7", flag.ContinueOnError) + cmd.SetFlags(f7) + f7.Parse([]string{"add", "AllProjectsMustHaveALicense", "src/foo/bar", "-bug", "b/123"}) + if status := cmd.Execute(ctx, f7); status != subcommands.ExitUsageError { + t.Errorf("Expected ExitUsageError for misplaced flags, got %v", status) + } } func TestPolicyCommand_Execute_AssemblyFailure(t *testing.T) { @@ -145,7 +153,7 @@ ctx := context.Background() f := flag.NewFlagSet("test_failure", flag.ContinueOnError) cmd.SetFlags(f) - f.Parse([]string{"-bug", "b/123", "add", "AllProjectsMustHaveALicense", "src/foo/bar"}) + f.Parse([]string{"add", "-bug", "b/123", "AllProjectsMustHaveALicense", "src/foo/bar"}) if status := cmd.Execute(ctx, f); status != subcommands.ExitFailure { t.Errorf("Expected ExitFailure for assembly error, got %v", status) @@ -187,7 +195,7 @@ f := flag.NewFlagSet("test_relative", flag.ContinueOnError) cmd.SetFlags(f) - f.Parse([]string{"-bug", "b/123", "add", "AllProjectsMustHaveALicense", "."}) // target is "." (src/my_project) + f.Parse([]string{"add", "-bug", "b/123", "AllProjectsMustHaveALicense", "."}) // target is "." (src/my_project) if status := cmd.Execute(ctx, f); status != subcommands.ExitSuccess { t.Errorf("Expected ExitSuccess, got %v", status) @@ -214,7 +222,7 @@ ctx := context.Background() f := flag.NewFlagSet("test_invalid_check", flag.ContinueOnError) cmd.SetFlags(f) - f.Parse([]string{"-bug", "b/123", "add", "InvalidCheckName", "src/foo/bar"}) + f.Parse([]string{"add", "-bug", "b/123", "InvalidCheckName", "src/foo/bar"}) if status := cmd.Execute(ctx, f); status != subcommands.ExitUsageError { t.Errorf("Expected ExitUsageError for invalid check name, got %v", status)