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 #35713 - Host details tab for Debian packages #10744

Merged
merged 1 commit into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/controllers/katello/api/v2/debs_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,13 @@ def available_for_content_view_version(version)
def custom_index_relation(collection)
applicable = ::Foreman::Cast.to_bool(params[:packages_restrict_applicable]) || params[:host_id]
upgradable = ::Foreman::Cast.to_bool(params[:packages_restrict_upgradable])
not_installed = ::Foreman::Cast.to_bool(params[:packages_restrict_not_installed])

if upgradable
collection = collection.installable_for_hosts(@hosts)
elsif not_installed && params[:host_id]
host = @hosts.first
collection = Katello::Deb.apt_installable_for_host(host)
elsif applicable
collection = collection.applicable_to_hosts(@hosts)
end
Expand Down
14 changes: 13 additions & 1 deletion app/controllers/katello/api/v2/host_debs_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,26 @@ class Api::V2::HostDebsController < Api::V2::ApiController

api :GET, "/hosts/:host_id/debs", N_("List deb packages installed on the host")
param :host_id, :number, :required => true, :desc => N_("ID of the host")
param :include_latest_upgradable, :boolean, :desc => N_("Also include the latest upgradable package version for each host package")
param :status, String, :desc => N_("Return only packages of a particular status (upgradable or up-to-date)"), :required => false
param_group :search, Api::V2::ApiController
add_scoped_search_description_for(Katello::InstalledDeb)
def index
collection = scoped_search(index_relation, :name, :asc, :resource_class => ::Katello::InstalledDeb)
collection[:results] = HostDebPresenter.with_latest(collection[:results], @host) if ::Foreman::Cast.to_bool(params[:include_latest_upgradable])
respond_for_index(:collection => collection)
end

def index_relation
@host.installed_debs
packages = @host.installed_debs
upgradable_packages = ::Katello::Deb.installable_for_hosts([@host]).select(:name)
if params[:status].present?
packages = case params[:status]
when 'up-to-date' then packages.where.not(name: upgradable_packages)
when 'upgradable' then packages.where(name: upgradable_packages)
end
end
packages
end

def resource_class
Expand Down
26 changes: 26 additions & 0 deletions app/models/katello/concerns/host_managed_extensions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,14 @@ def rhel_eos_schedule_index
end

def package_names_for_job_template(action:, search:, versions: nil)
if self.operatingsystem.family == 'Debian'
deb_names_for_job_template(action, search)
else
yum_names_for_job_template(action, search, versions)
end
end

def yum_names_for_job_template(action:, search:, versions: nil)
actions = %w(install remove update).freeze
case action
when 'install'
Expand All @@ -560,6 +568,24 @@ def package_names_for_job_template(action:, search:, versions: nil)
end
end

def deb_names_for_job_template(action:, search:)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could avoid adding a new method here, and just check self.operatingsystem.family within package_names_for_job_template instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremylenz I think, it is a good idea and I have a added a commit on top of the actual commit for the host details page. However, rubocop does not like the changes. Not sure if I can make this any simpler.

[2024-01-05T09:07:35.947Z] Offenses:

[2024-01-05T09:07:35.947Z] /home/jenkins/workspace/katello-pr-test/app/models/katello/concerns/host_managed_extensions.rb:541:7: C: Metrics/AbcSize: Assignment Branch Condition size for package_names_for_job_template is too high. [<9, 56, 21> 60.48/60]

[2024-01-05T09:07:35.947Z] def package_names_for_job_template(action:, search:, versions: nil) ...

[2024-01-05T09:07:35.947Z] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[2024-01-05T09:07:35.947Z] /home/jenkins/workspace/katello-pr-test/app/models/katello/concerns/host_managed_extensions.rb:541:7: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for package_names_for_job_template is too high. [16/14]

[2024-01-05T09:07:35.947Z] def package_names_for_job_template(action:, search:, versions: nil) ...

[2024-01-05T09:07:35.947Z] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[2024-01-05T09:07:35.947Z] /home/jenkins/workspace/katello-pr-test/app/models/katello/concerns/host_managed_extensions.rb:541:7: C: Metrics/MethodLength: Method has too many lines. [34/30]

[2024-01-05T09:07:35.947Z] def package_names_for_job_template(action:, search:, versions: nil) ...

Copy link
Member

Choose a reason for hiding this comment

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

@nadjaheitmann Two options here. I would be fine with either one:

  1. Extract your if / else block to its own method. I would probably also do it as a case statement (case operatingsystem.family)
  2. Or, just disable that cop and enable it at the end of the method. We do that in several places.

Copy link
Contributor Author

@nadjaheitmann nadjaheitmann Jan 9, 2024

Choose a reason for hiding this comment

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

I tried option one, although I did not want to use a case statement. Seems safer to me to have Debian and everything else :)

actions = %w(install remove update).freeze
case action
when 'install'
::Katello::Deb.apt_installable_for_host(self).search_for(search).distinct.pluck(:name)
when 'remove'
return [] if search.empty?

installed_debs.search_for(search).distinct.pluck(:name)
when 'update'
return [] if search.empty?

installed_debs.search_for(search).distinct.pluck(:name)
else
fail ::Foreman::Exception.new(N_("deb_names_for_job_template: Action must be one of %s"), actions.join(', '))
end
end

def advisory_ids(search:)
::Katello::Erratum.installable_for_hosts([self]).search_for(search).pluck(:errata_id)
end
Expand Down
8 changes: 8 additions & 0 deletions app/models/katello/deb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class Deb < Katello::Model
:dependent => :destroy, :inverse_of => :deb
has_many :content_facets, :through => :content_facet_applicable_debs, :class_name => "Katello::Host::ContentFacet"

scoped_search :on => :id, :complete_value => true
scoped_search :on => :name, :complete_value => true
scoped_search :on => :version, :complete_value => true
scoped_search :on => :architecture, :complete_value => true
Expand Down Expand Up @@ -95,6 +96,13 @@ def self.applicable_to_hosts(hosts)
where("#{Katello::Host::ContentFacet.table_name}.host_id" => hosts).distinct
end

# Return deb packages that are not installed on a host, but could be installed
# the word 'installable' has a different meaning here than elsewhere
def self.apt_installable_for_host(host)
repos = host.content_facet.bound_repositories.pluck(:id)
Katello::Deb.in_repositories(repos).where.not(name: host.installed_debs.pluck(:name)).order(:name)
end

def self.latest(_relation)
fail 'NotImplemented'
end
Expand Down
1 change: 1 addition & 0 deletions app/models/katello/installed_deb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class InstalledDeb < Katello::Model
has_many :host_installed_debs, :class_name => "Katello::HostInstalledDeb", :dependent => :destroy, :inverse_of => :installed_deb
has_many :hosts, :through => :host_installed_debs, :class_name => "::Host"

scoped_search :on => :id, :complete_value => true
scoped_search :on => :name, :complete_value => true
scoped_search :on => :version
scoped_search :on => :architecture
Expand Down
23 changes: 23 additions & 0 deletions app/presenters/katello/host_deb_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
module Katello
class HostDebPresenter < SimpleDelegator
attr_accessor :installed_package, :upgradable_versions, :deb_id

def initialize(installed_package, upgradable_versions, deb_id)
@installed_package = installed_package
@upgradable_versions = upgradable_versions
@deb_id = deb_id
super(@installed_package)
end

def self.with_latest(packages, host)
upgradable_packages_map = ::Katello::Deb.installable_for_hosts([host]).select(:id, :name, :architecture, :version).order(version: :desc).group_by { |i| [i.name, i.architecture] }
installed_packages_map = ::Katello::Deb.where(version: packages.map(&:version)).select(:id, :architecture, :name).group_by { |i| [i.name, i.architecture] }

packages.map do |p|
upgrades = upgradable_packages_map[[p.name, p.architecture]]&.pluck(:version)
installed = installed_packages_map[[p.name, p.architecture]]&.first&.id
HostDebPresenter.new(p, upgrades, installed)
end
end
end
end
2 changes: 2 additions & 0 deletions app/views/katello/api/v2/host_debs/base.json.rabl
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ object @resource

attributes :id, :name
attributes :version, :architecture
attributes :upgradable_versions
attributes :deb_id
53 changes: 53 additions & 0 deletions test/presenters/host_deb_presenter_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
require 'katello_test_helper'

module Katello
class HostDebPresenterTest < ActiveSupport::TestCase
def setup
@repo = katello_repositories(:debian_10_amd64_dev)
@deb = katello_debs(:one_new)
end

let(:installed_deb) { Katello::InstalledDeb.create(name: 'uno', version: @deb.version, architecture: @deb.architecture) }
let(:new_version) { '1.2' }

test "deb with set upgradable_version" do
presenter = HostDebPresenter.new(installed_deb, [new_version], @deb.id)

assert_equal presenter.upgradable_versions, [new_version]
assert_equal presenter.name, installed_deb.name
assert_equal presenter.deb_id, @deb.id
end

test "with nil upgradable_version" do
presenter = HostDebPresenter.new(installed_deb, nil, @deb.id)

assert_nil presenter.upgradable_versions
assert_equal presenter.name, installed_deb.name
assert_equal presenter.deb_id, @deb.id
end

test "with_latest" do
host = katello_content_facets(:content_facet_one).host
host.content_facet.bound_repositories << @repo
update = Katello::Deb.create(name: 'uno', pulp_id: 'uno-new-uuid', version: '1.2', architecture: 'amd64')
::Katello::Deb.stubs(:installable_for_hosts).returns(Katello::Deb.where(id: update.id))
presenter = HostDebPresenter.with_latest([installed_deb], host).first

assert_equal presenter.upgradable_versions, [new_version]
assert_equal presenter.name, installed_deb.name
assert_equal presenter.deb_id, @deb.id
end

test "with arch" do
host = katello_content_facets(:content_facet_one).host
host.content_facet.bound_repositories << @repo
update = Katello::Deb.create(name: 'one', pulp_id: 'one-new-uuid', version: '1.2', architecture: 'noarch')
::Katello::Deb.stubs(:installable_for_hosts).returns(Katello::Deb.where(id: update.id))
presenter = HostDebPresenter.with_latest([installed_deb], host).first

assert_nil presenter.upgradable_versions
assert_equal presenter.name, installed_deb.name
assert_equal presenter.deb_id, @deb.id
end
end
end
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import { Route, Switch, Redirect } from 'react-router-dom';
import { PackagesTab } from '../PackagesTab/PackagesTab.js';
import { DebsTab } from '../DebsTab/DebsTab.js';
import { ErrataTab } from '../ErrataTab/ErrataTab.js';
import { ModuleStreamsTab } from '../ModuleStreamsTab/ModuleStreamsTab';
import RepositorySetsTab from '../RepositorySetsTab/RepositorySetsTab';
Expand All @@ -11,6 +12,9 @@ const SecondaryTabRoutes = () => (
<Route path={route('packages')}>
<PackagesTab />
</Route>
<Route path={route('debs')}>
<DebsTab />
</Route>
<Route path={route('errata')}>
<ErrataTab />
</Route>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { translate as __ } from 'foremanReact/common/I18n';
import { hideRepoSetsTab } from '../RepositorySetsTab/RepositorySetsTab';
import { hideModuleStreamsTab } from '../ModuleStreamsTab/ModuleStreamsTab';
import { hideDebsTab } from '../DebsTab/DebsTab';
import { hidePackagesTab } from '../PackagesTab/PackagesTab';

const SECONDARY_TABS = [
{ key: 'packages', title: __('Packages') },
{ key: 'debs', hideTab: hideDebsTab, title: __('Packages') },
{ key: 'packages', hideTab: hidePackagesTab, title: __('Packages') },
{ key: 'errata', title: __('Errata') },
{ key: 'module-streams', hideTab: hideModuleStreamsTab, title: __('Module streams') },
{ key: 'Repository sets', hideTab: hideRepoSetsTab, title: __('Repository sets') },
Expand Down
Loading
Loading