Fixes #36897 - Hosts now follow existing facet standards for hostgroup inheritance

[qcjames53]

What are the changes introduced in this pull request?

This was a rather simple bug which exposed some not so simple issues in Katello's handing of host group -> host inheritance. More details on this process in the consideration section...

Hosts will now always inherit the lifecycle environment and the content view from their host group on creation and update. This matches the behavior of other attributes in Foreman. In order to prevent the LE and CV from 'resetting' on an update, the user will now have to manually specify the LE and CV in the properties of the host.

The 'Edit host' form now contains hidden fields to prevent the LE and CV from 'resetting' on an update. These fields duplicate the disabled but still visible dropdowns menus on host edit, allowing the LE and CV info to still be sent in the PATCH request.

Considerations taken when implementing this change?

This bug involved quite a large amount of archeology. Big thanks to @jeremylenz and @ShimShtein for their help with this.

Host update in Katello needs to be handled in a roundabout fashion due to some of the ways ActiveRecord stores the LE and CV info. These fields need to be given to the controller as a host.content_facet.lifecycle_environment_id property, but are stored in ActiveRecord differently to account for multi CV support in the future. A consequence of this was that LE and CV ids needed to be cleared from the attributes of the ActiveRecord request before save, or else the model would throw errors.

Katello was implementing this parameter deletion twice, once before and once during host update. The original bug of 'resetting' CV and LEs stemmed from the implementation of facet inheritance on the Foreman side: any property not explicitly set on an update or create is allowed to be overriden by the host group's property. When we were deleting the CV and LE params before the host update, facet inheritance was automatically overriding these with the host group's CV and LE. By keeping these params until midway through the host update, we can appease both the facet inheritance and the ActiveRecord sides of this issue.

What are the testing steps for this pull request?

1. Create two Lifecycle Environments. Create two host groups and promote each to one LE.
2. Create a Host Group using the first LE.
3. Create a host, assigned to this host group. On creation, ensure that it inherits the LE and CV from the hostgroup.
4. On the host details page, click the menu next to 'Content View Details' and select 'Edit content view assignment'. Select the other LE and CV. Ensure that this updates successfully.
5. On the host details page, click the 'Edit' button. The LE and CV dropdowns should be disabled and look normal.
6. Change any field here (I used the comments box). Press 'Submit'. Ensure that the changed field updated successfully and that the LE and CV are still set to the second LE and CV from earlier. They should not have inherited from the hostgroup.
7. Check the logs for any 'activerecord' or 'nomethod' errors. It should be clean. (We are checking this because some methods I deleted involved PRs with these errors on update)
8. Consider testing some other cases:
   Hosts that inherit LE from one hostgroup and CV from another
   Hosts that only inherit LE from a hostgroup
   Creating / updating hosts using hammer and the API

Update 2024-01-11: Reworded changes introduced description.
Update 2024-01-19: Reworked PR to integrate existing facet inheritance scheme in Foreman.

[ekohl]

Not a proper review, just some coding style things I noticed.

[qcjames53]

Just made some big changes after a few meetings with the team. Will update PR description later today.

[qcjames53]

@parthaa This should be good for review. Jeremy suggested you could get a head start while I work on the test cases.

[ShimShtein]

Looks good as far as facets/hostgroup relations go.

[jeremylenz]

I wonder if this also addresses https://projects.theforeman.org/issues/35215, since we're now ensuring that the host form sends existing values for CV/LCE.

[parthaa]

Changes here are good. Couple of issues I found

1. Create a Hostgroup with Library/ Default CV
2. Create a child hostgroup with Dev/Different CV
3. Create a child of the the child host group
4. Click on the drop down for LCE/CV .
5. Notice it says "Inherit Library"
6. Create a host and choose the child hostgroup
7. Notice the lce and cv it chooses are from parent.

Recommended Patch

diff --git a/app/models/katello/concerns/hostgroup_extensions.rb b/app/models/katello/concerns/hostgroup_extensions.rb
index e08b8d07ae..9966e7c8a7 100644
--- a/app/models/katello/concerns/hostgroup_extensions.rb
+++ b/app/models/katello/concerns/hostgroup_extensions.rb
@@ -122,7 +122,7 @@ module Katello
           # it will return the value 2.
           facet_model = Facets.registered_facets[facet].hostgroup_configuration.model
           value = facet_model.where.not(attribute => nil).joins(:hostgroup).merge(
-            ::Hostgroup.where(id: self.ancestor_ids).reorder(ancestry: :desc)
+            ::Hostgroup.where(id: self.ancestor_ids).reorder("#{::Hostgroup.table_name}.ancestry desc nulls last")
           ).pick(attribute)
         end
         value

[qcjames53]

Changes here are good. Couple of issues I found

1. Create a Hostgroup with Library/ Default CV
2. Create a child hostgroup with Dev/Different CV
3. Create a child of the the child host group
4. Click on the drop down for LCE/CV .
5. Notice it says "Inherit Library"
6. Create a host and choose the child hostgroup
7. Notice the lce and cv it chooses are from parent.

Recommended Patch

diff --git a/app/models/katello/concerns/hostgroup_extensions.rb b/app/models/katello/concerns/hostgroup_extensions.rb
index e08b8d07ae..9966e7c8a7 100644
--- a/app/models/katello/concerns/hostgroup_extensions.rb
+++ b/app/models/katello/concerns/hostgroup_extensions.rb
@@ -122,7 +122,7 @@ module Katello
           # it will return the value 2.
           facet_model = Facets.registered_facets[facet].hostgroup_configuration.model
           value = facet_model.where.not(attribute => nil).joins(:hostgroup).merge(
-            ::Hostgroup.where(id: self.ancestor_ids).reorder(ancestry: :desc)
+            ::Hostgroup.where(id: self.ancestor_ids).reorder("#{::Hostgroup.table_name}.ancestry desc nulls last")
           ).pick(attribute)
         end
         value

Thanks for the fix @parthaa; I just pushed it through. I finally had some time to repair the broken test cases so we should be go to go here provided testing passes.

[jeremylenz]

C: Layout/TrailingWhitespace: Trailing whitespace detected.

🙈

[parthaa]

I tested inheritance of host groups (4 levels) and they worked as desired,
Nice work @qcjames53 and @jeremylenz !
Thank you @ShimShtein for your guidance :)

APJ