Skip to content

Commit 0879e8d

Browse files
committed
address review comments
Signed-off-by: Sanjit Mohanty <[email protected]>
1 parent 432bbd4 commit 0879e8d

File tree

9 files changed

+227
-49
lines changed

9 files changed

+227
-49
lines changed

acceptance/handle_config_file_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ username123: username
102102
Expect(err).ToNot(HaveOccurred())
103103

104104
Eventually(session, "5s").Should(gexec.Exit(1))
105-
Expect(session.Err.Contents()).To(ContainSubstring("unknown flag `username123'"))
105+
Expect(session.Err.Contents()).To(ContainSubstring("unknown flag(s) [\"--username123\"]"))
106106
})
107107
})
108108

cmd/loadConfigFile.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func loadConfigFile(args []string, envFunc func() []string) ([]string, error) {
4444
Vars []string `long:"var" short:"v"`
4545
}
4646

47-
parser := flags.NewParser(&config, flags.None)
47+
parser := flags.NewParser(&config, flags.IgnoreUnknown)
4848
args, err = parser.ParseArgs(args)
4949
configFile := config.ConfigFile
5050
if configFile == "" {

cmd/loadConfigFile_test.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package cmd
22

33
import (
4-
"testing"
5-
64
. "github.com/onsi/ginkgo/v2"
75
. "github.com/onsi/gomega"
86
)
@@ -22,8 +20,3 @@ var _ = Describe("parseOptions", func() {
2220

2321
})
2422
})
25-
26-
func TestCmds(t *testing.T) {
27-
RegisterFailHandler(Fail)
28-
RunSpecs(t, "Cmds")
29-
}

cmd/main.go

Lines changed: 125 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -659,11 +659,14 @@ func Main(sout io.Writer, serr io.Writer, version string, applySleepDurationStri
659659
return err
660660
}
661661

662-
parser.Options |= flags.HelpFlag
663-
664-
_, err = parser.ParseArgs(args)
665-
662+
// Strict flag validation block
663+
err = validateCommandFlags(parser, args)
666664
if err != nil {
665+
return err
666+
}
667+
668+
parser.Options |= flags.HelpFlag
669+
if _, err = parser.ParseArgs(args); err != nil {
667670
if e, ok := err.(*flags.Error); ok {
668671
switch e.Type {
669672
case flags.ErrHelp, flags.ErrCommandRequired:
@@ -787,3 +790,121 @@ func checkForVars(opts *options) error {
787790

788791
return nil
789792
}
793+
794+
// validateCommandFlags checks if the provided command flags are valid for the given command.
795+
func validateCommandFlags(parser *flags.Parser, args []string) error {
796+
// If no args, return nil
797+
if len(args) == 0 {
798+
return nil
799+
}
800+
801+
// Find the command to validate flags for
802+
cmdName := args[0]
803+
var selectedCmd *flags.Command
804+
for _, cmd := range parser.Commands() {
805+
if cmd.Name == cmdName || contains(cmd.Aliases, cmdName) {
806+
selectedCmd = cmd
807+
break
808+
}
809+
}
810+
// Unknown command, let parser handle it
811+
if selectedCmd == nil {
812+
return nil
813+
}
814+
815+
// Find unknown flags
816+
invalidFlags := findUnknownFlags(selectedCmd, args)
817+
818+
// If there are unknown flags, print an error and return
819+
if len(invalidFlags) > 0 {
820+
fmt.Fprintf(os.Stderr, "Error: unknown flag(s) %q for command '%s'\n", invalidFlags, selectedCmd.Name)
821+
fmt.Fprintf(os.Stderr, "See 'om %s --help' for available options.\n", selectedCmd.Name)
822+
return fmt.Errorf("unknown flag(s) %q for command '%s'", invalidFlags, selectedCmd.Name)
823+
}
824+
return nil
825+
}
826+
827+
// findUnknownFlags checks for unknown flags in the provided args for the given command.
828+
func findUnknownFlags(selectedCmd *flags.Command, args []string) []string {
829+
validFlags := make(map[string]bool)
830+
addFlag := func(name string, takesValue bool) {
831+
validFlags[name] = takesValue
832+
}
833+
for _, opt := range selectedCmd.Options() {
834+
val := opt.Value()
835+
_, isBool := val.(*bool)
836+
_, isBoolSlice := val.(*[]bool)
837+
takesValue := !(isBool || isBoolSlice)
838+
if ln := opt.LongNameWithNamespace(); ln != "" {
839+
addFlag("--"+ln, takesValue)
840+
}
841+
if opt.ShortName != 0 {
842+
addFlag("-"+string(opt.ShortName), takesValue)
843+
}
844+
}
845+
addFlag("--help", false)
846+
addFlag("-h", false)
847+
848+
var invalidFlags []string
849+
i := 1
850+
for i < len(args) {
851+
arg := args[i]
852+
if !strings.HasPrefix(arg, "-") {
853+
// Not a flag, just a value
854+
// Example: args = ["upload-product", "file.pivotal"]
855+
// "file.pivotal" is a positional argument or a value for a previous flag
856+
i++
857+
continue
858+
}
859+
860+
// Split flag and value if --flag=value
861+
flagName, hasEquals := arg, false
862+
if eqIdx := strings.Index(arg, "="); eqIdx != -1 {
863+
flagName = arg[:eqIdx]
864+
hasEquals = true
865+
// Example: arg = "--product=foo.pivotal" -> flagName = "--product", value = "foo.pivotal"
866+
}
867+
868+
takesValue, isValid := validFlags[flagName]
869+
if !isValid {
870+
// Unknown flag
871+
// Example: arg = "--notaflag" (not defined in command options)
872+
invalidFlags = append(invalidFlags, flagName)
873+
i++
874+
continue
875+
}
876+
877+
if takesValue {
878+
if hasEquals {
879+
// --flag=value, value is in this arg
880+
// Example: arg = "--product=foo.pivotal"
881+
i++
882+
} else if i+1 < len(args) {
883+
// --flag value, value is next arg (even if it looks like a flag)
884+
// Example: args = ["--product", "--notaflag"]
885+
// "--notaflag" is treated as the value for --product, not as a flag
886+
i += 2
887+
} else {
888+
// --flag with missing value.
889+
// No need to handle this here as this will handled appropriately by the parser.
890+
// Example: args = ["--product"] (no value provided)
891+
i++
892+
}
893+
} else {
894+
// Boolean flag, no value expected
895+
// Example: arg = "--help"
896+
i++
897+
}
898+
}
899+
return invalidFlags
900+
}
901+
902+
// contains checks if a string is present in a list of strings.
903+
func contains(list []string, s string) bool {
904+
for _, v := range list {
905+
if v == s {
906+
return true
907+
}
908+
}
909+
return false
910+
}

cmd/suite_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package cmd
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/ginkgo/v2"
7+
. "github.com/onsi/gomega"
8+
)
9+
10+
func TestCmd(t *testing.T) {
11+
RegisterFailHandler(Fail)
12+
RunSpecs(t, "Cmd Suite")
13+
}

cmd/validate_command_flags_test.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package cmd
2+
3+
import (
4+
. "github.com/onsi/ginkgo/v2"
5+
. "github.com/onsi/gomega"
6+
7+
"github.com/jessevdk/go-flags"
8+
)
9+
10+
var _ = Describe("validateCommandFlags", func() {
11+
type uploadProductOptions struct {
12+
Product string `long:"product" short:"p" description:"path to product" required:"true"`
13+
PollingInterval int `long:"polling-interval" short:"i" description:"interval (in seconds) at which to print status" default:"1"`
14+
Shasum string `long:"shasum" description:"shasum of the provided product file to be used for validation"`
15+
Version string `long:"product-version" description:"version of the provided product file to be used for validation"`
16+
Config string `long:"config" short:"c" description:"path to yml file for configuration"`
17+
VarsEnv string `long:"vars-env" description:"load variables from environment variables matching the provided prefix"`
18+
VarsFile []string `long:"vars-file" short:"l" description:"load variables from a YAML file"`
19+
Var []string `long:"var" short:"v" description:"load variable from the command line. Format: VAR=VAL"`
20+
}
21+
22+
var parser *flags.Parser
23+
24+
BeforeEach(func() {
25+
parser = flags.NewParser(nil, flags.Default)
26+
parser.AddCommand("upload-product", "desc", "long desc", &uploadProductOptions{})
27+
})
28+
29+
DescribeTable("flag validation",
30+
func(args []string, wantErr bool, errMsg string) {
31+
err := validateCommandFlags(parser, args)
32+
if wantErr {
33+
Expect(err).To(HaveOccurred())
34+
Expect(err.Error()).To(ContainSubstring(errMsg))
35+
} else {
36+
Expect(err).ToNot(HaveOccurred())
37+
}
38+
},
39+
Entry("no args", []string{}, false, ""),
40+
Entry("valid flags", []string{"upload-product", "--product", "file.pivotal", "--polling-interval", "5", "--shasum", "abc123", "--product-version", "2.3.4"}, false, ""),
41+
Entry("valid short flags", []string{"upload-product", "-p", "file.pivotal", "-i", "5"}, false, ""),
42+
Entry("all config interpolation flags", []string{"upload-product", "-c", "config.yml", "--vars-env", "MY", "-l", "vars1.yml", "-l", "vars2.yml", "-v", "FOO=bar", "-v", "BAZ=qux", "-p", "file.pivotal"}, false, ""),
43+
Entry("mix config and main flags", []string{"upload-product", "-p", "file.pivotal", "-c", "config.yml", "--vars-env", "MY", "--shasum", "abc123", "-l", "vars.yml", "-v", "FOO=bar"}, false, ""),
44+
Entry("unknown flag with config flags", []string{"upload-product", "-p", "file.pivotal", "-c", "config.yml", "--notaflag"}, true, "unknown flag(s)"),
45+
Entry("unknown flag", []string{"upload-product", "--notaflag"}, true, "unknown flag(s)"),
46+
Entry("multiple unknown flags", []string{"upload-product", "--foo", "--bar"}, true, "unknown flag(s)"),
47+
Entry("flag value looks like flag", []string{"upload-product", "--product", "--notaflag"}, false, ""),
48+
Entry("unknown short flags", []string{"upload-product", "-p", "file.pivotal", "-x", "18000", "-z", "18000"}, true, "unknown flag(s)"),
49+
)
50+
})

go.mod

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,8 @@ require (
9797
github.com/golang-jwt/jwt/v4 v4.5.1 // indirect
9898
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
9999
github.com/golang/snappy v0.0.4 // indirect
100-
github.com/google/go-cmp v0.6.0 // indirect
101-
github.com/google/pprof v0.0.0-20241101162523-b92577c0c142 // indirect
100+
github.com/google/go-cmp v0.7.0 // indirect
101+
github.com/google/pprof v0.0.0-20250403155104-27863c87afa6 // indirect
102102
github.com/google/s2a-go v0.1.8 // indirect
103103
github.com/google/uuid v1.6.0 // indirect
104104
github.com/googleapis/enterprise-certificate-proxy v0.3.4 // indirect
@@ -143,21 +143,21 @@ require (
143143
go.opentelemetry.io/otel/sdk v1.32.0 // indirect
144144
go.opentelemetry.io/otel/sdk/metric v1.32.0 // indirect
145145
go.opentelemetry.io/otel/trace v1.32.0 // indirect
146-
golang.org/x/crypto v0.29.0 // indirect
147-
golang.org/x/mod v0.22.0 // indirect
148-
golang.org/x/net v0.31.0 // indirect
149-
golang.org/x/sync v0.9.0 // indirect
150-
golang.org/x/sys v0.27.0 // indirect
151-
golang.org/x/term v0.26.0 // indirect
152-
golang.org/x/text v0.20.0 // indirect
146+
golang.org/x/crypto v0.36.0 // indirect
147+
golang.org/x/mod v0.24.0 // indirect
148+
golang.org/x/net v0.37.0 // indirect
149+
golang.org/x/sync v0.12.0 // indirect
150+
golang.org/x/sys v0.32.0 // indirect
151+
golang.org/x/term v0.30.0 // indirect
152+
golang.org/x/text v0.23.0 // indirect
153153
golang.org/x/time v0.8.0 // indirect
154-
golang.org/x/tools v0.27.0 // indirect
154+
golang.org/x/tools v0.31.0 // indirect
155155
google.golang.org/genproto v0.0.0-20241104194629-dd2ea8efbc28 // indirect
156156
google.golang.org/genproto/googleapis/api v0.0.0-20241104194629-dd2ea8efbc28 // indirect
157157
google.golang.org/genproto/googleapis/rpc v0.0.0-20241104194629-dd2ea8efbc28 // indirect
158158
google.golang.org/grpc v1.68.0 // indirect
159159
google.golang.org/grpc/stats/opentelemetry v0.0.0-20241028142157-ada6787961b3 // indirect
160-
google.golang.org/protobuf v1.35.1 // indirect
160+
google.golang.org/protobuf v1.36.5 // indirect
161161
gopkg.in/cheggaaa/pb.v1 v1.0.28 // indirect
162162
gopkg.in/go-playground/assert.v1 v1.2.1 // indirect
163163
)

0 commit comments

Comments
 (0)