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

Make apt_updates facts use /usr/bin/apt-get. #581

Merged
merged 1 commit into from
Apr 7, 2016
Merged

Make apt_updates facts use /usr/bin/apt-get. #581

merged 1 commit into from
Apr 7, 2016

Conversation

robinelfrink
Copy link

apt_updates.rb currently uses /usr/lib/update-notifier/apt-check to check for available updates.
This is part of update-notifier-common, which is not available on all systems.

@daenney
Copy link

daenney commented Jan 15, 2016

I'm OK with this, but it's broken all the tests around it.

@robinelfrink
Copy link
Author

Yes, I noticed. Never occured to me to change those too. Will update the request.

@robinelfrink
Copy link
Author

Tests have been changed.

@daenney
Copy link

daenney commented Feb 11, 2016

Perfect, thanks. Could you squash and rebase this so we can merge?

/usr/lib/update-notifier/apt-check is not available on all systems,
but /usr/bin/apt-get is.
@robinelfrink
Copy link
Author

Sqashed and rebased.

@daenney
Copy link

daenney commented Feb 14, 2016

Thanks!

@daenney
Copy link

daenney commented Feb 14, 2016

@tphoney @bmjen Thoughts? Looks good to me but since we stub so much in tests I'd like to be a bit cautious about just merging. Does anyone have a machine where we can test the current and suggested implementation and ensure we end up with the same output on Facter 2 and 3?

/ Debian-Security:/,
/ Ubuntu[^\s]+-security /,
/ gNewSense[^\s]+-security /
]
Copy link

Choose a reason for hiding this comment

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

I'm concerned about security_matches not working correctly on all target distributions such as Linux Mint etc.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, security_matches will not match every current and future security repository. But the current list of matches is a representation of what was found in /usr/lib/update-notifier/apt-check on Ubuntu. Mint has the same file with the same checks as far as I can see.

Copy link

Choose a reason for hiding this comment

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

Thanks, that's good to know!

@bmjen
Copy link

bmjen commented Feb 16, 2016

@daenney PCCI is currently on the fritz. Once that's back up I will take a look at the results of the acceptance tests run.

@daenney
Copy link

daenney commented Feb 16, 2016

@bmjen Doesn't PL have its own infra for this? 😛

@daenney
Copy link

daenney commented Mar 1, 2016

Nudge @bmjen.

@bmjen
Copy link

bmjen commented Mar 1, 2016

@daenney sorry. Forgot about this. Acceptance tests pass on trusty for this PR. Still need to investigate the facter question you posed.

@jonnytdevops
Copy link

@bmjen Do you think we can merge this now? Is Facter 2 a major concern for us at present? Perhaps it is if Puppet 3.8 uses it? Thanks

@bmjen
Copy link

bmjen commented Mar 24, 2016

@jonnytpuppet @daenney I think we should have a ticket for one of us to run this PR locally with facter 2.x and 3.x to make sure we don't break compatibility before merging.

@daenney
Copy link

daenney commented Mar 24, 2016

MODULES-3198 it is.

@hunner hunner merged commit 00a2107 into puppetlabs:master Apr 7, 2016
@daenney
Copy link

daenney commented Apr 7, 2016

Thank you @hunner for checking the specs and thanks @robinelfrink for the contribution!

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