Refactor CLI Interface, Add Scan fail-on-warnings #197

byronic · 2019-05-09T22:03:06Z

[Finishes #165899506]; [Finishes #165899494]; factors out command line arguments reading; adds fail-on-warnings support to cfn_nag_scan; removes repeated code across cfn_nag_scan and cfn_nag by refactoring into an executor. Post-refactor, rubocop and testing required the following included supplmental changes: CfnNagExecutor is too large if read_cli_options is included, so factors that function out to its own file; CfnNag class is too large with the added fail_on_warnings option, refactors initialize parameters as a separate CfnNagConfig class; refactors the cfn_nag integration tests to use the new signatures

ghost · 2019-05-10T15:20:46Z

lib/cfn-nag/cfn_nag_executor.rb

+    end
+  end
+
+  def make_config(opts)


i'd just call this method what it is without the verb: cfn_nag_config(opts)

ghost · 2019-05-10T15:21:23Z

lib/cfn-nag/read_cli_options.rb

+# rubocop:disable Metrics/BlockLength
+# rubocop:disable Metrics/AbcSize
+# rubocop:disable Metrics/MethodLength
+def read_cli_options(require_input_path: false)


i'd just name this: cli_options()

[Finishes #165899506]; [Finishes #165899494]; factors out command line arguments reading; adds fail-on-warnings support to cfn_nag_scan; removes repeated code across cfn_nag_scan and cfn_nag by refactoring into an executor. Post-refactor, rubocop and testing required the following included supplmental changes: CfnNagExecutor is too large if read_cli_options is included, so factors that function out to its own file; CfnNag class is too large with the added fail_on_warnings option, refactors initialize parameters as a separate CfnNagConfig class; refactors the cfn_nag integration tests to use the new signatures

Fixes a missing call to validate_options in the executor; refactors to stay within the 15 line per function bounds of rubocop

Adds naming corrections per @erickascic 's review; adds tests for CfnNagExecutor class at 88%

Adds additional tests to get cfn_nag_executor and other introduced files to 100% coverage

Updates cli_options to be backwards compatible -- removes alphabetical ordering, splits into two options categories for cfn_nag and cfn_nag_scan

ghost · 2019-05-15T23:34:25Z

lib/cfn-nag/cfn_nag_executor.rb

+  end
+
+  def scan(aggregate_output: true,
+           cli_supplier: cli_options(cfn_nag_scan: @require_input_path),


if i follow the code, cli_supplier and arg_supplier are exposed here to improve testability? imho this is a tradeoff for complicating the interface - i.e. anybody who calls this might scratch their head and likely never change the defaults. another approach i can suggest is to have "factory methods" for these two arguments, and then in your testing do a partial mock to replace the factory methods. easy on the testing side, and nothing exposed to the consumer

ghost · 2019-05-15T23:37:33Z

lib/cfn-nag/cfn_nag_executor.rb

+    @parameter_values_string = nil
+  end
+
+  def scan(aggregate_output: true,


the require_input_path and aggregate_output are giving me a little trouble. i know what they're there for.... but i'm wondering if you turn the other scan args into factory methods.... er do you need both? or could you have them both be on the scan() instead of split between the constructor and scan()? there are basically two outcomes - single or aggregate, and only one of them requires the input path?

ghost · 2019-05-15T23:38:45Z

lib/cfn-nag/cli_options.rb

+# rubocop:disable Metrics/AbcSize
+# rubocop:disable Metrics/MethodLength
+def cli_options(cfn_nag_scan: false)
+  options_message = '[options] <cloudformation template path ...>|' \


nit: move the messages into the "scope" where they are relevant and have the shared stuff up here only

ghost · 2019-05-15T23:39:30Z

lib/cfn-nag/cli_options.rb

+  if cfn_nag_scan
+    Trollop.options do
+      version version
+      opt :input_path,


can we do one set of Trollop.options, but use the "cfn_nag_scan" flag to determine which short arg to force per opt for backward compat?

also break the options into common vs. divergent.

ghost

comments inline

Refactors cli_options and argf_supplier into a factory-pattern from lambdas; changes the CfnNagExecutor to have no initializer arguments and scan to properly have a cfn_nag_scan argument instead, for clarity

Raises Argf and Options test coverage to 100%

Incorporates Eric's changeset from Slack; removes a test that is both no longer necessary and not working under the new changes; resolves rubocop issues

…est updates

Removes @ items used in Trollop.options block -- those are unsupported and cause nil class exceptions

An extraneous puts options call in cfn_nag_executor was causing failures in the e2e tests; removes

Complies with Jesse's PR #215, adding output format option to cfn_nag

ghost · 2019-05-31T12:30:12Z

lib/cfn-nag/argf.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class Argf


should be able to drop this whole file?

ghost

sorry to beat the dead horse on this one - it looks good, i just have three final nitpicks around naming:

i believe argf wrapper class is dead code in light of the argf_* wrapper methods?
i know this came from my code so mea culpa, but i'm not sure i like the options_type value "cli" (scan is cool). it's all cli after all. I'm not sure what a better word is.... explicit? singular? hold that thought
the name execute_single_scan() doesn't quite capture what is going on. ARGF will iterate through a list of file paths and process them. like for options_type.... i'm not sure what a better name is though - execute_scan_against_explicit_files() perhaps?

Per Eric Kascic's review, removes the Argf class (which was unused after prior changes); renames execute_single_scan execute_file_or_piped_scan since the argument can be an explicit file or piped input; changes the options type value for file/piped scans to 'file' from 'cli'

ghost

woohoo!

byronic requested review from jesseadams and a user May 9, 2019 22:03

byronic assigned jesseadams and ghost May 9, 2019

ghost reviewed May 10, 2019

View reviewed changes

byronic added 5 commits May 14, 2019 11:17

Fix Missing validate_options Call

8a6c746

Fixes a missing call to validate_options in the executor; refactors to stay within the 15 line per function bounds of rubocop

Executor Tests, Name Fixes

4c91709

Adds naming corrections per @erickascic 's review; adds tests for CfnNagExecutor class at 88%

Test Coverage Bump

f972590

Adds additional tests to get cfn_nag_executor and other introduced files to 100% coverage

Keep cli_options Backwards Compatible

087337d

Updates cli_options to be backwards compatible -- removes alphabetical ordering, splits into two options categories for cfn_nag and cfn_nag_scan

byronic force-pushed the feature/cli-consolidation branch from f9ad74f to 087337d Compare May 15, 2019 15:45

byronic requested a review from a user May 15, 2019 15:49

ghost reviewed May 15, 2019

View reviewed changes

ghost suggested changes May 15, 2019

View reviewed changes

Factory Pattern, cleanup CfnNagExecutor arguments

852bde6

Refactors cli_options and argf_supplier into a factory-pattern from lambdas; changes the CfnNagExecutor to have no initializer arguments and scan to properly have a cfn_nag_scan argument instead, for clarity

byronic requested a review from a user May 17, 2019 15:04

byronic added 7 commits May 17, 2019 10:32

Test Coverage

bbb7db4

Raises Argf and Options test coverage to 100%

Resolve Test Issues, Incorporate Eric's Changeset

57a3945

Incorporates Eric's changeset from Slack; removes a test that is both no longer necessary and not working under the new changes; resolves rubocop issues

Merge branch 'master' into feature/cli-consolidation for end-to-end t…

4de8c99

…est updates

No @ Allowed In Trollop.options

465b815

Removes @ items used in Trollop.options block -- those are unsupported and cause nil class exceptions

Resolve Failing e2e Tests - Extraneous puts

f810616

An extraneous puts options call in cfn_nag_executor was causing failures in the e2e tests; removes

Add output format option to cfn_nag

83d4ea3

Complies with Jesse's PR #215, adding output format option to cfn_nag

Merge branch 'master' into feature/cli-consolidation

6b5a3b8

ghost reviewed May 31, 2019

View reviewed changes

lib/cfn-nag/argf.rb Outdated

@@ -0,0 +1,36 @@

# frozen_string_literal: true

class Argf

Copy link

ghost May 31, 2019

Choose a reason for hiding this comment

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

should be able to drop this whole file?

ghost suggested changes May 31, 2019

View reviewed changes

ghost approved these changes May 31, 2019

View reviewed changes

ghost merged commit dfaed0f into master May 31, 2019

byronic mentioned this pull request May 31, 2019

Add --output-format back to cfn_nag #214

Closed

ghost deleted the feature/cli-consolidation branch January 6, 2020 21:58

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CLI Interface, Add Scan fail-on-warnings #197

Refactor CLI Interface, Add Scan fail-on-warnings #197

byronic commented May 9, 2019

ghost May 10, 2019 •

edited by ghost

ghost May 10, 2019

ghost May 15, 2019

ghost May 15, 2019

ghost May 15, 2019

ghost May 15, 2019

ghost May 15, 2019

ghost left a comment

ghost May 31, 2019

ghost left a comment

ghost left a comment

Refactor CLI Interface, Add Scan fail-on-warnings #197

Refactor CLI Interface, Add Scan fail-on-warnings #197

Conversation

byronic commented May 9, 2019

ghost May 10, 2019 • edited by ghost

Choose a reason for hiding this comment

ghost May 10, 2019

Choose a reason for hiding this comment

ghost May 15, 2019

Choose a reason for hiding this comment

ghost May 15, 2019

Choose a reason for hiding this comment

ghost May 15, 2019

Choose a reason for hiding this comment

ghost May 15, 2019

Choose a reason for hiding this comment

ghost May 15, 2019

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost May 31, 2019

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost May 10, 2019 •

edited by ghost