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

Only install InSpec if not installed or version provided #203

Merged
merged 2 commits into from Apr 3, 2017

Conversation

Projects
None yet
4 participants
@adamleff
Collaborator

adamleff commented Mar 22, 2017

To assist with air-gapped installations, the audit cookbook is
being modified to only install InSpec if it's not already installed
or if the user has explicitly stated a version to be installed.

This will allow users to use other methods of installing the gem
in the chef gem path, or take advantage of InSpec being included
in chef client in the future.

Signed-off-by: Adam Leff adam@leff.co

@adamleff adamleff requested review from alexpop and chris-rock Mar 23, 2017

resources/inspec_gem.rb Outdated
action_class do
def install_inspec_gem(gem_version, gem_source)
chef_gem 'inspec' do

This comment has been minimized.

@chris-rock

chris-rock Mar 25, 2017

Collaborator

Do you think it makes sense to remove inspec and train before reinstall? Especially if a version is set, we may get conflicts, since newer versions of the dependencies are not installed.

This comment has been minimized.

@adamleff

adamleff Mar 27, 2017

Collaborator

Yup, since a user may downgrade an image, etc... I think it's fine to do that. I'll write that in.

@@ -17,7 +17,7 @@
# controls inspec gem version to install
# example values: '1.1.0', 'latest'
default['audit']['inspec_version'] = '1.15.0'
default['audit']['inspec_version'] = nil

This comment has been minimized.

@chris-rock

chris-rock Mar 25, 2017

Collaborator

we should document that we use latest by default now

This comment has been minimized.

@adamleff

adamleff Mar 27, 2017

Collaborator

I'll update the README.

default_action :install
action :install do
if !version.nil?

This comment has been minimized.

@chris-rock

chris-rock Mar 25, 2017

Collaborator

That is a great improvement!

resources/inspec_gem.rb Outdated
action :install do
if !version.nil?
converge_by "install InSpec version #{version}" do

This comment has been minimized.

@chris-rock

chris-rock Mar 25, 2017

Collaborator

Should we double-check the existing version? Since it tries to install it every time once a version is set.

This comment has been minimized.

@adamleff

adamleff Mar 27, 2017

Collaborator

I'm partially against that since we should be trusting chef (and chef_gem) to do what we tell it to. But I'm all for setting us up for greater success... to your next comment about removing-then-installing, I think that's a fine idea and will test that.

@@ -11,7 +11,7 @@
# TODO: change once gem resource handles alternate path to `gem` command
describe command('/opt/chef/embedded/bin/gem list --local -a -q inspec | grep \'^inspec\' | awk -F"[()]" \'{printf $2}\'') do

This comment has been minimized.

@chris-rock

chris-rock Mar 25, 2017

Collaborator

we should use the new method once its merged to inspec, see inspec/inspec#1596

@alexpop

Do you want to merge before chef-client 13 is stable with inspec included?

I'm in favor of bumping the major version of the cookbook.

I would also add readme section on how it will work with chef-client 13.

@adamleff

This comment has been minimized.

Collaborator

adamleff commented Mar 27, 2017

@alexpop I wrote this change in a way that we should hopefully not have to wait until Chef 13, but assuming my Chef change is merged, this should only be better in Chef 13, eliminating most of the gem installs.

I'm fine bumping the major version. The only major change IMO is that we'll install the latest InSpec instead of being hard-locked to 1.15.0. If we feel this is a breaking change, I'm fine with it. However, I'd venture to say hard-locking to 1.15 is semi-bug-worthy :)

@chris-rock

This comment has been minimized.

Collaborator

chris-rock commented Mar 27, 2017

I agree with @adamleff Since inspec is still installed by default, it is not a breaking change. This would also mean, that we remove https://github.com/chef-cookbooks/audit/blob/master/recipes/default.rb#L20 once Chef 13 is out and bump the major version.

@adamleff

This comment has been minimized.

Collaborator

adamleff commented Mar 29, 2017

I thought about this some more, and I think the change in install behavior (i.e. don't install if it's already installed) could technically be perceived as a breaking change, so I'm going to leave the change I introduced to bump the major version. Upgrading will only help most of our customers, but if anyone was expecting a version other than 1.15.0 to be installed every time, they're going to be caught off-guard.

Bumping the major version still feels like the right thing to do here. I expect most people aren't pinning this cookbook anyways but I'll post to discourse prior to the release.

Only install InSpec if not installed or version provided
To assist with air-gapped installations, the audit cookbook is
being modified to only install InSpec if it's not already installed
or if the user has explicitly stated a version to be installed.

This will allow users to use other methods of installing the gem
in the chef gem path, or take advantage of InSpec being included
in chef client in the future.

Signed-off-by: Adam Leff <adam@leff.co>

@adamleff adamleff requested a review from arlimus Mar 31, 2017

@smurawski

This comment has been minimized.

Contributor

smurawski commented Mar 31, 2017

This would also resolve #200

@@ -0,0 +1,6 @@
#inspec (1.16.1)

This comment has been minimized.

@alexpop

alexpop Mar 31, 2017

Collaborator

v1.16.1 needs updating or just not mention the version in the comments

use new gem option to test chef gems
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
@chris-rock

Thank you @adamleff for this improvement and the preparation for Chef 13

@chris-rock chris-rock merged commit 6562734 into master Apr 3, 2017

2 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chris-rock chris-rock deleted the adamleff/install-only-if-needed branch Apr 3, 2017

@chris-rock chris-rock removed the in progress label Apr 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment