Add `rpm_dbpath` support to the package resource #1960

jerryaldrichiii · 2017-06-23T06:24:10Z

This adds support for querying a specific RPMDB

Example:

describe package('my_package', rpm_dbpath: '/var/lib/my_rpmdb') do
  it { should be_installed }
end

Fixes #1959

chris-rock

This looks really great @jerryaldrichiii I just added a suggestion for a minor improvement

chris-rock · 2017-06-23T09:44:36Z

lib/resources/package.rb

+      @dbpath = opts.fetch(:rpm_dbpath, nil)
+    end
+
+    def rpm_command(package_name)


that should be a private function of that class

Agreed. I'll make the change now!

chris-rock

Awesome, very quick turn-around @jerryaldrichiii

adamleff

@jerryaldrichiii this is a great contribution, thank you!

My main concern is the one that I raised with you directly regarding returning false if the RPM DB cannot be found. We've already seen similar situations confuse our users (i.e. returning nil file contents simply because the InSpec user didn't have permissions to read, but not giving them that feedback). In this case, we'd return false but if they log in manually and poke around, they'd see it's installed. Ideally, we'd be able to raise an exception and provide the user with more helpful error messaging, but until we implement #1205, our best option is to skip_resource.

Since you can't skip_resource in an object that's not an Inspec::Resource, you need to handle this in the top-level of the resource. Take a look at the missing_requirements pattern in the host resource:

https://github.com/chef/inspec/blob/master/lib/resources/host.rb#L68-L71
https://github.com/chef/inspec/blob/master/lib/resources/host.rb#L136-L140
https://github.com/chef/inspec/blob/master/lib/resources/host.rb#L182-L190

I'd love to see something like that implemented for this before we merge. Then, after #1205 is implemented, we can change that skip_resource into an exception that either skips or fails the test as necessary. I'm happy to pair with you if needed.

jerryaldrichiii · 2017-06-25T05:58:21Z

Great feedback @adamleff and thank you for the reference material!

I added something similar by doing the following:

Create a list for missing_requirements so Inspec::Resources::Package can support multiple reasons for skipping a test
Add a private method to Inspec::Resources::Package to evaluate missing requirements (allowing for multiple reasons to skip the resource)
Add a specific evaluator for @pkgman missing requirements (this sets a pattern for others going forward)
Add a public missing_requirements method to Inspec::Resources::Rpm which allows for more than one argument to be validated going forward
Add a private validate_arguments method Inspec::Resoruces::Rpm to append missing requirements to the missing_requirements list for command arguments
Add a private validate_rpm_dbpath method Inspec::Resoruces::Rpm to append missing requirements to the missing_requirements list specific to the --dbpath command line argument
Modify the Unit tests to match Minitest patterns (no context available like RSpec). NOTE: Technically this describe block should move to another file since it is separate from the Inspec::Resources::Package module

Implementation changed

jerryaldrichiii · 2017-06-26T05:39:52Z

@adamleff, I took your offline feedback and made some modifications.

Now the PkgManagement class has a missing_requirements method that the inheriting classes can use. This removes that defined? logic.

adamleff

@jerryaldrichiii this is much closer to what I'd like to see! I think there's a couple of small clean-up things we can do and then this will be good to go.

adamleff · 2017-06-26T16:06:03Z

lib/resources/package.rb

@@ -44,8 +44,10 @@ def initialize(package_name = nil) # rubocop:disable Metrics/AbcSize
      elsif ['hpux'].include?(os[:family])
        @pkgman = HpuxPkg.new(inspec)
      else
-        return skip_resource 'The `package` resource is not supported on your OS yet.'
+        skip_resource 'The `package` resource is not supported on your OS yet.'


You probably want to leave the return here - there's no reason to continue to evaluating missing requirements if the resource isn't supported on the target OS.

Agreed, will fix!

adamleff · 2017-06-26T16:06:43Z

lib/resources/package.rb

      end
+
+      evaluate_missing_requirements(@pkgman.missing_requirements) if @pkgman


If you leave the return above, you'll always have a populated @pkgman variable, so I'd just remove if @pkgman

Also, since the pkg manager is stored in an instance variable, you probably don't need to pass it. Just let evaluate_missing_requirements take zero arguments and it can refer to @pkgman by itself.

Agreed, will fix!

adamleff · 2017-06-26T16:11:25Z

lib/resources/package.rb

+
+    private
+
+    def evaluate_missing_requirements(missing_requirements)


You want to bail out of a method as quickly as possible if you can. In this case, you build up a skip_msg that you may never use because you don't check for emptiness of the missing requirements until the very last step.

I'd rewrite this method like this:

def evaluate_missing_requirements missing_requirements = @pkgman.missing_requirements return if missing_requirements.empty? skip_resource "The following requirements are not met for this resource: #{missing_requirements.join(', ')}" end

I don't know that you need to compact and uniq unless you're doing something in your code that you think would require that... in which case I'd encourage you to solve that problem since you control the whole kit and kaboodle. 🙂

Agreed, will fix!

adamleff · 2017-06-26T16:13:42Z

lib/resources/package.rb

+
+    private
+
+    def validate_arguments


It feels like this method and the one below it are unnecessary layers of abstraction. Any reason not just to fold these into missing_requirements in the Rpm class? If we collect additional requirements later, we can break those down until separate methods but it feels like a bunch of hopping around between methods to get to a simple "if this parameter isn't nil, check to make sure it's actually a file on disk" check.

Agreed, will fix!

Signed-off-by: Jerry Aldrich III <jerry@chef.io>

jerryaldrichiii · 2017-06-26T17:28:12Z

@adamleff I pushed up my recent changes based on your feedback.

Keep it coming!

adamleff · 2017-06-27T16:45:22Z

Looks great, @jerryaldrichiii - thanks!

chris-rock

Great addition @jerryaldrichiii

chris-rock · 2017-06-28T10:20:39Z

lib/resources/package.rb

+
+    private
+
+    def evaluate_missing_requirements


While I agree that we need to find a pattern for checking missing options, the current approach feels bloated and very complicated. I'd like to replace this with a version that is more lightweight and more flexible to be used in all resources. I do not have an idea right now, therefore this is not a PR blocker.

adamhjk added the in progress label Jun 23, 2017

jerryaldrichiii requested review from nsdavidson and removed request for nsdavidson June 23, 2017 08:06

chris-rock reviewed Jun 23, 2017

View reviewed changes

chris-rock previously approved these changes Jun 23, 2017

View reviewed changes

chris-rock requested a review from adamleff June 23, 2017 10:19

adamleff suggested changes Jun 23, 2017

View reviewed changes

adamleff suggested changes Jun 26, 2017

View reviewed changes

Add rpm_dbpath support to the package resource

b2c758a

Signed-off-by: Jerry Aldrich III <jerry@chef.io>

adamleff approved these changes Jun 27, 2017

View reviewed changes

adamleff requested review from chris-rock and arlimus June 27, 2017 16:45

adamleff added the Type: Enhancement Improves an existing feature label Jun 27, 2017

chris-rock approved these changes Jun 28, 2017

View reviewed changes

chris-rock merged commit cc6f1e9 into inspec:master Jun 28, 2017

chris-rock removed the in progress label Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rpm_dbpath` support to the package resource #1960

Add `rpm_dbpath` support to the package resource #1960

jerryaldrichiii commented Jun 23, 2017 •

edited by adamleff

Loading

chris-rock left a comment

chris-rock Jun 23, 2017

jerryaldrichiii Jun 23, 2017

chris-rock left a comment

adamleff left a comment

jerryaldrichiii commented Jun 25, 2017 •

edited

Loading

jerryaldrichiii commented Jun 26, 2017

adamleff left a comment

adamleff Jun 26, 2017

jerryaldrichiii Jun 26, 2017

adamleff Jun 26, 2017

adamleff Jun 26, 2017

jerryaldrichiii Jun 26, 2017

adamleff Jun 26, 2017

jerryaldrichiii Jun 26, 2017

adamleff Jun 26, 2017

jerryaldrichiii Jun 26, 2017

jerryaldrichiii commented Jun 26, 2017

adamleff commented Jun 27, 2017

chris-rock left a comment

chris-rock Jun 28, 2017

		end

		evaluate_missing_requirements(@pkgman.missing_requirements) if @pkgman


		private

		def evaluate_missing_requirements(missing_requirements)

Add rpm_dbpath support to the package resource #1960

Add rpm_dbpath support to the package resource #1960

Conversation

jerryaldrichiii commented Jun 23, 2017 • edited by adamleff Loading

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

jerryaldrichiii commented Jun 25, 2017 • edited Loading

jerryaldrichiii commented Jun 26, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryaldrichiii commented Jun 26, 2017

adamleff commented Jun 27, 2017

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `rpm_dbpath` support to the package resource #1960

Add `rpm_dbpath` support to the package resource #1960

jerryaldrichiii commented Jun 23, 2017 •

edited by adamleff

Loading

jerryaldrichiii commented Jun 25, 2017 •

edited

Loading