Page MenuHomePhabricator

Puppet Proposal to remove require_package
Open, LowPublic

Description

The require_packages function call is used to declare one or more packages a dependency for the current scope. at a high level the function does the following

With a starting manifest like:

class {'foobar':
  require_package('foo', 'bar')
  file {'/etc/foobar.conf':
    ensure => present,
  }
   service {'foobar':
    ensure => running,
    require => File[/etc/foobar.conf'],
  }
}
class {'foobar': }

we end up, in the compiled catalogue, with something a bit more like this:

class {'packages::foo':
  package{'foo':
    ensure => 'present'
  }
}
class {'packages::bar':
  package{'bar':
    ensure => 'present'
  }
}
class {'foobar':
  file {'/etc/foobar.conf':
    ensure => present,
  }
   service {'foobar':
    ensure => running,
    require => File[/etc/foobar.conf'],
  }
}
class {'packages::foo':}
class {'packages::bar':}
class {'foobar': 
  require => Class['packages::foo', 'packages::bar'],
}

The other benefit of this function is that it also handles duplicate resources declarations, similar to the puppetlabs-stdlib ensure_packages function.

the require_packages function predates my time here however the initial commit message gives a good explanation for its intention and benefits over ensure_packages:

It's similar to ensure_packages(), but it's cleaner and faster. It takes a
single package name as an argument and it creates a virtual class that wraps it
and makes it a requirement for the current scope. We can use this to avoid
duplicate def'n errors for packages that may legitimately be specified in the
software stacks of multiple modules / roles.

However I'm not sure if theses claims are still valid. Specifically when compared to ensure_packages:

  • ensure_packages can now also take a single string or array of strings.
  • The cleaner point is subjective however the ensure_packages function in stdlib has received some refactoring since require_package was initially merged
  • I have not tested the faster claim

The thing that ensure_packages doesn't have is the ability to "create a virtual class that wraps [the package] and makes it a requirement for the current scope". Im ensure of the specific problem this was trying to resolve however in the early days of puppet dependency mapping was a bit of a pain and not necessarily intuitive as such dependencies where often missed. Puppet has improved this behavior somewhat by honoring manifest order when applying resources which reduces the need to explicitly define most dependencies and for complex dependencies that do need to be defined I believe it is clearer to explicitly define them. I also feel that this function is rather un-standard (i.e. dynamic resource creation and catalogue manipulation) and causes unexpected results when looking at the raw catalogue.

As such and assuming i havn;t missed something, i propose working towards dropping require_packages from wmflib and migrating to ensure_packages.

Taking the foobar class a migration to ensure_packages would look as follows

class {'foobar':
  $packages = ['foo', 'bar']
  ensure_packages($packages)
  file {'/etc/foobar.conf':
    ensure => present,
    require => Package[$packages]
  }
   service {'foobar':
    ensure => running,
    require => File[/etc/foobar.conf'],
  }
}
class {'foobar': }

tagging @Joe/@akosiaris to correct anything i may have got wrong or what i may have missed

Details

ProjectBranchLines +/-Subject
operations/puppetproduction+5 -1
operations/puppetproduction+1 -1
operations/puppetproduction+1 -1
operations/puppetproduction+1 -1
operations/puppetproduction+1 -1
operations/puppetproduction+2 -2
operations/puppetproduction+2 -2
operations/puppetproduction+937 -905
operations/puppetproduction+2 -7
operations/puppetproduction+2 -2
operations/puppetproduction+1 -1
operations/puppetproduction+1 -2
operations/puppetproduction+1 -1
operations/puppetproduction+2 -2
operations/puppetproduction+14 -31
operations/puppetproduction+19 -14
operations/puppetproduction+1 -1
operations/puppetproduction+3 -3
operations/puppetproduction+49 -99
operations/puppetproduction+3 -3
operations/puppetproduction+26 -16
operations/puppetproduction+15 -17
operations/puppetproduction+3 -3
operations/puppetproduction+3 -3
operations/puppetproduction+110 -135
operations/puppetproduction+10 -13
operations/puppetproduction+7 -5
operations/puppetproduction+18 -10
operations/puppetproduction+16 -16
operations/puppetproduction+10 -17
operations/puppetproduction+2 -2
operations/puppetproduction+4 -6
operations/puppetproduction+1 -1
operations/puppetproduction+4 -7
operations/puppetproduction+6 -35
operations/puppetproduction+14 -13
operations/puppetproduction+9 -9
operations/puppetproduction+14 -16
operations/puppetproduction+3 -3
operations/puppetproduction+8 -12
operations/puppetproduction+9 -10
operations/puppetproduction+5 -5
operations/puppetproduction+2 -2
operations/puppetproduction+5 -6
operations/puppetproduction+9 -10
operations/puppetproduction+4 -4
operations/puppetproduction+6 -6
operations/puppetproduction+16 -16
operations/puppetproduction+5 -5
operations/puppetproduction+19 -10
operations/puppetproduction+3 -3
operations/puppetproduction+10 -10
operations/puppetproduction+8 -8
operations/puppetproduction+2 -2
operations/puppetproduction+1 -1
operations/puppetproduction+12 -10
operations/puppetproduction+16 -15
operations/puppetproduction+5 -5
operations/puppetproduction+3 -3
operations/puppetproduction+7 -7
operations/puppetproduction+2 -1
operations/puppetproduction+4 -4
operations/puppetproduction+2 -2
operations/puppetproduction+21 -26
operations/puppetproduction+15 -17
operations/puppetproduction+4 -4
operations/puppetproduction+2 -1
operations/puppetproduction+5 -5
operations/puppetproduction+1 -1
operations/puppetproduction+5 -5
operations/puppetproduction+8 -8
operations/puppetproduction+8 -8
operations/puppetproduction+6 -6
operations/puppetproduction+14 -12
operations/puppetproduction+3 -3
operations/puppetproduction+5 -5
operations/puppetproduction+1 -1
operations/puppetproduction+2 -2
operations/puppetproduction+3 -4
operations/puppetproduction+16 -10
operations/puppetproduction+13 -14
operations/puppetproduction+3 -3
operations/puppetproduction+7 -5
operations/puppetproduction+1 -1
operations/puppetproduction+6 -5
operations/puppetproduction+2 -2
operations/puppetproduction+5 -6
operations/puppetproduction+6 -6
operations/puppetproduction+4 -4
operations/puppetproduction+13 -11
operations/puppetproduction+18 -60
operations/puppetproduction+2 -2
operations/puppetproduction+17 -23
operations/puppetproduction+15 -21
operations/puppetproduction+12 -9
operations/puppetproduction+1 -1
operations/puppetproduction+2 -2
operations/puppetproduction+4 -5
operations/puppetproduction+3 -3
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 639858 merged by Jbond:
[operations/puppet@production] swift: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639858

Change 639857 merged by Jbond:
[operations/puppet@production] sudo: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639857

Change 639853 merged by Jbond:
[operations/puppet@production] rsyslog: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639853

Change 639854 merged by Jbond:
[operations/puppet@production] service: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639854

Change 639856 merged by Jbond:
[operations/puppet@production] smart: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639856

Change 639852 merged by Jbond:
[operations/puppet@production] wmcs: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639852

Change 639850 merged by Jbond:
[operations/puppet@production] simplelap: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639850

Change 639851 merged by Jbond:
[operations/puppet@production] striker: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639851

Change 639849 merged by Jbond:
[operations/puppet@production] simplelamp2: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639849

Change 639848 merged by Jbond:
[operations/puppet@production] puppetmaster::standalone: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639848

Change 639845 merged by Jbond:
[operations/puppet@production] lists: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639845

Change 639843 merged by Jbond:
[operations/puppet@production] racktables: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639843

Change 639841 merged by Jbond:
[operations/puppet@production] query_service: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639841

Change 639837 merged by Jbond:
[operations/puppet@production] P:puppet_compiler::packages: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639837

Change 639836 merged by Jbond:
[operations/puppet@production] P:prometheus: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639836

Change 640128 had a related patch set uploaded (by Jbond; owner: Jbond):
[operations/puppet@production] P:prometheus: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640128

Change 639786 merged by Jbond:
[operations/puppet@production] mediawiki: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639786

Change 639847 merged by Jbond:
[operations/puppet@production] mediawaki::memcached: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639847

Change 639833 merged by Jbond:
[operations/puppet@production] P:zuul::server: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639833

Change 639820 merged by Jbond:
[operations/puppet@production] P:puppetmaster::common: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639820

Change 639832 merged by Jbond:
[operations/puppet@production] P:zookeeper: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639832

Change 639827 merged by Jbond:
[operations/puppet@production] P:webperf::xhgui: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639827

Change 639829 merged by Jbond:
[operations/puppet@production] P:wikistats::httpd: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639829

Change 639831 merged by Jbond:
[operations/puppet@production] P:wmcs: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639831

Change 639763 merged by Jbond:
[operations/puppet@production] dumps::generation: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639763

Change 639825 merged by Jbond:
[operations/puppet@production] P:tlsproxy::envoy: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639825

Change 639822 merged by Jbond:
[operations/puppet@production] P:python37: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639822

Change 639818 merged by Jbond:
[operations/puppet@production] P:phabricator: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639818

Change 639816 merged by Jbond:
[operations/puppet@production] P:openstack: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639816

Change 639817 merged by Jbond:
[operations/puppet@production] P:parsoid: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639817

Change 639823 merged by Jbond:
[operations/puppet@production] P:spicerack: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639823

Change 639814 merged by Jbond:
[operations/puppet@production] P:mediawiki: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639814

Change 639844 merged by Jbond:
[operations/puppet@production] O:alerting_host: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639844

Change 639809 merged by Jbond:
[operations/puppet@production] P:lists: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639809

Change 639810 merged by Jbond:
[operations/puppet@production] P:mariadb: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639810

Change 639805 merged by Jbond:
[operations/puppet@production] P:cyberbot::exec: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639805

Change 639807 merged by Jbond:
[operations/puppet@production] P:docker::engine: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639807

Change 639808 merged by Jbond:
[operations/puppet@production] P:java::java_8: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639808

Change 639800 merged by Jbond:
[operations/puppet@production] profile::ceph: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639800

Change 639803 merged by Jbond:
[operations/puppet@production] P:conf::client: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639803

Change 639801 merged by Jbond:
[operations/puppet@production] P:ci: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639801

Change 639795 merged by Jbond:
[operations/puppet@production] postgresql::server: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639795

Change 639798 merged by Jbond:
[operations/puppet@production] profile::analytics: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639798

Change 639799 merged by Jbond:
[operations/puppet@production] profile::backup: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639799

Change 639782 merged by Jbond:
[operations/puppet@production] librenms: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639782

Change 639783 merged by Jbond:
[operations/puppet@production] lxc: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639783

Change 639787 merged by Jbond:
[operations/puppet@production] mjolnir: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639787

Change 639788 merged by Jbond:
[operations/puppet@production] motd: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639788

Change 639793 merged by Jbond:
[operations/puppet@production] openstack: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639793

Change 639772 merged by Jbond:
[operations/puppet@production] java: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639772

Change 639774 merged by Jbond:
[operations/puppet@production] jupyterhub: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639774

Change 639777 merged by Jbond:
[operations/puppet@production] libraryupgrader: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639777

Change 639840 merged by Jbond:
[operations/puppet@production] puppetdb/puppetmaster: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639840

Change 639769 merged by Jbond:
[operations/puppet@production] haproxy: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639769

Change 639764 merged by Jbond:
[operations/puppet@production] envoproxy: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639764

Change 639747 merged by Jbond:
[operations/puppet@production] base:standard_packages: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639747

Change 639819 merged by Jbond:
[operations/puppet@production] P:proxysql: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/639819

Change 640181 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] P:prometheus: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640181

Change 640181 merged by Jbond:
[operations/puppet@production] P:prometheus: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640181

Change 640128 abandoned by Jbond:
[operations/puppet@production] P:prometheus: migrate to debian::codename and ensure_packages

Reason:
in favour of Iec2815a4a05a423d5e1bd6b0d8ee31f4bc451a16

https://gerrit.wikimedia.org/r/640128

Change 640221 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] debian::codename::require: update code to use new debian:: functions

https://gerrit.wikimedia.org/r/640221

Change 640221 merged by Jbond:
[operations/puppet@production] debian::codename::require: update code to use new debian:: functions

https://gerrit.wikimedia.org/r/640221

Change 640387 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] P:tendril::webserver: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640387

Change 640387 merged by Jbond:
[operations/puppet@production] P:tendril::webserver: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640387

Change 640389 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] P:base: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640389

Change 640389 merged by Jbond:
[operations/puppet@production] P:base: migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640389

Change 640406 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] P:base migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640406

Change 640406 merged by Jbond:
[operations/puppet@production] P:base migrate to debian::codename and ensure_packages

https://gerrit.wikimedia.org/r/640406

Change 640688 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] puppet: migrate from require_package to ensure_packages

https://gerrit.wikimedia.org/r/640688

Change 639824 merged by Jbond:
[operations/puppet@production] P:tendril::webserver: migrate to ensure_packages

https://gerrit.wikimedia.org/r/639824

Change 639785 merged by Jbond:
[operations/puppet@production] mariadb: migrate to ensure_packages and minor refactor

https://gerrit.wikimedia.org/r/639785

Change 641309 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] mailman: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641309

Change 641310 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] iegreview: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641310

Change 641311 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] httpbb: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641311

Change 641312 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] aptrepo: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641312

Change 641313 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] noc: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641313

Change 641314 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] git: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641314

dancy updated the task description. (Show Details)Tue, Nov 17, 4:52 PM
dancy updated the task description. (Show Details)
dpifke added a subscriber: dpifke.Tue, Nov 17, 4:55 PM

Change 641312 merged by Dzahn:
[operations/puppet@production] aptrepo: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641312

Change 641309 merged by Dzahn:
[operations/puppet@production] mailman: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641309

Change 641310 merged by Dzahn:
[operations/puppet@production] iegreview: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641310

Change 641313 merged by Dzahn:
[operations/puppet@production] noc: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641313

Change 641311 merged by Dzahn:
[operations/puppet@production] httpbb: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641311

Dzahn added a comment.Tue, Nov 17, 7:48 PM

I merged a couple changes you can see above (aptrepo, mailman, iegreview, noc, httpbb,..) and they were all unproblematic and noop.

But then I got to the git module and this one here fails with " Duplicate declaration: Package[git] is already declared at (line: 3); cannot redeclare (file: /srv/workspace/puppet/modules/git/manifests/clone.pp, line: 72) (file: /srv/workspace/puppet/modules/git/manifests/clone.pp, line: 72, column: 5) (file: /srv/workspace/puppet/modules/puppetmaster/manifests/base_repo.pp, line: 18)

https://gerrit.wikimedia.org/r/641314

So there might be some of those popping up if we just merged the large https://gerrit.wikimedia.org/r/c/operations/puppet/+/640688

Change 641314 merged by Dzahn:
[operations/puppet@production] git: replace require_package with ensure_packages

https://gerrit.wikimedia.org/r/641314

Change 641792 had a related patch set uploaded (by Muehlenhoff; owner: Muehlenhoff):
[operations/puppet@production] openldap: Stop using require_package()

https://gerrit.wikimedia.org/r/641792

Change 641795 had a related patch set uploaded (by Muehlenhoff; owner: Muehlenhoff):
[operations/puppet@production] debmonitor: Move to ensure_packages

https://gerrit.wikimedia.org/r/641795

Change 641796 had a related patch set uploaded (by Muehlenhoff; owner: Muehlenhoff):
[operations/puppet@production] profile::base::linux419: Move away from require_package()

https://gerrit.wikimedia.org/r/641796

Change 641795 merged by Muehlenhoff:
[operations/puppet@production] debmonitor: Move to ensure_packages

https://gerrit.wikimedia.org/r/641795

Change 641796 merged by Muehlenhoff:
[operations/puppet@production] profile::base::linux419: Move away from require_package()

https://gerrit.wikimedia.org/r/641796

Change 641792 merged by Muehlenhoff:
[operations/puppet@production] openldap: Stop using require_package()

https://gerrit.wikimedia.org/r/641792

Change 641856 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] smokeping: require_package -> ensure_packages

https://gerrit.wikimedia.org/r/641856

Change 641858 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] tcpircbot: require_package -> ensure_packages

https://gerrit.wikimedia.org/r/641858

Change 641857 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] docker_registry_ha: require_package->ensure_packages

https://gerrit.wikimedia.org/r/641857

Change 641856 merged by Dzahn:
[operations/puppet@production] smokeping: require_package -> ensure_packages

https://gerrit.wikimedia.org/r/641856

Change 641857 merged by Dzahn:
[operations/puppet@production] docker_registry_ha: require_package->ensure_packages

https://gerrit.wikimedia.org/r/641857

Change 641858 merged by Dzahn:
[operations/puppet@production] tcpircbot: require_package -> ensure_packages

https://gerrit.wikimedia.org/r/641858

Change 644350 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] deployment::server: buster support, use mariadb-client, not mysql-client

https://gerrit.wikimedia.org/r/644350