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

Detect install type via install path #4856

Merged
merged 7 commits into from Feb 10, 2020
Merged

Conversation

clintoncwolfe
Copy link
Contributor

Signed-off-by: Clinton Wolfe clintoncwolfe@gmail.com

Description

Provides several new predicates and one classification method that guess at how InSpec was installed. Possible values include "chef-workstation", "chefdk", "docker", "habitat", "omnibus", "rubygem", and "source".

A unit test file is provided. This would be very impractical to test functionally, but I got the values in the test file (and in the implementation) by manually running each of the different scenarios. That said, I'm sure this could be easily fooled.

Related Issue

Closes #4585

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New content (non-breaking change)
  • Breaking change (a content change which would break existing functionality or processes)

Checklist:

  • I have read the CONTRIBUTING document.

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
test/unit/utils/install_context_test.rb Outdated Show resolved Hide resolved
def source_install?
# These are a couple of examples of dirs removed during packaging
%w{habitat test}.all? do |devdir|
Dir.exist?("#{src_root}/#{devdir}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly didn't know this method existed... Usually we use File.directory?.

test/unit/utils/install_context_test.rb Outdated Show resolved Hide resolved
lib/inspec/utils/install_context.rb Outdated Show resolved Hide resolved
lib/inspec/utils/install_context.rb Show resolved Hide resolved
lib/inspec/globals.rb Outdated Show resolved Hide resolved
end

def rubygem_install?
!!src_root.match(%r{gems/inspec-\d+\.\d+\.\d+})
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm... vendor for bundler stuff? I'm thinking of the caching we use on BK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may need this explained to me.

When vendored, Bundler makes a directory vendor/cache full of gems - but the gems are not yet installed/expanded. Presumably when Bundler / Rubygems activates a gem it then installs the gem somewhere? So... wouldn't the above regex work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think when vendored, they're installed under vendor as well... Our BK "bundle install" now takes <20s because we've untarred the vendor directory from cache.

test/unit/utils/install_context_test.rb Outdated Show resolved Hide resolved
lib/inspec/utils/install_context.rb Show resolved Hide resolved
end

def chef_workstation_install?
!!(src_root.start_with?("/opt/chef-workstation") || src_root.start_with?("C:/opscode/chef-workstation"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really need true/false results, do we? Is this just for testing?

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Copy link
Contributor

@miah miah left a comment

Choose a reason for hiding this comment

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

I left a few comments inline. I'm 99% approve on this, #docker_install? is my only worry.

…v check for docker mode

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

should_be_true = ["#{test_expected_to_be_true}_install?".tr("-", "_").to_sym]
should_be_true += [:rubygem_install?] if also_rubygem
should_be_true.each { |m| expect(Inspec.method(m).call).must_equal true }
should_be_true.each { |m| expect(Inspec.send(m)).must_equal true }
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you debug when all it says isExpected true, got false ?

Compare to Expected Inspec to be docker (these names aren't exactly adjectives... so it's a little clunky)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I described above, your suggestion is not an equivalent test. We are testing predicates with expected boolean results, not referencing a method on a module.

When a test fails, it's easy to see which one - I've forced a failure here:

Inspec::InstallContextHelpers::when it looks like a Habitat installation#test_0001_should properly detect a habitat install [/Users/cwolfe/sandbox/inspec/inspec-telemetry-2/test/unit/utils/install_context_test.rb:53]:
Expected: false
  Actual: true

I do wish I had a better expectation - perhaps one that could take a custom message, in which case I could provide user context. But this is not worthwhile at this point; it's a simple matter to add a pry statement should it ever break.

lib/inspec/utils/install_context.rb Outdated Show resolved Hide resolved
test/unit/utils/install_context_test.rb Show resolved Hide resolved
test/unit/utils/install_context_test.rb Show resolved Hide resolved
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
@codeclimate
Copy link

codeclimate bot commented Feb 10, 2020

Code Climate has analyzed commit 73b3336 and detected 13 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 13

View more on Code Climate.

@clintoncwolfe clintoncwolfe merged commit 774b594 into master Feb 10, 2020
@clintoncwolfe clintoncwolfe deleted the cw/install-context-detect branch February 10, 2020 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telemetry: detect installation type
3 participants