Fixes #3969 - Add template combinations commands #378

shiramax · 2018-07-30T07:57:45Z

No description provided.

theforeman-bot · 2018-07-30T07:57:50Z

@shiramax, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #6969: foreman-debug is missing log files which are rotated is the correct one
move ticket #6969 from Foreman to the Hammer CLI project
or file a new ticket in the Hammer CLI project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

adamruzicka

The output fields need to have human-readable descriptions

adamruzicka · 2018-07-31T09:02:07Z

lib/hammer_cli_foreman/combinations.rb

+    class InfoCombination < HammerCLIForeman::InfoCommand
+      output do
+        field :id, _('Id')
+        field :provisioning_template_id, _('provisioning_template_id')


The descriptions should be human readable and properly capitalized so _('Provisioning template ID')

shiramax · 2018-07-31T12:07:05Z

Thanks, @adamruzicka , I Fixed all the descriptions.

adamruzicka

Left some nitpicks inline, otherwise looks good

adamruzicka · 2018-07-31T13:55:27Z

lib/hammer_cli_foreman/combinations.rb

+    command_name 'combinations'
+    desc _("Manage template combinations")
+
+


two empty lines

adamruzicka · 2018-07-31T13:57:58Z

lib/hammer_cli_foreman/combinations.rb

+        params = super
+        params["template_combination"] =  {}
+        params["template_combination"]["hostgroup_id"] = params["hostgroup_id"].to_s
+        params["template_combination"]["environment_id"] = params["environment_id"].to_s


this could most likely be shortened to

def request_params combination_params = { 'hostgroup_id' => params['hostgroup_id'].to_s, 'environment_id' => params['environment_id'].to_s } super.merge('template_combination' => combination_params) end

adamruzicka · 2018-07-31T14:00:11Z

lib/hammer_cli_foreman/combinations.rb

+        field :environment_id, _('Environment ID')
+        field :environment_name, _('Environment name')
+        field :config_template_id, _('Config_template ID')
+        field :config_template_name, _('Config_template name')


There are still underscores in the descriptions.

shiramax · 2018-07-31T14:41:22Z

tThanks @adamruzicka, could you please review again?

adamruzicka

Left one nitpick inline, otherwise it looks good to me and works as expected.

I'd still like to hear opinions from our hammer gurus. @tstrachota, @mbacovsky could you take a look?

adamruzicka · 2018-08-01T08:06:58Z

lib/hammer_cli_foreman/combinations.rb

+      end
+
+      build_options
+    end


There should probably be one empty line between the classes

done :) thanks!

mbacovsky

@shiramax thanks for adding this long-time missing command 🍨. Overall it looks very good. I've suggested a few optimizations inline.

There are some options to the commands that I'd prefer be hidden in the help:

$  hammer template combinations list -h                       
Usage:
    hammer template combinations list [OPTIONS]

Options:
 --config-template CONFIG_TEMPLATE_NAME              Name to search by
 --config-template-id CONFIG_TEMPLATE_ID              
 --environment ENVIRONMENT_NAME                      Environment name
 --environment-id ENVIRONMENT_ID                      
 --hostgroup HOSTGROUP_NAME                          Hostgroup name
 --hostgroup-id HOSTGROUP_ID                          
 --hostgroup-title HOSTGROUP_TITLE                   Hostgroup title
 --location LOCATION_NAME                            Location name
 --location-id LOCATION_ID                            
 --location-title LOCATION_TITLE                     Location title
 --organization ORGANIZATION_NAME                    Organization name
 --organization-id ORGANIZATION_ID                   Organization ID
 --organization-title ORGANIZATION_TITLE             Organization title
 --provisioning-template PROVISIONING_TEMPLATE_NAME  Name to search by
 --provisioning-template-id PROVISIONING_TEMPLATE_ID  
 -h, --help                                          Print help

The --config-templates* should not be shown neither here nor in the other commands. In the list command the --hostgroup* and --environment* do not have any impact on the result so it would be better to remove them.

I'm not sure what was our conclusion on extending template info with combinations data. The data are already returned by the server and extending the output should be an easy and useful addition. Do you intend to add it? If so I leave it up to you if you want to include it in here or in separate PR. Let me know otherwise as we should create a new issue to track it.

Let me know if I can help.

mbacovsky · 2018-08-02T14:20:37Z

lib/hammer_cli_foreman/template.rb

@@ -194,6 +194,10 @@ def method_options_for_params(params, include_nil = true)
      build_options
    end

+    lazy_subcommand('combinations', _("Manage template combinations"),


For a resource with subcommands we usually use name in singular - combination.

mbacovsky · 2018-08-02T14:27:10Z

lib/hammer_cli_foreman/combinations.rb

+        field :hostgroup_name, _('Hostgroup name')
+        field :environment_id, _('Environment ID')
+        field :environment_name, _('Environment name')
+        field :config_template_id, _('Config template ID')


no config templates mentioned anywhere as it is deprecated in core. This is new command so we don't want to expose it

mbacovsky · 2018-08-02T15:03:33Z

lib/hammer_cli_foreman/combinations.rb

+        field :id, _('ID')
+        field :provisioning_template_id, _('Provisioning template ID')
+        field :provisioning_template_name, _('Provisioning template name')
+        field :hostgroup_id, _('Hostgroup ID')


field nil, _("Hostgroup"), Fields::SingleReference, :key => :hostgroup

should replace the two hosthgroup fields.
You can use same approach for env and template. It is less code with better control over the output.

mbacovsky · 2018-08-02T15:06:38Z

lib/hammer_cli_foreman/combinations.rb

+    end
+
+    class InfoCombination < HammerCLIForeman::InfoCommand
+      output do


you can use

output ListCommand.output_definition do

to re-use and extend output definition form Index and make it more DRY

mbacovsky · 2018-08-02T15:15:11Z

lib/hammer_cli_foreman/combinations.rb

+    end
+    autoload_subcommands
+  end
+end


We usually require a new line at the end. Some good reasons can be found e.g. https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file. You may want to consider tuning your editor to do it for you automatically.

shiramax · 2018-08-06T14:39:34Z

Thanks @mbacovsky !
could you please review again?

mbacovsky · 2018-08-20T07:41:24Z

lib/hammer_cli_foreman/template.rb

+        collection :template_combinations, 'Template Combinations' do
+          field :id, _('ID')
+          field :provisioning_template_id, _('Provisioning template ID')
+          field :provisioning_template_name, _('Provisioning template name')


:provisioning_template* could be excluded as it is always same as the template. Also we usually do not include the other ids here so that the output is not too long.

mbacovsky · 2018-08-20T07:45:56Z

@shiramax, thanks for the update and extending the info! I have just one inline comment and one thing that I overlooked last time:

hammer template combination update -h
...
 --name NAME                                         >Name to search by<
 --new-name NEW_NAME      
...

The combinations do not have name so we should not offer the related options. This could be fixed in IdResolver.

mbacovsky · 2018-08-20T07:49:18Z

One more thing, I'm missing the template combination delete command.

mbacovsky · 2018-08-20T12:10:36Z

test/functional/template_test.rb

+    it 'should create new combination' do
+      params = ['create','--provisioning-template-id=10', '--hostgroup-id=1', '--environment-id=1']
+      expected_result = success_result("Template combination created.\n")
+      api_expects(:template_combinations, :create, 'Create Compute template combination') do |params|


template combination

mbacovsky · 2018-08-20T12:10:46Z

test/functional/template_test.rb

+    it 'should update combination' do
+      params = ['update','--id=3','--provisioning-template-id=10', '--hostgroup-id=1', '--environment-id=1']
+      expected_result = success_result("Template combination updated.\n")
+      api_expects(:template_combinations, :update, 'Update Compute template combination') do |params|


mbacovsky · 2018-08-20T15:06:53Z

👍 Thanks @shiramax, well done!

theforeman-bot added the Waiting on contributor label Jul 30, 2018

theforeman-bot added Needs testing Not yet reviewed labels Jul 30, 2018

shiramax force-pushed the 3969 branch from 2183c25 to d746bf2 Compare July 30, 2018 08:06

theforeman-bot added Needs testing and removed Waiting on contributor labels Jul 30, 2018

shiramax changed the title ~~Fixes #6969 - Add template combinations commands~~ Fixes #3969 - Add template combinations commands Jul 30, 2018

adamruzicka requested changes Jul 31, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jul 31, 2018

shiramax force-pushed the 3969 branch from d746bf2 to bac1444 Compare July 31, 2018 12:05

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 31, 2018

adamruzicka requested changes Jul 31, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 31, 2018

shiramax force-pushed the 3969 branch from bac1444 to 7200369 Compare July 31, 2018 14:40

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 31, 2018

adamruzicka reviewed Aug 1, 2018

View reviewed changes

shiramax force-pushed the 3969 branch from 7200369 to 1b9a4da Compare August 2, 2018 07:30

mbacovsky suggested changes Aug 2, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Aug 2, 2018

shiramax force-pushed the 3969 branch from 1b9a4da to 3aebb82 Compare August 6, 2018 14:38

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Aug 6, 2018

mbacovsky reviewed Aug 20, 2018

View reviewed changes

shiramax force-pushed the 3969 branch 3 times, most recently from 8286ec2 to 6d29da5 Compare August 20, 2018 14:38

Fixes #3969 - Add template combinations commands

08f3d5d

shiramax force-pushed the 3969 branch from 6d29da5 to 08f3d5d Compare August 20, 2018 14:43

mbacovsky approved these changes Aug 20, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 20, 2018

mbacovsky merged commit de248b8 into theforeman:master Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3969 - Add template combinations commands #378

Fixes #3969 - Add template combinations commands #378

shiramax commented Jul 30, 2018

theforeman-bot commented Jul 30, 2018

adamruzicka left a comment

adamruzicka Jul 31, 2018

shiramax commented Jul 31, 2018

adamruzicka left a comment

adamruzicka Jul 31, 2018

adamruzicka Jul 31, 2018

adamruzicka Jul 31, 2018

shiramax commented Jul 31, 2018

adamruzicka left a comment

adamruzicka Aug 1, 2018

shiramax Aug 2, 2018

mbacovsky left a comment

mbacovsky Aug 2, 2018

mbacovsky Aug 2, 2018

mbacovsky Aug 2, 2018

mbacovsky Aug 2, 2018

mbacovsky Aug 2, 2018

shiramax commented Aug 6, 2018

mbacovsky Aug 20, 2018

mbacovsky commented Aug 20, 2018

mbacovsky commented Aug 20, 2018

mbacovsky Aug 20, 2018

mbacovsky Aug 20, 2018

mbacovsky commented Aug 20, 2018

		command_name 'combinations'
		desc _("Manage template combinations")

Fixes #3969 - Add template combinations commands #378

Fixes #3969 - Add template combinations commands #378

Conversation

shiramax commented Jul 30, 2018

theforeman-bot commented Jul 30, 2018

adamruzicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiramax commented Jul 31, 2018

adamruzicka left a 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

shiramax commented Jul 31, 2018

adamruzicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky left a 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

shiramax commented Aug 6, 2018

Choose a reason for hiding this comment

mbacovsky commented Aug 20, 2018

mbacovsky commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky commented Aug 20, 2018