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.

Sign up for GitHub

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 setting var type with configure_server_settings.rb #17039

Merged
merged 11 commits into from
Feb 27, 2018
Merged

Support setting var type with configure_server_settings.rb #17039

Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b55557b
Don't break configuration if advanced config requires an int.
rvalente Feb 23, 2018
b36583c
fixing violations
rvalente Feb 23, 2018
473bfc6
fixed space, #ocd
rvalente Feb 23, 2018
fcad45f
Merge branch 'bug-intval-support' of https://github.com/rvalente/mana…
rvalente Feb 23, 2018
0d2d2ff
support any config type and cast based on user input
rvalente Feb 26, 2018
8a48156
added force option
rvalente Feb 26, 2018
64b11ce
declare newval outside of case
rvalente Feb 26, 2018
b892264
cleanup word array
rvalente Feb 26, 2018
d7c045f
adding more details to failure output
rvalente Feb 27, 2018
c081bd2
fixed string not getting set properly, and fix symbol example
rvalente Feb 27, 2018
ceccacc
cleanup assignment and provide a better string example
rvalente Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions tools/configure_server_settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"Example: #{__FILE__} -d -s 1 -p reporting/history/keep_reports -v 42\n" \
"Example: #{__FILE__} -s 1 -p workers/worker_base/queue_worker_base/ems_metrics_collector_worker/defaults/count -v 4 -i"

opt :dry_run, "Dry Run", :short => "d"
opt :serverid, "Server Id", :short => "s", :default => 0
opt :path, "Path within advanced settings hash", :short => "p", :default => ""
opt :value, "New value for setting", :short => "v", :default => ""
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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll improve the output

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


depends :path, :serverid, :value
end

puts opts.inspect

Trollop.die :serverid, "is required" unless opts[:serverid_given]
Trollop.die :path, "is required" unless opts[:path_given]
Trollop.die :value, "is required" unless opts[:value_given]
# Grab the value that we have set
newval = if opts[:integer]
opts[:value].to_i
else
opts[:value]
end

server = MiqServer.where(:id => opts[:serverid]).take
unless server
Expand All @@ -32,7 +39,7 @@
keys.each { |p| path = path[p.to_sym] }

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

path[key] = opts[:value]
path[key] = newval

valid, errors = VMDB::Config::Validator.new(settings).validate
unless valid
Expand Down