From a4421f741227e6b58154162097444de124747916 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 25 Oct 2021 15:17:43 +0200 Subject: [PATCH 1/3] add pkg/flag from Skopeo Add pkg/flag to properly parse optional bools. Skopeo is using this code in `cmd/skopeo` for parsing the `--tls-verify` flag. Moving the code into a new package here allows for code share. Backport of commit 82bebf36462a for fixing an issue in Podman v3.4. Signed-off-by: Valentin Rothberg --- pkg/flag/flag.go | 174 +++++++++++++++++++++++++++++++++ pkg/flag/flag_test.go | 222 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 396 insertions(+) create mode 100644 pkg/flag/flag.go create mode 100644 pkg/flag/flag_test.go diff --git a/pkg/flag/flag.go b/pkg/flag/flag.go new file mode 100644 index 000000000..52eb50da0 --- /dev/null +++ b/pkg/flag/flag.go @@ -0,0 +1,174 @@ +package flag + +import ( + "strconv" + + "github.com/spf13/pflag" +) + +// OptionalBool is a boolean with a separate presence flag and value. +type OptionalBool struct { + present bool + value bool +} + +// Present returns the bool's presence flag. +func (ob *OptionalBool) Present() bool { + return ob.present +} + +// Present returns the bool's value. Should only be used if Present() is true. +func (ob *OptionalBool) Value() bool { + return ob.value +} + +// optionalBool is a cli.Generic == flag.Value implementation equivalent to +// the one underlying flag.Bool, except that it records whether the flag has been set. +// This is distinct from optionalBool to (pretend to) force callers to use +// optionalBoolFlag +type optionalBoolValue OptionalBool + +// OptionalBoolFlag creates new flag for an optional in the specified flag with +// the specified name and usage. +func OptionalBoolFlag(fs *pflag.FlagSet, p *OptionalBool, name, usage string) *pflag.Flag { + flag := fs.VarPF(internalNewOptionalBoolValue(p), name, "", usage) + flag.NoOptDefVal = "true" + flag.DefValue = "false" + return flag +} + +// WARNING: Do not directly use this method to define optionalBool flag. +// Caller should use optionalBoolFlag +func internalNewOptionalBoolValue(p *OptionalBool) pflag.Value { + p.present = false + return (*optionalBoolValue)(p) +} + +// Set parses the string to a bool and sets it. +func (ob *optionalBoolValue) Set(s string) error { + v, err := strconv.ParseBool(s) + if err != nil { + return err + } + ob.value = v + ob.present = true + return nil +} + +// String returns the string representation of the string. +func (ob *optionalBoolValue) String() string { + if !ob.present { + return "" // This is, sadly, not round-trip safe: --flag is interpreted as --flag=true + } + return strconv.FormatBool(ob.value) +} + +// Type returns the type. +func (ob *optionalBoolValue) Type() string { + return "bool" +} + +// IsBoolFlag indicates that it's a bool flag. +func (ob *optionalBoolValue) IsBoolFlag() bool { + return true +} + +// OptionalString is a string with a separate presence flag. +type OptionalString struct { + present bool + value string +} + +// Present returns the strings's presence flag. +func (os *OptionalString) Present() bool { + return os.present +} + +// Present returns the string's value. Should only be used if Present() is true. +func (os *OptionalString) Value() string { + return os.value +} + +// optionalString is a cli.Generic == flag.Value implementation equivalent to +// the one underlying flag.String, except that it records whether the flag has been set. +// This is distinct from optionalString to (pretend to) force callers to use +// newoptionalString +type optionalStringValue OptionalString + +// NewOptionalStringValue returns a pflag.Value fo the string. +func NewOptionalStringValue(p *OptionalString) pflag.Value { + p.present = false + return (*optionalStringValue)(p) +} + +// Set sets the string. +func (ob *optionalStringValue) Set(s string) error { + ob.value = s + ob.present = true + return nil +} + +// String returns the string if present. +func (ob *optionalStringValue) String() string { + if !ob.present { + return "" // This is, sadly, not round-trip safe: --flag= is interpreted as {present:true, value:""} + } + return ob.value +} + +// Type returns the string type. +func (ob *optionalStringValue) Type() string { + return "string" +} + +// OptionalInt is a int with a separate presence flag. +type OptionalInt struct { + present bool + value int +} + +// Present returns the int's presence flag. +func (oi *OptionalInt) Present() bool { + return oi.present +} + +// Present returns the int's value. Should only be used if Present() is true. +func (oi *OptionalInt) Value() int { + return oi.value +} + +// optionalInt is a cli.Generic == flag.Value implementation equivalent to +// the one underlying flag.Int, except that it records whether the flag has been set. +// This is distinct from optionalInt to (pretend to) force callers to use +// newoptionalIntValue +type optionalIntValue OptionalInt + +// NewOptionalIntValue returns the pflag.Value of the int. +func NewOptionalIntValue(p *OptionalInt) pflag.Value { + p.present = false + return (*optionalIntValue)(p) +} + +// Set parses the string to an int and sets it. +func (ob *optionalIntValue) Set(s string) error { + v, err := strconv.ParseInt(s, 0, strconv.IntSize) + if err != nil { + return err + } + ob.value = int(v) + ob.present = true + return nil +} + +// String returns the string representation of the int. +func (ob *optionalIntValue) String() string { + if !ob.present { + return "" // If the value is not present, just return an empty string, any other value wouldn't make sense. + } + return strconv.Itoa(int(ob.value)) +} + +// Type returns the int's type. +func (ob *optionalIntValue) Type() string { + return "int" +} diff --git a/pkg/flag/flag_test.go b/pkg/flag/flag_test.go new file mode 100644 index 000000000..8db97f31d --- /dev/null +++ b/pkg/flag/flag_test.go @@ -0,0 +1,222 @@ +package flag + +import ( + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOptionalBoolSet(t *testing.T) { + for _, c := range []struct { + input string + accepted bool + value bool + }{ + // Valid inputs documented for strconv.ParseBool == flag.BoolVar + {"1", true, true}, + {"t", true, true}, + {"T", true, true}, + {"TRUE", true, true}, + {"true", true, true}, + {"True", true, true}, + {"0", true, false}, + {"f", true, false}, + {"F", true, false}, + {"FALSE", true, false}, + {"false", true, false}, + {"False", true, false}, + // A few invalid inputs + {"", false, false}, + {"yes", false, false}, + {"no", false, false}, + {"2", false, false}, + } { + var ob OptionalBool + v := internalNewOptionalBoolValue(&ob) + require.False(t, ob.Present()) + err := v.Set(c.input) + if c.accepted { + assert.NoError(t, err, c.input) + assert.Equal(t, c.value, ob.Value()) + } else { + assert.Error(t, err, c.input) + assert.False(t, ob.Present()) // Just to be extra paranoid. + } + } + + // Nothing actually explicitly says that .Set() is never called when the flag is not present on the command line; + // so, check that it is not being called, at least in the straightforward case (it's not possible to test that it + // is not called in any possible situation). + var globalOB, commandOB OptionalBool + actionRun := false + app := &cobra.Command{ + Use: "app", + } + OptionalBoolFlag(app.PersistentFlags(), &globalOB, "global-OB", "") + cmd := &cobra.Command{ + Use: "cmd", + RunE: func(cmd *cobra.Command, args []string) error { + assert.False(t, globalOB.Present()) + assert.False(t, commandOB.Present()) + actionRun = true + return nil + }, + } + OptionalBoolFlag(cmd.Flags(), &commandOB, "command-OB", "") + app.AddCommand(cmd) + app.SetArgs([]string{"cmd"}) + err := app.Execute() + require.NoError(t, err) + assert.True(t, actionRun) +} + +func TestOptionalBoolString(t *testing.T) { + for _, c := range []struct { + input OptionalBool + expected string + }{ + {OptionalBool{present: true, value: true}, "true"}, + {OptionalBool{present: true, value: false}, "false"}, + {OptionalBool{present: false, value: true}, ""}, + {OptionalBool{present: false, value: false}, ""}, + } { + var ob OptionalBool + v := internalNewOptionalBoolValue(&ob) + ob = c.input + res := v.String() + assert.Equal(t, c.expected, res) + } +} + +func TestOptionalBoolIsBoolFlag(t *testing.T) { + // IsBoolFlag means that the argument value must either be part of the same argument, with =; + // if there is no =, the value is set to true. + // This differs form other flags, where the argument is required and may be either separated with = or supplied in the next argument. + for _, c := range []struct { + input []string + expectedOB OptionalBool + expectedArgs []string + }{ + {[]string{"1", "2"}, OptionalBool{present: false}, []string{"1", "2"}}, // Flag not present + {[]string{"--OB=true", "1", "2"}, OptionalBool{present: true, value: true}, []string{"1", "2"}}, // --OB=true + {[]string{"--OB=false", "1", "2"}, OptionalBool{present: true, value: false}, []string{"1", "2"}}, // --OB=false + {[]string{"--OB", "true", "1", "2"}, OptionalBool{present: true, value: true}, []string{"true", "1", "2"}}, // --OB true + {[]string{"--OB", "false", "1", "2"}, OptionalBool{present: true, value: true}, []string{"false", "1", "2"}}, // --OB false + } { + var ob OptionalBool + actionRun := false + app := &cobra.Command{Use: "app"} + cmd := &cobra.Command{ + Use: "cmd", + RunE: func(cmd *cobra.Command, args []string) error { + assert.Equal(t, c.expectedOB, ob) // nolint + assert.Equal(t, c.expectedArgs, args) //nolint + actionRun = true + return nil + }, + } + OptionalBoolFlag(cmd.Flags(), &ob, "OB", "") + app.AddCommand(cmd) + + app.SetArgs(append([]string{"cmd"}, c.input...)) + err := app.Execute() + require.NoError(t, err) + assert.True(t, actionRun) + } +} + +func TestOptionalStringSet(t *testing.T) { + // Really just a smoke test, but differentiating between not present and empty. + for _, c := range []string{"", "hello"} { + var os OptionalString + v := NewOptionalStringValue(&os) + require.False(t, os.Present()) + err := v.Set(c) + assert.NoError(t, err, c) + assert.Equal(t, c, os.Value()) + } + + // Nothing actually explicitly says that .Set() is never called when the flag is not present on the command line; + // so, check that it is not being called, at least in the straightforward case (it's not possible to test that it + // is not called in any possible situation). + var globalOS, commandOS OptionalString + actionRun := false + app := &cobra.Command{ + Use: "app", + } + app.PersistentFlags().Var(NewOptionalStringValue(&globalOS), "global-OS", "") + cmd := &cobra.Command{ + Use: "cmd", + RunE: func(cmd *cobra.Command, args []string) error { + assert.False(t, globalOS.Present()) + assert.False(t, commandOS.Present()) + actionRun = true + return nil + }, + } + cmd.Flags().Var(NewOptionalStringValue(&commandOS), "command-OS", "") + app.AddCommand(cmd) + app.SetArgs([]string{"cmd"}) + err := app.Execute() + require.NoError(t, err) + assert.True(t, actionRun) +} + +func TestOptionalStringString(t *testing.T) { + for _, c := range []struct { + input OptionalString + expected string + }{ + {OptionalString{present: true, value: "hello"}, "hello"}, + {OptionalString{present: true, value: ""}, ""}, + {OptionalString{present: false, value: "hello"}, ""}, + {OptionalString{present: false, value: ""}, ""}, + } { + var os OptionalString + v := NewOptionalStringValue(&os) + os = c.input + res := v.String() + assert.Equal(t, c.expected, res) + } +} + +func TestOptionalStringIsBoolFlag(t *testing.T) { + // NOTE: OptionalStringValue does not implement IsBoolFlag! + // IsBoolFlag means that the argument value must either be part of the same argument, with =; + // if there is no =, the value is set to true. + // This differs form other flags, where the argument is required and may be either separated with = or supplied in the next argument. + for _, c := range []struct { + input []string + expectedOS OptionalString + expectedArgs []string + }{ + {[]string{"1", "2"}, OptionalString{present: false}, []string{"1", "2"}}, // Flag not present + {[]string{"--OS=hello", "1", "2"}, OptionalString{present: true, value: "hello"}, []string{"1", "2"}}, // --OS=true + {[]string{"--OS=", "1", "2"}, OptionalString{present: true, value: ""}, []string{"1", "2"}}, // --OS=false + {[]string{"--OS", "hello", "1", "2"}, OptionalString{present: true, value: "hello"}, []string{"1", "2"}}, // --OS true + {[]string{"--OS", "", "1", "2"}, OptionalString{present: true, value: ""}, []string{"1", "2"}}, // --OS false + } { + var os OptionalString + actionRun := false + app := &cobra.Command{ + Use: "app", + } + cmd := &cobra.Command{ + Use: "cmd", + RunE: func(cmd *cobra.Command, args []string) error { + assert.Equal(t, c.expectedOS, os) // nolint + assert.Equal(t, c.expectedArgs, args) // nolint + actionRun = true + return nil + }, + } + cmd.Flags().Var(NewOptionalStringValue(&os), "OS", "") + app.AddCommand(cmd) + app.SetArgs(append([]string{"cmd"}, c.input...)) + err := app.Execute() + require.NoError(t, err) + assert.True(t, actionRun) + } +} From 86c1511eb2d87ad85b4a1242747f28cd61f42a08 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 30 Nov 2021 10:57:37 +0100 Subject: [PATCH 2/3] v0.44.4 * add pkg/flag from Skopeo (backport) Signed-off-by: Valentin Rothberg --- version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/version.go b/version/version.go index 1b96108c9..f9c779e87 100644 --- a/version/version.go +++ b/version/version.go @@ -1,4 +1,4 @@ package version // Version is the version of the build. -const Version = "0.44.4-dev" +const Version = "0.44.4" From 7334a53d7294172eb0d846bbcbf35b349b0deea8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 30 Nov 2021 11:00:12 +0100 Subject: [PATCH 3/3] bump to v0.44.5-dev Signed-off-by: Valentin Rothberg --- version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/version.go b/version/version.go index f9c779e87..833012552 100644 --- a/version/version.go +++ b/version/version.go @@ -1,4 +1,4 @@ package version // Version is the version of the build. -const Version = "0.44.4" +const Version = "0.44.5-dev"