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.

Sign up for GitHub

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

Jump to bottom

Bug fix for loading hashmap inputs consistently #5446

Merged
merged 5 commits into from
Apr 7, 2021
Merged

Bug fix for loading hashmap inputs consistently #5446

merged 5 commits into from
Apr 7, 2021

Conversation

Nik08
Copy link
Contributor

@Nik08 Nik08 commented Mar 31, 2021 •
edited
Loading

Signed-off-by: Nikita Mathur nikita.mathur@chef.io

Description

Issues:

  1. The inputs loaded from external file and metadata file were read in two different formats by InSpec, one hash with symbol keys and another hash with string as keys
  2. Hash inputs loaded using runner API and profile DSL also, returns success only in one of the formats.

Impact of solution:

This approach will solve reading inputs irrespective of their formats, which enables using hashes inside InSpec controls in any way preferred.

Related Issue

Fixes #4057

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.

@Nik08
Bug fix for loading hashmap inputs consistently from external and met…
c0b213c 
…adata file

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
@Nik08 Nik08 requested a review from a team as a code owner March 31, 2021 06:16
@netlify
Copy link

netlify bot commented Mar 31, 2021 •
edited
Loading

Deploy preview for chef-inspec canceled.

Built with commit 82e1267

https://app.netlify.com/sites/chef-inspec/deploys/606d8bf9b313d6000850652f

@Nik08
Improvements in various descriptions and a better way to check for ha…
a55b083 
…sh type

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
@clintoncwolfe clintoncwolfe added Component: Inputs Parameter Input system (formerly attributes) Aspect: Correctness Is it actually right? Does it match a specification? labels Apr 2, 2021
@clintoncwolfe
Copy link
Contributor

@aaronlippold Does this approach - using HashWithIndifferentAccess - work for your use case?

clintoncwolfe
clintoncwolfe reviewed Apr 2, 2021
View reviewed changes
lib/inspec/input_registry.rb Outdated
@@ -256,7 +256,7 @@ def bind_inputs_from_input_files(profile_name, file_list)

data.inputs.each do |input_name, input_value|
evt = Inspec::Input::Event.new(
value: input_value,
value: input_value.is_a?(Hash) ? Thor::CoreExt::HashWithIndifferentAccess.new(input_value) : input_value,
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, I wonder if we should make a similar change in bind_inputs_from_runner_api? Testing that would be difficult, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for consistency that seems right. Let me look into the testing front for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a method to handle this conversion input_value.is_a?(Hash) ? Thor::CoreExt::HashWithIndifferentAccess.new(input_value) : input_value so that we can just call that method wherever required and code will be maintainable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes these checks could be re-used by having just a method :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clintoncwolfe I observed this issue not only additionally happens for runner api input but also for other ways of input. Will try an alternate solution which could work for all the input types, rather than handling it in each method definition for different input types. Hope that would be a better solution.

@aaronlippold
Copy link
Collaborator

I'm not sure given that there isn't really a description of the change. Can you elaborate on what is being updated and remind me which use case we're talking about :-)

@Nik08
Hash with indifference changes for inputs used via runner api
ec30fc3 
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
@Nik08
Refactored logic to enable hash indifference in all input types
094293b 
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
@Nik08 Nik08 changed the title Bug fix for loading hashmap inputs consistently from external and metadata file Bug fix for loading hashmap inputs consistently Apr 5, 2021
@Nik08 Nik08 requested a review from clintoncwolfe April 7, 2021 09:07
@Nik08
Improvements in hashmap profiles
82e1267 
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
clintoncwolfe
clintoncwolfe approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Great testing - very good work!

@clintoncwolfe clintoncwolfe merged commit fa41586 into master Apr 7, 2021
@clintoncwolfe clintoncwolfe mentioned this pull request Apr 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clintoncwolfe clintoncwolfe clintoncwolfe approved these changes

@Vasu1105 Vasu1105 Awaiting requested review from Vasu1105

Assignees

@Nik08 Nik08

Labels
Aspect: Correctness Is it actually right? Does it match a specification? Component: Inputs Parameter Input system (formerly attributes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hashes in inputs from inspec.yml and inputs file loaded differently
4 participants
@Nik08 @clintoncwolfe @aaronlippold @Vasu1105