Unify config #38

reidmv · 2017-06-13T18:14:33Z

Builds on #37.

This is effectively a cleanup of the way config options work. Any config option can now be read from a file, OR passed on the CLI. Valid config options must be specified as part of the SETTINGS array.

npwalker · 2017-06-13T18:24:52Z

files/tk_metrics

+  clientcert: { type: :string  },
+  pe_version: { type: :string  },
+  print:      { type: :boolean },
+  ssl:        { type: :boolean },


What happens if ssl is not specified in the config file or on the CLI? It currently defaults to true if unspecified

npwalker · 2017-06-13T18:27:35Z

files/tk_metrics

-options = {}
+#===========================================================================#
+# BUILD CONFIGURATION OPTIONS                                               #
+#===========================================================================#


Are you planning on doing anything with amq_metrics?

Not in this PR, no. But yeah, if this works here it's a forkliftable pattern.

reidmv · 2017-06-13T19:16:58Z

@npwalker Added defaults to the configs.

npwalker · 2017-06-13T19:17:40Z

files/tk_metrics

+  metrics:    { type: :list    },
+  clientcert: { type: :string  },
+  pe_version: { type: :string  },
+  print:      { type: :boolean, default: false },


print defaults to true.

npwalker · 2017-06-13T19:30:03Z

files/tk_metrics

+SETTINGS = {
+  output_dir: { type: :string  },
+  hosts:      { type: :list    },
+  port:       { type: :string  },


I'm not 100% convinced that metrics will always run on the same port as the status endpoint so I don't see a need to rename this to port.

If we decide to allow running the status endpoint on a different port so that even when a component is overloaded we can still get the status output then I'd like to be able to have metrics_port and status_port.

Makes sense. Changed it back to metrics_port.

npwalker · 2017-06-13T20:56:10Z

files/tk_metrics

+  REQUIRED_FLAGS.merge(SETTINGS).each do |opt,attrs|
+    case attrs[:type]
+    when :string, nil
+      opts.on("--#{opt} ARG", '(no description)')  { |arg| config[opt] = arg }


Are descriptions not useful in the --help output?

They can be, yeah. This comment prompted a semi-big rewrite of the consolidation. New commits pushed.

So that any config option can be specified on the CLI, or read from the file.

This aligns the CLI more closely with modern Puppet tooling. Also, we've used --config-dir with a hyphen already, so deciding on and choosing a consistent word separator seems like a postivie thing.

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.

Rather than making a human type an option name twice, add another lambda to generate the option string based on argument type - value or boolean.

npwalker · 2017-06-15T00:23:17Z

files/tk_metrics

+  option = lambda do |name,desc,fopt,fsave,default=nil|
+    settings[name] = { default: default }
+    parser.on(fopt.call(name.to_s), desc) do |arg|
+      fsave.call(name, arg)


If I'm understanding this correctly... is this really saving code / increasing clarity over?

if fsave == 'ident' then config[name] = arg else if fsave == 'list' then config[name] = arg.split(',') end

not guaranteed to compile

or maybe just

if fsave == 'list' then config[name] = arg.split(',') else config[name] = arg end

I think it's just a difference between a functional style and a procedural style. I like the functional style, but I'm sure a procedural equivalent could be done too. I originally chose functional because I wanted to clearly contain all of the logic for how to build out config options (starts on line 33) INSIDE the OptionParser#new block, not rely on any external def's, and not do any weird scoped def's inside the block. Since I was doing one lambda anyway I figured I'd go all-in on the functional.

npwalker · 2017-06-15T00:25:26Z

files/tk_metrics

+  option.call(:"metrics-type", 'Type of metric to collect', opt_value, save_ident)
+
+  # Kinda pointless to include descriptions unless there's a help flag
+  parser.on("-h", "--help", "Prints this help") { puts parser; exit }


I think you just get this for free. The current implementation has it.

[root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --help Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics -h Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics

I added this when I tried -h and it didn't work. Does -h work for you without it?

Anything short of help and help itself seems to work.

[root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics -h Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --help Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --h Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --he Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --blah /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics:19:in `<main>': invalid option: --blah (OptionParser::InvalidOption) [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --hel Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics --helps /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics:19:in `<main>': invalid option: --helps (OptionParser::InvalidOption) [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics -help Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics -hel Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics -he Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics [root@master201646-centos puppet_enterprise]# /opt/puppetlabs/pe_metric_curl_cron_jobs/scripts/tk_metrics -h Usage: tk_metrics [options] -p, --[no-]print Print to stdout -m, --metrics_type [TYPE] Type of metric to collect -o, --output-dir [DIR] Directory to save output to --metrics_port [PORT] The port the metrics service runs on --[no-]ssl Whether or not to use SSL when gather metrics

npwalker · 2017-06-15T00:31:25Z

files/tk_metrics

+
+end.parse!
+
+raise('ERROR: Missing configuration option "metrics-type"') if config[:"metrics-type"].nil?


I assume this equates to the below. Pure curiosity, why is this preferred?

if config[:"metrics-type"].nil? raise('ERROR: Missing configuration option "metrics-type"') end

That's an interesting question, I don't know. At some point I got used to writing "abort"-type statements in a way that emphasized the abort part over the if part. Happy to switch it around if you prefer the above.

This feels as though it could be one of those Rubyisms. I find some rubyisms strange but I may just not be that familiar with it. Was mostly a curiosity question we can leave it

npwalker · 2017-06-15T00:38:43Z

files/tk_metrics

+# command line are given precedence.
+settings.each do |opt,attrs|
+  possibilities = [config[opt], file_config[opt], attrs[:default]]
+  config[opt] = possibilities.find {|p| !p.nil? }


Thoughts on reading in the config file before parsing command line args. Then you can check

if the command line arg is nil
use the config file setting
if the config file setting is nil use the default
else fail

I'd like to change how the config file works. What I most want to do is allow a config file path to be specified as an option flag. Regardless though, it's currently difficult to parse a config file first since the config file location depends on a flag (--metrics-type or maybe --config), and I don't know if there's a way for optparse to find and evaluate a particular flag first. I think it evaluates flags in the order they're given on the command line...

Make a new option, --config, which is used to directly specify the configuration file to use. Also clean up the variable names so the code is easier to follow in terms of what it's doing. The old behavior where --metrics-type implied a config file name is still supported as legacy.

reidmv · 2017-06-15T17:04:30Z

@npwalker added a commit that should simplify a bit how the config file works.

npwalker · 2017-06-15T18:55:59Z

files/tk_metrics

  option.call(:"metrics-type", 'Type of metric to collect', opt_value, save_ident)

  # Kinda pointless to include descriptions unless there's a help flag
  parser.on("-h", "--help", "Prints this help") { puts parser; exit }

 end.parse!

-raise('ERROR: Missing configuration option "metrics-type"') if config[:"metrics-type"].nil?
+# This is to support legacy from the time when passing --metrics-type was what
+# determined the filename of the config file, rather than the --config option.


Is this important? I'm inclined to just switch to --config not support just --metrics-type on its own

The script must now be invoked with the --config option to specify a configuration file, OR all required configuration parameters must be passed as command-line flags. Because this involves a change in the API to interact with the script, the amq_metrics script is now in scope also, and has had the same option parsing interface applied to it.

reidmv · 2017-07-10T15:24:11Z

@npwalker bump

npwalker · 2017-07-28T15:20:36Z

files/amq_metrics

        Dir.mkdir(host) unless File.exist?(host)
        File.open(File.join(host, filename), 'w') do |file|
          file.write(json_dataset)
        end
      end
    end
-    if options[:print] != false then
+    if config[:print] then


The default behavior of the script is currently to print. I think this breaks that unless you've defaulted print to true somehow. Will test.

Option definition is on line 41. Default value is True.

npwalker · 2017-07-28T15:23:03Z

files/tk_metrics


    json_dataset = JSON.pretty_generate(dataset)

-    unless OUTPUT_DIR.nil? then
-      Dir.chdir(OUTPUT_DIR) do
+    unless config[:"output-dir"] == false then


output-dir is a path not a boolean?

Yep. With a default value of False. Part of the option definition on line 35. Could probably change it to a symbol like :undef, should have roughly the same effect. This if-statement is just checking to see if the option has a valid value, or if it's the not-nil-but-not-valid default.

@reidmv is there a reason we can't just leave unset options nil and check them for being nil?

I certainly do find giving something a default of false when it's expected to be a path to be confusing.

npwalker · 2017-07-31T22:18:59Z

files/tk_metrics

        Dir.mkdir(host) unless File.exist?(host)
        File.open(File.join(host, filename), 'w') do |file|
          file.write(json_dataset)
        end
      end
    end
-    if options[:print] != false then
+    if config[:print] != false then


this doesn't match the amq script

Previously some optional values had a default value of False. In order to indicate that the user doesn't need to supply a value a default is required, but False as the default did not make it clear that the value simply hadn't been supplied. This commit switches such values to use :undef as their default instead, thus making it clearer what is happening.

reidmv · 2017-08-01T18:20:17Z

@npwalker Cleaned up based on comments. Unsupplied, optional values now default to :undef instead of false. Treatment of config[:print] made uniform across the two scripts.

reidmv · 2017-09-12T18:25:47Z

@npwalker ping - needs decision

npwalker reviewed Jun 13, 2017

View reviewed changes

reidmv force-pushed the unify_config branch from bc48808 to db6df5d Compare June 13, 2017 19:16

npwalker reviewed Jun 13, 2017

View reviewed changes

reidmv force-pushed the unify_config branch from db6df5d to 3925c58 Compare June 13, 2017 19:22

npwalker reviewed Jun 13, 2017

View reviewed changes

reidmv force-pushed the unify_config branch from 3925c58 to 231c398 Compare June 13, 2017 20:27

npwalker reviewed Jun 13, 2017

View reviewed changes

reidmv force-pushed the unify_config branch 2 times, most recently from c38a27a to 5d2bf7a Compare June 13, 2017 21:45

reidmv added 3 commits June 14, 2017 10:38

Provide unified method of setting config options

46cd6ef

So that any config option can be specified on the CLI, or read from the file.

Use -'s in configuration options, not _'s

9c75e1b

This aligns the CLI more closely with modern Puppet tooling. Also, we've used --config-dir with a hyphen already, so deciding on and choosing a consistent word separator seems like a postivie thing.

reidmv force-pushed the unify_config branch from 5d2bf7a to e5835ed Compare June 14, 2017 17:40

Remove opportunity for human error in option def

3b46f0b

Rather than making a human type an option name twice, add another lambda to generate the option string based on argument type - value or boolean.

npwalker reviewed Jun 15, 2017

View reviewed changes

reidmv force-pushed the unify_config branch 4 times, most recently from 9d228ab to a2e1df5 Compare June 15, 2017 22:50

reidmv force-pushed the unify_config branch from a2e1df5 to 5784aae Compare June 15, 2017 22:51

npwalker reviewed Jul 28, 2017

View reviewed changes

npwalker reviewed Jul 31, 2017

View reviewed changes

reidmv force-pushed the unify_config branch from 176d06e to 1fb4fb5 Compare August 1, 2017 18:19

Merge branch 'master' into unify_config

3501dfd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify config #38

Unify config #38

reidmv commented Jun 13, 2017

npwalker Jun 13, 2017 •

edited

Loading

npwalker Jun 13, 2017

reidmv Jun 13, 2017

reidmv commented Jun 13, 2017

npwalker Jun 13, 2017

npwalker Jun 13, 2017 •

edited

Loading

reidmv Jun 13, 2017

npwalker Jun 13, 2017

reidmv Jun 13, 2017

npwalker Jun 15, 2017

npwalker Jun 15, 2017

reidmv Jun 15, 2017

npwalker Jun 15, 2017

reidmv Jun 15, 2017

npwalker Jun 15, 2017

npwalker Jun 15, 2017

reidmv Jun 15, 2017

npwalker Jun 15, 2017

npwalker Jun 15, 2017

reidmv Jun 15, 2017

reidmv commented Jun 15, 2017

npwalker Jun 15, 2017

reidmv commented Jul 10, 2017

npwalker Jul 28, 2017

reidmv Jul 29, 2017

npwalker Jul 28, 2017

reidmv Jul 29, 2017

npwalker Jul 31, 2017

npwalker Jul 31, 2017

reidmv commented Aug 1, 2017

reidmv commented Sep 12, 2017


		end.parse!

		raise('ERROR: Missing configuration option "metrics-type"') if config[:"metrics-type"].nil?

Unify config #38

Are you sure you want to change the base?

Unify config #38

Conversation

reidmv commented Jun 13, 2017

npwalker Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidmv commented Jun 13, 2017

Choose a reason for hiding this comment

npwalker Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidmv commented Jun 15, 2017

Choose a reason for hiding this comment

reidmv commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidmv commented Aug 1, 2017

reidmv commented Sep 12, 2017

npwalker Jun 13, 2017 •

edited

Loading

npwalker Jun 13, 2017 •

edited

Loading