(PDK-401, PDK-402, PDK-403, PDK-404) Update validators to handle targets better #248

bmjen · 2017-08-14T01:30:53Z

Updates validators to handle targets consistently and disallows input of
explicit targets that are incompatible with the validator's defined
filter pattern. Also updates the spinner text to be informative about what
kind of files the validator is validating against.

Relates to #239

bmjen · 2017-08-14T01:31:42Z

I'll need to add some test coverage to the spec and acceptance tests. Will also need to integrate with @james-stocks 's metadata changes to complete the UX improvements on validation target handling.

coveralls · 2017-08-14T01:38:13Z

Coverage decreased (-1.2%) to 92.474% when pulling 63093e3 on bmjen:validator-handling into 53436e1 on puppetlabs:master.

coveralls · 2017-08-14T02:12:08Z

Coverage decreased (-0.1%) to 93.559% when pulling 73f3263 on bmjen:validator-handling into 53436e1 on puppetlabs:master.

james-stocks · 2017-08-14T10:04:35Z

lib/pdk/validators/base_validator.rb

@@ -25,17 +25,29 @@ def self.parse_targets(options)
                    options[:targets]
                  end

-        targets.map { |target|
+        skipped = []


It would be great to add yardoc to describe what the method now returns; and also to briefly explain what skipped and invalid mean

james-stocks · 2017-08-14T10:07:18Z

lib/pdk/validators/base_validator.rb

+                target
+              else
+                skipped << target
+                next


Is this next needed? It looks like there's only end lines below it

james-stocks · 2017-08-14T10:15:00Z

lib/pdk/validators/base_validator.rb

+          report.add_event(
+            file:     skipped_target,
+            source:   name,
+            message:  'Target skipped. No files to validate.',


From the code above; I believe that skipped means that the file/directory given by the user as a target does exist on the filesystem, but does not match the pattern built into the validator.
I'm not sure this message conveys that. And 'No files to validate' might be confusing if other targets in the command did have valid targets.

message: 'Target was skipped as it does not match the validator\'s pattern'

?

james-stocks · 2017-08-14T10:30:03Z

lib/pdk/validators/base_validator.rb

          if respond_to?(:pattern)
            if File.directory?(target)
-              Array[pattern].flatten.map { |p| Dir.glob(File.join(target, p)) }
+              target_list = Array[pattern].flatten.map { |p| Dir.glob(File.join(target, p)) }


At this point if the target directory contains a mixture of file types; we glob those that match the pattern and ignore those that don't.
We don't report those unmatching files as skipped... I think that's OK.

james-stocks · 2017-08-14T10:34:56Z

lib/pdk/validators/metadata/metadata_syntax.rb

@@ -42,7 +42,7 @@ def self.stop_spinner(exit_code)
      end

      def self.invoke(report, options = {})
-        targets = parse_targets(options)
+        targets, _skipped, _invalid = parse_targets(options)


This class's invoke method does not report on _skipped or _invalid and doesn't seem to call super.invoke - should reporting of skipped and invalid targets be added?

james-stocks · 2017-08-14T10:58:56Z

lib/pdk/validators/ruby/rubocop.rb

@@ -16,8 +16,12 @@ def self.cmd
        'rubocop'
      end

+      def self.pattern
+        '**/**.rb'


Rubocop checks a number of different files e.g. .irbrc, Rakefile, Gemfile - should this pattern definitely restrict to *.rb ?
I think for a Puppet module we want {Rakefile,Gemfile,**/**.rb} (or whatever the glob syntax for or is )

Currently we have Gemfile and Rakefile excluded in .rubocop.yml. I don't think there are anymore ruby files that don't end in .rb we need to worry about.

james-stocks · 2017-08-14T11:00:01Z

lib/pdk/validators/metadata/metadata_syntax.rb

@@ -11,7 +11,7 @@ def self.name
      end

      def self.pattern
-        'metadata.json'
+        '**/metadata.json'


PDK-401 specifies Only validate <module_root>/metadata.json so I think this pattern should be unchanged.

james-stocks · 2017-08-14T11:00:22Z

lib/pdk/validators/metadata/metadata_json_lint.rb

@@ -26,7 +26,7 @@ def self.spinner_text(targets = [])
      end

      def self.pattern
-        'metadata.json'
+        '**/metadata.json'


PDK-401 specifies Only validate <module_root>/metadata.json so I think this pattern should be unchanged.

james-stocks · 2017-08-14T11:02:48Z

spec/acceptance/validate_metadata_spec.rb

@@ -100,7 +100,7 @@
      end

      its(:stdout) do
-        is_expected.to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes(
+        is_expected.not_to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes(
          'classname' => 'metadata-json-lint.dependencies',
          'name'      => 'broken.json',
        ).that_failed


.that_failed can be removed now that we're asserting the testcase does not exist

james-stocks · 2017-08-14T11:03:34Z

spec/acceptance/validate_metadata_spec.rb

@@ -100,7 +100,7 @@
      end

      its(:stdout) do
-        is_expected.to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes(
+        is_expected.not_to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes(


May now want to assert that metadata.json was included in the test results

james-stocks · 2017-08-14T11:07:11Z

spec/support/it_accepts_metadata_json_targets.rb

        end
      end
    end

-    context 'when given specific target files' do
+    context 'when given unmatched target files' do


Not sure I'd understand this immediately when reading.

Perhaps

context 'when given unmatching target files' do

or

context 'when given a target that will not match the validator\s pattern' do

?

james-stocks · 2017-08-14T11:08:41Z

spec/support/it_accepts_metadata_json_targets.rb

-      it 'returns the targets' do
-        expect(parsed_targets).to eq(targets)
+      it 'skips the targets' do
+        expect(parsed_targets.first).to eq([])


Might be valuable to also put an expectation on the skipped and invalid values

james-stocks

I left a number of comments. The one I'd request a change on is where the **/metadata.json pattern is being used - PDK-401 specifies that we only validate {module_root}/metadata.json

coveralls · 2017-08-14T21:28:42Z

Coverage decreased (-0.3%) to 93.388% when pulling b6ad5ac on bmjen:validator-handling into 671df45 on puppetlabs:master.

coveralls · 2017-08-14T21:46:57Z

Coverage decreased (-0.2%) to 93.452% when pulling b997b67 on bmjen:validator-handling into 671df45 on puppetlabs:master.

coveralls · 2017-08-14T21:49:06Z

Coverage decreased (-0.2%) to 93.452% when pulling c366c2e on bmjen:validator-handling into 671df45 on puppetlabs:master.

austb · 2017-08-14T23:57:37Z

lib/pdk/validators/metadata/metadata_syntax.rb

+        targets, skipped, invalid = parse_targets(options)
+
+        process_skipped(skipped)
+        process_invalid(invalid)


I think we need to pass the report object here. process_skipped(report, skipped) and process_invalid(report, invalid)

👍 good catch.

coveralls · 2017-08-14T23:58:41Z

Coverage decreased (-0.03%) to 93.651% when pulling 534caaf on bmjen:validator-handling into 671df45 on puppetlabs:master.

coveralls · 2017-08-15T00:23:31Z

Coverage decreased (-0.03%) to 93.651% when pulling 2b2848b on bmjen:validator-handling into 671df45 on puppetlabs:master.

Changes were implemented, and I like green.

Only check metadata.json. If the user passes targets through the CLI, inform user they are being ignored.

Updates validators to handle targets consistently and disallows input of explicit targets that are incompatible with the validator's defined filter pattern. Also updates the spinner text to be informative about what kind of files the validator is validating against.

rodjek · 2017-08-15T01:59:38Z

Squashed the fixup commits into their counterparts

coveralls · 2017-08-15T02:04:59Z

Coverage decreased (-0.03%) to 93.671% when pulling b416c8e on bmjen:validator-handling into e0ec017 on puppetlabs:master.

bmjen requested review from rodjek, DavidS and james-stocks August 14, 2017 01:31

bmjen force-pushed the validator-handling branch from 63093e3 to 73f3263 Compare August 14, 2017 02:06

james-stocks reviewed Aug 14, 2017

View reviewed changes

james-stocks previously requested changes Aug 14, 2017

View reviewed changes

james-stocks mentioned this pull request Aug 14, 2017

(PDK-401) Do not accept targets for metadata validator #239

Closed

DavidS added the feature label Aug 14, 2017

bmjen force-pushed the validator-handling branch 2 times, most recently from 21b03b6 to b6ad5ac Compare August 14, 2017 21:22

bmjen requested a review from scotje August 14, 2017 21:25

bmjen force-pushed the validator-handling branch 2 times, most recently from b997b67 to c366c2e Compare August 14, 2017 21:43

bmjen changed the title ~~(PDK-402, PDK-403, PDK-404) Update validators to handle targets better~~ (PDK-401, PDK-402, PDK-403, PDK-404) Update validators to handle targets better Aug 14, 2017

bmjen requested a review from austb August 14, 2017 21:52

austb reviewed Aug 14, 2017

View reviewed changes

austb approved these changes Aug 15, 2017

View reviewed changes

james-stocks and others added 3 commits August 15, 2017 11:34

(PDK-401) Do not accept targets for metadata validator

81b1712

Only check metadata.json. If the user passes targets through the CLI, inform user they are being ignored.

(PDK-404) Adds validator filter pattern to spinner text.

003f27f

rodjek force-pushed the validator-handling branch from 2b2848b to b416c8e Compare August 15, 2017 01:58

rodjek approved these changes Aug 15, 2017

View reviewed changes

rodjek merged commit 407b70d into puppetlabs:master Aug 15, 2017

bmjen deleted the validator-handling branch November 14, 2017 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-401, PDK-402, PDK-403, PDK-404) Update validators to handle targets better #248

(PDK-401, PDK-402, PDK-403, PDK-404) Update validators to handle targets better #248

bmjen commented Aug 14, 2017

bmjen commented Aug 14, 2017

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017 •

edited

Loading

james-stocks Aug 14, 2017

bmjen Aug 14, 2017 •

edited

Loading

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks Aug 14, 2017

james-stocks left a comment

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

austb Aug 14, 2017

bmjen Aug 15, 2017

coveralls commented Aug 14, 2017

coveralls commented Aug 15, 2017

rodjek commented Aug 15, 2017

coveralls commented Aug 15, 2017

(PDK-401, PDK-402, PDK-403, PDK-404) Update validators to handle targets better #248

(PDK-401, PDK-402, PDK-403, PDK-404) Update validators to handle targets better #248

Conversation

bmjen commented Aug 14, 2017

bmjen commented Aug 14, 2017

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmjen Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks left a comment

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017

coveralls commented Aug 15, 2017

rodjek commented Aug 15, 2017

coveralls commented Aug 15, 2017

james-stocks Aug 14, 2017 •

edited

Loading

bmjen Aug 14, 2017 •

edited

Loading