BZ #20538 - add compute profile commands #366

shiramax · 2018-04-15T13:42:32Z

No description provided.

shiramax · 2018-04-29T12:40:46Z

needs to merge :
theforeman/hammer-cli#274

shiramax · 2018-06-14T06:39:11Z

@mbacovsky please review :)

ofedoren

I found some minor issues. I think it's good to have them fixed anyway.

ofedoren · 2018-06-20T13:28:52Z

lib/hammer_cli_foreman/compute_profile.rb

+      params = {}
+      params['compute_attribute'] = {}
+      profile = HammerCLIForeman.record_to_common_format(
+        HammerCLIForeman.foreman_resource(:compute_profiles).call(:show, {'id' => options['option_compute_profile_id'] })


Not important, but still: Redundant curly braces around a hash parameter

ofedoren · 2018-06-20T13:30:48Z

lib/hammer_cli_foreman/compute_profile.rb

+      profile = HammerCLIForeman.record_to_common_format(
+        HammerCLIForeman.foreman_resource(:compute_profiles).call(:show, {'id' => options['option_compute_profile_id'] })
+      )
+      HammerCLIForeman.foreman_resource(:compute_resources).call(:show, {'id' => options['option_compute_resource_id'] })


ditto curly braces

ofedoren · 2018-06-20T13:38:30Z

lib/hammer_cli_foreman/compute_profile.rb

+        HammerCLIForeman.foreman_resource(:compute_profiles).call(:show, {'id' => options['option_compute_profile_id'] })
+      )
+      HammerCLIForeman.foreman_resource(:compute_resources).call(:show, {'id' => options['option_compute_resource_id'] })
+      params['compute_attribute'] = profile['compute_attributes'].select{ |hash|  hash['compute_resource_id']== options['option_compute_resource_id']}[0] || {}


again, not important, but some spaces are missing .select{ and hash['compute_resource_id']==

ofedoren · 2018-06-20T13:38:34Z

lib/hammer_cli_foreman/compute_profile.rb

+    end
+
+    class CreateCommand < HammerCLIForeman::CreateCommand
+      success_message _('Compute profile created')


Almost all success messages should end with '.' at the end. You might use the test from this PR to check yourself.

ofedoren · 2018-06-20T13:38:42Z

lib/hammer_cli_foreman/compute_profile.rb

+      validate_options do
+        any(:option_name,:option_id).required
+      end
+      build_options :without => :name


Most likely I'm mistaken, but doesn't seem it does anything.

ofedoren · 2018-06-20T13:46:03Z

lib/hammer_cli_foreman/compute_profile.rb

+      build_options :without => :id
+    end
+
+    class AddInterface < HammerCLIForeman::UpdateCommand


Maybe it should inherit CreateCommand instead of UpdateCommand?

Also it doesn't seem to work. I tried hammer compute-profile add-interface --compute-resorce tstracho-laptop --compute-profile 1-Small --set-values type=network,bridge=br1 and got Could not create interface: Error: undefined method `[]' for nil:NilClass. Does it work for you?

bridge is not allowed attribute for any of the compute resources
check hammer compute-profile add-interface --help
and I can't reproduce this error at all.. are the compute profile and compute resource name exists?
what is the compute resource your testing on? ovirt?

I think UpdateCommand is correct as we add interface attrs by modifying existing compute_attribute. I was not able to reproduce the error. However it is a bit unclear what is the relation - can each compute profile have only one compute attribute per compute resource?

@shiramax I think it would be worth a comment in the code explaining why we use the Update predecessor. We will have to care about the --name and --new-name parameters. Option builder removed the --id but those left. Let me know if you don't find out how to remove it.

the relation - can each compute profile have only one compute attribute per compute resource?
^^ you can have only one compute attribute, but inside the compute attribute you can have multiple volumens and interfaces.

ofedoren · 2018-06-20T13:47:21Z

lib/hammer_cli_foreman/compute_profile.rb

+      build_options :without => :id
+    end
+
+    class RemoveInterface < HammerCLIForeman::UpdateCommand


Maybe it should inherit DeleteCommand instead of UpdateCommand?

I think the UpdateCommand is needed in this case because the remove is done by updating the resource.

@shiramax Could you add a explanation to the code and remove the --name options?

ofedoren · 2018-06-20T13:48:27Z

lib/hammer_cli_foreman/compute_profile.rb

+      build_options :without => :id
+    end
+
+    class RemoveVolume < HammerCLIForeman::UpdateCommand


same I mentioned above

ofedoren · 2018-06-20T13:51:02Z

lib/hammer_cli_foreman/compute_profile.rb

+    class RemoveInterface < HammerCLIForeman::UpdateCommand
+      command_name _('remove-interface')
+      resource :compute_attributes
+      desc _('Update compute profile interface')


Maybe it should say "Remove compute ..."?

ofedoren · 2018-06-20T13:51:22Z

lib/hammer_cli_foreman/compute_profile.rb

+    class RemoveVolume < HammerCLIForeman::UpdateCommand
+      command_name _('remove-volume')
+      resource :compute_attributes
+      desc _('Update compute profile volume')


same I mentioned above

shiramax · 2018-06-24T14:09:16Z

@ofedoren thanks! I fixed all the comments :)

ofedoren · 2018-07-04T11:55:41Z

@shiramax great, thank you!

But still I get

Could not create interface:
  Error: undefined method `[]' for nil:NilClass

while trying

hammer compute-profile add-interface --compute-resource-id 3 --compute-profile-id 1 --set-values type=network,bridge=br1

Is it only me?

ofedoren · 2018-07-04T12:13:48Z

lib/hammer_cli_foreman/compute_profile.rb

+      resource :compute_attributes
+      desc _('Add volume for Compute Profile')
+
+      option '--set-values', 'VOLUME', _('Volume parameters, should be comma separated list of values.'),


Sorry for bothering you with this again, but there is a convention, that says option descriptions should not end with a dot. Simply remove '.' at the end of option description.

ofedoren · 2018-07-04T12:16:23Z

lib/hammer_cli_foreman/compute_profile.rb

+      resource :compute_attributes
+      desc _('Update compute profile volume')
+
+      option '--set-values', 'VOLUME', _('Volume parameters, should be comma separated list of values.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:16:49Z

lib/hammer_cli_foreman/compute_profile.rb

+      command_name _('remove-volume')
+      resource :compute_attributes
+      desc _('Remove compute profile volume')
+      option '--volume-id', 'VOLUME_ID', _('Volume id.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:17:10Z

lib/hammer_cli_foreman/compute_profile.rb

+
+      desc _('Update compute profile interface')
+
+      option '--set-values', 'SET VALUES', _('Interface parameters, should be comma separated list of values.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:17:17Z

lib/hammer_cli_foreman/compute_profile.rb

+      option '--set-values', 'SET VALUES', _('Interface parameters, should be comma separated list of values.'),
+             :required => true,
+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
+      option '--interface-id', 'INTERFACE_ID', _('Interface id.'),


ditto "dot at the end"

i don't really understand where to put the dot here

ofedoren · 2018-07-04T12:17:50Z

lib/hammer_cli_foreman/compute_profile.rb

+      command_name _('add-interface')
+      resource :compute_attributes
+      desc _('Add interface for Compute Profile')
+      option '--set-values', 'SET VALUES', _('Interface parameters, should be comma separated list of values.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:21:07Z

lib/hammer_cli_foreman/compute_profile.rb

+
+      option '--compute-attributes', 'COMPUTE_ATTRS', _('Compute resource attributes.'),
+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
+      option '--interface', 'INTERFACE', _('interface parameters, should be comma separated list of values.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:21:12Z

lib/hammer_cli_foreman/compute_profile.rb

+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
+      option '--interface', 'INTERFACE', _('interface parameters, should be comma separated list of values.'),
+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
+      option '--volume', 'VOLUME', _('Volume parameters, should be comma separated list of values.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:21:56Z

lib/hammer_cli_foreman/compute_profile.rb

+      command_name _('update-attributes')
+      resource :compute_attributes
+
+      option '--compute-attributes', 'COMPUTE_ATTRS', _('Compute resource attributes, should be comma separated list of values.'),


ditto "dot at the end"

ofedoren · 2018-07-04T12:22:14Z

lib/hammer_cli_foreman/compute_profile.rb

+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
+      option '--interface-id', 'INTERFACE_ID', _('Interface id'),
+             :format => HammerCLI::Options::Normalizers::Number.new
+      option '--volume', 'VOLUME', _('Volume parameters, should be comma separated list of values.'),


ditto "dot at the end" and 2 above

ofedoren · 2018-07-04T15:23:46Z

@shiramax I apologize for any misunderstanding, everything is fine now :) Thanks!

iNecas · 2018-07-31T10:21:55Z

@mbacovsky mind finishing the review?

mbacovsky

@shiramax thanks for this extensive work! I've put my comments inline. For the first round I focused on the commands and structure of the code. Overall it looks very good. It passed some basic testing with exception for the translated commands and extra `--name' options.

I still need to go through the tests and help extensions withregard on the other help related work. Will update.

mbacovsky · 2018-08-21T13:50:45Z

lib/hammer_cli_foreman/compute_profile.rb

+    end
+
+    class SetAttributes < HammerCLIForeman::CreateCommand
+      command_name _('set-attributes')


Do not mark commands for translations

mbacovsky · 2018-08-21T14:23:48Z

lib/hammer_cli_foreman/compute_profile.rb

+        HammerCLIForeman::References.taxonomies(self)
+        HammerCLIForeman::References.timestamps(self)
+
+        collection :compute_attributes, 'Compute attributes' do


Compute attributes needs translation

mbacovsky · 2018-08-21T14:55:35Z

lib/hammer_cli_foreman/compute_profile.rb

+      success_message _('Compute profile created.')
+      failure_message _('Could not create a compute profile')
+
+      validate_options do


The name is required by API no need for the validator.

mbacovsky · 2018-08-29T09:16:36Z

lib/hammer_cli_foreman/compute_profile.rb

+      profile = HammerCLIForeman.record_to_common_format(
+        HammerCLIForeman.foreman_resource(:compute_profiles).call(:show, 'id' => options['option_compute_profile_id'] )
+      )
+      HammerCLIForeman.foreman_resource(:compute_resources).call(:show, 'id' => options['option_compute_resource_id'] )


Is this value used anywhere?

Do we need this call in here?

mbacovsky · 2018-08-29T10:02:04Z

lib/hammer_cli_foreman/compute_profile.rb

+      end
+
+      def request_params
+        params = {}


if you use params = super here you don't have to care about compute attribute and compute profile name resolution later in this method

mbacovsky · 2018-08-29T11:15:52Z

lib/hammer_cli_foreman/compute_profile.rb

+      build_options :without => :id
+    end
+
+    class AddInterface < HammerCLIForeman::UpdateCommand


I think UpdateCommand is correct as we add interface attrs by modifying existing compute_attribute. I was not able to reproduce the error. However it is a bit unclear what is the relation - can each compute profile have only one compute attribute per compute resource?

@shiramax I think it would be worth a comment in the code explaining why we use the Update predecessor. We will have to care about the --name and --new-name parameters. Option builder removed the --id but those left. Let me know if you don't find out how to remove it.

mbacovsky · 2018-08-29T11:23:18Z

lib/hammer_cli_foreman/compute_profile.rb

+    end
+
+    class UpdateInterface < HammerCLIForeman::UpdateCommand
+      command_name _('update-interface')


There are --name and --new-name options left in the command.

mbacovsky · 2018-08-29T11:25:22Z

lib/hammer_cli_foreman/compute_profile.rb

+      build_options :without => :id
+    end
+
+    class RemoveInterface < HammerCLIForeman::UpdateCommand


@shiramax Could you add a explanation to the code and remove the --name options?

mbacovsky · 2018-08-29T13:34:43Z

lib/hammer_cli_foreman/compute_profile.rb

+    end
+
+    class AddVolume < HammerCLIForeman::UpdateCommand
+      command_name _('add-volume')


Same as for the commands above

mbacovsky · 2018-08-29T13:43:53Z

lib/hammer_cli_foreman/compute_profile.rb

+      build_options :without => :id
+    end
+
+    class UpdateVolume < HammerCLIForeman::UpdateCommand


Same as above

shiramax · 2018-08-30T14:49:07Z

@mbacovsky could you please remind me in which file I can configure the help to show the translation?
thanks.

mbacovsky · 2018-09-06T09:52:19Z

@shiramax you need to set this option https://github.com/theforeman/hammer-cli/blob/master/config/cli_config.template.yml#L10. It is most likely in ~/.hammer/cli_config.yml

mbacovsky · 2018-10-16T14:11:06Z

lib/hammer_cli_foreman/compute_profile.rb

+    end
+
+    class DeleteCommand < HammerCLIForeman::DeleteCommand
+      success_message 'Compute profile deleted.'


Translation marker is missing

mbacovsky · 2018-10-16T14:11:26Z

lib/hammer_cli_foreman/compute_profile.rb

+
+    class DeleteCommand < HammerCLIForeman::DeleteCommand
+      success_message 'Compute profile deleted.'
+      failure_message 'Could not delete the Compute profile'


Translation marker is missing

mbacovsky · 2018-10-16T14:14:39Z

lib/hammer_cli_foreman/compute_profile.rb

+
+      option '--compute-attributes', 'COMPUTE_ATTRS', _('Compute resource attributes'),
+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
+      option '--interface', 'INTERFACE', _('interface parameters, should be comma separated list of values'),


Capitalize "Interface"

mbacovsky · 2018-10-16T14:16:04Z

lib/hammer_cli_foreman/compute_profile.rb

+      command_name 'add-interface'
+      resource :compute_attributes
+      desc _('Add interface for Compute Profile.')
+      option '--set-values', 'SET VALUES', _('Interface parameters, should be comma separated list of values'),


underscore missing in 'SET_VALUES'

mbacovsky · 2018-10-16T14:16:40Z

lib/hammer_cli_foreman/compute_profile.rb

+
+      desc _('Update compute profile interface.')
+
+      option '--set-values', 'SET VALUES', _('Interface parameters, should be comma separated list of values'),


Underscore is missing in 'SET_VALUES"

mbacovsky · 2018-10-16T15:27:49Z

lib/hammer_cli_foreman/compute_profile.rb

+        end
+      end
+
+      def validate_options


Validators are preferred in a block:

validate_options do any(:option_compute_profile_id, :option_compute_profile_name ).required any(:option_compute_resource_id, :option_compute_resource_name).required end

mbacovsky · 2018-10-16T15:37:23Z

lib/hammer_cli_foreman/compute_profile.rb

+      end
+
+      def validate_options
+        super


We prefer validators in a block (see comment above).

mbacovsky · 2018-10-16T15:48:47Z

lib/hammer_cli_foreman/compute_profile.rb

+        super
+        validator.any(:option_compute_profile_id, :option_compute_profile_name ).required
+        validator.any(:option_compute_resource_id, :option_compute_resource_name).required
+        validator.any(:option_interface, :option_volume, :option_compute_attributes).required


How do I identify what compute attribute I'm going to update? I'd expect --compute-attribute or --compute-attribute-id.
Is compute-attribute-name unique or do we need to add also compute profile or compute resource to search for the compute attribute by name? If so It should be required only when there is compute attribute name given.

Or is it that (compute profile, compute resource) has at most one compute attribute?

compute attribute is a combination of compute profile and compute resource. It doesn't have a name

mbacovsky · 2018-10-16T16:12:43Z

lib/hammer_cli_foreman/compute_profile.rb

+
+    # Using the Update command because adding a new interface is done by modifying existing compute_attribute
+    class AddInterface < HammerCLIForeman::UpdateCommand
+      command_name 'add-interface'


What is the difference between compute-profile update-attributes --volume <attrs> and compute-profile update-volume --set-values <attrs>?
If it is the same functionality isn't it confusing for the user to have two similar ways how to achieve the same?

Yes, I I remember correctly when designing the commands, we talked about it.
the update attributes will let the user update in one command interface & volume & other attributes.
update-volume is easier if the user wants to update the volume.

I agree it might be confusing, what do you think is the best approach here?

mbacovsky · 2018-10-16T19:04:13Z

@shiramax, thanks for fixing the issues mentioned in my comments!

I finally did another round of review and found out the UX a bit unclear. The most prominent thing is that it is not clear to me what is the difference between compute-profile update-attributes --volume <attrs> and compute-profile update-volume --set-values <attrs>. Do we need both?

Another thing are the parameters used to address the compute attributes. What I'm concerned about is I didn't found 'compute attributes' mentioned in the UI (besides the URLs). In the UI we present the compute profile as a set of compute profiles with same name for different resources. Do you think it would make sense to mimic this in the CLI somehow? Was the UX design discussed before?

mbacovsky · 2018-10-16T19:08:04Z

Would you mind and rebase this PR? Also it seems the validations will be easier to do once I merge theforeman/hammer-cli#242 so it can wait till the merge happens and the UX is resolved.

shiramax · 2018-10-21T12:28:32Z

@mbacovsky ,
You are right, I thought about it too..The compute attributes are not presented in the UI, except in the URLs.
As you said, In the UI the compute profile is presented as a set of compute profiles with the same name for different resources, This behavior is more confusing to me.
And In the Cli the approach is to use compute attributes, and this is much more make sense to me.

please tell me what do you think, thanks for the review!

theforeman-bot added Needs testing Not yet reviewed labels Apr 15, 2018

shiramax force-pushed the 20538 branch from 3d19cf8 to 511fb2a Compare April 15, 2018 13:47

shiramax changed the title ~~20538~~ WIP #20538 - add compute profile commands Apr 15, 2018

shiramax force-pushed the 20538 branch 2 times, most recently from 3128983 to 2ed61ba Compare April 15, 2018 13:59

shiramax force-pushed the 20538 branch from 2ed61ba to b37b145 Compare April 29, 2018 12:29

shiramax changed the title ~~WIP #20538 - add compute profile commands~~ BZ #20538 - add compute profile commands Apr 29, 2018

ofedoren reviewed Jun 20, 2018

View reviewed changes

shiramax force-pushed the 20538 branch from b37b145 to 0f5227f Compare June 24, 2018 14:08

ofedoren reviewed Jul 4, 2018

View reviewed changes

shiramax force-pushed the 20538 branch from 0f5227f to db8ed11 Compare July 4, 2018 14:34

mbacovsky suggested changes Aug 29, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Aug 29, 2018

shiramax force-pushed the 20538 branch from db8ed11 to 9532f36 Compare September 2, 2018 13:37

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

shiramax force-pushed the 20538 branch 2 times, most recently from fa59974 to 621837c Compare September 6, 2018 06:57

shiramax force-pushed the 20538 branch from 621837c to a48704c Compare September 6, 2018 13:50

mbacovsky reviewed Oct 16, 2018

View reviewed changes

BZ #20538 - add compute profile commands

e141462

shiramax force-pushed the 20538 branch from a48704c to e141462 Compare October 21, 2018 12:29

shiramax closed this Dec 12, 2018


		desc _('Update compute profile interface')

		option '--set-values', 'SET VALUES', _('Interface parameters, should be comma separated list of values.'),

BZ #20538 - add compute profile commands #366

BZ #20538 - add compute profile commands #366

Conversation

shiramax commented Apr 15, 2018

shiramax commented Apr 29, 2018

shiramax commented Jun 14, 2018

ofedoren 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

Choose a reason for hiding this comment

shiramax Jul 4, 2018 • edited Loading

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

shiramax commented Jun 24, 2018

ofedoren commented Jul 4, 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

ofedoren commented Jul 4, 2018 • edited Loading

iNecas commented Jul 31, 2018

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

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 30, 2018

mbacovsky commented Sep 6, 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

Choose a reason for hiding this comment

mbacovsky commented Oct 16, 2018

mbacovsky commented Oct 16, 2018

shiramax commented Oct 21, 2018

shiramax Jul 4, 2018 •

edited

Loading

ofedoren commented Jul 4, 2018 •

edited

Loading