Refactor #4899
closed
Updating subnet fields to match its output
Description
QE relies on what hammer cli outputs in order to create a python dictionary that can be used within the testing framework for validations and assertions. For most resources, the output for a given field correspond directly to what the model field is named in Foreman. For example, when you run hammer organization info --id=1 then you get a list of fields and values, where the field can be name, label, ID, etc. These fields map a field on Foreman named :name, :label, :id and so on.
The discrepancy happens for subnet with a few fields, namely, :dns_primary, :dns_secondary and :vlanid. When QE's automation runs hammer cli for subnet, instead of getting the values "DNS Primary", "DNS Secondary" and "vlanid", it gets "Primary DNS", "Secondary DNS" and "vlan id", which may not look like such a big deal, but it screws up our mapping when dealing with Forman data as objects.
I'd like thus to propose the following changes:
Modified lib/hammer_cli_foreman/subnet.rb
diff --git a/lib/hammer_cli_foreman/subnet.rb b/lib/hammer_cli_foreman/subnet.rb
index 4fc13d3..74043ec 100644
--- a/lib/hammer_cli_foreman/subnet.rb
+++ b/lib/hammer_cli_foreman/subnet.rb
@@ -23,14 +23,14 @@ module HammerCLIForeman
field :priority, _("Priority")
field :dns_id, _("DNS id")
field :dns, _("DNS"), Fields::Server
- field :dns_primary, _("Primary DNS")
- field :dns_secondary, _("Secondary DNS")
+ field :primary_dns, _("Primary DNS")
+ field :secondary_dns, _("Secondary DNS")
field :domain_ids, _("Domain ids"), Fields::List
field :tftp, _("TFTP"), Fields::Server
field :tftp_id, _("TFTP id")
field :dhcp, _("DHCP"), Fields::Server
field :dhcp_id, _("DHCP id")
- field :vlanid, _("vlan id")
+ field :vlan_id, _("vlan id")
field :gateway, _("Gateway")
field :from, _("From")
field :to, _("To")
I'm also willing to provide a patch for Foreman if it makes sense to also update the fields in there:
diff --git a/app/controllers/api/v1/subnets_controller.rb b/app/controllers/api/v1/subnets_controller.rb
index 5ef0dcc..2b618ea 100644
--- a/app/controllers/api/v1/subnets_controller.rb
+++ b/app/controllers/api/v1/subnets_controller.rb
@@ -29,11 +29,11 @@ module Api
param :network, String, :desc => 'Subnet network', :required => true
param :mask, String, :desc => 'Netmask for this subnet', :required => true
param :gateway, String, :desc => 'Primary DNS for this subnet'
- param :dns_primary, String, :desc => 'Primary DNS for this subnet'
- param :dns_secondary, String, :desc => 'Secondary DNS for this subnet'
+ param :primary_dns, String, :desc => 'Primary DNS for this subnet'
+ param :secondary_dns, String, :desc => 'Secondary DNS for this subnet'
param :from, String, :desc => 'Starting IP Address for IP auto suggestion'
param :to, String, :desc => 'Ending IP Address for IP auto suggestion'
- param :vlanid, String, :desc => 'VLAN ID for this subnet'
+ param :vlan_id, String, :desc => 'VLAN ID for this subnet'
param :domain_ids, Array, :desc => 'Domains in which this subnet is part'
param :dhcp_id, :number, :desc => 'DHCP Proxy to use within this subnet'
param :tftp_id, :number, :desc => 'TFTP Proxy to use within this subnet'
@@ -52,11 +52,11 @@ module Api
param :network, String, :desc => 'Subnet network'
param :mask, String, :desc => 'Netmask for this subnet'
param :gateway, String, :allow_nil => true, :desc => 'Primary DNS for this subnet'
- param :dns_primary, String, :allow_nil => true, :desc => 'Primary DNS for this subnet'
- param :dns_secondary, String, :allow_nil => true, :desc => 'Secondary DNS for this subnet'
+ param :primary_dns, String, :allow_nil => true, :desc => 'Primary DNS for this subnet'
+ param :secondary_dns, String, :allow_nil => true, :desc => 'Secondary DNS for this subnet'
param :from, String, :allow_nil => true, :desc => 'Starting IP Address for IP auto suggestion'
param :to, String, :allow_nil => true, :desc => ' Ending IP Address for IP auto suggestion'
- param :vlanid, String, :allow_nil => true, :desc => 'VLAN ID for this subnet'
+ param :vlan_id, String, :allow_nil => true, :desc => 'VLAN ID for this subnet'
param :domain_ids, Array, :allow_nil => true, :desc => 'Domains in which this subnet is part'
param :dhcp_id, :number, :allow_nil => true, :desc => 'DHCP Proxy to use within this subnet'
param :tftp_id, :number, :allow_nil => true, :desc => 'TFTP Proxy to use within this subnet'
diff --git a/app/controllers/api/v2/subnets_controller.rb b/app/controllers/api/v2/subnets_controller.rb
index cdababf..fcf4822 100644
--- a/app/controllers/api/v2/subnets_controller.rb
+++ b/app/controllers/api/v2/subnets_controller.rb
@@ -32,11 +32,11 @@ module Api
param :network, String, :desc => 'Subnet network', :required => true
param :mask, String, :desc => 'Netmask for this subnet', :required => true
param :gateway, String, :desc => 'Primary DNS for this subnet'
- param :dns_primary, String, :desc => 'Primary DNS for this subnet'
- param :dns_secondary, String, :desc => 'Secondary DNS for this subnet'
+ param :primary_dns, String, :desc => 'Primary DNS for this subnet'
+ param :secondary_dns, String, :desc => 'Secondary DNS for this subnet'
param :from, String, :desc => 'Starting IP Address for IP auto suggestion'
param :to, String, :desc => 'Ending IP Address for IP auto suggestion'
- param :vlanid, String, :desc => 'VLAN ID for this subnet'
+ param :vlan_id, String, :desc => 'VLAN ID for this subnet'
param :domain_ids, Array, :desc => 'Domains in which this subnet is part'
param :dhcp_id, :number, :desc => 'DHCP Proxy to use within this subnet'
param :tftp_id, :number, :desc => 'TFTP Proxy to use within this subnet'
diff --git a/app/views/subnets/_fields.html.erb b/app/views/subnets/_fields.html.erb
index aa95922..3863930 100644
--- a/app/views/subnets/_fields.html.erb
+++ b/app/views/subnets/_fields.html.erb
@@ -2,8 +2,8 @@
<%= text_f f, :network %>
<%= text_f f, :mask, :help_inline => _("Netmask for this subnet") %>
<%= text_f f, :gateway, :help_inline => _("Optional: Gateway for this subnet") %>
-<%= text_f f, :dns_primary, :help_inline => _("Optional: Primary DNS for this subnet") %>
-<%= text_f f, :dns_secondary, :help_inline => _("Optional: Secondary DNS for this subnet") %>
+<%= text_f f, :primary_dns, :help_inline => _("Optional: Primary DNS for this subnet") %>
+<%= text_f f, :secondary_dns, :help_inline => _("Optional: Secondary DNS for this subnet") %>
<%= text_f f, :from, :help_inline => _("Optional: Starting IP Address for IP auto suggestion") %>
<%= text_f f, :to, :help_inline => _("Optional: Ending IP Address for IP auto suggestion") %>
-<%= text_f f, :vlanid, :help_inline => _("Optional: VLAN ID for this subnet") %>
+<%= text_f f, :vlan_id, :help_inline => _("Optional: VLAN ID for this subnet") %>
Please advise.
Updated by Martin Bacovsky about 10 years ago
- Category changed from Hammer core to Foreman commands (obsolete)
- Status changed from New to Need more information
- Assignee set to Martin Bacovsky
Thanks for the report. The suggested fix on the API level would be more clean solution, but there would need to be addad matching fix to the API controllers (not just the apipie descriptions) and possibly even some renaming on model level, as these are tightly connected. It seems unlikely to be accepted. But we can change the Issue Component to Foreman API if you wish give it a try, Hammer will adapt.
Maybe there is a way to fix that just on Hammer level, could you describe what is the problem from your testsuite perspective? Where the names colide?
Updated by Anonymous
Updated by Tomáš Strachota over 6 years ago
- Status changed from Need more information to New
- Priority changed from Normal to Low
I don't think this is an issue on the API level. Input parameters are the same as fields returned from the API so everything is correct there.
In the CLI we give the attributes labels with human readable word order like "Primary DNS". This should probably be reflected also in the option names.
All in all I think this is a minor issue and if we want to fix it, we should add deprecation to the original options and replace it with new ones.
I'm also fine with closing this issue as "Wontfix".
Updated by Tomáš Strachota almost 6 years ago
- Category changed from Foreman commands (obsolete) to Networking
Updated by Shira Maximov about 4 years ago
- Status changed from New to Rejected
Closing this base on the comments above.