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.

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

(PDK-1112) Create json schema to validate pdk config file #742

Merged
merged 7 commits into from
Sep 20, 2019

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Sep 2, 2019

  • Address TODOs
  • Finish yard docs for methods
  • What happens if the file is not valid BEFORE the PDK tries to read it? We can never save it due to the validation.

Previously the Config Namespace class conflated a few concepts together and had
less than ideal method naming. This commit:

  • Changes the name of a namespace value to a namespace setting. This is because
    the class held more than just a value, such as the default value and
    validators. Therefore it made more sense to call it a a Namespace Setting.

  • The namespace had an abstract method called parse_data which was passed both
    the data and the name to the file. This didn't make sense as the filename is
    all that's required, not the filename. The method was also renamed to
    parse_file which is more accurate name.

  • The parse_file used to return an array of objects, but this create extra array
    objects during parsing. This commit changes the method to instead yield
    namespace setting objects. This will allow namespaces to create their own
    settings objects instead of only using the one.

  • Refactors common file based namespace tests to an rspec shared example.

  • Adds tests for assigning values to a setting


Previously there was no formal schema, or a way to validate a schema, for
configuration settings. This commit adds two new namespace types, YAML and
JSON files with JSON schema validation.

These namespaces will also take a JSON Schema file and use that to provide
defaults and validation. Any settings NOT in the schema will not be visible in
the namespace, but will not be lost when saving the namespace to disk; That is
the namespace will only modify settings that are in the schema. This also
includes tests for any schema files, to ensure that they are indeed valid.

This commit adds a schema for the PDK analytics configuration.

This commit introduces a new setting type, JSONSchemaSetting which uses the
schema to validate and get default values for a setting from the schema.


This commit adds the tests for the new schema based namespaces. This commit
also refactors common tests between JSON, YAML and non-schema namespaces into
rspec shared example groups.

Note that due to a bug in json-schema object, the tests fail on non-Windows
platforms.


Unfortunately the json-schema gem fails to load filed based schema on Windows
due to URI encoding issues. Also the json-schema gem is abandoned so it get
fixed either, and other gems don't have support for old ruby 2.1. This commit
monkey patches the JSON Schema read_file method to munge the pathname on
Windows platforms.

This commit removes the test guards on non-Windows platforms.

@glennsarti glennsarti added the WIP label Sep 2, 2019
@glennsarti glennsarti self-assigned this Sep 2, 2019
@glennsarti glennsarti changed the title (PDK-1112) Create json schema to validate pdk config file {WIP}(PDK-1112) Create json schema to validate pdk config file Sep 2, 2019
@coveralls
Copy link

coveralls commented Sep 2, 2019

Coverage Status

Coverage increased (+0.2%) to 92.258% when pulling c6411c3 on glennsarti:pdk-1112-config-schema into 378f22d on puppetlabs:master.

@glennsarti glennsarti force-pushed the pdk-1112-config-schema branch 3 times, most recently from 8ace120 to 9598537 Compare September 3, 2019 03:29
@glennsarti glennsarti removed the WIP label Sep 3, 2019
@glennsarti glennsarti changed the title {WIP}(PDK-1112) Create json schema to validate pdk config file (PDK-1112) Create json schema to validate pdk config file Sep 3, 2019
@glennsarti glennsarti added this to the September 2019 milestone Sep 4, 2019
lib/pdk/config/json.rb Show resolved Hide resolved
lib/pdk/config/setting.rb Show resolved Hide resolved
lib/pdk/config/yaml.rb Show resolved Hide resolved
lib/pdk/config/json_schema_namespace.rb Outdated Show resolved Hide resolved
lib/pdk/config/json_schema_namespace.rb Show resolved Hide resolved
Previously the Config Namespace class conflated a few concepts together and had
less than ideal method naming.  This commit:

* Changes the name of a namespace value to a namespace setting. This is because
  the class held more than just a value, such as the default value and
  validators. Therefore it made more sense to call it a a Namespace Setting.

* The namespace had an abstract method called parse_data which was passed both
  the data and the name to the file. This didn't make sense as the filename is
  all that's required, not the filename.  The method was also renamed to
  parse_file which is more accurate name.

* The parse_file used to return an array of objects, but this create extra array
  objects during parsing. This commit changes the method to instead yield
  namespace setting objects.  This will allow namespaces to create their own
  settings objects instead of only using the one.

* Refactors common file based namespace tests to an rspec shared example.

* Adds tests for assigning values to a setting
Previously there was no formal schema, or a way to validate a schema, for
configuration settings. This commit adds two new namespace types, YAML and
JSON files with JSON schema validation.

These namespaces will also take a JSON Schema file and use that to provide
defaults and validation.  Any settings NOT in the schema will not be visible in
the namespace, but will not be lost when saving the namespace to disk; That is
the namespace will only modify settings that are in the schema. This also
includes tests for any schema files, to ensure that they are indeed valid.

This commit adds a schema for the PDK analytics configuration.

This commit introduces a new setting type, JSONSchemaSetting which uses the
schema to validate and get default values for a setting from the schema.
This commit adds the tests for the new schema based namespaces. This commit
also refactors common tests between JSON, YAML and non-schema namespaces into
rspec shared example groups.

Note that due to a bug in json-schema object, the tests fail on non-Windows
platforms.
Unfortunately the json-schema gem fails to load filed based schema on Windows
due to URI encoding issues. Also the json-schema gem is abandoned so it won't get
fixed either, and other gems don't have support for old ruby 2.1. This commit
monkey patches the JSON Schema read_file method to munge the pathname on
Windows platforms.

This commit removes the test guards on non-Windows platforms.
This commit removes some TODOs from the yard documentation for the
PDK::Config::Setting class.
The PDK::Config::Validator class is no longer used.  This commit removes the
class from the PDK.
Previously the previous_setting attribute was added without much documentation.
This command adds comments to describe how this works.
Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Looks good to me @glennsarti !

@glennsarti glennsarti merged commit 51f36f1 into puppetlabs:master Sep 20, 2019
@glennsarti glennsarti deleted the pdk-1112-config-schema branch March 5, 2020 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants