Fixes #8015 - Enable adding defaults options for commands. #174

alongoldboim · 2015-07-19T12:49:01Z

To add default option to your hammer cli commands you need to define the key value pair(e.g organization_id: 3) under global_defaults in the cli_config.yml(the option will be added if the command permits it in its options).
you can specify a particular resource you want the default option will take effected on by placing the key value pair under _defauls, this can be done either in the cli_config.yml or in any other plugin config file(e.g foreman.yml).
Need to add a generate defaults command according to plugin and a managing command(add,remove).

alongoldboim · 2015-07-19T12:51:18Z

test/unit/apipie/command_test.rb

+    before :each do
+      HammerCLI::Settings.load(:global_defaults => {:organization_id => 3})
+    end
+


These tests needs some refinements but I just wanted to check we are on the right track and didn't want to waist a lot of time.

mbacovsky · 2015-07-20T09:08:33Z

lib/hammer_cli/settings.rb

@@ -82,6 +82,14 @@ def self.deep_merge!(h, other_h)
    def self.symbolize_hash(h)
      h = h.inject({}) { |sym_hash,(k,v)| sym_hash.update(k.to_sym => v) }
    end
+
+    def self.get_defaults_by_key(key, option)


I'd prefer to keep settings untouched. Could this be moved to separate class and use Settings just as a storage?

Sure ill add a wrapper for it.

mbacovsky · 2015-07-20T09:31:48Z

Thanks for the update. It looks clean so far. I'd like to see the create command and handling of the defaults defined on the server as part of the prove of concept, because it may influence the design significantly.

I don't understand the use-case for global_defaults vs. user_defaults, could you explain, please?

I'd also separate the defaults under some key, not to mix with other config stuff:

:defaults:
  :organization_id: 3
  :location_id: 2

Probably not necessary to resolve now, but any idea how to handle name vs. id resource definition? What if user wants to defaults create --name organization --value 'My Org' is it stored as organization_id or organization, do we want detect conflict if one wants to store both?

alongoldboim · 2015-07-20T13:55:11Z

OK as for the create commands I still need to implement them I just wanted to make sure I'm going in the right direction first, the create shouldn't problem because i can define it on cli_config under global section or if a resource is given then under the resource.

As for generate defaults for plugins, i thought of making a defaults class (will also be the settings wrapper) and on it to have a method generate_defaults(options={}) that will use the create_defaults command to place the options hash to the config file, all the plugins will be able to override and supply their own hash.

The global vs resource (user is just an example of a resource), the purpose was to give the users an option to define a default on every request(if the request has the option available) as opposed to placing the default on just one resource.

The key separation that i thought of is the _defaults so u'd be able to pretty much define it in any of the config files and all the keys will have the _defaults in the end.

About your last point, you think we should deal with that? it wont really be a problem unless the two options have values that don't match. When ever a default value is being used its printed to the screen, so in case of collisions the user will know right away.

meanwhile ill add the defaults class and the create option. tell me if you agree on the design so far.
thanks :).

mbacovsky · 2015-07-27T15:49:18Z

config/cli_config_example.yml

+  :organization_id: 293
+  :organization_name: Redhat
+:user_defaults:
+  :organization_id: 2


Please nest the options one level deeper so it doesn't mess with other config options. Then the fragile _defaults suffix is not needed and it can make human editing and even processing more netural.

:defaults: :_global: :organization_id: 293 :organization_name: Redhat :user: :organization_id: 2

ok, can do that.

mbacovsky · 2015-07-27T15:50:48Z

I'm not sure about the 'global' vs 'resource related' distinction. Do we have any real use- case for that?
What if user wants to use the default org_id in more Katello resources, does she end up with 10 same org_id occurrences in the config?
Would it be possible to have it just global and wait with adding resource blacklist/whitelist after having some users feedback?

mbacovsky · 2015-07-27T16:24:15Z

lib/hammer_cli/defaults.rb

+    def self.find_valid_conf_file
+      path = nil
+      HammerCLI::Settings.path_history.each do |p|
+        path = p if p.include?("cli_config.yml")


Good idea to use the path_history. It lead me to interesting idea:
I use my hammer to manage multiple foreman/sat instances. It would be nice to have different defaults for each of them as the org_id will probably differ. I usually use hammer -c ~/.hammer/dev.yml commands... where the dev.yml sets the right url and credentials.
Not sure how to detect the right file in that case though. Or we could put the defaults to separate file and make the path part of the configuration:

:defaults_path: '~/.hammer/dev_defaults.yml'

Then you could create the file for the user if it doesn't exist.
What do you think?

We can also do that, but the thing is that we said that its better the same file.
I think that for now we can leave it like that, if we would like to change it afterwards, its a small fix.
How common it is for a regular user to manage multiple instances?

mbacovsky · 2015-07-27T16:35:19Z

Besides the comments above I'd recommend to keep the verbosity as low as possible not to complicate automation. Try to use logger with verbosity of your choice where possible instead of print_message. Also make sure all the strings visible to user are wrapped in _()
Thanks for the updates, it looks it is on right track 🍺

alongoldboim · 2015-07-27T18:58:15Z

OK i agree with you, better putting it in the logger, and ill wrap the strings.
Thanks for the over view, ill make an update soon.

mbacovsky · 2015-08-03T11:32:26Z

I looked again at the generate defaults command. Would you please explain the purpose and usage of this command? Why defaults add couldn't do the job?

alongoldboim · 2015-08-03T11:49:25Z

The defaults add command is for a user to add a defaults setting on his own, the generate is a method that every plugin can override and then it can generate defaults for you(e.g in foreman the generate method will already place org_id and loc_id taken from the web UI) , in every plugin we got for hammer it will be implemented differently.

mbacovsky · 2015-12-02T12:33:23Z

Btw how do we want to handle nested params? Let's say I have organization_id set to '5'.
If I create new host should it be created within organization with id 5 by default or not?
It is not now as I tried it. Is it a bug or expected behavior? @alongoldboim, @tstrachota ?

tstrachota · 2015-12-02T12:48:23Z

@mbacovsky imho it should assign host to that organization. It should behave the same as when you switch context in UI.

alongoldboim · 2015-12-02T13:29:02Z

@mbacovsky I agree with @tstrachota, ill check the reason it's not behaving like that now.

alongoldboim · 2015-12-06T11:23:19Z

@mbacovsky @tstrachota changed a bit of the logic on when the defaults are added, and changed the tests accordingly, please give it a check.

mbacovsky · 2015-12-07T11:28:00Z

lib/hammer_cli/abstract.rb

-        h
+      self.class.recognised_options.inject({}) do |hash, opt|
+        hash[opt.attribute_name] = send(opt.read_method)
+        hash[opt.attribute_name] = context[:defaults].get_defaults(opt.attribute_name) if hash[opt.attribute_name].nil? && context[:defaults]


Can you keep the logging of the default being used that was present in previous version. It can make bug hunting easier in the future.

mbacovsky · 2015-12-07T11:33:40Z

@alongoldboim, I like the new location of getting the defaults. It seems more flexible as you can defaults for options that are not comming from API. Well played. It also works fine and the user experience is good.

When the logging is added I'm for merging it. 🎺

alongoldboim · 2015-12-07T11:37:59Z

@mbacovsky yea that was part of the idea in moving it there :), thanks, will add logging.

alongoldboim · 2015-12-07T15:24:30Z

@mbacovsky @tstrachota added some fixes to improve performance.

mbacovsky · 2015-12-07T23:29:27Z

lib/hammer_cli/abstract.rb

+      @all_options
+    end
+
+    def add_custom_defaults(attr)


I'm still missing the logging of the defaults being used. The add_custom_defaults is good candidate for doing that:

def add_custom_defaults(attr) if context[:defaults] value = context[:defaults].get_defaults(attr) logger.info("Custom default value #{value} was used for attribute #{attr}") if value end value end

I'd also make it private, as it is not intended to be part of public command interface

alongoldboim · 2015-12-08T05:59:57Z

@mbacovsky logger added, moved to private.

mbacovsky · 2015-12-08T07:27:11Z

@alongoldboim, thanks! I still need to test it, but it looks good now.

mbacovsky · 2015-12-08T12:10:09Z

@alongoldboim It looks good and works as expected now! Thank you. 🍻 👷
👍 form me. @tstrachota could you confirm it is okay to merge?

alongoldboim · 2015-12-08T12:19:58Z

👍 :)

tstrachota · 2015-12-08T12:36:12Z

test/functional/defaults_test.rb

+  end
+
+  describe 'defaults providers' do
+	  header = ['---------|-------------------',


Tab indentation

tstrachota · 2015-12-08T12:39:46Z

@alongoldboim 👍 pending the whitespace issues. Please set your editor to use spaces instead of tabs.

First patch replacing tabs with spaces reuse directory constant - it should be defined at a single place to avoid complications when there's need to rename - such constants should be extracted from translation strings fixed indentation variables renamed to more meaningful names - just a cosmetic thing - you had the file contents named file which I found confusing removed comment that was no longer true defaults moved to context - avoids mocking the global defaults class which makes testing a lot easier. See my next commit with tests. adding tests for the commands - before they weren't tested at all assert_equal takes expected value as the first parameter - in assertions the first parameter is always the expected one - see for example http://ruby-doc.org/stdlib-2.0.0/libdoc/minitest/rdoc/MiniTest/Assertions.html fixing test that actually tested only the mocking mechanism - changed data of one test to make sure the tested modification actually happens. Before it was in the expected state already. - "should get the default param, with provider" actually only tested mocking use specific exceptions - formerly used NameError can be raised also when a method is missing, which makes debugging more difficult - similar goes for StandardError use instance of defaults - allows for injecting the filename - no need to pollute the global defaults when testing it

alongoldboim · 2015-12-08T13:09:13Z

@tstrachota @mbacovsky done deal.

mbacovsky · 2015-12-08T15:02:26Z

@alongoldboim Thanks, merging!!! 🍸

Fixes #8015 - Enable adding defaults options for commands.

theforeman-bot added Needs testing Not yet reviewed labels Jul 19, 2015

alongoldboim reviewed Jul 19, 2015
View reviewed changes

alongoldboim force-pushed the 8015 branch from 35ce5a1 to 06943bb Compare July 19, 2015 13:04

mbacovsky reviewed Jul 20, 2015
View reviewed changes

alongoldboim force-pushed the 8015 branch 7 times, most recently from 83c6d5d to 0a687a6 Compare July 23, 2015 09:04

mbacovsky reviewed Jul 27, 2015
View reviewed changes

alongoldboim force-pushed the 8015 branch 8 times, most recently from ee25a80 to 7d7bbb2 Compare August 3, 2015 10:45

alongoldboim force-pushed the 8015 branch from 7d7bbb2 to e4aece1 Compare August 3, 2015 11:44

alongoldboim force-pushed the 8015 branch 4 times, most recently from f1daf20 to f51fa73 Compare December 6, 2015 11:22

mbacovsky reviewed Dec 7, 2015
View reviewed changes

alongoldboim force-pushed the 8015 branch from f51fa73 to 43be9e8 Compare December 7, 2015 15:22

mbacovsky reviewed Dec 7, 2015
View reviewed changes

alongoldboim force-pushed the 8015 branch 2 times, most recently from ec43546 to dbec9e7 Compare December 8, 2015 05:59

tstrachota reviewed Dec 8, 2015
View reviewed changes

test/functional/defaults_test.rb

end

describe 'defaults providers' do

header = ['---------|-------------------',

Copy link

Member

tstrachota Dec 8, 2015

Choose a reason for hiding this comment

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

Tab indentation

alongoldboim force-pushed the 8015 branch from dbec9e7 to 417e716 Compare December 8, 2015 13:07

mbacovsky added a commit that referenced this pull request Dec 8, 2015

Merge pull request #174 from alongoldboim/8015

31b7822

Fixes #8015 - Enable adding defaults options for commands.

mbacovsky merged commit 31b7822 into theforeman:master Dec 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #8015 - Enable adding defaults options for commands. #174

Fixes #8015 - Enable adding defaults options for commands. #174

alongoldboim commented Jul 19, 2015

alongoldboim Jul 19, 2015

mbacovsky Jul 20, 2015

alongoldboim Jul 20, 2015

mbacovsky commented Jul 20, 2015

alongoldboim commented Jul 20, 2015

mbacovsky Jul 27, 2015

alongoldboim Jul 27, 2015

mbacovsky commented Jul 27, 2015

mbacovsky Jul 27, 2015

alongoldboim Jul 27, 2015

mbacovsky commented Jul 27, 2015

alongoldboim commented Jul 27, 2015

mbacovsky commented Aug 3, 2015

alongoldboim commented Aug 3, 2015

mbacovsky commented Dec 2, 2015

tstrachota commented Dec 2, 2015

alongoldboim commented Dec 2, 2015

alongoldboim commented Dec 6, 2015

mbacovsky Dec 7, 2015

mbacovsky commented Dec 7, 2015

alongoldboim commented Dec 7, 2015

alongoldboim commented Dec 7, 2015

mbacovsky Dec 7, 2015

alongoldboim commented Dec 8, 2015

mbacovsky commented Dec 8, 2015

mbacovsky commented Dec 8, 2015

alongoldboim commented Dec 8, 2015

tstrachota Dec 8, 2015

tstrachota commented Dec 8, 2015

alongoldboim commented Dec 8, 2015

mbacovsky commented Dec 8, 2015

Fixes #8015 - Enable adding defaults options for commands. #174

Fixes #8015 - Enable adding defaults options for commands. #174

Conversation

alongoldboim commented Jul 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky commented Jul 20, 2015

alongoldboim commented Jul 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky commented Jul 27, 2015

alongoldboim commented Jul 27, 2015

mbacovsky commented Aug 3, 2015

alongoldboim commented Aug 3, 2015

mbacovsky commented Dec 2, 2015

tstrachota commented Dec 2, 2015

alongoldboim commented Dec 2, 2015

alongoldboim commented Dec 6, 2015

Choose a reason for hiding this comment

mbacovsky commented Dec 7, 2015

alongoldboim commented Dec 7, 2015

alongoldboim commented Dec 7, 2015

Choose a reason for hiding this comment

alongoldboim commented Dec 8, 2015

mbacovsky commented Dec 8, 2015

mbacovsky commented Dec 8, 2015

alongoldboim commented Dec 8, 2015

Choose a reason for hiding this comment

tstrachota commented Dec 8, 2015

alongoldboim commented Dec 8, 2015

mbacovsky commented Dec 8, 2015