Fixes #6159 - move image_id to be under the compute_attributes #404

shiramax · 2018-12-23T14:08:24Z

No description provided.

mbacovsky · 2019-01-16T00:42:39Z

[test hammer]

mbacovsky

@shiramax thanks for this addition. Works fine but this PR needs rebase and also some tests. I'd also like to discuss my concerns about the helper method in Host command (See inline please)

mbacovsky · 2019-02-27T11:47:20Z

lib/hammer_cli_foreman/host.rb

+      def request_params
+        params = super
+        if option_image_id
+          HammerCLIForeman::Host.set_image_uuid(params,option_image_id)


If we split (Single Responsibility) the set_image_uuid we could move getting the image to ComputeResource which would be more expected place for the code. I also don't think HammerCLIForeman::Host super-command is a good place for storing helper methods for individual subcommands. I'd keep the assignment in the Update/Create commands. If you'd need to make it more DRY it would be better to create a module with the helper method that is included in the subcommands.

compute_resource_id = params['host']['compute_resource_id'] image_uuid = HammerCLIForeman::ComputeResource::Utils.image_uuid(compute_resource_id, options['option_image_id']) params['host']['compute_attributes']['image_id'] = image uuid

we could move getting the image to ComputeResource - which file?

mbacovsky · 2019-04-12T15:02:18Z

@shiramax looks a way better now! I realized we have to move all the code from host.rb to the CommonUpdateOptions module we break the existing request_params otherwise. As a bonus we get more DRY code and set_image_uuid won't be misplaced.

get_image_id should be better named get_image_uuid and while it is not exactly a help util we should rename the module to just utils to cover both methods purpose.

shiramax · 2019-04-14T14:02:40Z

thanks @mbacovsky, please review again

mbacovsky

@shiramax, thanks for moving the logic to proper place. I realized some more problems please see inline.

mbacovsky · 2019-04-15T12:47:57Z

lib/hammer_cli_foreman/hosts/common_update_options.rb

@@ -50,9 +50,15 @@ def self.ask_password
        HammerCLI.interactive_output.ask(prompt) { |q| q.echo = false }
      end

+      def self.set_image_uuid(params, image_id)


A couple of concerns here:

it is not called from outside of the module so there is no reason to make it an instance method - no self.

no reason to make it public (move it to the private section below)

it breaks single responsibility principle - it should just get the image_id, setting the params is out of scope of this method (see how other methods here e.g. subnet_id are used)

it does not work when --compute-resource-id is not given. I'm not sure what is the plan here. I as an user would expect hammer to try to load existing cr-id from the server on update raise an usage error otherwise.

How can I load cr-id from the server? You can have multiple compute resource on the servers

It would be just for a case of an update of an existing host. We could try to load the host and check if the CR is set there. If not we should rise usage/validation error or similar

mbacovsky · 2019-04-15T12:59:33Z

lib/hammer_cli_foreman/hosts/common_update_options.rb

@@ -69,6 +75,7 @@ def request_params
          params['host']['build'] = true if option_build.nil?
          params['host']['managed'] = true if option_managed.nil?
          params['host']['enabled'] = true if option_enabled.nil?
+          HammerCLIForeman::Hosts::CommonUpdateOptions::set_image_uuid(params, options["option_image_id"]) if options["option_image_id"]


When image_uuid is an instance method you don't have to scope it. I'd expect something like:

params['host']['compute_attributes']['image_id'] = image_uuid(params['host']['compute_resource_id'], options["option_image_id"]) if options["option_image_id"]

Please note that you completely rewrite params['host']['compute_attributes'] a couple of lines below. Was this ever tested?

mbacovsky · 2019-04-15T13:08:48Z

lib/hammer_cli_foreman/hosts/common_update_options.rb

@@ -78,6 +85,7 @@ def request_params
        if action == :update
          params['host']['compute_attributes']['volumes_attributes'] = nested_attributes(option_volume_list) unless option_volume_list.empty?
          params['host']['interfaces_attributes'] = interfaces_attributes unless option_interface_list.empty?
+          HammerCLIForeman::Hosts::CommonUpdateOptions::set_image_uuid(params, options["option_image_id"])  if options["option_image_id"]


Not sure if this is the best place but we should load cr_id somewhere. How about?

options["option_image_id"] compute_resource_id = params['host']['compute_resource_id'] || load_compute_resource(options["option_id"]) || <rise some error> params['host']['compute_attributes']['image_id'] = image_uuid(compute_resource_id, options["option_image_id"]) end

shiramax · 2019-05-02T08:15:55Z

thanks @mbacovsky ,
please review again.

mbacovsky

👍 Thanks @shiramax for the updates and all the effort on this. It works well for me now and the code looks clean. 🎆

theforeman-bot added Needs testing Not yet reviewed labels Dec 23, 2018

shiramax force-pushed the 6159 branch from e84a02f to 26577ec Compare January 24, 2019 14:57

mbacovsky suggested changes Feb 27, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 27, 2019

shiramax force-pushed the 6159 branch 2 times, most recently from dd1f86c to 0fec4b4 Compare April 10, 2019 13:38

theforeman-bot added Needs re-review and removed Waiting on contributor labels Apr 10, 2019

shiramax force-pushed the 6159 branch 3 times, most recently from fcf26fc to 6e180c3 Compare April 14, 2019 13:57

mbacovsky suggested changes Apr 15, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Apr 15, 2019

Fixes #6159 - move image_id to be under the compute_attributes

7a0bfa3

shiramax force-pushed the 6159 branch from 6e180c3 to 7a0bfa3 Compare May 2, 2019 08:04

theforeman-bot added Needs re-review and removed Waiting on contributor labels May 2, 2019

mbacovsky approved these changes May 20, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label May 20, 2019

mbacovsky merged commit ca785b3 into theforeman:master May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6159 - move image_id to be under the compute_attributes #404

Fixes #6159 - move image_id to be under the compute_attributes #404

shiramax commented Dec 23, 2018

mbacovsky commented Jan 16, 2019

mbacovsky left a comment

mbacovsky Feb 27, 2019

shiramax Apr 8, 2019

mbacovsky commented Apr 12, 2019

shiramax commented Apr 14, 2019

mbacovsky left a comment

mbacovsky Apr 15, 2019

shiramax Apr 18, 2019

mbacovsky Apr 18, 2019

mbacovsky Apr 15, 2019

mbacovsky Apr 15, 2019

shiramax commented May 2, 2019

mbacovsky left a comment

Fixes #6159 - move image_id to be under the compute_attributes #404

Fixes #6159 - move image_id to be under the compute_attributes #404

Conversation

shiramax commented Dec 23, 2018

mbacovsky commented Jan 16, 2019

mbacovsky left a comment

Choose a reason for hiding this comment

mbacovsky Feb 27, 2019

Choose a reason for hiding this comment

shiramax Apr 8, 2019

Choose a reason for hiding this comment

mbacovsky commented Apr 12, 2019

shiramax commented Apr 14, 2019

mbacovsky left a comment

Choose a reason for hiding this comment

mbacovsky Apr 15, 2019

Choose a reason for hiding this comment

shiramax Apr 18, 2019

Choose a reason for hiding this comment

mbacovsky Apr 18, 2019

Choose a reason for hiding this comment

mbacovsky Apr 15, 2019

Choose a reason for hiding this comment

mbacovsky Apr 15, 2019

Choose a reason for hiding this comment

shiramax commented May 2, 2019

mbacovsky left a comment

Choose a reason for hiding this comment