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

Ensure min version of inspec is used #237

Merged
merged 2 commits into from
Jul 3, 2017
Merged

Conversation

alexpop
Copy link
Contributor

@alexpop alexpop commented Jun 8, 2017

With the cookbook no longer specifying an inspec_version in attributes, it's important to ensure a compatible inspec is used.

I'm tempted to bump the required version to at least 1.25.0 because of this: inspec/inspec#1816

or should I make it 1.26 or 1.27?

@alexpop alexpop self-assigned this Jun 8, 2017
@alexpop alexpop requested review from arlimus and adamleff June 8, 2017 11:42
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.

I think there's a larger issue here... PR #235 highlights it. We made backward-incompatible changes to InSpec, and now the audit cookbook isn't backward compatible either. We're going to need to get #235 in ASAP and then decide how best to proceed.

I also think this type of change leads to a bad UX given that we made changes to the audit cookbook to not install InSpec if it's already installed. So if a user is happily running with a particular version of InSpec (that either they installed, or we provided via the Chef Omnibus installation), and they pull in a newer audit cookbook, they're just going to break.

We may actually want to get on a Zoom and discuss this as a larger issue.

Chef::Log.info "Initialize InSpec #{::Inspec::VERSION}"
Chef::Log.info "Using InSpec #{::Inspec::VERSION}"

if Gem::Version.new(Inspec::VERSION) < Gem::Version.new('1.24.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd store this in a constant somewhere near the top since the version string is used in two places: the comparison and the log message. That way we only have to change it in one location.


if Gem::Version.new(Inspec::VERSION) < Gem::Version.new('1.24.0')
Chef::Log.error "This audit cookbook version requires InSpec 1.24.0 or newer, aborting compliance scan..."
return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd honestly raise this as an exception. People will miss the log message and wonder why they're losing data.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on @adamleff proposal. It is better to fail early

@chris-rock
Copy link
Contributor

With Chef 13 we need to work with the version that is shipped with Chef. We may need to shim parts if we really require it here. Otherwise we need to raise the requirement for Chef client version

@chris-rock
Copy link
Contributor

Just double-checked all inspec versions in Chef 13:

chef 13.0.113: InSpec 1.19.1
chef 13.0.118: InSpec 1.19.1
chef 13.1.31: InSpec 1.25.1

Therefore lets use the minimum version of 1.25.1. This allows us to establish a clear path for customers:

  • chef 12 + InSpec 1.25.1
  • chef 13.1.31

@chris-rock
Copy link
Contributor

I am going to bump the requirement to 1.25.1 to ensure everything works with that version. We may need to add some shims later.

@chris-rock chris-rock added this to the 4.1.0 milestone Jun 29, 2017
@chris-rock chris-rock force-pushed the ap/inspec-version-check branch 2 times, most recently from b5347fd to 10fd099 Compare July 3, 2017 07:03
Signed-off-by: Alex Pop <apop@chef.io>
@chris-rock chris-rock force-pushed the ap/inspec-version-check branch from 10fd099 to 61d37f6 Compare July 3, 2017 07:04
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
@chris-rock chris-rock force-pushed the ap/inspec-version-check branch from 89f277c to b1789a0 Compare July 3, 2017 07:18
@chris-rock
Copy link
Contributor

Thank you @alexpop and @adamleff

@chris-rock chris-rock merged commit 8902359 into master Jul 3, 2017
@chris-rock chris-rock deleted the ap/inspec-version-check branch July 3, 2017 07:35
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.

3 participants