Skip to content

Validate configuration against json schema #17

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

Merged
merged 1 commit into from
Jun 8, 2020
Merged

Validate configuration against json schema #17

merged 1 commit into from
Jun 8, 2020

Conversation

belemaire
Copy link
Member

Fail early and clearly in case some configuration provided to the cli or any services is invalid.

What's changed

  • loadConfig<T> function is now asynchronous
  • Extracted YAML file loading logic from loadConfig<T> to a dedicated new function loadYamlFile<T>

What's new

  • Added JSON schemas for LiveBundle store, qrcode, github and cli configurations.
  • Added JSON schema for LiveBundle SDK task (referenced in configuration schemas)
  • Added dependency on ajv (used for schema validation) to livebundle-utils
  • Added new function schemaValidate to livebundle-utils, using ajv to validate an object against a given schema.
  • Updated. loadConfig<T> function, now accepting two more properties schema and refSchemas to be used to provide a schema (and optional reference schemas) to validate configuration against.
  • Updated all existing client calls to loadConfig<T> to provide the proper schemas.
  • Added resolveJsonModule property to tsconfig.json and tsconfig.build.json. Needed to allow direct import of json files.
  • Added simple tests just to make sure that default configurations could still be properly loaded after these changes. More tests will come in a future PR, specifically dedicated to the improvement of our test coverage.

Remarks

  • The URIs specified in the schemas ($id) are only used to identify the schemas (and required to properly use schema references). It is totally OK for these URIs not to really exist. According to JSON schema specifiation, validators should not expect to be able to download the schemas from these URIs.
  • The update of "include": ["src/**/*"] to "include": ["./src/**/*", "./src/**/*.json"] in modules tsconfig.build.json might seem unnecessary. It is however needed. See this issue for reference.

Closes #16

Copy link
Member

@friederbluemle friederbluemle left a comment

Choose a reason for hiding this comment

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

👍

Interesting.. It looks like there are some issues with pull requests from forks to private repos and GitHub Actions (no build was kicked off)... :/

https://github.bestrui.topmunity/t/will-github-actions-support-pull-request-events-from-a-fork-to-a-private-base-repository/17471

Just saw this, haven't looked more into it.

Locally I'm getting this test error (you probably already know about it) ;)

  config
    1) should not fail loading default config


  0 passing (11ms)
  1 failing

  1) config
       should not fail loading default config:
     TypeError [ERR_INVALID_ARG_TYPE]: The "promiseFn" argument must be of type function or an instance of Promise. Received an instance of Object
      at waitForActual (assert.js:716:11)
      at Function.doesNotReject (assert.js:838:39)
      at Context.<anonymous> (test/config.test.ts:10:11)
      at processImmediate (internal/timers.js:456:21)

@belemaire
Copy link
Member Author

belemaire commented Jun 1, 2020

That's weird I'm not running into this test error :( Otherwise wouldn't have pushed in the first place.

Have you properly run yarn install after checking out my branch, prior to running yarn test ?
This is not done automatically in case of dependency changes as it is for electrode-native repo.

Also just noticed you'll have to run yarn build prior to yarn test. It needs some stuff from dist, will add build as a pre step to test script through another PR.

Just tried it out from a fresh clone and it works fine.

Let me know if that clears it out for you.

@friederbluemle
Copy link
Member

Ooh.. yeah, I did install the deps, but I guess I didn't run yarn build before yarn test. Why is that necessary?

@belemaire
Copy link
Member Author

It shouldn't be necessary, will have a closer look.

@belemaire belemaire merged commit 9323aa5 into electrode-io:master Jun 8, 2020
@belemaire belemaire deleted the schemas branch June 9, 2020 17:31
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.

Configuration validation
2 participants