(PDK-470) Validation of task metadata. #301
Conversation
bmjen
changed the title
|
I still need to fix-up the remaining spec tests as well as update the Acceptance tests to reflect the new changes to the info/debug output strings. |
| @@ -0,0 +1,65 @@ | |||
| module PDK | |||
There was a problem hiding this comment.
Is it possible to have the schema as a separate file instead of embedded? That way other tools can consume it
There was a problem hiding this comment.
Also means the schema file itself can be validated
There was a problem hiding this comment.
There's been some discussion on tech-discuss and I've proposed hosting a JSON Schema for task (and module) metadata on the Forge. I've got a PR for that with just a placeholder so we could move this schema into that PR:
https://github.com/puppetlabs/puppet-forge-api/pull/371
There was a problem hiding this comment.
Alternatively we could publish at http://schemastore.org/json/
There was a problem hiding this comment.
Because two sources of truth is rarely a good thing. Pick one.
There was a problem hiding this comment.
Then I'll easily pick the one we have direct control over.
Seems like the best approach would be to host it ourselves and just add it to the schemastore catalog.
lib/pdk/validators/base_validator.rb
Outdated
| @@ -45,7 +45,7 @@ def self.parse_targets(options) | |||
| skipped << target if target_list.flatten.empty? | |||
| target_list | |||
| elsif File.file?(target) | |||
| if target.eql? pattern | |||
| if Array(pattern).flatten.include? target | |||
There was a problem hiding this comment.
No need to call flatten after Array() unless pattern is going to be a multidimensional array (Array([1, 2, 3]) => [1, 2, 3])
| module PDK | ||
| module Validate | ||
| module TaskMetadataSchema | ||
| SCHEMA = '{ |
There was a problem hiding this comment.
Please point here to the canonical version of this schema, so that there can be no doubt about which copy is the primary.
| raise PDK::CLI::FatalError, _('Unable to download Task Metadata Schema file. Please check internet connectivity and retry this action.') | ||
| end | ||
| end | ||
| JSON.parse(schema) |
There was a problem hiding this comment.
Consider rescuing JSON parse error here (and logging a nicer error message) in case the schema is corrupted somehow
| @@ -28,6 +28,7 @@ Gem::Specification.new do |spec| | |||
| spec.add_runtime_dependency 'tty-spinner', '0.5.0' | |||
| spec.add_runtime_dependency 'tty-prompt', '0.13.1' | |||
| spec.add_runtime_dependency 'json_pure', '~> 2.1.0' | |||
| spec.add_runtime_dependency 'json-schema', '2.8.0' | |||
There was a problem hiding this comment.
Is there a ticket or PR for adding this to the vanagon repo?
There was a problem hiding this comment.
(and it's runtime dependencies)
|
Left a couple small comments but overall looking good @bmjen ! |
bmjen
changed the title
| rescue OpenSSL::SSL::SSLError => e | ||
| ssl_failures += 1 | ||
|
|
||
| unless ssl_failures < 2 && Gem.win_platform? && PDK::Util.gem_install? |
There was a problem hiding this comment.
Does this mean that on a Windows system with a package install getting SSL errors, we'll retry forever?
There was a problem hiding this comment.
See ssl_failures variable. Although we might need to do an explicit "begin" context instead of rescuing the whole method.
There was a problem hiding this comment.
yep, I see the counter. I misread the conditional itself.
I've refactored the task metadata validation to validate against a json-schema per recommendation/request from @garethr .
This PR also includes functionality to extend the MetadataSyntax validator to validate the tasks JSON as well.