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

Enable Inspec caching #297

Merged
merged 3 commits into from
Dec 6, 2017
Merged

Enable Inspec caching #297

merged 3 commits into from
Dec 6, 2017

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Dec 5, 2017

Description

Add inspec caching attribute which defaults to true. This enables the new inspec caching when used by the Audit cookbook.

Issues Resolved

Fixes #296

Signed-off-by: Jared Quick jquick@chef.io

Signed-off-by: Jared Quick <jquick@chef.io>
@jquick jquick requested a review from a team December 5, 2017 18:16
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Great first pass. We should also modify the metadata.rb file and bump the major version. Because of the new InSpec constraint and a change in default behavior, this will require a major version bump.

@@ -124,6 +129,7 @@ def get_opts(format, quiet, attributes)
'format' => format,
'output' => output,
'logger' => Chef::Log, # Use chef-client log level for inspec run,
:backend_cache => node['audit']['inspec_backend_cache'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to use the new hash syntax instead of hash-rockets?

backend_cache: node['audit']['inspec_backend_cache']

... the stuff above this apparently needs to be strings (but maybe it doesn't, we should probably check on that as some point), but if this key is expected to be a symbol, we should be consistent. 🙂

@@ -38,6 +38,11 @@ def report
# load inspec, supermarket bundle and compliance bundle
load_needed_dependencies

# check if we have a valid version for backend caching
if node['audit']['inspec_backend_cache'] && (Gem::Version.new(::Inspec::VERSION) < Gem::Version.new('1.47.0'))
Copy link
Contributor

Choose a reason for hiding this comment

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

We're now checking for a specific version constraint in two different places within this file... here, and:

https://github.com/chef-cookbooks/audit/blob/master/files/default/handler/audit_report.rb#L134-L137

I think we should move the MIN_INSPEC_VERSION one to be right above this, and out of the #call method. Or, possibly better yet, extract both to a validate_inspec_version! method and call it here in the #report method in place of this stanza. What do you think?

@@ -38,6 +38,11 @@ def report
# load inspec, supermarket bundle and compliance bundle
load_needed_dependencies

# check if we have a valid version for backend caching
if node['audit']['inspec_backend_cache'] && (Gem::Version.new(::Inspec::VERSION) < Gem::Version.new('1.47.0'))
Chef::Log.warn 'inspec_backend_cache requires Inspec version >= 1.47.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

InSpec - capital S, please :)

let(:mynode) { Chef::Node.new }

before :each do
mynode.default['audit']['inspec_backend_cache'] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test that assures that opts[:backend_cache] gets set to false if the node attribute calls for it? I hate to only have a test for the default path.

metadata.rb Outdated
@@ -5,7 +5,7 @@
license 'Apache-2.0'
description 'Allows for fetching and executing compliance profiles, and reporting its results'
long_description IO.read(File.join(File.dirname(__FILE__), 'README.md'))
version '5.0.4'
version '5.0.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

Gotta be a major bump for a change like this.

@jquick jquick force-pushed the jq/enable_inspec_caching branch from 0bea7c9 to 34c4640 Compare December 5, 2017 18:50
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick jquick force-pushed the jq/enable_inspec_caching branch from 34c4640 to dda288b Compare December 5, 2017 18:59
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick
Copy link
Contributor Author

jquick commented Dec 5, 2017

These issues have been resolved.

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Nice work, @jquick. Looks good to me.

Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Fantastic to see the new caching mechanism, thank you @jquick !!

@arlimus arlimus merged commit 3434c9e into master Dec 6, 2017
@arlimus arlimus deleted the jq/enable_inspec_caching branch December 6, 2017 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants