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

inspec-habitat: create mock backend properly #3785

Merged
merged 3 commits into from Feb 7, 2019

Conversation

Projects
None yet
3 participants
@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii commented Feb 7, 2019

With the changes in PR #3750 Inspec::Backend.create needs to support both being passed a Hash and being passed an Inspec::Config. This adds a line to convert a passed Hash to an Inspec::Config.

This also adds unit tests for Inspec::Backend because they were missing.

Fix Inspec::Config regression in Inspec::Backend
With the changes in PR #3750, `Inspec::Backend.create` needs to support
both being passed a Hash and being passed an Inspec::Config. This adds
a line to convert a passed Hash to an Inspec::Config.

This also adds unit tests for Inspec::Backend because they were missing.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

@jerryaldrichiii jerryaldrichiii force-pushed the ja/fix-inspec-backend branch from 1793b8a to a6f74a3 Feb 7, 2019

@jerryaldrichiii jerryaldrichiii changed the title Add support for Inspec::Config to Inspec::Backend Fix Inspec::Config regression in Inspec::Backend Feb 7, 2019

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Feb 7, 2019

#3750 did a lot of work to eliminate places where Inspec::Backend#create was being passed a Hash. As this is all internal code and we own inspec-habitat, I'd expect inspec-habitat to be modified to pass a Config.

That lets us move forward in a Config-based world, without having to support two calling modes.

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Feb 7, 2019

The Inspec::Backend unit tests are a great addition!

@jerryaldrichiii

This comment has been minimized.

Copy link
Contributor Author

jerryaldrichiii commented Feb 7, 2019

Yup, I agree @clintoncwolfe. I'm taking a stab at reworking that plugin now actually. That change didn't seem like it belonged in this PR.

This method needs to support a Hash though. Otherwise, #3750 is a breaking change.

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Feb 7, 2019

It's only a breaking change if we consider the interface to Inspec::Backend to be a published interface. I don't; if people want to run InSpec from ruby, they are supposed to use Inspec::Runner; if people want to create Train transports, they should use Train#create. None of that is clear from InSpec's docs, but it does reflect our integration with other tooling.

To me, the change that is needed here is

backend: Inspec::Backend.create(target: 'mock://'),
should be changed to call Config.mock, like here. This is indeed a bug - I missed a spot where we create a mock backend, because it is not tested (see #3786 for that).

@jerryaldrichiii

This comment has been minimized.

Copy link
Contributor Author

jerryaldrichiii commented Feb 7, 2019

Fair enough. I'd like to see what we interfaces we consider public and what we don't documented at some point though. This will break any one who passes a Hash and I don't see an issue with converting it.

I don't think we pass a Hash with any of our products though.

@jerryaldrichiii

This comment has been minimized.

Copy link
Contributor Author

jerryaldrichiii commented Feb 7, 2019

Been thinking a bit (thanks @clintoncwolfe for helping me at our standup). People should be using Inspec::Runner and that is the interface that should allow a Hash (it already does). As far as documenting public interfaces that seems futile. InSpec is fundamentally a CLI application, and if people are using it as a library (incorrectly) then likely they will encounter way more issues than just this.

Respond to @clintoncwolfe's feedback
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

@jerryaldrichiii jerryaldrichiii force-pushed the ja/fix-inspec-backend branch from 6124259 to 7134989 Feb 7, 2019

Show resolved Hide resolved test/unit/backend_test.rb Outdated
Remove `encoding: utf-8`
Thanks @miah

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

it 'enables caching when using a mock backend' do
backend.backend.cache_enabled?(:file).must_equal true
backend.backend.cache_enabled?(:command).must_equal true

This comment has been minimized.

@miah

miah Feb 7, 2019

Contributor

Can we change the name here so we don't have 'backend.backend' ?

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Feb 7, 2019

Author Contributor

To what though? I also struggled because I'd have to use a custom one or change the let(:backend) block.

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Thanks for the discussion and taking the other approach, @jerryaldrichiii . It's good to have more test coverage, too.

@miah

miah approved these changes Feb 7, 2019

@clintoncwolfe clintoncwolfe changed the title Fix Inspec::Config regression in Inspec::Backend inspec-habitat: create mock backend properly Feb 7, 2019

@clintoncwolfe clintoncwolfe merged commit 7fcffd2 into master Feb 7, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment