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

Update command resource to check for mock backend #2353

Merged
merged 1 commit into from
Dec 5, 2017

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Nov 29, 2017

This change removes the command errors when using a mock backend that does not set a os.name. Ex: command(docker).exist? is not supported on your OS: unknown

This change goes in tandem with inspec/train#221. It can be merged early if needed but we will not see the errors go away till both are merged and inspec is updated to use the new version of train.

Signed-off-by: Jared Quick [email protected]

@jquick jquick requested a review from a team as a code owner November 29, 2017 20:04
@jquick jquick force-pushed the jq/check_command_for_mock_backend branch from ea47bf7 to 289609f Compare November 29, 2017 20:12
@jquick jquick requested a review from adamleff November 29, 2017 20:13
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 this is a fine approach. Another would be to do inspec.backend.is_a?(Train::Transports::Mock::Connection) but either way, we're tying ourselves to a specific Train implementation detail, and I'd rather name the mock OS something other than "unknown" and leave "unknown" for OSs we literally cannot detect.

Copy link
Contributor

@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 catch @jquick :D thank you

@arlimus arlimus merged commit 578577f into master Dec 5, 2017
@arlimus arlimus deleted the jq/check_command_for_mock_backend branch December 5, 2017 13:21
@arlimus
Copy link
Contributor

arlimus commented Dec 5, 2017

Fully agree Adam, great points on unknown

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