katello icon indicating copy to clipboard operation
katello copied to clipboard

Fixes #35215 - Handle cloned hostgroups in hosts_and_hostgroups_helper

Open jeremylenz opened this issue 1 year ago • 3 comments

Requires https://github.com/theforeman/foreman/pull/10055

What are the changes introduced in this pull request?

When cloning a hostgroup, all existing hostgroup attributes should be pre-filled on the new form. However, some fields were not being pre-populated, including

  • Content source
  • Lifecycle environment
  • Content view

There were two reasons for this.

First, I discovered that when a hostgroup is cloned, its content facet was not also being cloned. Because of this, only the "main" attributes were being copied over, and not the content facet attributes above. That should be addressed for all facets, not just content, so that fix is in https://github.com/theforeman/foreman/pull/10055.

Second, host_and_hostgroups_helper has a few methods to fetch the existing values:

  • fetch_lifecycle_environment
  • fetch_content_view
  • fetch_content_source

These methods were returning nil for the newly-cloned hostgroup. I think this is because they relied on Rails associations which assume all the records are already saved in the database. The problem with cloned hostgroups is that the record is a new record, and is not yet saved in the database. For example, when calling host_or_hostgroup.lifecycle_environment (assuming host_or_hostgroup is a ::Hostgroup), Rails normally looks at its associations:

# app/models/katello/concerns/hostgroup_extensions.rb

has_one :lifecycle_environment, :through => :content_facet

Rails is probably trying to look up the content facet via its ID, so that it can then retrieve the lifecycle environment. But since the content facet is a new record and does not have an ID, it would not be able to do this. I got around this by calling host_or_hostgroup.content_facet.lifecycle_environment directly, which seems to work.

I also made similar changes for content view and content source.

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Clone a hostgroup and verify that content source, lifecycle environment, and content view values are properly cloned Save the cloned hostgroup and verify everything works as expected

jeremylenz avatar Feb 16 '24 20:02 jeremylenz

[test katello]

jeremylenz avatar Feb 28 '24 15:02 jeremylenz

rebased

jeremylenz avatar Feb 28 '24 17:02 jeremylenz

In my testing this is still needed even after the changes to https://github.com/theforeman/foreman/pull/10055.

jeremylenz avatar Mar 05 '24 19:03 jeremylenz