Project

General

Profile

Refactor #4899

Updating subnet fields to match its output

Added by Og Maciel almost 8 years ago. Updated about 2 years ago.

Status:
Rejected
Priority:
Low
Assignee:
-
Category:
Networking
Target version:
-
Difficulty:
Triaged:
No
Bugzilla link:
Pull request:
Team Backlog:
Fixed in Releases:
Found in Releases:
In Kanboard:
yes

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.

History

#1 Updated by Martin Bacovsky almost 8 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?

#2 Updated by Anonymous over 4 years ago

what's the status here?

#3 Updated by Tomáš Strachota over 4 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".

#4 Updated by Tomáš Strachota over 3 years ago

  • Category changed from Foreman commands (obsolete) to Networking

#5 Updated by Shira Maximov about 2 years ago

  • Assignee deleted (Martin Bacovsky)

#6 Updated by Shira Maximov about 2 years ago

  • In Kanboard set to yes

Changing the

#7 Updated by Shira Maximov about 2 years ago

  • Status changed from New to Rejected

Closing this base on the comments above.

Also available in: Atom PDF