(PDK-1592) Refactor PDK validators to be more singular purpose #831

glennsarti · 2020-01-17T06:48:07Z

Blocked by #849

Currently the base_validator class has functionality for a single validator, a group of validators, Spinners. And has some "magic" methods e.g. pattern

This makes it awkward to add new features or make granular changes because the one class (or Module) has too many concerns.

This PR refactors all of the validators to have a single concern, making it easier to add more validators (e.g. Puppetfile) and default behaviours (e.g. control repo validation) in the future

                             +-----------+
                             |           |
                   +---------+ Validator +----------------------+
                   |         |           |                      |
                   |         +-----------+                      |
                   |                                            |
                   |                                            |
            +------v---------+                       +----------v---------+
            |                |                       |                    |
            | ValidatorGroup |                +------+ InvokableValidator +-----+
            |                |                |      |                    |     |
            +------+---------+                |      +--------------------+     |
                   |                          |                                 |
                   |                          |                                 |
+------------------v---------+       +--------v--------------+   +--------------v-----------+
|                            |       |                       |   |                          |
| e.g MetadataValidatorGroup |       | InternalRubyValidator |   | ExternalCommandValidator |
|                            |       |                       |   |                          |
+----------------------------+       +--------+--------------+   +--------------+-----------+
                                              |                                 |
                                              |                                 |
                              +---------------v--------------+   +--------------v-----------------+
                              |                              |   |                                |
                              | e.g. MetadataSyntaxValidator |   | e.g. MetadataJSONLintValidator |
                              |                              |   |                                |
                              +------------------------------+   +--------------------------------+

Validator

A base abstract class (Validator) with methods that are common/required for ALL validators. All validators should eventually inherit from this class.

Validator Group

An abstract class for running a group of Validators. This abstract class expects a validators method to be overridden which will list an array of Classes. The classes could be either a Validator or even another ValidatorGroup, to create a tree of Validators.

It is expected that concrete implementations would inherit this class

An example of this is the MetadataValidatorGroup class which then calls the MetadataSyntaxValidator and MetadataJSONLintValidator validators

InvokableValidator

An abstract class which is a Validator that can invoked against a list of targets. This abstract class is responsible for parsing target lists and setting up the UI for the Validator.

ExternalCommandValidator and InternalRubyValidator

These two classes provide all of the work to validate either; By running an external command in the bundle context or by running a ruby block internal to the PDK Ruby runtime respectively.

For example the MetadataJSONLintValidator uses the ExternalCommandValidator would be used to run the metadata-json-lint command

The MetadataSyntaxValidator uses the InternalRubyValidator would to parse a JSON file using the JSON ruby libraries

TODO

~~Are the names for the classes correct? The one had trouble naming the most was InvokableValidator~~ We can't think of a better name so 🤷‍♂️
~~Is there enough YARD style documentation for the abstract (and even concrete) classes?~~ There wasn't. Added more

coveralls · 2020-01-17T06:59:49Z

Coverage increased (+1.0%) to 91.366% when pulling d4271f6 on glennsarti:refactor-validators into 942885b on puppetlabs:master.

glennsarti · 2020-01-30T12:50:02Z

Blocked on #837 being merged.

rodjek

A couple of minor nits that we discussed on bluejeans, otherwise 👍

rodjek · 2020-02-10T08:40:17Z

spec/unit/pdk/validate/validator_spec.rb

+    it 'indicates the validator is prepared' do
+      # This test is a little fragile as it's using a private
+      # instance variable
+      expect(validator.instance_variable_get(:@prepared)).to eq(false)


Can we add a prepared? method to the base class rather than test this implementation detail?

Done. Added an attr_reader from those.

glennsarti · 2020-02-12T06:11:26Z

@rodjek All nits should be addressed in b10d9b1

Asking for a new review from yourself

glennsarti · 2020-02-12T06:45:41Z

Hrmmm something weird in the Travis CI cell

Could not retrieve fact='os', resolution='<anonymous>': can't alloc thread

glennsarti · 2020-02-12T06:49:10Z

Hrmmm... new version of concurrent-ruby has been released in the last day, and the code that's stalling Travis is in a Concurrent block :-(

glennsarti · 2020-02-12T08:24:36Z

The Travis failure is incidental to this PR.

Currently the base_validator class has functionality for a single validator, a group of validators, Spinners. And has some "magic" methods e.g. pattern This makes it awkward to add new features or make granular changes because the one class (or Module) has too many concerns. This commit adds a base abstract class (Validator) with methods that are common/ required for ALL validators. All validators should eventually inherit from this class. Later commits will add the concrete implementations of each of the validators and groups of validators. This commit also adds tests for the new abstract classes.

This commit also adds an abstract class for running a group of Validators (ValidatorGroup). This abstract class expects a `validators` method to be overridden which will list an array of Classes. The classes could be either a Validator or even another ValidatorGroup, to create a tree of Validators. This commit also adds tests for the new abstract class.

This commit adds an abstract class which is a Validator that can invoked against a list of targets. This abstract class is responsible for parsing target lists and setting up the UI for the Validator. Sub-classes will then implement the actual validation (via the `invoke` method). This commit also adds tests for the new abstract class.

This commit adds two abstract classes ExternalCommandValidator and InternalRubyValidator. These two classes provide all of the work to validate either; By running an external command in the bundle context or by running a ruby block internal to the PDK Ruby runtime respectively. For example the ExternalCommandValidator would be used to run rubocop and the InternalRubyValidator would be used to parse a JSON file using the JSON ruby class. Sub-classes will then implement the actual validation. This commit also adds tests for the new abstract class.

This commit adds the metadata validators using the new abstract validation classes. This code is based mostly on the existing metadata validators. This commit also adds tests for the new abstract classes.

This commit adds the puppet validators using the new abstract validation classes. This code is based mostly on the existing puppet validators. This commit also adds tests for the new abstract classes.

This commit adds the ruby validators using the new abstract validation classes. This code is based mostly on the existing ruby validators. This commit also adds tests for the new abstract classes.

This commit adds the tasks validators using the new abstract validation classes. This code is based mostly on the existing tasks validators. Unfortunately as the existing validator classes are defined as 'class Tasks' instead of 'module Tasks', they had to be removed as well. This commit also adds tests for the new abstract classes.

This commit adds the yaml validators using the new abstract validation classes. This code is based mostly on the existing yaml validators. Unfortunately as the existing validator classes are defined as 'class YAML' instead of 'module YAML', they had to be removed as well. This commit also adds tests for the new abstract classes.

Now that the new validators have been created, this commit modifies the various validation classes (pdk/cli/validate and pdk/validate.rb) to use the new classes. This commit: * Updates the execution group (ExecGroup) to use an object factory style pattern and removes the spinner text management as it is not required. * Removes any redundant validators * Updates the release classes for the new validator invoker * Updates the acceptance tests for the spinner changes

This commit cleans up tests to not use private instance variables and adds code comments to the validator abstract classes. * Adds private testing methods to PDK::CLI::Exec::Command (spinner) PDK::Validate::Validator (prepared) These have been marked as private and should only be used for unit testing * Changes one-line arrays to multiline to improve diff visibility in the future * Adds documentation for all the abstract classes in PDK::Validate::*

glennsarti · 2020-02-17T02:41:30Z

Ain't no party like a refactor party 🎉 🎉 🎉

glennsarti added the blocked label Jan 17, 2020

glennsarti force-pushed the refactor-validators branch 6 times, most recently from 4c57acb to b524501 Compare January 24, 2020 06:36

glennsarti force-pushed the refactor-validators branch 5 times, most recently from 00408b5 to d5762bc Compare January 30, 2020 12:33

glennsarti marked this pull request as ready for review January 30, 2020 12:50

glennsarti requested a review from a team as a code owner January 30, 2020 12:50

glennsarti changed the title ~~{WIP}(PDK-???) Refactor validators~~ (PDK-1592) Refactor PDK validators to be more singular purpose Jan 30, 2020

glennsarti removed the blocked label Feb 3, 2020

glennsarti force-pushed the refactor-validators branch from d5762bc to 4ecc74d Compare February 3, 2020 02:42

rodjek approved these changes Feb 11, 2020

View reviewed changes

glennsarti force-pushed the refactor-validators branch from 624cb99 to b10d9b1 Compare February 12, 2020 06:10

glennsarti requested a review from rodjek February 12, 2020 06:11

glennsarti added blocked and removed blocked labels Feb 12, 2020

glennsarti added 2 commits February 13, 2020 11:02

glennsarti added 9 commits February 13, 2020 11:02

(PDK-1592) Add Metadata validators

6077cea

This commit adds the metadata validators using the new abstract validation classes. This code is based mostly on the existing metadata validators. This commit also adds tests for the new abstract classes.

(PDK-1592) Add Puppet validators

1a52ae9

This commit adds the puppet validators using the new abstract validation classes. This code is based mostly on the existing puppet validators. This commit also adds tests for the new abstract classes.

(PDK-1592) Add Ruby validators

17e2eeb

This commit adds the ruby validators using the new abstract validation classes. This code is based mostly on the existing ruby validators. This commit also adds tests for the new abstract classes.

glennsarti force-pushed the refactor-validators branch from b10d9b1 to d4271f6 Compare February 13, 2020 03:03

rodjek approved these changes Feb 17, 2020

View reviewed changes

glennsarti merged commit 49b6dc7 into puppetlabs:master Feb 17, 2020

glennsarti deleted the refactor-validators branch March 5, 2020 04:57

chelnak added the enhancement label Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-1592) Refactor PDK validators to be more singular purpose #831

(PDK-1592) Refactor PDK validators to be more singular purpose #831

glennsarti commented Jan 17, 2020 •

edited

coveralls commented Jan 17, 2020 •

edited

glennsarti commented Jan 30, 2020

rodjek left a comment

rodjek Feb 10, 2020

glennsarti Feb 12, 2020

glennsarti commented Feb 12, 2020

glennsarti commented Feb 12, 2020 •

edited

glennsarti commented Feb 12, 2020

glennsarti commented Feb 12, 2020

glennsarti commented Feb 17, 2020

(PDK-1592) Refactor PDK validators to be more singular purpose #831

(PDK-1592) Refactor PDK validators to be more singular purpose #831

Conversation

glennsarti commented Jan 17, 2020 • edited

coveralls commented Jan 17, 2020 • edited

glennsarti commented Jan 30, 2020

rodjek left a comment

Choose a reason for hiding this comment

rodjek Feb 10, 2020

Choose a reason for hiding this comment

glennsarti Feb 12, 2020

Choose a reason for hiding this comment

glennsarti commented Feb 12, 2020

glennsarti commented Feb 12, 2020 • edited

glennsarti commented Feb 12, 2020

glennsarti commented Feb 12, 2020

glennsarti commented Feb 17, 2020

glennsarti commented Jan 17, 2020 •

edited

coveralls commented Jan 17, 2020 •

edited

glennsarti commented Feb 12, 2020 •

edited