Support setting var type with configure_server_settings.rb #17039

rvalente · 2018-02-23T11:33:19Z

https://bugzilla.redhat.com/show_bug.cgi?id=1550157

Changing the worker count across our environment for ems_metrics_collector_worker from 2 to 4 caused C & U to stop function because the value was change from 2 to '4'. Subsequently miq_worker.rb:149 was trying to compare current workers [0] to ['4'] and throwing a string comparison exception.

Here is the view in the advanced config screen before the fix

and after

Steps for Testing/QA

This will break with both the new code and the existing code.

./tools/configure_server_settings.rb -s 1 -p workers/worker_base/queue_worker_base/ems_metrics_collector_worker/defaults/count -v 4

The following will cast the the value as an integer and it will update the configuration accordingly.

./tools/configure_server_settings.rb -s 1 -p workers/worker_base/queue_worker_base/ems_metrics_collector_worker/defaults/count -v 4 -t integer

…geiq into bug-intval-support

rvalente · 2018-02-23T18:05:27Z

@jrafanie heh I don't trust users as much as you do, I wanted an explicit decision by the user that they were providing an Integer.

jrafanie · 2018-02-23T19:21:36Z

tools/configure_server_settings.rb

@@ -4,19 +4,26 @@

 opts = Trollop.options(ARGV) do
  banner "USAGE:   #{__FILE__} -s <server id> -p <settings path separated by a /> -v <new value>\n" \
-         "Example: #{__FILE__} -d -s 1 -p reporting/history/keep_reports -v 42"


hmm, was this wrong previously? Perhaps this example should also specify a -i

added new line because a 3rd example was added below with -i

jrafanie · 2018-02-23T19:22:29Z

tools/configure_server_settings.rb

+  opt :serverid, "Server Id",                          :short => "s", :type => :integer, :required => true
+  opt :path,     "Path within advanced settings hash", :short => "p", :type => :string,  :required => true
+  opt :value,    "New Value for setting",              :short => "v", :type => :string,  :required => true
+  opt :integer,  "Value Provided is an Integer",       :short => "i", :type => :boolean, :default => false


I didn't go this route in #17043 because it's not obvious it's required for some values...

See below @rvalente, I think warning the user they're changing an existing Integer value to a String and telling them to look at the help banner to specify an integer may make this option easier to use.

Agreed, I'll improve the output

@rvalente Was looking at #17043 with @jrafanie and we were thinking that this approach would be better if, instead of having the boolean option :integer

opt :integer, "Value Provided is an Integer", :short => "i", :type => :boolean, :default => false

an option that specifies the type of the value, defaulting to string and only accepts string, integer, boolean, symbol and float. Something like this -

TYPES = %w{ string integer boolean symbol float } opt :type, "Type of value provided, #{TYPES.inspect}", :short => "t", :type => :string, :default => "string" Trollop.die :type, "must be one of #{TYPES.inspect}" unless TYPES.include?(opts[:type])

I think it would still be good to warn if the new type of the new value is different than the old one.

I agree, this make sense to me. In this way, we're not forcing casting of strings to other types.

We default to string and use the check mentioned below to detect when we're changing a sensible default from the correct type to a String type (count from 1,2,3, etc. to "1", "2", "3") and request the user to either allow this to happen or inform them they need to specify the type.

Note, integer was requested here, but boolean was also requested here so having a single flag that allows us to specify a type would help solve all of these at once.

jrafanie · 2018-02-23T19:33:03Z

tools/configure_server_settings.rb

@@ -32,7 +39,7 @@
 keys.each { |p| path = path[p.to_sym] }

 puts "Setting [#{opts[:path]}], old value: [#{path[key]}], new value: [#{opts[:value]}]"


Should we cherry-pick the first commit from #17043 so we can see the class?

Or perhaps we don't need to care about telling the user about the string/integer class unless they don't match:

# allow user to escape if the new value's class is not the same as the original, # such as setting a String where it was previously an Integer if path[key] && path[key].class != opts[:value].class STDERR.puts "The new value's class #{opts[:value].class} does not match the prior one's #{path[key].class}. Do you want to continue? (Y/N)" exit 1 if STDERR.gets.strip.downcase == "n" end

Perhaps the above condition could advise the user to run tools/configure_server_settings.rb -h to see how to specify Integer.

That is a good idea, essentially being able to tell the user that the input made is not valid.

gtanzillo

LGTM 👍

jrafanie · 2018-02-26T23:42:45Z

tools/configure_server_settings.rb

+if opts[:force]
+  puts "Change [#{opts[:path]}], old class: [#{path[key].class}], new class: [#{newval.class}]"
+elsif path[key] && path[key].class != newval.class
+  STDERR.puts "The new value's class #{newval.class} does not match the prior one's #{path[key].class}.  Use -f to force update, this may break things!"


Use -f to force update, this may break things!"

"Use -t to specify the type for the provided value. Use -f to force changing this value. Note, -f may break things! See -h for examples."

abraverm · 2018-02-27T08:17:22Z

Looks good but what about lists?

jrafanie · 2018-02-27T15:25:27Z

Looks good but what about lists?

@abraverm I think that's reasonable to want do but then it starts getting really complicated. As soon as we do that, we'll want an ability to append/prepend to an array/list instead of replacing it because the list is really long. I'm not sure command line is best for something like that. Perhaps, taking an input settings.yml and blasting it out to specific server settings is another way to solve arrays/hashes or anything more complicated than a single value.

Either way, I'm of the opinion that it's outside the scope for this PR and would require more discussion on how much that feature is desired and how much effort it is to write and maintain.

abraverm · 2018-02-27T16:01:57Z

@jrafanie I agree, array configurations seem like advanced setting which may require a different approach, for exmaple augeas.

rvalente · 2018-02-27T16:13:18Z

@jrafanie I agree, array configurations seem like advanced setting which may require a different approach, for exmaple augeas.

@abraverm yes, it does start getting a little out of control at that point. At that point I would prefer to leverage the API directly to make changes or an external configuration key/value store all together a la etcd or consul or OpenShift ConfigMaps.

jrafanie · 2018-02-27T17:00:35Z

tools/configure_server_settings.rb

 opts = Trollop.options(ARGV) do
  banner "USAGE:   #{__FILE__} -s <server id> -p <settings path separated by a /> -v <new value>\n" \
-         "Example: #{__FILE__} -d -s 1 -p reporting/history/keep_reports -v 42"
+         "Example (String): #{__FILE__} -s 1 -p reporting/history/keep_reports -v 42\n" \


Using a duration is probably a more helpful example for a string:
Example (String): #{__FILE__} -s 1 -p reporting/history/keep_reports -v '3.months'

I realize that value was there already but now that you changed it, it seems like a strange example for a string.

👍 yeah - when I was testing moving from 6.months -> 42 caught me off guard.

jrafanie · 2018-02-27T17:03:24Z

tools/configure_server_settings.rb

+  newval = opts[:value].to_sym
+when "float"
+  newval = opts[:value].to_f
+end


This would eliminate some of the extra assignments: (I hope rubocop doesn't hate me):

newval = case opts[:type] when "integer" opts[:value].to_i when "boolean" if opts[:value].downcase == "true" true elsif opts[:value].downcase == "false" false end when "symbol" opts[:value].to_sym when "float" opts[:value].to_f end

jrafanie

Looks great other than a few minor nitpicks mentioned below.

rvalente · 2018-02-27T17:53:47Z

@jrafanie should be all set, thanks for the feedback! I was not happy with all that assignment either. 👊

miq-bot · 2018-02-27T17:58:03Z

Checked commits https://github.com/rvalente/manageiq/compare/b55557bf5668581cfcc13b85de7f7fda173df02b~...ceccaccb2f3fd16a1fccc15bc5a67fdbf5e2b9c4 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

jrafanie · 2018-02-27T18:38:27Z

Thanks @rvalente 👍

jrafanie · 2018-02-28T16:30:41Z

Added a BZ link to the description so we can backport this change.

jrafanie · 2018-02-28T16:36:29Z

Note, this script only exists on gaprindashvili/master, not euwe or fine.

Support setting var type with configure_server_settings.rb (cherry picked from commit 533fb20) https://bugzilla.redhat.com/show_bug.cgi?id=1552804

simaishi · 2018-03-07T19:41:49Z

Gaprindashvili backport details:

$ git log -1
commit c1b9b6f037af35e238896f1a403e9f752ee548d3
Author: Joe Rafaniello <[email protected]>
Date:   Tue Feb 27 13:38:00 2018 -0500

    Merge pull request #17039 from rvalente/bug-intval-support
    
    Support setting var type with configure_server_settings.rb
    (cherry picked from commit 533fb206eabe3d64fc590739fbc9b60bc1332e6b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552804

rvalente added 4 commits February 23, 2018 06:24

Don't break configuration if advanced config requires an int.

b55557b

fixing violations

b36583c

fixed space, #ocd

473bfc6

Merge branch 'bug-intval-support' of https://github.com/rvalente/mana…

fcad45f

…geiq into bug-intval-support

jrafanie mentioned this pull request Feb 23, 2018

configure_server_settings loses integer values #17043

Closed

jrafanie reviewed Feb 23, 2018

View reviewed changes

jrafanie mentioned this pull request Feb 26, 2018

Eval value passed to configure_server settings #16992

Closed

rvalente added 4 commits February 26, 2018 15:51

support any config type and cast based on user input

0d2d2ff

added force option

8a48156

declare newval outside of case

64b11ce

cleanup word array

b892264

rvalente changed the title ~~Support Integers with configure_server_settings.rb~~ Support setting var type with configure_server_settings.rb Feb 26, 2018

gtanzillo approved these changes Feb 26, 2018

View reviewed changes

jrafanie reviewed Feb 26, 2018

View reviewed changes

adding more details to failure output

d7c045f

fixed string not getting set properly, and fix symbol example

c081bd2

jrafanie reviewed Feb 27, 2018

View reviewed changes

jrafanie requested changes Feb 27, 2018

View reviewed changes

cleanup assignment and provide a better string example

ceccacc

jrafanie approved these changes Feb 27, 2018

View reviewed changes

jrafanie merged commit 533fb20 into ManageIQ:master Feb 27, 2018

jrafanie added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 27, 2018

jrafanie added bug tools labels Feb 27, 2018

jrafanie self-assigned this Feb 27, 2018

jrafanie mentioned this pull request Feb 27, 2018

Configure server settings cleanup #17059

Merged

bdunne removed this from the Sprint 80 Ending Feb 26, 2018 milestone Feb 28, 2018

bdunne added the gaprindashvili/yes label Feb 28, 2018

bdunne added this to the Sprint 81 Ending Mar 12, 2018 milestone Feb 28, 2018

simaishi pushed a commit that referenced this pull request Mar 7, 2018

Merge pull request #17039 from rvalente/bug-intval-support

c1b9b6f

Support setting var type with configure_server_settings.rb (cherry picked from commit 533fb20) https://bugzilla.redhat.com/show_bug.cgi?id=1552804

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 7, 2018

jrafanie mentioned this pull request Apr 10, 2018

Not able to raise memory threshold for MiqEmsRefreshCoreWorker #16945

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting var type with configure_server_settings.rb #17039

Support setting var type with configure_server_settings.rb #17039

rvalente commented Feb 23, 2018 •

edited by jrafanie

Loading

rvalente commented Feb 23, 2018

jrafanie Feb 23, 2018

rvalente Feb 25, 2018

jrafanie Feb 23, 2018

jrafanie Feb 23, 2018

rvalente Feb 25, 2018

gtanzillo Feb 26, 2018

jrafanie Feb 26, 2018

jrafanie Feb 26, 2018

jrafanie Feb 23, 2018

jrafanie Feb 23, 2018

rvalente Feb 25, 2018

gtanzillo left a comment

jrafanie Feb 26, 2018

abraverm commented Feb 27, 2018

jrafanie commented Feb 27, 2018

abraverm commented Feb 27, 2018

rvalente commented Feb 27, 2018

jrafanie Feb 27, 2018

jrafanie Feb 27, 2018

rvalente Feb 27, 2018

jrafanie Feb 27, 2018

jrafanie left a comment

rvalente commented Feb 27, 2018

miq-bot commented Feb 27, 2018

jrafanie commented Feb 27, 2018

jrafanie commented Feb 28, 2018

jrafanie commented Feb 28, 2018

simaishi commented Mar 7, 2018

		@@ -32,7 +39,7 @@
		keys.each { \|p\| path = path[p.to_sym] }

		puts "Setting [#{opts[:path]}], old value: [#{path[key]}], new value: [#{opts[:value]}]"

Support setting var type with configure_server_settings.rb #17039

Support setting var type with configure_server_settings.rb #17039

Conversation

rvalente commented Feb 23, 2018 • edited by jrafanie Loading

Steps for Testing/QA

rvalente commented Feb 23, 2018

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

gtanzillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abraverm commented Feb 27, 2018

jrafanie commented Feb 27, 2018

abraverm commented Feb 27, 2018

rvalente commented Feb 27, 2018

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

jrafanie left a comment

Choose a reason for hiding this comment

rvalente commented Feb 27, 2018

miq-bot commented Feb 27, 2018

jrafanie commented Feb 27, 2018

jrafanie commented Feb 28, 2018

jrafanie commented Feb 28, 2018

simaishi commented Mar 7, 2018

rvalente commented Feb 23, 2018 •

edited by jrafanie

Loading