add support for pins class parameter #561
Conversation
This lets you send apt::pin resources as an apt param hash. This is a very simple change that is just like sources, ppa, settings and other options.
| @@ -4,7 +4,7 @@ | |||
| define apt::pin( | |||
| $ensure = present, | |||
| $explanation = undef, | |||
| $order = undef, | |||
| $order = 50, | |||
There was a problem hiding this comment.
Where does this come from, and why? This affects far more than just the feature you're trying to introduce.
There was a problem hiding this comment.
I'm not sure how. Nobody can use the module without setting a pin order when $order = undef. If you try then you get this error message:
Error: Could not retrieve catalog from remote server: Error 400 on SERVER: apt::setting priority must be an integer or a zero-padded integer on node vagrant.box
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run
Which is confusing since the problem comes from "order" and not "priority" and from apt::pin and not apt::setting. I had to read the source to figure out what was going on.
So I could update the docs to show that order is a required value for apt::pin, but it's not reflected in any examples:
apt::pin { 'karmic': priority => 700 }
apt::pin { 'karmic-updates': priority => 700 }
apt::pin { 'karmic-security': priority => 700 }
It's mentioned in the apt::pin documentation but only to say it's an optional value and the default is 50.
Sorry to slip this in. I honestly thought it was a minor change that was probably what everyone intended. If this is truly not the case then I could update the documentation as stated, or leave it to someone who knows better what apt::pin is supposed to be doing. :)
I'll upload some tests in a couple of minutes.
|
As daenney say, please do not change the behaviour there. Fixing the documentation instead is much less surprising to people having that in production. Additionally, please add a test for passing the data through correctly, like it is done for ppas: https://github.com/puppetlabs/puppetlabs-apt/blob/master/spec/classes/apt_spec.rb#L206 |
Futher clarification is needed on what to do, but if it should be changed or documented it should probably be in a different pull request.
|
Alright, this is looking much better. If you could squash everything into a single commit with a good description like "Add support for creating pins from main class" and if you feel like elaborating further in the commit that would be great. Once that's done I'm OK on merging this. |
|
Closing this since it has been reopened with a new squashed commit. |
|
Merged in #564 |
This lets you send apt::pin resources as an apt param hash. This is a
very simple change that is just like sources, ppa, settings and other
options.
This is mainly to make supporting pins in foreman easier.