search

puppetlabs / puppetlabs-apt

Puppet module to help manage Apt
https://forge.puppetlabs.com/puppetlabs/apt
Apache License 2.0
216 stars 463 forks source link

Verify that $_proxy is set before dict lookup. #1152

Closed rjemanuele closed 6 months ago

rjemanuele commented 6 months ago

Fixes puppetlabs/puppetlabs-apt#1151

CLAassistant commented 6 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

bastelfreak commented 6 months ago

@rjemanuele thanks for the PR. Can you please add a test to reproduce the original bug?

rjemanuele commented 6 months ago

Ok, so apt is a requirement of the theforeman-forman package but nothing there declares the apt module.

In my case this fails (warnings are hidden by the agent):

root@zzzz:~# puppet apply --modulepath=/root/test_modules/ -e "apt::ppa{ 'ppa:ondrej/php': }"
Warning: Unknown variable: 'apt::ppa_options'. (file: /root/test_modules/apt/manifests/ppa.pp, line: 28, column: 43)
Warning: Unknown variable: 'apt::ppa_package'. (file: /root/test_modules/apt/manifests/ppa.pp, line: 31, column: 43)
Warning: Unknown variable: 'apt::_proxy'. (file: /root/test_modules/apt/manifests/ppa.pp, line: 76, column: 15)
Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Operator '[]' is not applicable to an Undef Value. (file: /root/test_modules/apt/manifests/ppa.pp, line: 77, column: 8) (line: 1) on node zzzz

But this succeeds:

root@zzzz:~# puppet apply --modulepath=/root/test_modules/ -e "include apt ; apt::ppa{ 'ppa:ondrej/php': }"
Notice: Compiled catalog for zzzz in environment production in 0.09 seconds
Notice: /Stage[main]/Apt/File[preferences]/ensure: created
Notice: /Stage[main]/Apt/Apt::Setting[conf-update-stamp]/File[/etc/apt/apt.conf.d/15update-stamp]/ensure: defined content as '{sha256}2e6eb1f5f20262bfc6b7dfb26a302f00b4ab5fee803abd9e07ad8378cce067d5'
Notice: /Stage[main]/Apt::Update/Exec[apt_update]: Triggered 'refresh' from 1 event
Notice: Applied catalog in 4.92 seconds
bastelfreak commented 6 months ago

yes, you've to declare the apt class first.

rjemanuele commented 6 months ago

Thanks for ride.

kenyon commented 6 months ago

I think the fix for that would be to just include apt at the top of ppa.pp. It was originally there: https://github.com/puppetlabs/puppetlabs-apt/blob/f848bac6072f99e1cd60a0cc0b84e5679c598dab/manifests/ppa.pp#L6

Changed to a require in https://github.com/puppetlabs/puppetlabs-apt/blob/ed2d19e2f3aa025871ec8334d48db53b800a91f2/manifests/ppa.pp#L6

Then replaced with a chaining arrow in https://github.com/puppetlabs/puppetlabs-apt/commit/18f614b89be97040815f46ba95677b4d69ed305f

And that chaining was removed in https://github.com/puppetlabs/puppetlabs-apt/commit/e3784987fce98bf3c8c6cc40e73bb6c9582c0696

The other includes were finally removed in 501a1b5627687b13c0809b82db03f9df168fbd7a and 0d9bab38cc252edb1af333e7cd6cee11a2c71f65.

These other defined types do include apt like this:

https://github.com/puppetlabs/puppetlabs-apt/blob/e0b3a5db6abb043f106614dffe341f68d88158ab/manifests/source.pp#L98 https://github.com/puppetlabs/puppetlabs-apt/blob/e0b3a5db6abb043f106614dffe341f68d88158ab/manifests/backports.pp#L54