Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

Adds json schema validator to fix issue #14 #17

Closed
wants to merge 3 commits into from

Conversation

nyoungstudios
Copy link
Contributor

Adds json schema validator to fix issue #14
Adds moment.js to dependencies
And changes the test-service to use local module for development

@nyoungstudios
Copy link
Contributor Author

Only problem is that I couldn't figure out how to add a custom error message. So, currently it prints out all the valid timezone names if the provided timezone doesn't match the schema.

@edlea
Copy link

edlea commented Sep 14, 2021

Rather than add a dependency on moment-timezone it might be simpler just to set the type of timezone as string

@KillDozerX2
Copy link
Contributor

@pgrzesik Can this be merged? I've got another PR if you don't like this, can move it here directly

Copy link
Contributor

@pgrzesik pgrzesik 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 overall, but requires an adjustment to not. break for older Framework versions cc @KillDozerX2

@@ -121,6 +122,16 @@ function convertCrontabs() {
class ServerlessLocalCrontabs {
constructor(serverless, options) {
this.serverless = serverless;

// Create schema for your properties. For reference use https://github.com/ajv-validator/ajv
this.serverless.configSchemaHandler.defineFunctionEventProperties('aws', 'schedule', {
Copy link
Contributor

Choose a reason for hiding this comment

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

it should check if this.serverless.configSchemaHandler is defined first to avoid breaking for older Framework versions

Copy link
Contributor

Choose a reason for hiding this comment

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

I will send PR directly then

@pgrzesik
Copy link
Contributor

Close in favor of: #25

@pgrzesik pgrzesik closed this Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants