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

Formalize Config File #3750

Merged
merged 24 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@clintoncwolfe
Copy link
Contributor

clintoncwolfe commented Jan 24, 2019

This PR implements part of #3661.

  • Creates a dedicated Inspec::Config class to represent the runtime configuration
  • Moves config file reading and merging from lib/inspec/base_cli.rb to the config class. No attempt is made to refactor or otherwise improve the code moved from base_cli - that is out of scope for this PR.
  • Adds validation to the config file
  • Modifies the CLI command handlers to use the config object.

This PR obsoletes #3710 and #3712.

A later PR, similar to #3713, will be required to complete #3661.

BEFORE MERGING, train/pull/394 must be merged, and the Gemfile reset in this PR.

@clintoncwolfe
Copy link
Contributor Author

clintoncwolfe left a comment

I've gone through the PR and left some notes, hopefully that should reduce the load!

@@ -30,7 +30,7 @@ def profiles
desc 'exec PROFILE', 'execute a Supermarket profile'
exec_options
def exec(*tests)
o = opts(:exec).dup
o = config

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

Nearly all CLI command implementation method begin with a call to the opts method, which would read the config, merge, and finagle the options. config does all that too, but does so in a more self-contained way.

You'll see a similar change with every CLI command method, sometimes with logging or other adjustments included.

conf = Train.target_config(config)
name = Train.validate_backend(conf)
transport = Train.create(name, conf)
train_credentials = config.unpack_train_credentials

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

These two stanzas demonstrate the strange coupling between InSpec and Train. InSpec is in charge of obtaining connection options / credentials from the user, but Train is responsible for loading the correct Transport, and validating the options. Finally, InSpec asks Train to create a connection, implicitly using the options the transport validated.

This PR is mostly about putting config in a clean class, so no attempt is made to decouple that mess; here we really just rename some vars and methods to get some clarity.

@@ -118,92 +119,7 @@ def self.exec_options
desc: 'Exit with code 101 if any tests fail, and 100 if any are skipped (default). If disabled, exit 0 on skips and 1 for failures.'
end

def self.default_options

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

A major piece of this PR is simply moving code from lib/inspec/base_cli.rb to lib/inspec/config.rb. Most of the methods were class methods on the CLI base class, which wasn't really needed (though they did not access state); so when moved to Config, they became instance methods. Also, they were given clearer (IMHO) names.

Many test files and other classes sometimes called these methods; so you'll see changes in other files to update them fetching it from the new place.

}
end

def self.parse_reporters(opts) # rubocop:disable Metrics/AbcSize

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

Reporter parsing and validation will eventually be reworked on #3667. For now, we just move the code; it could certainly be clarified.

puts
end

def opts(type = nil)

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

This is the main entry point that most CLI commands have to the options system. This was replaced with config, which instantiates a Config object.

Show resolved Hide resolved test/functional/inspec_archive_test.rb
end
end

# TODO - Use bash redirection to populate STDIN

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

Open to suggestions on this one.


describe 'when reading from the default location' do
# Should read from File.join(config_dir_path, 'fakehome-2', '.inspec', 'config.json')
let(:env) { { 'HOME' => File.join(config_dir_path, 'fakehome-2') } }

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

Here's how we currently handle ENV vars with parallelized tests: since these are functional tests, they're all running inspec subprocesses - so we only set env vars in the subprocess invocation, and don't touch the test harness's ENV.

@@ -7,105 +7,19 @@
describe 'BaseCLI' do
let(:cli) { Inspec::BaseCLI.new }

describe 'opts' do

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

These tests were moved to test/unit/config_test.rb

@@ -90,6 +90,7 @@
let(:profile_id) { 'windows-only' }

it 'loads our profile but skips loading controls' do
skip 'Mock loader always supports all platforms - bad test, ref #3750 '

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Author Contributor

I found a bug - I'm not sure this test was being run, as I don't think this test would have passed prior to this PR (or if it did, I have no idea how).

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Feb 1, 2019

Contributor

Missed this on the other review. Could you open up an issue?

This comment has been minimized.

@clintoncwolfe
@clintoncwolfe

This comment has been minimized.

Copy link
Contributor Author

clintoncwolfe commented Jan 28, 2019

Integration tests under kitchen are failing due to needing the updated Train branch inspec/train#394 - see https://travis-ci.org/inspec/inspec/jobs/485232092#L1959

name = Train.validate_backend(conf)
transport = Train.create(name, conf)
train_credentials = config.unpack_train_credentials
transport_name = Train.validate_backend(train_credentials)

This comment has been minimized.

@tas50

tas50 Jan 28, 2019

Contributor

thank you 1,000 times for using less generic variable names throughout this PR

@clintoncwolfe clintoncwolfe requested a review from miah Jan 30, 2019

@miah

miah approved these changes Jan 30, 2019

Copy link
Contributor

miah left a comment

This is great. Thanks @clintoncwolfe

@clintoncwolfe clintoncwolfe changed the title Formalize Config File [DO NOT MERGE] Formalize Config File Jan 30, 2019

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

This looks good, save for the fact that I couldn't get running from a config passed via STDIN to work. Attached is a patch that made it work for me.

This was the command that I was using to test:

cat test/unit/mock/config_dirs/json-config/good.json | inspec exec test/unit/mock/profiles/simple-metadata --config -

allow_stdin.txt

Show resolved Hide resolved lib/inspec/dsl.rb Outdated
Show resolved Hide resolved test/functional/inspec_archive_test.rb

if path == '-'
Inspec::Log.warn 'Reading JSON config from standard input' if STDIN.tty?
path = STDIN

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 31, 2019

Contributor

I know this was copied over, but in this case STDIN is a IO object and not a string. So the path returned doesn't behave as expected in resolve_cfg_io. See final comment for a patch that gives an example of how I made reading from STDIN possible and added a test which also "fixes" this.

EDIT: s/see final comment/see above/

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Feb 1, 2019

Author Contributor

Very good catch, @jerryaldrichiii - I temporarily added your proposed test (but not the fix yet, in the spirit of TDD) and I got:

Expected "/Users/cwolfe/sandbox/inspec/inspec-train/lib/inspec/config.rb:150:in `initialize': no implicit conversion of IO into String (TypeError)

Right on the money.

clintoncwolfe added some commits Jan 8, 2019

Create config object and units tests
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Create config object and units tests
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Use new config file system to read config
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Use new config file system to read config
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
TEMPORARY - point to train from a github branch
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
More things
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
More things
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Fix a unit test
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Refactor resolve_cfg_io, add duck check
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add bang to validate_reporters
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add comment about missing values
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add clarifying comments to unit test for malformed json test
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add bang to validate_reporters in tests, too
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Comment on doubt of truthiness
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Correct formatting test
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Allow passing opts to mock Config; and use mock config in one more pl…
…ace in test helper

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Skip test that had always been broken; how was this every passing?
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

clintoncwolfe added some commits Jan 28, 2019

Two more test files needed mock updates
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Remove temporary reliance on Train branch
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Test for piped config, and fix to make it work
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add deprecation hook for --json-config
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

@clintoncwolfe clintoncwolfe force-pushed the cw/cred-set-support-02 branch from 5666063 to a5fbecc Feb 1, 2019

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

Looks good to me. Nothing that would block a merge, just a few questions.

Show resolved Hide resolved lib/inspec/config.rb
Show resolved Hide resolved lib/inspec/config.rb

def check_for_piped_config(cli_opts)
cli_opt = cli_opts[:config] || cli_opts[:json_config]
Inspec.deprecate(:cli_option_json_config, '') if cli_opts.key?(:json_config)

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Feb 1, 2019

Contributor

I feel like this should be moved outside of this method. It feels out of place since it has nothing to do with a piped config.

Maybe resolve_cfg_io or higher up?

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Feb 1, 2019

Author Contributor

I'm calling Inspec.deprecate in both places where the code would have to be altered when the deprecation is completed, and the fucntionality removed.

Show resolved Hide resolved lib/inspec/config.rb
@@ -90,6 +90,7 @@
let(:profile_id) { 'windows-only' }

it 'loads our profile but skips loading controls' do
skip 'Mock loader always supports all platforms - bad test, ref #3750 '

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Feb 1, 2019

Contributor

Missed this on the other review. Could you open up an issue?

clintoncwolfe added some commits Feb 6, 2019

Relax pin on train in inspec-core
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Temporarily enable RDP login for appveyor
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Disable RDP login for appveyor
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

@clintoncwolfe clintoncwolfe merged commit 5a5eb4a into master Feb 6, 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

jerryaldrichiii added a commit that referenced this pull request Feb 7, 2019

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

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

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

jerryaldrichiii added a commit that referenced this pull request Feb 7, 2019

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

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

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

jerryaldrichiii added a commit that referenced this pull request Feb 7, 2019

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

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

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

jerryaldrichiii added a commit that referenced this pull request Feb 7, 2019

Add support for Inspec::Config to 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 added a commit that referenced this pull request Feb 7, 2019

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment