search

cloudfoundry-community / firehose-to-syslog

Send firehose events from Cloud Foundry to syslog.
MIT License
44 stars 58 forks source link

--enable-stats-server and --help output #190

Closed lrstanley closed 6 years ago

lrstanley commented 6 years ago

I'm not sure if this is really a worthwhile thing to look into, but I'm noticing that within the help output, for --enable-stats-server, it specifically states:

--enable-stats-server          Will enable stats server on 8080

It doesn't show a default, like with other flags.. e.g:

--fh-keep-alive=25s            Keep Alive duration for the firehose consumer

So, when reviewing the help output, it's implied that the builtin stats http server isn't enabled by default, when it actually is. I'm not sure if there is a way with kingpin to show true vs. false defaults for those types of flags, but it may be helpful. Could also add suffix (default: true) to the description maybe?

I'd also recommend not enabling it without it explicitly being specified, as users may not actually know it's binding those things and may be an issue security wise, but that may be considered a breaking change if people use it without explicitly using the flag.

lrstanley commented 6 years ago

Actually, now that I look at it further, I apparently can't even specify false (also tried 0).. hah.

$ firehose-to-syslog [...] --enable-stats-server=false
firehose-to-syslog: error: unexpected false, try --help

and can only do --no-enable-stats-server.. which isn't documented, isn't really a standard, etc.. This is why I usually don't use kingpin, and usually use https://github.com/jessevdk/go-flags (e.g. https://github.com/lrstanley/links#usage which doesn't show the benefit of default text, since I don't have any in that application, but nonetheless)

shinji62 commented 6 years ago

thanks, for the issue, but I am not agree I like Kingpin.

The flag is simple if you want to enable just add the flag.. if there is no flag stats servers is disable. Description is clear too Will enable stats server on 8080.

You have the same in your own example --tls.enable run tls server rather than standard http .

lrstanley commented 6 years ago

No, if you don't specify --enable-stats-server, it still enables the stats endpoint.

https://github.com/cloudfoundry-community/firehose-to-syslog/blob/f5a1f1204d7a66ef362311ae5a76aad8653b417a/cli.go#L37

Notice .Default("true")...

shinji62 commented 6 years ago

Ok that an issue.

I will fix that soon :) thanks for reporting.

shinji62 commented 6 years ago

Fix in the 4.1.0 release.