Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #28769 - add more loader macros #7364

Merged
merged 1 commit into from Mar 8, 2020
Merged

Conversation

ares
Copy link
Member

@ares ares commented Jan 15, 2020

There are multiple objects that has Jail defined but can't be loaded in
a sane way. This patch adds load_* macros for all such objects. It also
extracts loaders from base macros to separate file. Load macros are
defined dynamically to avoid repetition of definition and to unify
capabilities of all such macros.

[ :load_host_groups, Hostgroup, :view_hostgroups ],
[ :load_domains, Domain, :view_domains ],
[ :load_realms, Realm, :view_realms ],
[ :load_users, User, :view_users ]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

[ :load_hosts, Host, :view_hosts ],
[ :load_operaitng_systems, Operatingsystem, :view_operatingsystems ],
[ :load_subnets, Subnet, :view_subnets ],
[ :load_smart_proxies, SmartProxy , :view_smart_proxies ],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceBeforeComma: Space found before comma.

@theforeman-bot
Copy link
Member

Issues: #28769

@ares
Copy link
Member Author

ares commented Jan 15, 2020

There are tests for load_users and load_hosts already, that should cover the dynamic definition pretty well. I'm also adding Jails for orgs and locs, which was the primary motivation for adding new loaders. I could extract that to a separate commit, but I think it's related and this avoids unnecessary rebasing.

@tbrisker
Copy link
Member

looks like katello test failures are related.

@ares
Copy link
Member Author

ares commented Jan 16, 2020

Katello/katello#8521 should help to make Katello green again, thanks @timogoebel I squashed your fix

@ares
Copy link
Member Author

ares commented Jan 21, 2020

[test katello], the linked PR was merged

@ares
Copy link
Member Author

ares commented Jan 31, 2020

@lzap any chance we could get this in before cfgmgmt camp next week? :-) it's pretty straightforward I think

lzap
lzap previously approved these changes Mar 6, 2020
Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine, please rebase to 714429384780ceae0c7ee9769f00867bc0047919 and THEN apply the following patch:

diff --git a/app/models/domain.rb b/app/models/domain.rb
index 4b573ff33..b89d53a23 100644
--- a/app/models/domain.rb
+++ b/app/models/domain.rb
@@ -48,7 +48,7 @@ class Domain < ApplicationRecord
   }
 
   class Jail < Safemode::Jail
-    allow :name, :fullname
+    allow :id, :name, :fullname
   end
 
   # return the primary name server for our domain based on DNS lookup
diff --git a/app/models/hostgroup.rb b/app/models/hostgroup.rb
index 6700093a0..d0b0f7f7c 100644
--- a/app/models/hostgroup.rb
+++ b/app/models/hostgroup.rb
@@ -104,7 +104,7 @@ class Hostgroup < ApplicationRecord
   }
 
   class Jail < Safemode::Jail
-    allow :name, :diskLayout, :puppetmaster, :operatingsystem, :architecture,
+    allow :id, :name, :diskLayout, :puppetmaster, :operatingsystem, :architecture,
       :environment, :ptable, :url_for_boot, :params, :puppetproxy,
       :puppet_ca_server, :indent, :os, :arch, :domain, :subnet,
       :subnet6, :realm, :root_pass, :description, :pxe_loader, :title
diff --git a/app/models/nic/base.rb b/app/models/nic/base.rb
index 38a9ee419..4894cca64 100644
--- a/app/models/nic/base.rb
+++ b/app/models/nic/base.rb
@@ -78,7 +78,7 @@ module Nic
     serialize :compute_attributes, Hash
 
     class Jail < ::Safemode::Jail
-      allow :managed?, :subnet, :subnet6, :virtual?, :physical?, :mac, :ip, :ip6, :identifier, :attached_to,
+      allow :id, :managed?, :subnet, :subnet6, :virtual?, :physical?, :mac, :ip, :ip6, :identifier, :attached_to,
         :link, :tag, :domain, :vlanid, :mtu, :bond_options, :attached_devices, :mode,
         :attached_devices_identifiers, :primary, :provision, :alias?, :inheriting_mac,
         :children_mac_addresses, :nic_delay, :fqdn, :shortname
diff --git a/app/models/operatingsystem.rb b/app/models/operatingsystem.rb
index 69447bc6f..ff306dd7c 100644
--- a/app/models/operatingsystem.rb
+++ b/app/models/operatingsystem.rb
@@ -76,7 +76,7 @@ class Operatingsystem < ApplicationRecord
   graphql_type '::Types::Operatingsystem'
 
   class Jail < Safemode::Jail
-    allow :name, :media_url, :major, :minor, :family, :to_s, :==, :release, :release_name, :kernel, :initrd, :pxe_type, :boot_files_uri, :password_hash, :mediumpath
+    allow :id, :name, :media_url, :major, :minor, :family, :to_s, :==, :release, :release_name, :kernel, :initrd, :pxe_type, :boot_files_uri, :password_hash, :mediumpath
   end
 
   def self.title_name
diff --git a/app/models/realm.rb b/app/models/realm.rb
index e443eb25f..714fa9233 100644
--- a/app/models/realm.rb
+++ b/app/models/realm.rb
@@ -35,6 +35,6 @@ class Realm < ApplicationRecord
   }
 
   class Jail < ::Safemode::Jail
-    allow :name, :realm_type
+    allow :id, :name, :realm_type
   end
 end
diff --git a/app/models/smart_proxy.rb b/app/models/smart_proxy.rb
index 3ebbd7266..9aef6c768 100644
--- a/app/models/smart_proxy.rb
+++ b/app/models/smart_proxy.rb
@@ -172,6 +172,6 @@ class SmartProxy < ApplicationRecord
   end
 
   class Jail < ::Safemode::Jail
-    allow :name
+    allow :id, :name
   end
 end
diff --git a/app/models/ssh_key.rb b/app/models/ssh_key.rb
index 9010ca4eb..3b35a908b 100644
--- a/app/models/ssh_key.rb
+++ b/app/models/ssh_key.rb
@@ -35,7 +35,7 @@ class SshKey < ApplicationRecord
   delegate :login, to: :user, prefix: true
 
   class Jail < ::Safemode::Jail
-    allow :name, :user, :key, :to_export, :fingerprint, :length, :ssh_key, :type, :comment
+    allow :id, :name, :user, :key, :to_export, :fingerprint, :length, :ssh_key, :type, :comment
   end
 
   def to_export
diff --git a/app/models/subnet.rb b/app/models/subnet.rb
index a98c9e720..613d0ce73 100644
--- a/app/models/subnet.rb
+++ b/app/models/subnet.rb
@@ -107,7 +107,7 @@ class Subnet < ApplicationRecord
   delegate :supports_ipam_mode?, :supported_ipam_modes, :show_mask?, to: 'self.class'
 
   class Jail < ::Safemode::Jail
-    allow :name, :network, :mask, :cidr, :title, :to_label, :gateway, :dns_primary, :dns_secondary, :dns_servers,
+    allow :id, :name, :network, :mask, :cidr, :title, :to_label, :gateway, :dns_primary, :dns_secondary, :dns_servers,
       :vlanid, :mtu, :nic_delay, :boot_mode, :dhcp?, :nil?, :has_vlanid?, :dhcp_boot_mode?, :description, :present?
   end
 
diff --git a/app/models/template.rb b/app/models/template.rb
index 9f35a0d88..06170bf35 100644
--- a/app/models/template.rb
+++ b/app/models/template.rb
@@ -26,7 +26,7 @@ class Template < ApplicationRecord
   attr_exportable :name, :description, :snippet, :template_inputs, :model => ->(template) { template.class.to_s }
 
   class Jail < Safemode::Jail
-    allow :name
+    allow :id, :name
   end
 
   def skip_strip_attrs
diff --git a/app/models/token.rb b/app/models/token.rb
index 2cbfe4c45..c0e092b36 100644
--- a/app/models/token.rb
+++ b/app/models/token.rb
@@ -5,7 +5,7 @@ class Token < ApplicationRecord
   validates :value, :host_id, :presence => true
 
   class Jail < ::Safemode::Jail
-    allow :host, :value, :expires, :nil?, :present?
+    allow :id, :host, :value, :expires, :nil?, :present?
   end
 
   def to_s
diff --git a/app/models/user.rb b/app/models/user.rb
index 24290b0eb..d8a8c82b9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -143,7 +143,7 @@ class User < ApplicationRecord
   end
 
   class Jail < ::Safemode::Jail
-    allow :login, :ssh_keys, :ssh_authorized_keys, :description, :firstname, :lastname, :mail, :last_login_on
+    allow :id, :login, :ssh_keys, :ssh_authorized_keys, :description, :firstname, :lastname, :mail, :last_login_on
   end
 
   # we need to allow self-editing and self-updating
diff --git a/app/models/usergroup.rb b/app/models/usergroup.rb
index 7d1f6af5c..82fc47cdb 100644
--- a/app/models/usergroup.rb
+++ b/app/models/usergroup.rb
@@ -43,7 +43,7 @@ class Usergroup < ApplicationRecord
   accepts_nested_attributes_for :external_usergroups, :reject_if => ->(a) { a[:name].blank? }, :allow_destroy => true
 
   class Jail < ::Safemode::Jail
-    allow :ssh_keys, :all_users, :ssh_authorized_keys
+    allow :id, :ssh_keys, :all_users, :ssh_authorized_keys
   end
 
   # This methods retrieves all user addresses in a usergroup

Report I tested this with:

<%#
name: Database Test IDs
description: Testing report
snippet: false
model: ReportTemplate
-%>
<%-
  load_organizations.each_record { |rec| report_row({'ID': rec.id}) }
  load_locations.each_record { |rec| report_row({'ID': rec.id}) }
  load_hosts.each_record { |rec| report_row({'ID': rec.id}) }
  load_operating_systems.each_record { |rec| report_row({'ID': rec.id}) }
  load_subnets.each_record { |rec| report_row({'ID': rec.id}) }
  load_smart_proxies.each_record { |rec| report_row({'ID': rec.id}) }
  load_user_groups.each_record { |rec| report_row({'ID': rec.id}) }
  load_host_groups.each_record { |rec| report_row({'ID': rec.id}) }
  load_domains.each_record { |rec| report_row({'ID': rec.id}) }
  load_realms.each_record { |rec| report_row({'ID': rec.id}) }
  load_users.each_record { |rec| report_row({'ID': rec.id}) }
-%>
<%= report_render -%>

There are multiple objects that has Jail defined but can't be loaded in
a sane way. This patch adds load_* macros for all such objects. It also
extracts loaders from base macros to separate file. Load macros are
defined dynamically to avoid repetition of definition and to unify
capabilities of all such macros.
@ares
Copy link
Member Author

ares commented Mar 6, 2020

good call, added and squashed

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTM

@tbrisker tbrisker merged commit 4e1a67a into theforeman:develop Mar 8, 2020
@tbrisker
Copy link
Member

tbrisker commented Mar 8, 2020

Thanks @ares ! pushed the green button for you @lzap 😉

@lzap
Copy link
Member

lzap commented Mar 11, 2020

Thanks, I had the tab opened but... Looks like "RTM" works, keep doing that! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants