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

Add the InSpec backend_cache option #155

Merged
merged 2 commits into from
Dec 12, 2017
Merged

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Dec 7, 2017

Fixes #154

This PR introduces the new backend_cache option. This flag will default to true if not defined in your kitchen.yml.

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

@jquick jquick requested a review from a team as a code owner December 7, 2017 15:29
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick jquick force-pushed the jq/add_backend_cache_option branch from 53c401c to 918ddef Compare December 7, 2017 15:40
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.

This is a great change, I just have an issue with the default behavior choice.

backend_cache_msg = "backend_cache requires InSpec version >= 1.47.0"
logger.warn backend_cache_msg if Gem::Version.new(::Inspec::VERSION) < Gem::Version.new("1.47.0")
end
runner_options[:backend_cache] = config[:backend_cache].nil? ? true : config[: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.

This places the default behavior of the caching in two places - InSpec, and kitchen-inspec. And technically, this is a breaking change by defaulting it to true. We can get away with that since we're still technically 0.x SemVer version, but I still think it's the wrong choice.

I think it would be better to let this logic live with InSpec until there's a specific use case where we have to deviate the default behavior. Therefore, I'd change this to be:

if config[:backend_cache]
  backend_cache_msg = "backend_cache requires InSpec version >= 1.47.0"
  logger.warn backend_cache_msg if Gem::Version.new(::Inspec::VERSION) < Gem::Version.new("1.47.0")

  runner_options[:backend_cache] = config[:backend_cache]
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After talking it over with Adam we decided to have it default to false here. This is due to how we are grabbing the internals of inspec without going through the normal default option initiation.

expect(config.to_hash).to include(backend_cache: false)
end

it "backend_cache option defaults to true" do
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will need to be updated if you take my advice, obviously :)

Signed-off-by: Jared Quick <jquick@chef.io>
@jquick
Copy link
Contributor Author

jquick commented Dec 12, 2017

@adamleff - This is ready for another look.

Copy link
Collaborator

@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.

Great addition @jquick thank you!

@arlimus arlimus merged commit 485fa17 into master Dec 12, 2017
@arlimus arlimus deleted the jq/add_backend_cache_option branch December 12, 2017 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants