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

add facts showing available updates #319

Merged
merged 1 commit into from
Jun 27, 2014
Merged

Conversation

damoxc
Copy link

@damoxc damoxc commented Jun 26, 2014

Making use of the apt-check command from the 'update-notifier-common'
package (if available) display the number of available updates, number of
security updates as well as the update package names.

@@ -0,0 +1,11 @@
Facter.add("apt_update_packages") do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer apt_package_updates, that reads more natural and is more in line with apt_security_updates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easily changed, will do so in another commit.

@daenney
Copy link

daenney commented Jun 26, 2014

Looks good to me now! @mhaskel What do you think, facts okay?

@underscorgan
Copy link

@daenney @damoxc I'm good with there being facts, but I'd like to see some docs and at least unit testing, I suspect the facts can be tested thoroughly enough with unit tests to not need acceptance tests.

@damoxc
Copy link
Author

damoxc commented Jun 26, 2014

Are you able to point me in the direction of some examples of docs / unit tests for the facts please?

@daenney
Copy link

daenney commented Jun 26, 2014

I think with docs @mhaskel simply meant an update to the README that we ship these facts, what they mean and mention the difference in behaviour between Facter 1.x and 2.x.

As far as the tests go, unfortunately just about any kind of documentation on that subject is missing which is why I didn't ask for them. The best I can do right now is point you at http://unethicalblogger.com/2014/03/01/testing-custom-facts-with-rspec.html and https://github.com/puppetlabs/facter/tree/master/spec/unit

@adrienthebo might be able to be of some help on the matter.

@underscorgan
Copy link

@damoxc @daenney correct, for docs I was just looking for README updates.

@damoxc
Copy link
Author

damoxc commented Jun 26, 2014

Is the doc update at https://github.com/cloudbuy/puppetlabs-apt/tree/docs-update#facts OK? Just looking at sorting the unit tests now.

@underscorgan
Copy link

@damoxc Could you make the docs H3 instead of H2 (### instead of ##). As it is it makes it look like some later examples are nested under facts which is a bit confusing.

@damoxc
Copy link
Author

damoxc commented Jun 26, 2014

Done, and unit tests added (hopefully correct unit tests!)

@daenney
Copy link

daenney commented Jun 26, 2014

It's looking good.

However, by now we're up to 6 commits for a single feature. In order to keep the history nice and tidy I would prefer it if this could be squashed into a single commit as it essentially is one change/feature.

@damoxc
Copy link
Author

damoxc commented Jun 26, 2014

That OK?

@underscorgan
Copy link

As long as it passes travis-ci this should be good to merge!

@daenney
Copy link

daenney commented Jun 26, 2014

@damoxc Excellent, thank you! It's very nice to work with people who are this responsive to their PR's.

Travis seems to be a bit backed up right now so it might take a while before this goes green.

@damoxc
Copy link
Author

damoxc commented Jun 26, 2014

My pleasure, thanks for all the guidance. First time using rspec really. Just hope it does go green!

@daenney
Copy link

daenney commented Jun 27, 2014

@damoxc So Travis broke on Puppet 3.4.0 releases:

  1) apt_package_updates fact on Debian based distro with Facter 2.0+ should == ["puppet-common", "linux-generic", "linux-image-generic"]
     Failure/Error: it { should == ['puppet-common', 'linux-generic', 'linux-image-generic'] }
       expected: ["puppet-common", "linux-generic", "linux-image-generic"]
            got: "puppet-common,linux-generic,linux-image-generic" (using ==)
     # ./spec/unit/facter/apt_package_updates_spec.rb:21

It looks like something went wrong there, your spec is expecting an array but we get the stringified version.

I'd try and install Puppet 3.4.0 locally and run the specs with that, see what happens.

@damoxc
Copy link
Author

damoxc commented Jun 27, 2014

Just having my morning caffeine requirement then going to tackle it.

@damoxc
Copy link
Author

damoxc commented Jun 27, 2014

Looks like the Puppet 3.4 gem pulls in Facter 1.7 so in the first test case where I wasn't explicitly setting the Facter version it would see Facter 1.7.6 in both test cases. I've decided to just merge the 2 test cases since one of the environments covers both code paths. Seem OK?

Making use of the apt-check command from the 'update-notifier-common'
package (if available) display the number of available updates, number of
security updates as well as the update package names.
@damoxc
Copy link
Author

damoxc commented Jun 27, 2014

Just noticed there was some funky spacing in the unit tests so fixed that and re-pushed.

daenney pushed a commit that referenced this pull request Jun 27, 2014
add facts showing available updates
@daenney daenney merged commit 943be40 into puppetlabs:master Jun 27, 2014
@daenney
Copy link

daenney commented Jun 27, 2014

Thank you for sticking with us and Travis!

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.

5 participants