-
Notifications
You must be signed in to change notification settings - Fork 682
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
EL package resource improvements: catch missing newlines & add release info #248
Conversation
Hi @troyready Thanks for this PR. You raised a great question with this PR. I am all in on delivering the release version of a package. Currently I am having some concerns with having all We could just always return the full version like In addition it would be super helpful if you could update the tests as well here: https://github.com/chef/inspec/blob/master/test/unit/resources/package_test.rb |
@chris-rock yeah, that totally works for me. I was originally just being overly conservative, but I agree that it's more logical to make the platforms consistent and make the version data actually usable for common use cases. I've squashed the commits down accordingly and it should be ready to merge now. While checking the documentation to see if any edits would need to be made, I came across a small typo that I've fixed here as well. |
@troyready This is great. Thanks for the effort. I attached my comments to your code. |
@@ -98,10 +98,22 @@ def info(package_name) | |||
assignment_re: /^\s*([^:]*?)\s*:\s*(.*?)\s*$/, | |||
multiple_values: false, | |||
).params | |||
# On some (all?) systems, the linebreak before the vendor line is missing | |||
if params['Version'] =~ /\s*Vendor:/ | |||
v = params['Version'].split(' ')[0] |
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.
Can you provide an example where you get this? I would like to add this in our integration tests. I need to double-check if we can fix the problem before we start parsing the data. This would allow us to use just params['Version']
and params['Release']
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.
@chris-rock Sure: the sudo
example in my first comment in this pull request is the exact output from a RHEL 6.4 system. Here's another example from a CentOS 5.4 system:
Name : sudo Relocations: (not relocatable)\nVersion : 1.6.9p17 Vendor: CentOS\nRelease : 5.el5 Build Date: Sat 19 Sep 2009 07:02:06 PM PDT\nInstall Date: Mon 10 Jan 2011 03:19:05 AM PST Build Host: builder16.centos.org\nGroup : Applications/System Source RPM: sudo-1.6.9p17-5.el5.src.rpm\nSize : 467528 License: BSD\nSignature : DSA/SHA1, Sat 19 Sep 2009 08:55:01 PM PDT, Key ID a8a447dce8562897\nURL : http://www.courtesan.com/sudo/\nSummary : Allows restricted root access for specified users.\nDescription :\nSudo (superuser do) allows a system administrator to give certain\nusers (or groups of users) the ability to run some (or all) commands\nas root while logging all commands and arguments. Sudo operates on a\nper-command basis. It is not a replacement for the shell. Features\ninclude: the ability to restrict what commands a user may run on a\nper-host basis, copious logging of each command (providing a clear\naudit trail of who did what), a configurable timeout of the sudo\ncommand, and the ability to use the same configuration file (sudoers)\non many different machines.\n
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.
Thanks for pointing this out. It helps for narrowing this down.
@troyready I am happy to merge the latest PR. Would it be possible that you rebase it to the latest master? |
@chris-rock updated for the new wrlinux test & rebased; should be ready to merge now. |
@troyready Thanks for the support and the fixes. I know it is annoying, but unfortunately we just released a new version and I am not able to rebase your branch. If you rebase this once more, I merge it right now. |
Package release info (e.g. '19.el7') is often required to determine if a system has been properly patched. Lines like the following from rpm are messing up the version returned by the package resource: "...\nVersion : 1.8.6p3 Vendor: Red Hat, Inc.\n..." Correcting this with a new conditional check.
@chris-rock sure thing -- rebased, should be ready again now |
EL package resource improvements: catch missing newlines & add release info
3 commits here -- happy to adjust / squash / toss as appropriate:
c8bebf9: I'm seeing output like the following from the rpm command:
This is returning version strings like
1.8.6p3 Vendor: Red Hat, Inc.
. I've put in a simple check and correction for this.46a68de: Added a
release
method to get EL package release info in the DSLf429ae5: Added a
version_and_release
method so a simpleits('version_and_release') { should match 'foov1-el61.2' }
is possible. Let me know if there's a better way to do this or if the naming should be different or whatever.Once the technical particulars are worked out I can add some info to the documentation too.