-
Notifications
You must be signed in to change notification settings - Fork 461
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
Apt update tooling #349
Apt update tooling #349
Conversation
The current test failures aren't your fault. Travis installed puppet-lint 1.0.0 which is currently wreaking havoc on things that arre perfectly fine. Once we get this sorted we'll ask you to rebase your PR and we can take it from there. |
|
||
case $apt::apt_update_frequency { | ||
'always': { | ||
Exec <| title=='apt_update' |> { refreshonly => false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using collectors and overrides, could you just use a variable?
Could you rebase this on master for a sec? That should get the tests going and we can see if this can be merged. |
660c1c1
to
8c0cf9b
Compare
Done. Let me know if you need anything else here. |
Looks like this passes now, thanks! |
@@ -1,10 +1,59 @@ | |||
class apt::update { | |||
include apt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not include apt
here. It is fair to assume that anyone using apt::update
will have already included apt with any necessary parameters before they include this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? I've always been under the impression that it was a good practice to include the parent class when referencing parent class parameters/variables from a subclass. This gives one the ability to have unit tests behave as expected (although that could be solved by pre_condition assertions), and tries to do the right thing if the subclass is invoked before the parent class for some crazy reason. Is there any harm in this? or is it simply a smell issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just a smell issue, it can cause errors at runtime. It's a bit of a Puppet internal thing. The problem is the following. If you have multiple declarations of the same class in this order:
class { 'apt': params }
include ::apt
include ::apt
This will work.
However, if the evaluation order changes and we end up with:
include ::apt
class { 'apt': params }
include ::apt
This breaks. You can't re-include a class once you've initialised it with the class parameter syntax. So just including apt
everywhere can cause trouble.
This almost never happens in real life because everyone is a good citizen and just includes the top class in their catalog.
MY preferred way is to access variables from other classes using a 'normal' lookup, so $::apt::var
. In Puppet 2/3 this has the unfortunate side-effect of the variable becoming undef
if you forgot to include the top class so it might break in weird-ish ways, but it'll break. In Puppet 4 you'll simply get an error stating that $::apt::var
is not defined (turn strict variable checking on in Puppet 3 and you get the same).
@wolfspyre other than my two inline comments, I think all we need is for this to be squashed down to a single commit. Thanks. |
@wolfspyre Are you around to rebase and squash these commits? We'd like to release somewhere next week and have this ship with it. |
8c0cf9b
to
2dd23c5
Compare
- fix spec tests to include osfamily fact - add spec tests to verify current default behavior unimpacted. - manage the update-stamp file in puppet via content rather than a served file. - update custom fact to return -1 if the file doesn't exist - add spec test for custom fact - refactor to use a variable vs a collector/override - document parameters a bit more verbosely - remove empty unconstrained fact - Add osfamily fact to backports tests to facilitate functional tests on non-debian hosts
2dd23c5
to
7a192d7
Compare
@daenney How's that? |
Awesome, thank you. As soon as @mhaskel is back in the land of the conscious we'll get this done. |
@wolfspyre thanks for the contribution! |
This gives you the ability to have puppet run apt-get update a few different ways:
it determines when to trigger the apt-get exec via a combination of an addition of a script in apt.conf.d that touches a file when apt-get update runs, and a custom fact which checks the mtime of that file.
all behavior should be covered by spec tests.
additionally fixed up the spec tests to function when running natively on non-debian systems
Please let me know if there are any changes you'd like to see here in order to merge this