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

configure_server_settings loses integer values #17043

Conversation

jrafanie
Copy link
Member

  • Print the value's class before and after setting it (for clarity)
  • If STDIN strings are really integers, convert them

Although, we do special handling of "100.megabytes" strings, etc. later
when consuming the settings such as in worker_settings using
to_i_with_method, it makes sense to check for integers here at the point
in which the user provides them so we can store them "correctly".

Note, one could argue we should handle floats or other datatypes but
99.9% of our settings are strings or integers.

http://talk.manageiq.org/t/tune-workers-via-cli/3107/4452dd84

Before the fix

$ ./tools/configure_server_settings.rb -s 1 -p workers/worker_base/queue_worker_base/generic_worker/count -v 3
...
{:dry_run=>false, :serverid=>1, :path=>"workers/worker_base/queue_worker_base/generic_worker/count", :value=>"3", :help=>false, :serverid_given=>true, :path_given=>true, :value_given=>true}
Setting [workers/worker_base/queue_worker_base/generic_worker/count], old value: [2] (Integer), new value: [3](String)
Done

After

$ ./tools/configure_server_settings.rb -s 1 -p workers/worker_base/queue_worker_base/generic_worker/count -v 1
...
{:dry_run=>false, :serverid=>1, :path=>"workers/worker_base/queue_worker_base/generic_worker/count", :value=>"1", :help=>false, :serverid_given=>true, :path_given=>true, :value_given=>true}
Setting [workers/worker_base/queue_worker_base/generic_worker/count], old value: [3](Integer), new value: [1](Integer)
Done

Although, we do special handling of "100.megabytes" strings, etc. later
when consuming the settings such as in worker_settings using
to_i_with_method, it makes sense to check for integers here at the point
in which the user provides them so we can store them "correctly".

Note, one could argue we should handle floats or other datatypes but
99.9% of our settings are strings or integers.

http://talk.manageiq.org/t/tune-workers-via-cli/3107/4
@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2018

Checked commits jrafanie/manageiq@ecbc4af~...452dd84 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 3 offenses detected

tools/configure_server_settings.rb

@jrafanie
Copy link
Member Author

Alternative found in #17039

@jrafanie
Copy link
Member Author

Closing since #17039 looks to be going in the right direction to solve this more generically without adding too much code.

@jrafanie jrafanie closed this Feb 26, 2018
@jrafanie jrafanie deleted the configure_server_settings_loses_integer_values branch July 15, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants