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

Replace conditionals/custom checks with a JSON schema #68

Merged
merged 1 commit into from
Jun 3, 2017

Conversation

domcleal
Copy link
Contributor

@domcleal domcleal commented Jun 1, 2017

Validating metadata.json with a JSON schema provides very easy and
reliable type checking, key checking, and basic validation of string
formats and lengths without duplicated code or needing to add lots of
very similar acceptance tests.

It also standardises error responses, using the top-level key name in
the JSON format mode for any errors within that part of the document.


Obsoletes #59 and similar changes.

Validated against all VP modules, no new errors logged.

@domcleal
Copy link
Contributor Author

domcleal commented Jun 1, 2017

This refactoring PR can wait until the current round of 1.2.x bug fix PRs are reviewed and released, it's not urgent.

@domcleal domcleal requested review from rnelson0 and removed request for rnelson0 June 1, 2017 14:48
@ghoneycutt
Copy link
Member

+1 to this approach of using a schema

},
'summary' => {
'type' => 'string',
'maxLength' => 144, # from https://forge.puppetlabs.com/razorsedge/snmp/3.3.1/scores
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's being referenced at that URL, I don't see any error/warning there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No true, I copied this from the old check added in 6863e8f, not sure why there's no warning now. Perhaps the Forge behaviour has changed or the warning isn't displayed for old versions. I think I'll leave the check in though as the summary is meant to be "one line" according to the spec.

@rnelson0
Copy link
Member

rnelson0 commented Jun 1, 2017

I'd like to see if @jbondpdx can give the schema a once-over as well, to see if there are any known issues coming down the road, and to see if DOCUMENT-387 has a resolution before we commit to this.

@jbondpdx
Copy link
Contributor

jbondpdx commented Jun 2, 2017

No known issues pending afaik, but I am in fact working on a fix for DOCUMENT-387. It's taking longer than it should because I'm also restructuring the page, but I'm hoping to get a PR to the docs site tomorrow.

If it helps, the section on requirements says:

The requirements key specifies external requirements for the module, particularly the Puppet version required. Although you can express any requirement here, the Puppet Forge module pages and search function support only the "puppet" requirement for Puppet version.

Specify requirements in the following format:

  • "name": The name of the requirement.
  • "version_requirement": A semver version range similar to dependencies.
"requirements": [
  {"name": "pe”, “version_requirement”: “3.x”}
]

For Puppet Enterprise versions, specify the core Puppet version of that version of Puppet Enterprise. For example, Puppet Enterprise 2017.1 contained Puppet 4.9. We do not recommend expressing requirements for Puppet versions earlier than 3.0, because they do not follow semver.

For information about formatting version requirements, see the related topic about version specifiers in module metadata.

Validating metadata.json with a JSON schema provides very easy and
reliable type checking, key checking, and basic validation of string
formats and lengths without duplicated code or needing to add lots of
very similar acceptance tests.

It also standardises error responses, using the top-level key name in
the JSON format mode for any errors within that part of the document.
@domcleal
Copy link
Contributor Author

domcleal commented Jun 2, 2017

Rebased, removing the broken URL for the summary warning. I think the schema matches what you're proposing for DOCUMENT-387, so thanks for that.

@rnelson0
Copy link
Member

rnelson0 commented Jun 2, 2017

I think release 1.2.2 is okay now, but though I approved this I'm not hitting merge cause I haven't had time to look into it and I don't have the time right now to delve into that. @domcleal go ahead and merge if you think we're good to go.

@domcleal
Copy link
Contributor Author

domcleal commented Jun 3, 2017

Will do, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants