Fixes #37144 - Use OS family for host details page packages tab

[nadjaheitmann]

What are the changes introduced in this pull request?

At the moment, the packages sub tab on the new host details page is shown based on the description of the operating system (e.g. checking whether it should load rpm or debs). The condition checks for various strings, but the description itself can be randomly assigned by the user. Hence, this is not a safe method to determine which OS was used.

Considerations taken when implementing this change?

We need to find an unchangeable attribute to test against for the content packages tabs which is the operatingsystem family.

What are the testing steps for this pull request?

Have a host with packages and some OS. Change OS name to something like "Test OS" --> Host packages should still be available.

[nadjaheitmann]

@jeremylenz We found a bug partly introduced in #10744, here is a proposed fix :) Test failure seems unrelated if I am not mistaken.

[jeremylenz]

Thanks @nadjaheitmann! Yes, I agree this should be addressed.

I see we're adding an additional API call here. There are a couple other ways this could be accomplished, without adding an API call.

1. The host details response already includes operatingsystem_name and operatingsystem_id. We could add operatingsystem_family to that, either (a) via a Foreman code change so other plugins could take advantage; or (b) by extending it from Katello.
2. The response also already includes facts. Foreman uses the distribution::name fact to determine the operating system's family. We could look at that fact here as well. (see https://github.com/theforeman/foreman/blob/e87de8d1a30aeba3da6d0cb2518d94aa450db55e/app/services/katello/rhsm_fact_parser.rb#L49-L51)

[nadjaheitmann]

2. The response also already includes `facts`. Foreman uses the `distribution::name` fact to determine the operating system's family. We could look at that fact here as well.  (see https://github.com/theforeman/foreman/blob/e87de8d1a30aeba3da6d0cb2518d94aa450db55e/app/services/katello/rhsm_fact_parser.rb#L49-L51)

This was my first attempt. There is facts as part of the host details. What if the host is not managed by any config management system? Do we still have those facts? If you try accessing the distribution::name in facts hash it gives you an error as distribution::name is no valid JS syntax for hash keys.

1. The host details response already includes `operatingsystem_name` and `operatingsystem_id`. We could add `operatingsystem_family` to that, either (a) via a Foreman code change so other plugins could take advantage; or (b) by extending it from Katello.

I was actually looking into that one as well, but Host class does not have any attribute called operatingsystem_family and I don't know how to add it such that it could be exposed in the .rabl file for the API. Any hints?

This is why I finally came up with another API call.

[nadjaheitmann]

Also, we need to test the operatingsystem's major version for module streams, so we would also need to expose that one. I agree it would be much simpler if it was part of one API response.

[jeremylenz]

I was actually looking into that one as well, but Host class does not have any attribute called operatingsystem_family and I don't know how to add it such that it could be exposed in the .rabl file for the API. Any hints?

The problem is that the operatingsystems table doesn't actually have a family attribute; rather it's a column called type which is renamed to family.

You can see this by running

bundle exec rails db:schema:dump

and then look at the file db/structure.sql:

CREATE TABLE public.operatingsystems (
    id integer NOT NULL,
    major character varying(5) DEFAULT ''::character varying NOT NULL,
    name character varying(255) NOT NULL,
    minor character varying(16) DEFAULT ''::character varying NOT NULL,
    nameindicator character varying(3),
    created_at timestamp without time zone,
    updated_at timestamp without time zone,
    release_name character varying(255),
    type character varying(255),
    description character varying(255),
    password_hash character varying(255) DEFAULT 'SHA256'::character varying,
    title character varying(255)
);

So Rails doesn't add the operatingsystem_family convenience method like it did for name and id.

# app/models/operatingsystem.rb
def family
    self[:type]
  end

  def family=(value)
    self.type = value
  end

To add family to the rabl in Foreman, this would probably work:

# app/views/api/v2/hosts/main.json.rabl
node :operatingsystem_family do |host|
  host.operatingsystem.family
end

Or, to extend it from Katello, I think you could add the same to app/views/katello/api/v2/hosts/base.json.rabl

[nadjaheitmann]

Thanks for the detailed explanation, much appreciated! Do you actually need the operating system family anywhere outside Katello?

[jeremylenz]

If you're asking where the code should live, I'd be fine with keeping it in Katello.

[nadjaheitmann]

Yeah, thanks. I don't get it work in Katello, though. It does not seem to load the app/views/katello/api/v2/hosts/base.json.rabl

No problem to edit the .rabl file directly in Foreman.

[jeremylenz]

katello/app/controllers/katello/concerns/api/v2/hostgroups_controller_extensions.rb

Lines 31 to 35 in 5195646

	def show
	@render_template = 'katello/api/v2/hostgroups_extensions/show'
	render @render_template
	end
	end

Hm, maybe you'll need to add something similar to that ^, but in hosts_controller_extensions

[jeremylenz]

Another thing to try is extend_rabl_template. Seems like foreman_remote_execution does this:

https://github.com/theforeman/foreman_remote_execution/blob/e97021ca4417d737f6d60a7586efd786413041db/lib/foreman_remote_execution/engine.rb#L274-L278

[nadjaheitmann]

katello/app/controllers/katello/concerns/api/v2/hostgroups_controller_extensions.rb

Lines 31 to 35 in 5195646

	def show
	@render_template = 'katello/api/v2/hostgroups_extensions/show'
	render @render_template
	end
	end

Hm, maybe you'll need to add something similar to that ^, but in hosts_controller_extensions

This actually overrides the original template.

Another thing to try is extend_rabl_template. Seems like foreman_remote_execution does this:

This seems to do the trick, thanks!

[nadjaheitmann]

@jeremylenz Looks much better doesn't it :)

[jeremylenz]

Looking much better 😄

Also tested it and it's working well.

Just a couple small suggestions

[jeremylenz]

Suggested change

object @host

This seems to not be needed

[jeremylenz]

This seems to work also..

Suggested change

	node :operatingsystem_family do |host|
	host.operatingsystem.family
	end
	node :operatingsystem_major do |host|
	host.operatingsystem.major
	end
	node :operatingsystem_family do
	@resource.operatingsystem.family
	end
	node :operatingsystem_major do
	@resource.operatingsystem.major
	end

[nadjaheitmann]

True, fixed

[jeremylenz]

Once #10884 is merged let's see how the tests look. LGTM pending green tests. 👍

[jeremylenz]

[test katello]

[nadjaheitmann]

[2024-02-11T22:26:17.378Z] Error:
[2024-02-11T22:26:17.378Z] Katello::Api::Rhsm::CandlepinDynflowProxyControllerIntegrationTest#test_0001_params are not parsed in the controller:
[2024-02-11T22:26:17.378Z] Errno::ENOENT: No such file or directory @ rb_sysopen - /home/jenkins/workspace/katello-pr-test/foreman/test/fixtures/hosts.yml
[2024-02-11T22:26:17.378Z] test/test_helper.rb:77:in `before_setup'

Looks like a fixture cannot be found in foreman itself. Any idea how to fix this?

[jeremylenz]

Looks like a fixture cannot be found in foreman itself. Any idea how to fix this?

I don't even see how fixtures are being used -- that test uses FactoryBot, anyway. See if a rebase fixes it maybe? 🤔

[nadjaheitmann]

Now the katello test is green, it was one commit I have added - seems fishy... New failure though, which I assume is not related:

Failure:
Actions::Pulp3::CopyAllUnitsDockerRepositoryTest#test_exclusion_docker_filters [katello/test/actions/pulp3/orchestration/copy_all_units_test.rb:89]
Minitest::Assertion: Expected: ["musl", "glibc"]
Actual: ["glibc", "musl"]

[jeremylenz]

I re-ran that check and it's green now. Was just an array ordering failure that we get intermittently. ✔️

[jeremylenz]

The React failure is unrelated

[jeremylenz]

Works well

thanks @nadjaheitmann !

Will merge once we get the React failures sorted.