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

Fix apt_has_updates fact not parsing apt-check output correctly #403

Merged
merged 1 commit into from
Jan 20, 2015

Conversation

WolverineFan
Copy link

The /usr/lib/update-notifier/apt-check script returns its output
to STDERR but a recent change to the script redirects STDERR to
/dev/null. This will cause the array to always be empty.

Combined with that problem, while we were checking for the result
being nil, we never checked for an invalid array. As a result,
the apt_has_updates was always true and the apt_updates and
apt_security_updates facts were trying to read from an empty array
and failing.

@WolverineFan WolverineFan force-pushed the fix_apt_updates_facts branch from 07b3fe9 to 8c8a4b9 Compare January 8, 2015 23:38
@WolverineFan
Copy link
Author

Added 2 additional tests to make sure the kind of error output that caused /dev/null to be added in the first place is handled gracefully.

@WolverineFan
Copy link
Author

This resolves both #382 and #393 but with working test coverage and additional error checking

@WolverineFan
Copy link
Author

Sorry for the cross-pull talk. This seemed to be a fairly major problem because it is completely broken right now. Thanks for merging the other one, and I'll wait patiently for this one...

@WolverineFan WolverineFan force-pushed the fix_apt_updates_facts branch from 8c8a4b9 to cf7735f Compare January 15, 2015 04:04
@hunner
Copy link

hunner commented Jan 16, 2015

@WolverineFan Thanks for submitting this.

Could you please update the commit so that the unit tests will pass? Thanks.

@WolverineFan
Copy link
Author

The test failures aren't due to this commit. The CI server is having problems (or it was anyway). I can upload it again to see if the CI server is functional again.

@WolverineFan
Copy link
Author

Whoops! Apparently my last rebase was of the wrong branch! I'll fix that right now

@WolverineFan WolverineFan force-pushed the fix_apt_updates_facts branch from cf7735f to 7ad7669 Compare January 16, 2015 22:23
The /usr/lib/update-notifier/apt-check script returns its output
to STDERR but a recent change to the script redirects STDERR to
/dev/null.  This will cause the array to always be empty.

Combined with that problem, while we were checking for the result
being nil, we never checked for an invalid array.  As a result,
the apt_has_updates was always true and the apt_updates and
apt_security_updates facts were trying to read from an empty array
and failing.
@WolverineFan WolverineFan force-pushed the fix_apt_updates_facts branch from 7ad7669 to e7fee16 Compare January 16, 2015 22:46
@WolverineFan
Copy link
Author

Looks like @cyberious found and fixed the problem in #410. I'm guessing it's a new version of Facter on the CI server returning different results. Anyway, I pulled in his test update (since it's more related to this patch than to his). All green now.

@cyberious
Copy link

Yeah I noticed that on Travis-CI it was using facter 1.7.6 but locally it was pulling down facter 2.x

@daenney
Copy link

daenney commented Jan 20, 2015

I'm gonna click the merge button, we can see what breaks next. @mhaskel We need to revisit these when we go for 2.0.

daenney added a commit that referenced this pull request Jan 20, 2015
Fix apt_has_updates fact not parsing apt-check output correctly
@daenney daenney merged commit 6b23f3c into puppetlabs:master Jan 20, 2015
@WolverineFan WolverineFan deleted the fix_apt_updates_facts branch January 22, 2015 23:14
@LukasAud LukasAud added the bugfix label Jun 6, 2023
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