Refs #35438 - Enhance VMware listing commands and switch to cluster_name param #604

chris1984 · 2022-08-25T19:46:42Z

Requires theforeman/foreman#9383

chris1984 · 2022-08-25T19:49:08Z

@ofedoren can this get a review when you get a chance please?

ofedoren

Thanks, @chris1984! Just a few small nitpicks inline.

Also, I've read initial issue and it seems like in hammer we still have a problem with cluster_id:

And yes, I was testing this on the latest Foreman's develop so your changes are present.

lib/hammer_cli_foreman/compute_resource.rb

lib/hammer_cli_foreman/compute_resource/vmware.rb

…ame param

chris1984 · 2022-08-30T14:40:38Z

@ofedoren updated.

Changed cluster_id to cluster_name in the help text, but having that be used as the cluster_id since looking into the fog-vsphere code we don't use the raw id for anything. I didn't want to take away the ability to see id's. so to prevent confusion and users using the ID instead of the name, I changed the attribute to name but behind the scenes we use that as the ID to they pass in the name. Also did the gsub things we talked about so a user does not have to do %2F

** Hammer commands with normal cluster structure, with new cluster_name attribute:

*** Networks

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource networks --id 1 --cluster-name "Satellite-Engineering"
----------------|-------------------------------|----------------|------------------|--------
ID              | NAME                          | DATACENTER     | VIRTUAL SWITCH   | VLAN ID
----------------|-------------------------------|----------------|------------------|--------
dvportgroup-572 | ABJ-Network                   | RH_Engineering | Satellite-Switch | 209    
dvportgroup-56  | Host-Network                  | RH_Engineering | Satellite-Switch | 0  
----------------|-------------------------------|----------------|------------------|--------

*** Resource pools

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource resource-pools --id 1 --cluster-name "Satellite-Engineering"
-----------|-----------|-----------------------|---------------
ID         | NAME      | CLUSTER               | DATACENTER    
-----------|-----------|-----------------------|---------------
resgroup-8 | Resources | Satellite-Engineering | RH_Engineering
-----------|-----------|-----------------------|---------------

*** Storage pods

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource storage-pods --id 1 --cluster-name "Satellite-Engineering"
------------|-------------|---------------
ID          | NAME        | DATACENTER    
------------|-------------|---------------
group-p6123 | NFS-Cluster | RH_Engineering
------------|-------------|---------------

*** Storage Domains

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource storage-domains --id 1 --cluster-name "Satellite-Engineering"
----------------|----------------
ID              | NAME           
----------------|----------------
datastore-17566 | ISOS           
datastore-34    | Local-ESXI23   
datastore-3518  | Local-ESXi3
----------------|----------------

Hammer commands with nested cluster structure, with new cluster_name attribute:

*** Networks

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource networks --id 2 --cluster-name "Test-Folder/Test-Cluster"
--------------|------------|-------------|----------------|--------
ID            | NAME       | DATACENTER  | VIRTUAL SWITCH | VLAN ID
--------------|------------|-------------|----------------|--------
network-40902 | VM Network | Toledo-Test |                |        
--------------|------------|-------------|----------------|--------

*** Resource pools

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource resource-pools --id 2 --cluster-name "Test-Folder/Test-Cluster"
---------------|-----------|--------------------------|------------
ID             | NAME      | CLUSTER                  | DATACENTER 
---------------|-----------|--------------------------|------------
resgroup-40407 | Resources | Test-Folder/Test-Cluster | Toledo-Test
---------------|-----------|--------------------------|------------

*** Storage pods

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource storage-pods --id 2 --cluster-name "Test-Folder/Test-Cluster"
---|------|-----------
ID | NAME | DATACENTER
---|------|-----------

*** Storage Domains

[vagrant@centos7-hammer-devel hammer-cli-foreman]$ hammer compute-resource storage-domains --id 2 --cluster-name "Test-Folder/Test-Cluster"
----------------|-----------
ID              | NAME      
----------------|-----------
datastore-40901 | datastore1
----------------|-----------

chris1984 · 2022-08-30T14:43:32Z

If this looks good to you I will update the tests that failed

ofedoren · 2022-08-30T15:52:55Z

Although I'm OK with renaming the option, we would need to deprecate it first instead of removal.

…ame param

chris1984 · 2022-08-30T16:47:38Z

@ofedoren how does this look?

ofedoren

Thanks, @chris1984! It works now. Some code-related suggestions which also should fix almost all of the tests.

And can we have at least one more test to cover this specific case?

ofedoren · 2022-08-31T17:46:07Z

lib/hammer_cli_foreman/compute_resource.rb

+
+

Suggested change

ofedoren · 2022-08-31T17:46:42Z

lib/hammer_cli_foreman/compute_resource.rb

+      option '--cluster-name', "Name of cluster", _("Cluster name to search by"),
+        attribute_name: :option_cluster_id


Suggested change

option '--cluster-name', "Name of cluster", _("Cluster name to search by"),

attribute_name: :option_cluster_id

option '--cluster-name', 'NAME', _('Cluster name to search by'),

attribute_name: :option_cluster_id

ofedoren · 2022-08-31T18:04:23Z

lib/hammer_cli_foreman/compute_resource.rb

+      option '--cluster-name', "Name of cluster", _("Cluster name to search by"),
+        attribute_name: :option_cluster_id
+
+      option '--cluster-id', "ID of cluster", _("Deprecated, use cluster-name")


Suggested change

option '--cluster-id', "ID of cluster", _("Deprecated, use cluster-name")

option '--cluster-id', 'ID', _(Cluster ID'),

deprecated: _('Use --cluster-name instead')

ofedoren · 2022-08-31T18:07:05Z

lib/hammer_cli_foreman/compute_resource.rb

+      option '--cluster-name', "Name of cluster", _("Cluster name to search by"),
+        attribute_name: :option_cluster_id
+
+      option '--cluster-id', "ID of cluster", _("Deprecated, use cluster-name")


Those changes are quite repetitive. Let's extract them into a command extension:

module HammerCLIForeman module CommandExtensions class ComputeResourceSubcommand < HammerCLI::CommandExtensions option '--cluster-id', 'ID', _('Cluster ID'), deprecated: _('Use --cluster-name instead') option '--cluster-name', 'NAME', _('Cluster name or path to search by'), attribute_name: :option_cluster_id request_params do |params| params['cluster_id'] = params['cluster_id'].gsub('/', '%2F') if params['cluster_id'] end end end end

And then we can update the related commands like:

build_options without: :cluster_id extend_with( HammerCLIForeman::CommandExtensions::ComputeResourceSubcommand.new(only: %i[option request_params]) )

…ame param

chris1984 · 2022-09-06T16:02:04Z

@ofedoren I updated with your suggestions. When trying to run the test suite locally I get:

/home/vagrant/hammer-cli-foreman/lib/hammer_cli_foreman/compute_resource.rb:153:in `<class:AvailableNetworksCommand>': uninitialized constant HammerCLIForeman::CommandExtensions::ComputeResourceSubcommand (NameError)

ofedoren

Thanks, @chris1984, you need to also register the extension in https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/command_extensions.rb

ofedoren · 2022-09-06T16:27:00Z

lib/hammer_cli_foreman/compute_resource.rb

+      def request_params
+        params = super
+        params['cluster_id'] = params['cluster_id'].gsub('/', '%2F')
+        params
+      end


This is no longer needed since it's present in the extension.

ofedoren · 2022-09-06T16:27:14Z

lib/hammer_cli_foreman/compute_resource.rb

+      def request_params
+        params = super
+        params['cluster_id'] = params['cluster_id'].gsub('/', '%2F')
+        params
+      end


Same as above

ofedoren · 2022-09-06T16:27:22Z

lib/hammer_cli_foreman/compute_resource.rb

+      def request_params
+        params = super
+        params['cluster_id'] = params['cluster_id'].gsub('/', '%2F')
+        params
+      end


Same as above

ofedoren · 2022-09-06T16:27:31Z

lib/hammer_cli_foreman/compute_resource.rb

+      def request_params
+        params = super
+        params['cluster_id'] = params['cluster_id'].gsub('/', '%2F')
+        params
+      end


Same as above

…ame param

chris1984 · 2022-09-07T18:02:01Z

@ofedoren how does this look? I kept it in a separate commit so you can review it easier. If it all looks good I can squash it or I think you can squash them all too.

ofedoren

Thanks, @chris1984, here is 🍪 for you :)

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Aug 25, 2022

chris1984 mentioned this pull request Aug 25, 2022

Fixes #35438 - Unescape cluster on compute resource controller theforeman/foreman#9383

Merged

chris1984 force-pushed the vmware branch from d91d904 to b8a7741 Compare August 26, 2022 18:34

theforeman-bot removed the Waiting on contributor label Aug 26, 2022

ofedoren reviewed Aug 29, 2022

View reviewed changes

theforeman-bot added the Waiting on contributor label Aug 29, 2022

Refs #35438 - Enhance VMware listing commands and switch to cluster_n…

3a7c5e4

…ame param

chris1984 changed the title ~~Refs #35438 - Enhance VMware listing commands to include more info~~ Refs #35438 - Enhance VMware listing commands and switch to cluster_name param Aug 30, 2022

chris1984 force-pushed the vmware branch from b8a7741 to 3a7c5e4 Compare August 30, 2022 14:40

theforeman-bot removed the Waiting on contributor label Aug 30, 2022

Refs #35438 - Enhance VMware listing commands and switch to cluster_n…

41dfc8c

…ame param

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 30, 2022

ofedoren requested changes Aug 31, 2022

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Aug 31, 2022

Refs #35438 - Enhance VMware listing commands and switch to cluster_n…

345738e

…ame param

theforeman-bot added Needs re-review and removed Waiting on contributor labels Sep 6, 2022

ofedoren reviewed Sep 6, 2022

View reviewed changes

theforeman-bot added the Waiting on contributor label Sep 6, 2022

Refs #35438 - Enhance VMware listing commands and switch to cluster_n…

22b01d1

…ame param

theforeman-bot removed the Waiting on contributor label Sep 7, 2022

ofedoren approved these changes Sep 8, 2022

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Sep 8, 2022

ofedoren merged commit 8a4ebc2 into theforeman:master Sep 8, 2022

Gauravtalreja1 mentioned this pull request Dec 1, 2022

Update hammer-cli options test for 6.12 SatelliteQE/robottelo#10375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #35438 - Enhance VMware listing commands and switch to cluster_name param #604

Refs #35438 - Enhance VMware listing commands and switch to cluster_name param #604

chris1984 commented Aug 25, 2022

chris1984 commented Aug 25, 2022

ofedoren left a comment

chris1984 commented Aug 30, 2022 •

edited

chris1984 commented Aug 30, 2022

ofedoren commented Aug 30, 2022

chris1984 commented Aug 30, 2022

ofedoren left a comment

ofedoren Aug 31, 2022

ofedoren Aug 31, 2022

ofedoren Aug 31, 2022

ofedoren Aug 31, 2022

chris1984 commented Sep 6, 2022

ofedoren left a comment

ofedoren Sep 6, 2022

ofedoren Sep 6, 2022

ofedoren Sep 6, 2022

ofedoren Sep 6, 2022

chris1984 commented Sep 7, 2022

ofedoren left a comment

		option '--cluster-name', "Name of cluster", _("Cluster name to search by"),
		attribute_name: :option_cluster_id

	option '--cluster-id', "ID of cluster", _("Deprecated, use cluster-name")
	option '--cluster-id', 'ID', _(Cluster ID'),
	deprecated: _('Use --cluster-name instead')

Refs #35438 - Enhance VMware listing commands and switch to cluster_name param #604

Refs #35438 - Enhance VMware listing commands and switch to cluster_name param #604

Conversation

chris1984 commented Aug 25, 2022

chris1984 commented Aug 25, 2022

ofedoren left a comment

Choose a reason for hiding this comment

chris1984 commented Aug 30, 2022 • edited

chris1984 commented Aug 30, 2022

ofedoren commented Aug 30, 2022

chris1984 commented Aug 30, 2022

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

chris1984 commented Sep 6, 2022

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

chris1984 commented Sep 7, 2022

ofedoren left a comment

Choose a reason for hiding this comment

chris1984 commented Aug 30, 2022 •

edited