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

add schema dirs to dist dirs #272

Merged
merged 5 commits into from
Mar 28, 2018
Merged

add schema dirs to dist dirs #272

merged 5 commits into from
Mar 28, 2018

Conversation

sethvincent
Copy link
Contributor

@sethvincent sethvincent commented Mar 22, 2018

Summary: This makes sure the schemas directory is placed in the dist directory for tasks that use the cumulus-message-adapter.

Addresses CUMULUS-414: Schema validation not being performed on many tasks

Changes

  • add schema dirs to dist dirs

Example error

Here's an example of an exception thrown in discover-granules because of a failing schema validation:

{
  "error": "CumulusMessageAdapterExecutionError",
  "cause": "{\"errorMessage\":\"Unexpected error:<class 'jsonschema.exceptions.ValidationError'>. config schema: u'provider' is a required property\\n\\nFailed validating u'required' in schema:\\n    {u'description': u'Describes the config used by the discover-granules task',\\n     u'properties': {u'buckets': {u'description': u'aws s3 buckets used by this task',\\n                                  u'properties': {u'internal': {u'type': u'string'},\\n                                                  u'private': {u'type': u'string'},\\n                                                  u'protected': {u'type': u'string'},\\n                                                  u'public': {u'type': u'string'}},\\n                                  u'type': u'object'},\\n                     u'collection': {u'properties': {u'files': {u'items': {u'properties': {u'bucket': {u'type': u'string'},\\n                                                                                           u'regex': {u'type': u'string'},\\n                                                                                           u'sampleFileName': {u'type': u'string'}},\\n                                                                           u'type': u'object'},\\n                                                                u'type': u'array'},\\n                                                     u'granuleId': {u'type': u'string'},\\n                                                     u'granuleIdExtraction': {u'type': u'string'},\\n                                                     u'name': {u'type': u'string'},\\n                                                     u'provider_path': {u'type': u'string'},\\n                                                     u'sampleFileName': {u'type': u'string'},\\n                                                     u'url_path': {u'type': u'string'},\\n                                                     u'version': {u'type': u'string'}},\\n                                     u'required': [u'name', u'files'],\\n                                     u'type': u'object'},\\n                     u'provider': {u'properties': {u'globalConnectionLimit': {u'type': u'integer'},\\n                                                   u'host': {u'type': u'string'},\\n                                                   u'id': {u'type': u'string'},\\n                                                   u'password': {u'type': u'string'},\\n                                                   u'port': {u'type': u'integer'},\\n                                                   u'protocol': {u'enum': [u'ftp',\\n                                                                           u'sftp',\\n                                                                           u'http',\\n                                                                           u'https',\\n                                                                           u's3'],\\n                                                                 u'type': u'string'},\\n                                                   u'username': {u'type': u'string'}},\\n                                   u'required': [u'host', u'protocol'],\\n                                   u'type': u'object'},\\n                     u'useList': {u'default': False,\\n                                  u'description': u\\\"flag to tell ftp server to use 'LIST' instead of 'STAT'\\\",\\n                                  u'type': u'boolean'}},\\n     u'required': [u'provider'],\\n     u'title': u'DiscoverGranulesConfig',\\n     u'type': u'object'}\\n\\nOn instance:\\n    {u'bar': u'baz'}\",\"errorType\":\"CumulusMessageAdapterExecutionError\",\"stackTrace\":[\"\",\"Failed validating u'required' in schema:\",\"    {u'description': u'Describes the config used by the discover-granules task',\",\"     u'properties': {u'buckets': {u'description': u'aws s3 buckets used by this task',\",\"                                  u'properties': {u'internal': {u'type': u'string'},\",\"                                                  u'private': {u'type': u'string'},\",\"                                                  u'protected': {u'type': u'string'},\",\"                                                  u'public': {u'type': u'string'}},\",\"                                  u'type': u'object'},\",\"                     u'collection': {u'properties': {u'files': {u'items': {u'properties': {u'bucket': {u'type': u'string'},\",\"                                                                                           u'regex': {u'type': u'string'},\",\"                                                                                           u'sampleFileName': {u'type': u'string'}},\",\"                                                                           u'type': u'object'},\",\"                                                                u'type': u'array'},\",\"                                                     u'granuleId': {u'type': u'string'},\",\"                                                     u'granuleIdExtraction': {u'type': u'string'},\",\"                                                     u'name': {u'type': u'string'},\",\"                                                     u'provider_path': {u'type': u'string'},\",\"                                                     u'sampleFileName': {u'type': u'string'},\",\"                                                     u'url_path': {u'type': u'string'},\",\"                                                     u'version': {u'type': u'string'}},\",\"                                     u'required': [u'name', u'files'],\",\"                                     u'type': u'object'},\",\"                     u'provider': {u'properties': {u'globalConnectionLimit': {u'type': u'integer'},\",\"                                                   u'host': {u'type': u'string'},\",\"                                                   u'id': {u'type': u'string'},\",\"                                                   u'password': {u'type': u'string'},\",\"                                                   u'port': {u'type': u'integer'},\",\"                                                   u'protocol': {u'enum': [u'ftp',\",\"                                                                           u'sftp',\",\"                                                                           u'http',\",\"                                                                           u'https',\",\"                                                                           u's3'],\",\"                                                                 u'type': u'string'},\",\"                                                   u'username': {u'type': u'string'}},\",\"                                   u'required': [u'host', u'protocol'],\",\"                                   u'type': u'object'},\",\"                     u'useList': {u'default': False,\",\"                                  u'description': u\\\"flag to tell ftp server to use 'LIST' instead of 'STAT'\\\",\",\"                                  u'type': u'boolean'}},\",\"     u'required': [u'provider'],\",\"     u'title': u'DiscoverGranulesConfig',\",\"     u'type': u'object'}\",\"\",\"On instance:\",\"    {u'bar': u'baz'}\",\"CumulusMessageAdapterExecutionError (/var/task/index.js:8952:6)\",\"ChildProcess.cumulusMessageAdapter.on.code (/var/task/index.js:8982:85)\",\"emitTwo (events.js:106:13)\",\"ChildProcess.emit (events.js:191:7)\",\"maybeClose (internal/child_process.js:886:16)\",\"Process.ChildProcess._handle.onexit (internal/child_process.js:226:5)\"]}"
}

This ^ is the expected behavior that was missing that this PR addresses. It'll make it easier to debug collection/rule configuration problems.

Test Plan

Things that should succeed before merging.

  • Unit tests
  • Adhoc testing
  • Update CHANGELOG
  • Run ./bin/eslint-ratchet and verify that eslint errors have not increased

@marchuffnagle
Copy link
Contributor

Starting to review this now.

"build": "webpack --progress",
"watch": "webpack --progress -w",
"build": "rm -rf dist && mkdir dist && cp -R schemas dist/ && webpack --progress",
"watch": "rm -rf dist && mkdir dist && cp -R schemas dist/ && webpack --progress -w",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually need to be in the watch step? It only really matters for creating the zip file that's deployed, and that relies on the build step (if I'm correct).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually try to keep the behavior of build and watch really similar. In this case including it as part of watch will be useful for working on tasks when they are npm linked into deployments, which is something I've needed to do before.

Copy link
Contributor

@marchuffnagle marchuffnagle left a comment

Choose a reason for hiding this comment

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

👍

@scisco scisco merged commit f0e7601 into master Mar 28, 2018
@scisco scisco deleted the cumulus-414-schemas-in-dist branch March 28, 2018 13:19
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