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

Class for managing unattended-upgrades #153

Merged
merged 8 commits into from
Sep 16, 2013

Conversation

philipcohoe
Copy link

unattended-upgrades is an ubuntu package that can automatically install upgradeable packages. This class installs the unattended-upgrades package, and manages the 50unattended-upgrades and 10periodic configuration files.

@dcarley
Copy link

dcarley commented Sep 10, 2013

This looks good. Would really benefit from some spec tests though.

@apenney
Copy link

apenney commented Sep 13, 2013

As @dcarley said, only thing blocking this is spec tests!

@blkperl
Copy link

blkperl commented Sep 15, 2013

@apenney Tests were added needs your review.

@dcarley
Copy link

dcarley commented Sep 16, 2013

It would be good to test the default values of the params too. I'll add some more contexts and link you to a branch that you can pull from, if you don't mind?

Reduce some duplication of long file paths.
For params which have any logic embedded in the template:

- origins
- blacklist
- mail_to
- dl_limit
To ensure that the default configs from the package are always overwritten
within a single Puppet run.
@dcarley
Copy link

dcarley commented Sep 16, 2013

I've added some additional tests and tweaks here:

https://github.com/dcarley/puppetlabs-apt/compare/pr153-unattended_upgrades_patches

What do you think? Want to pull them into this PR?

@philipcohoe
Copy link
Author

Looks good! The commits have been pulled in. Thanks for your help!

@apenney
Copy link

apenney commented Sep 16, 2013

@dcarley You happy with the current state of things? I'm looking to merge. :)

@dcarley
Copy link

dcarley commented Sep 16, 2013

Yep, LGTM. Thanks @drbop

As a side note - I did think about whether the options would be better exposed as a single hash. Because it can be tricky to maintain classes with as many params as this. But given the complexity of the key names and that I don't know how apt will behave if some are missing, I think what we have is the right approach.

apenney pushed a commit that referenced this pull request Sep 16, 2013
Class for managing unattended-upgrades
@apenney apenney merged commit ef781c7 into puppetlabs:master Sep 16, 2013
@philipcohoe philipcohoe deleted the drbop_unattended_upgrades branch September 16, 2013 23:33
@dol
Copy link

dol commented Sep 20, 2013

I love this pull request. It fixes one of the last missing pieces.

But it's strange to define a parameter like 'email_to' with the default value of "NONE". undef would work the same and is more common and understood.
I also would change the default value for 'update', 'ensure', 'download', 'upgrade' to use a boolean, instead of '1' and '0'. It's not very intuitive so set a string of '0' or '1'. If you're familiar with the internals of unattended_upgrades config files it makes sense. But it's a puppet class.

@dcarley
Copy link

dcarley commented Sep 20, 2013

I'm OK with 'NONE'. It's a pretty common idiom because Puppet makes it so hard to differentiate between something that may be undef, 'undefined', :undefined, '', or maybe even nil.

I'm in two minds about the integer values. It is a bit weird and we can't easily do validation. But on the other hand, you probably need to lookup/understand the underlying internals to pick new values, and there are other non-boolean options in the same file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants