Refactor to be shorter, include descriptions

Descriptions can be nice. As a side effect the code is probably simpler too. Also revert opt "metrics" back to "additional-metrics" Well, "back to" is a bit strong since the original was "additional_metrics". Still, the idea's the same. It's more correct given what the parameter is for / does.
npwalker · Jun 14, 2017 · e5835ed · e5835ed
1 parent 9c75e1b
commit e5835ed
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 37 deletions.
diff --git a/files/tk_metrics b/files/tk_metrics
@@ -11,46 +11,41 @@ require 'yaml'
 # BUILD CONFIGURATION OPTIONS                                               #
 #===========================================================================#
 
-# Define the list of mandatory options that must be specified on the CLI
-REQUIRED_FLAGS = {
-  :"metrics-type" => { type: :string },
-}
-
-# Define the list of options that will be read from the config file, and which
-# may optionally be specified on the CLI.
-SETTINGS = {
-  :"output-dir"   => { type: :string, default: false }, # false = disabled
-  :hosts          => { type: :list },
-  :metrics        => { type: :list },
-  :"metrics-port" => { type: :string },
-  :clientcert     => { type: :string },
-  :"pe-version"   => { type: :string },
-  :print          => { type: :boolean, default: true },
-  :ssl            => { type: :boolean, default: true },
-}
-
+settings = { }
 config = { }
 
-OptionParser.new do |opts|
-  opts.banner = "Usage: tk_metrics [options]"
-
-  # Gather options
-  REQUIRED_FLAGS.merge(SETTINGS).each do |opt,attrs|
-    case attrs[:type]
-    when :string, nil
-      opts.on("--#{opt} ARG", '(no description)')  { |arg| config[opt] = arg }
-    when :boolean
-      opts.on("--[no-]#{opt}", '(no description)') { |arg| config[opt] = arg }
-    when :list
-      opts.on("--#{opt} ARG", '(no description)')  { |arg| config[opt] = arg.split(',') }
+OptionParser.new do |parser|
+  parser.banner = "Usage: tk_metrics [options]"
+
+  ident = lambda { |name,arg| config[name] = arg }
+  list = lambda { |name,arg| config[name] = arg.split(',') }
+
+  option = lambda do |name,opt,desc,save,default=nil|
+    settings[name] = { default: default }
+    parser.on(opt, desc) do |arg|
+      save.call(name, arg)
     end
   end
 
+  # Each possible configuration option is defined here
+  option.call(:"output-dir", '--output-dir ARG', 'Directory to save output to', ident, false)
+  option.call(:hosts, '--hosts ARG', 'Hosts to collect metrics from (comma-separated)', list, ['localhost'])
+  option.call(:"additional-metrics", '--additional-metrics ARG', 'Additional metrics to collect (comma-separated)', list, [])
+  option.call(:"metrics-port", '--metrics-port ARG', 'The port the metrics service runs on', ident)
+  option.call(:clientcert, '--clientcert ARG', 'Not used', ident)
+  option.call(:"pe-version", '--pe-version ARG', 'The version of PE in use', ident)
+  option.call(:print, '--[no-]print ARG', 'Print to stdout', ident, true)
+  option.call(:ssl, '--[no-]ssl ARG', 'Whether or not to use SSL when gather metrics', ident, true)
+
+  # This one is special because it's required
+  option.call(:"metrics-type", '--metrics-type ARG', 'Type of metric to collect', ident)
+
+  # Kinda pointless to include descriptions unless there's a help flag
+  parser.on("-h", "--help", "Prints this help") { puts parser; exit }
+
 end.parse!
 
-REQUIRED_FLAGS.each_key do |opt|
-  !config[opt].nil? || raise("ERROR: Missing configuration option \"#{opt}\"")
-end
+raise('ERROR: Missing configuration option "metrics-type"') if config[:"metrics-type"].nil?
 
 begin
   file = File.join(File.dirname(File.expand_path(__FILE__)),"#{config[:"metrics-type"]}_config.yaml")
@@ -63,7 +58,7 @@ end
 # For the options which MAY be read from a config file, retrieve those values
 # if they have not been specified during invocation. Values specified on the
 # command line are given precedence.
-SETTINGS.each do |opt,attrs|
+settings.each do |opt,attrs|
   possibilities = [config[opt], file_config[opt], attrs[:default]]
   config[opt] = possibilities.find {|p| !p.nil? }
   raise("ERROR: Missing configuration option \"#{opt}\"") if config[opt].nil?
@@ -182,8 +177,8 @@ config[:hosts].each do |host|
     status_output   = get_status_endpoint(host, config[:"metrics-port"], config[:ssl])
     dataset['servers'][hostkey] = {config[:"metrics-type"] => status_output}
 
-    unless config[:metrics].empty? then
-      metrics_array = retrieve_additional_metrics(host, config[:"metrics-port"], config[:metrics], config[:"pe-version"], config[:ssl])
+    unless config[:"additional-metrics"].empty? then
+      metrics_array = retrieve_additional_metrics(host, config[:"metrics-port"], config[:"additional-metrics"], config[:"pe-version"], config[:ssl])
 
       metrics_array.each do |metric_hash|
         metric_name = metric_hash['name']

diff --git a/manifests/pe_metric.pp b/manifests/pe_metric.pp
@@ -25,7 +25,7 @@
     'hosts'              => $hosts,
     'metrics-type'       => $metrics_type,
     'metrics-port'       => $metrics_port,
-    'metrics'            => $additional_metrics,
+    'additional-metrics' => $additional_metrics,
     'clientcert'         => $::clientcert,
     'pe-version'         => $facts['pe_server_version'],
     'ssl'                => $ssl,