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

Support Kingpin and improve help text #121

Merged
merged 2 commits into from
Aug 15, 2017
Merged

Conversation

joehandzik
Copy link
Contributor

This PR has two commits in it. The first commit adds support for Kingpin v2 and adds it to our vendor directory. The second commit switches the collector.* flags to use the Enum capability of Kingpin, which has the added benefit of filtering out invalid options. We also list out the valid options in the default helptext (though attempting to use an invalid option will result in the valid options being pushed to stdout as well).

This fixes #119 and fixes #120

Previously used the default "flags" library, but switched to Kinpin v2 to conform to Prometheus community choice.

Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
Copy link
Contributor

@roclark roclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I will wait to merge in case @mjtrangoni has any comments.

@mjtrangoni
Copy link
Contributor

Nice work! Thanks!
I have just one more thing I found right now. Could you sort the flags alphabetically? I am also seeing the same issue at the node_exporter. This is really strange.

Use the enum functionality of Kingpin v2 to enforce valid options for the collector.* flags, and update the help text to clarify what those valid options are.

Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
@joehandzik
Copy link
Contributor Author

@mjtrangoni I just made the ordering tweak that you requested. The gotcha here is that the help flag always comes first in the flag list, so I can't alphabetize all flags. I decided to put --help and --version together at the top of the list, (they fit together logically to me, since they don't actually result in a running exporter) and have the rest of the flags together for the rest of the list.

@mjtrangoni
Copy link
Contributor

Hi @joehandzik ,

Thanks! I find the '--help-man' option really cool too! You can create a manual on the fly.

@joehandzik
Copy link
Contributor Author

@mjtrangoni I thought it was a nice addition too. Kingpin was pretty much a drop-in replacement with some nice extra features, so thanks for motivating me to get it into our exporter.

@joehandzik joehandzik merged commit bf9e42d into master Aug 15, 2017
@joehandzik joehandzik deleted the wip-kingpin-and-helptext branch August 15, 2017 18:06
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 this pull request may close these issues.

Feature request: Switch to kingpin Feature request: -collector Options
3 participants