mwitkow/go-flagz

Dynamic flag management for Go.

go-flagz's Issues

Add support for <type>Var methods

FlagSets have methods for creating a flag with a pointer to the variable that stores the value. It would be nice to have that feature for consistency.

https://godoc.org/flag#FlagSet.BoolVar

Remove races due to using plain pointers

Normal pflag and normal flag package use plain pointers. This is useful if the assumption is that no values will change past initialization of a server.

However, for dynamic flag updating, it causes races. In practice, everything is alright if running on GOMAXPROC=1, but if you go to multiple threads, stuff can break.

An example of running go test -race . in etcd directory:

michal:~/code/mygo/src/github.com/mwitkow/go-flagz/etcd/ (dynamic_flags*) $ go test -race .                                                     [15:42:47]
2016-03-22 15:42:59.314408 E | etcdserver: cannot monitor file descriptor usage (cannot get FDUsage on darwin)
==================
WARNING: DATA RACE
Read by goroutine 10:
  runtime.convT2E()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/iface.go:128 +0x0
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdate.func1()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:99 +0x56
  github.com/mwitkow/go-flagz/etcd_test.eventually()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:168 +0x92
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdate()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:100 +0x990
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Previous write by goroutine 18:
  flag.(*intValue).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:117 +0x86
  flag.(*FlagSet).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:363 +0x2ab
  github.com/mwitkow/go-flagz/etcd.(*Updater).watchForUpdates()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:175 +0x10f2

Goroutine 10 (running) created at:
  testing.RunTests()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:582 +0xae2
  github.com/stretchr/testify/suite.Run()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:102 +0x7a3
  github.com/mwitkow/go-flagz/etcd_test.TestUpdaterSuite()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:156 +0x595
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Goroutine 18 (running) created at:
  github.com/mwitkow/go-flagz/etcd.(*Updater).Start()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:86 +0x14d
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdate()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:94 +0x3f4
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc
==================
==================
WARNING: DATA RACE
Read by goroutine 46:
  runtime.convT2E()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/iface.go:128 +0x0
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdateRestoresGoodState.func3()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:141 +0x56
  github.com/mwitkow/go-flagz/etcd_test.eventually()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:168 +0x92
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdateRestoresGoodState()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:142 +0x11f2
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Previous write by goroutine 58:
  flag.(*float64Value).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:206 +0x7f
  flag.(*FlagSet).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:363 +0x2ab
  github.com/mwitkow/go-flagz/etcd.(*Updater).watchForUpdates()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:175 +0x10f2

Goroutine 46 (running) created at:
  testing.RunTests()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:582 +0xae2
  github.com/stretchr/testify/suite.Run()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:102 +0x7a3
  github.com/mwitkow/go-flagz/etcd_test.TestUpdaterSuite()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:156 +0x595
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Goroutine 58 (running) created at:
  github.com/mwitkow/go-flagz/etcd.(*Updater).Start()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:86 +0x14d
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdateRestoresGoodState()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:118 +0x4ab
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc
==================
PASS
Found 2 data race(s)
FAIL    github.com/mwitkow/go-flagz/etcd    3.588s

xss security issue (flag values not html escaped)

you need to use template/html not template/text:

fortio/fortio@060ad61#diff-bada4417d86502adb33b78764d0e7a22

Make `flagz` be usable with normal `flag` and `spf13/pflag`

Currently we're tied to spf13/pflag due to two things.

First, we're using Annotations. It would be trivial to remove them and rely on whether the Value implements Type() and check the prefix for dyn_.

Secondly, and less trivially, the interfaces for pflag.Value and flag.Value don't match, as well as the types of Flag.

https://godoc.org/flag#Flag
https://godoc.org/flag#Value

vs

https://godoc.org/github.com/spf13/pflag#Flag
https://godoc.org/github.com/spf13/pflag#Value

For Dyn* flags, we could achieve complete independence from spf13/pflag vs flag if FlagSet.Var accepted a Value that could be abstracted as common.

However, for monitoring and debug endpoints, we need calls to VisitAll. These return an explicit Flag type that differs between the two.

The second problem can be worked around by just recompiling flagz with s/flag "github.com/spf13/flag"/"flag" ;)

maintenance, fork/copies

hi @mwitkow sorry to bug you.

I was curious if I could get your permission to take some/most of the go-flagz code and integrate it into a slightly different format (for instance using std go flags pattern and naming, a different logging/error handling etc...) probably as a new package in fortio (https://github.com/fortio/fortio) obviously I would preserve your name in the credits and copyright header but curious to get your blessing or hear objections? (do you plan on maintaining go-flagz further? is improbable-io going to?)

ps: if you want we can talk via email or twitter or linked in if that feels better

Implement Prometheus checksum monitoring

Prometheus is a very popular monitoring stack. Unfortunately, it doesn't support strings as values.

Use case:
being able to count the checksums of static and dynamic flags in a pre-defined flagset across many server.

The typical approach in Prometheus (e.g. for build labels) is to export a value 1 with the string inside a label. This allows for easy counting of servers that have that particular value.

The resulting Prometheus metrics would look as follows:

flagz_checksum{set="default",type="dynamic",checksum="9fa85e70"} 1
flagz_checksum{set="default",type="static",checksum="53fd4b02"} 1

These checksums would need to be calculated on-scrape. Prometheus already has support for it with GaugeFunc but it doesn't allow to set labels, and only outputs the value on scrape.

To achieve this, a custom Collector needs to be implemented.

Idea. Docker integration

Since the flags are dynamic this means that all your docker systems don't need to have configs that tell them about other systems they need to talk to.

If anyone has a decent example of 2 or 3 services being dockerised and using this go-flagz approach it would be very helpful think.

Maybe example the example of this being put into the code base in this repo.

It really shows off how this lib solves a tedious infrastructure problem !!