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

Add enablement parameter for bootstrap RPMs #501

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
36 changes: 28 additions & 8 deletions lib/puppet/provider/bootstrap_rpm/bootstrap_rpm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,28 @@ def create
@base_dir = nil
end

def destroy
FileUtils.rm(symlink) unless symlink.nil?

all_rpms.each do |rpm|
FileUtils.rm(rpm)
end

all_rpms(source: true).each do |rpm|
FileUtils.rm(rpm)
end
end

def exists?
copy_sources
write_specfile
build_rpm
if resource[:ensure] == :absent
Copy link
Member

Choose a reason for hiding this comment

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

This feels like an abuse of exists. I suppose it already was and I think this currently doesn't support noop mode, but this really is making it much worse. exists? should really answer the question if anything exists.

Looking at the source, I think this is the relevant bit:
https://github.com/puppetlabs/puppet/blob/e227c27540975c25aa22d533a52424a9d2fc886a/lib/puppet/property/ensure.rb#L68-L95

So if anything, it should be changed to:

if resource[:ensure] == :absent
  File.exist?(resource[:symlink]) || !all_rpms.empty? || !all_rpms(source: true).empty?
else
  !File.exist?(resource[:symlink]) || all_rpms.empty? || all_rpms(source: true).empty? || rpm_changed?
end

Then it will end up calling .create:
https://github.com/puppetlabs/puppet/blob/e227c27540975c25aa22d533a52424a9d2fc886a/lib/puppet/property/ensure.rb#L16-L23
Or .destroy:
https://github.com/puppetlabs/puppet/blob/e227c27540975c25aa22d533a52424a9d2fc886a/lib/puppet/property/ensure.rb#L25-L32

It's still not very nice, but since it's going away I think that's ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't work as you have to have this part:

    copy_sources
    write_specfile
    build_rpm

Which creates the temporary RPM to detect if the RPM has changed. Even if I include that in the else, all the tests break. That has been where this whole resource is tricky.

The RPM only needs to change if the input script, katello-rhsm-consumer, changes. To detect if the RPM exists with that script has mean that we need to temporarily generate the RPM, and then compare the generated RPM to the current latest one and see if it's changed.

File.exist?(resource[:symlink]) || !all_rpms.empty? || !all_rpms(source: true).empty?
else
copy_sources
write_specfile
build_rpm

!rpm_changed?
!rpm_changed?
end
end

def symlink
Expand All @@ -39,10 +55,7 @@ def symlink=(value)
end

def latest_rpm(source: false)
extension = source ? 'src.rpm' : 'noarch.rpm'

rpms = Dir.glob("#{resource[:dest]}/#{resource[:name]}*.#{extension}")
rpms = rpms.reject { |rpm| rpm.end_with?("latest.noarch.rpm") }
rpms = all_rpms(source: source)

return false if rpms.empty?

Expand All @@ -51,6 +64,13 @@ def latest_rpm(source: false)

private

def all_rpms(source: false)
extension = source ? 'src.rpm' : 'noarch.rpm'

rpms = Dir.glob("#{resource[:dest]}/#{resource[:name]}*.#{extension}")
rpms.reject { |rpm| rpm.end_with?("latest.noarch.rpm") }
end

def base_dir
@base_dir ||= Dir.mktmpdir
end
Expand Down
5 changes: 3 additions & 2 deletions manifests/bootstrap_rpm.pp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# This file is placed in $rpm_serve_dir.
# @api private
class foreman_proxy_content::bootstrap_rpm (
Enum['present', 'absent'] $ensure = 'present',
Stdlib::Fqdn $rhsm_hostname = $facts['networking']['fqdn'],
Stdlib::Port $rhsm_port = 443,
Pattern[/\A(\/[a-zA-Z0-9]+)+(\/)?\z/] $rhsm_path = '/rhsm',
Expand Down Expand Up @@ -48,7 +49,7 @@
require => File[$katello_server_ca_cert],
} ->
rhsm_reconfigure_script { "${rpm_serve_dir}/${katello_rhsm_setup_script}":
ensure => present,
ensure => $ensure,
default_ca_cert => $ca_cert,
server_ca_cert => $katello_server_ca_cert,
default_ca_name => $default_ca_name,
Expand All @@ -62,7 +63,7 @@
}

bootstrap_rpm { $bootstrap_rpm_name:
ensure => present,
ensure => $ensure,
script => "${rpm_serve_dir}/${katello_rhsm_setup_script}",
dest => $rpm_serve_dir,
symlink => "${rpm_serve_dir}/${candlepin_cert_rpm_alias_filename}",
Expand Down
8 changes: 7 additions & 1 deletion manifests/init.pp
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@
#
# $pulpcore_import_workers_percent:: What percentage of available-workers will pulpcore use for import tasks at a time
#
# $ensure_bootstrap_rpm:: Enable the deployment of the bootstrap RPM
#
class foreman_proxy_content (
Boolean $pulpcore_mirror = false,

Expand Down Expand Up @@ -110,6 +112,7 @@
Boolean $pulpcore_analytics = false,
Boolean $pulpcore_hide_guarded_distributions = true,
Optional[Integer[1,100]] $pulpcore_import_workers_percent = undef,
Enum['present', 'absent'] $ensure_bootstrap_rpm = 'present',
) inherits foreman_proxy_content::params {
include certs
include foreman_proxy
Expand All @@ -130,7 +133,9 @@
include certs::foreman_proxy
Class['certs::foreman_proxy'] ~> Service['foreman-proxy']

include foreman_proxy_content::pub_dir
class { 'foreman_proxy_content::pub_dir':
ensure_bootstrap => $ensure_bootstrap_rpm,
}

if $pulpcore_mirror {
$base_allowed_import_paths = ['/var/lib/pulp/sync_imports']
Expand Down Expand Up @@ -307,6 +312,7 @@
}

class { 'foreman_proxy_content::bootstrap_rpm':
ensure => $ensure_bootstrap_rpm,
rhsm_hostname => $client_facing_servername,
rhsm_port => $rhsm_port,
rhsm_path => $rhsm_path,
Expand Down
6 changes: 5 additions & 1 deletion manifests/pub_dir.pp
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@
#
# @param pub_dir_options
# The Directory options as Apache applies them
#
# @param ensure_bootstrap
# Enable installing the katello-client-bootstrap RPM
class foreman_proxy_content::pub_dir (
String $pub_dir_options = '+FollowSymLinks +Indexes',
Enum['present', 'absent'] $ensure_bootstrap = 'present',
) {
stdlib::ensure_packages('katello-client-bootstrap')
stdlib::ensure_packages('katello-client-bootstrap', { 'ensure' => $ensure_bootstrap })

include apache::mod::alias

Expand Down
32 changes: 32 additions & 0 deletions spec/acceptance/bootstrap_rpm_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -272,4 +272,36 @@ class { 'foreman_proxy_content::bootstrap_rpm':
it { should be_linked_to "/var/www/html/pub/katello-ca-consumer-#{host_inventory['fqdn']}-1.0-2.noarch.rpm" }
end
end

context 'removes RPM when ensure is false' do
it_behaves_like 'an idempotent resource' do
let(:manifest) do
<<-PUPPET
class { 'foreman_proxy_content::bootstrap_rpm':
ensure => 'absent',
}
PUPPET
end
end

describe file("/var/www/html/pub/katello-ca-consumer-#{host_inventory['fqdn']}-1.0-1.src.rpm") do
it { should_not exist }
end

describe file("/var/www/html/pub/katello-ca-consumer-#{host_inventory['fqdn']}-1.0-4.src.rpm") do
it { should_not exist }
end

describe file("/var/www/html/pub/katello-ca-consumer-#{host_inventory['fqdn']}-1.0-10.src.rpm") do
it { should_not exist }
end

describe file('/var/www/html/pub/katello-ca-consumer-latest.noarch.rpm') do
it { should_not exist }
end

describe file('/var/www/html/pub/katello-rhsm-consumer') do
it { should_not exist }
end
end
end
18 changes: 18 additions & 0 deletions spec/classes/foreman_proxy_content_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,24 @@ class { 'certs':
it { is_expected.not_to compile.with_all_deps }
end
end

context 'with boostrap_rpm false' do
let(:params) do
{
ensure_bootstrap_rpm: 'absent'
}
end

it { is_expected.to compile.with_all_deps }
it do
is_expected.to contain_package('katello-client-bootstrap')
.with_ensure('absent')
end
it do
is_expected.to contain_class('foreman_proxy_content::bootstrap_rpm')
.with_ensure('absent')
end
end
end
end
end
Loading