-
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
Packages resource support for RedHat #1487
Conversation
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.
Looks great @alexpop Just some minor requests
test/helper.rb
Outdated
"dpkg-query -W -f='${db:Status-Abbrev} ${Package} ${Version}\\n'" => cmd.call('dpkg-query-W'), | ||
# rpm query all packages | ||
"rpm -qa --queryformat '%{NAME} %{VERSION}-%{RELEASE}\\n'" => cmd.call('rpm-qa-queryformat'), |
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.
Do we need multiple spaces between name and version? better '%{NAME} %{VERSION}-%{RELEASE}\\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.
I can see why he's doing it... but is there another character that is guaranteed to not be part of a RPM package name and version string that we can use as the delimiter instead? If not, I'm fine with the two spaces since that character combo should never be in a name or version.
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.
Exactly, I played the paranoid here to avoid splitting on a space in package name or version. I'm fine with two spaces as well
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.
I am okay with taking that approach right now. We should document it in code to make other developers aware of the intended use of the whitespace
lib/resources/packages.rb
Outdated
os = inspec.os | ||
if os.debian? | ||
@pkgs = Debs.new(inspec) | ||
elsif %w{redhat suse amazon fedora}.include?(os[:family]) |
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 we use os.redhat?
? Otherwise this will not work on scientific or oracle linux
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.
I used the same checks as the existing package
resource:
elsif %w{redhat suse amazon fedora}.include?(os[:family])
@pkgman = Rpm.new(inspec)
Noticed that os.redhat?
is not matching Fedora:
inspec> os.redhat?
=> false
inspec> os.params
=> {:name=>"fedora", :family=>"fedora", :release=>"25", :arch=>"x86_64"}
If we go with os.redhat?
, will need something like:
lib/resources/grub_conf.rb
35: if os.redhat? || os[:name] == 'fedora'
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.
Thank you @alexpop yes os.redhat? || os[:name] == 'fedora'
sounds right and we should apply this to the package
resource as well.
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.
What about these?
inspec> os.redhat?
=> false
inspec> os.params
=> {:name=>"amazon", :family=>"amazon", :release=>"2016.09", :arch=>"x86_64"}
inspec> os.redhat?
=> false
inspec> os.params
=> {:name=>"suse", :family=>"suse", :release=>"11.4", :arch=>"x86_64"}
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.
As far as I know, Suse is using YAST and zypper. Lets add support for those systems seperately.
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.
Works for amazon linux:
[root@ip-172-31-12-168 ~]# rpm -qa --queryformat '%{NAME} %{VERSION}-%{RELEASE}\n'
sysvinit 2.87-6.dsf.15.amzn1
kbd-misc 1.15-11.4.amzn1
rubygem20-json 1.8.3-1.51.amzn1
glibc-common 2.17-106.168.amzn1
and SUSE Linux Enterprise Server 12:
ip-172-31-10-176:~ # rpm -qa --queryformat '%{NAME} %{VERSION}-%{RELEASE}\n'
nfs-client 1.3.0-26.3
sle-module-legacy-release 12-6.2
python-oauthlib 0.7.2-2.3
libwbclient0 4.4.2-29.4
procps 3.3.9-7.1
python-colorama 0.2.7-1.9
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.
@alexpop I agree with you. Lets add support for those systems, too
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.
in this case, do I make the condition:
if os.redhat? || %w{suse amazon fedora}.include?(os[:family])
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.
Sounds resonable
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.
I'd love it if we could align a bit more with Chef/Ohai in terms of platform / os / platform_family so it's not confusing for our users. Not something to address in this PR, but perhaps something we can consider for a future enhancement. Curious as to your thoughts.
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.
Looks fine to me. I echo the feedback from @chris-rock as well.
Fix dpkg trimming of first line Signed-off-by: Alex Pop <apop@chef.io>
…fileds delimited Signed-off-by: Alex Pop <apop@chef.io>
63d18f1
to
88975bf
Compare
Gents, updated and rebased based on the discussions above. |
processes
support for RedHat is needed to support the new CIS SCAP profiles.Also fixes dpkg trimming of first line.