Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String Slices from pflags are not handled correctly by viper. #200

Closed
aarondl opened this issue Jun 12, 2016 · 6 comments · Fixed by #296
Closed

String Slices from pflags are not handled correctly by viper. #200

aarondl opened this issue Jun 12, 2016 · 6 comments · Fixed by #296

Comments

@aarondl
Copy link

aarondl commented Jun 12, 2016

The slice of strings just contains one element that is the stringified version of the string
ie: []string{"[firstarg,secondarg]"}
or in the case of nil/empty: []string{"[]"}

It appears that this is coming from the Pflag libraries ValueString() function. I've included a patch that "helps" the issue but I suspect it's a little bit more deep-seated as the cast package also doesn't seem to handle this case well at all hence I'm not making trying to make a PR for this.

For others hitting this issue the workaround is to simply use pflags directly for string slices until this is fixed.

diff --git a/viper.go b/viper.go
index a2633b0..d1cce44 100644
--- a/viper.go
+++ b/viper.go
@@ -479,6 +479,8 @@ func (v *Viper) Get(key string) interface{} {
                                val = cast.ToInt(flag.ValueString())
                        case "bool":
                                val = cast.ToBool(flag.ValueString())
+                       case "stringSlice":
+                               val = []string{}
                        default:
                                val = flag.ValueString()
                        }
@@ -742,6 +744,8 @@ func (v *Viper) find(key string) interface{} {
                        return cast.ToInt(flag.ValueString())
                case "bool":
                        return cast.ToBool(flag.ValueString())
+               case "stringSlice":
+                       return strings.Split(strings.Trim(flag.ValueString(), "[]"), ",")
                default:
                        return flag.ValueString()
                }

Example program that shows the the failure:

package main

import (
    "fmt"
    "os"

    "github.com/davecgh/go-spew/spew"
    "github.com/spf13/cobra"
    "github.com/spf13/viper"
)

func main() {
    var rootCmd = &cobra.Command{
        Run: run,
    }

    rootCmd.PersistentFlags().StringSliceP("thing", "t", nil, "")
    viper.BindPFlags(rootCmd.PersistentFlags())

    if err := rootCmd.Execute(); err != nil {
        fmt.Printf("\n%+v\n", err)
        os.Exit(1)
    }
}

func run(cmd *cobra.Command, args []string) {
    str := viper.Get("thing")
    slice := viper.GetStringSlice("thing")
    slicePFlag, _ := cmd.PersistentFlags().GetStringSlice("thing")

    spew.Dump(str, slice, slicePFlag)
}

Output (annotated for clarity) of go run main.go

str   = (string) (len=2) "[]"
slice = ([]string) (len=1 cap=1) {
 (string) (len=2) "[]"
}
slicePFlag = ([]string) {}

Output (annotated for clarity) of go run main.go -t thing1 -t thing2

str   = (string) (len=15) "[thing1,thing2]"
slice = ([]string) (len=1 cap=1) {
 (string) (len=15) "[thing1,thing2]"
}
slicePFlag = ([]string) (len=2 cap=2) {
 (string) (len=6) "thing1",
 (string) (len=6) "thing2"
}
@r2d4
Copy link

r2d4 commented Aug 10, 2016

cc @bep

Hi, just wondering whats the status of this issue? Is this the recommended workaround?

r2d4 added a commit to r2d4/minikube that referenced this issue Aug 12, 2016
This allows most flags for the minikube start command to be
configurable by viper as well.  If a flag is present, it will take
precedence over the value supplied in viper.

Viper doesn't handle string slices correctly (see
spf13/viper#200) so the string slices that we
pass in as flags such as docker-env and insecure-registry are still only
handled by flags
r2d4 added a commit to r2d4/minikube that referenced this issue Aug 24, 2016
This allows most flags for the minikube start command to be
configurable by viper as well.  If a flag is present, it will take
precedence over the value supplied in viper.

Viper doesn't handle string slices correctly (see
spf13/viper#200) so the string slices that we
pass in as flags such as docker-env and insecure-registry are still only
handled by flags
r2d4 added a commit to r2d4/minikube that referenced this issue Aug 24, 2016
This allows most flags for the minikube start command to be
configurable by viper as well.  If a flag is present, it will take
precedence over the value supplied in viper.

Viper doesn't handle string slices correctly (see
spf13/viper#200) so the string slices that we
pass in as flags such as docker-env and insecure-registry are still only
handled by flags

Delete custom flag for human readable disk size

Since the value can now be passed in multiple ways (environment
variable, flag, or config file), moving the conversion out of the flag
and straight into cmd/minikube/cmd/start.go

This changes the helptext for gendocs
jimmidyson pushed a commit to minishift/minishift that referenced this issue Sep 5, 2016
This allows most flags for the minikube start command to be
configurable by viper as well.  If a flag is present, it will take
precedence over the value supplied in viper.

Viper doesn't handle string slices correctly (see
spf13/viper#200) so the string slices that we
pass in as flags such as docker-env and insecure-registry are still only
handled by flags

Delete custom flag for human readable disk size

Since the value can now be passed in multiple ways (environment
variable, flag, or config file), moving the conversion out of the flag
and straight into cmd/minikube/cmd/start.go

This changes the helptext for gendocs
r2d4 added a commit to r2d4/minikube that referenced this issue Sep 19, 2016
This allows most flags for the minikube start command to be
configurable by viper as well.  If a flag is present, it will take
precedence over the value supplied in viper.

Viper doesn't handle string slices correctly (see
spf13/viper#200) so the string slices that we
pass in as flags such as docker-env and insecure-registry are still only
handled by flags

Delete custom flag for human readable disk size

Since the value can now be passed in multiple ways (environment
variable, flag, or config file), moving the conversion out of the flag
and straight into cmd/minikube/cmd/start.go

This changes the helptext for gendocs
@moorereason
Copy link
Contributor

@aarondl,

PR #244 submitted:

$ go run main.go 
([]string) {
}
([]string) {
}
([]string) {
}

$ go run main.go -t thing1 -t thing2
([]string) (len=2 cap=2) {
 (string) (len=6) "thing1",
 (string) (len=6) "thing2"
}
([]string) (len=2 cap=2) {
 (string) (len=6) "thing1",
 (string) (len=6) "thing2"
}
([]string) (len=2 cap=2) {
 (string) (len=6) "thing1",
 (string) (len=6) "thing2"
}

@awfm9
Copy link

awfm9 commented Sep 27, 2016

#240 might fix this, will check.

@Willyham
Copy link

The latest master still seems to have this problem:

$ go run foo.go --thing test --thing test
(string) (len=9) "test,test"
([]string) (len=1 cap=1) {
 (string) (len=9) "test,test"
}
([]string) (len=2 cap=2) {
 (string) (len=4) "test",
 (string) (len=4) "test"
}

@dansteen
Copy link

dansteen commented Jan 16, 2017

I have this issue as well. I am also using the latest version.

@orian
Copy link
Contributor

orian commented Jan 17, 2017

@dansteen The issue is there. In my pull request I've added a test and fix.

@bep bep closed this as completed in #296 Apr 17, 2017
concaf added a commit to concaf/opencompose that referenced this issue Jun 13, 2017
This commit updates the vendor directory using the
`make update-vendor` command.

This is required as an effort to resolve redhat-developer#167, but this is not the
complete resolution. This takes us a step forward by updating
github.com/spf13/viper, which resolves the issue as mentioned in
spf13/viper#200.
aduffeck added a commit to cloudfoundry-incubator/fissile that referenced this issue Apr 23, 2018
aduffeck added a commit to cloudfoundry-incubator/fissile that referenced this issue Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants