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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement property schema validation #74

Conversation

crunchie84
Copy link
Contributor

Fixes #73

TODOS

  • document schema
  • manually verify JSON schema (https://www.jsonschemavalidator.net/)[https://www.jsonschemavalidator.net/]
  • Verify that unit tests still run
    • I didn't see an easy way to add unit tests for the json schema 馃
  • Test that when using this version of the plugin the console warning is no longer rendered
    • If the maintainer of this repo easily can do the test that would be really great, need to figure out how to link a local version of a plugin into my codebase

Copy link

@danielharbor danielharbor left a comment

Choose a reason for hiding this comment

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

Hi @crunchie84, is there anything currently blocking this PR? I ask because this fix turns out to be one that my project is currently in need of.

@crunchie84 crunchie84 changed the title WIP: Implement property schema validation Implement property schema validation Mar 23, 2023
@crunchie84
Copy link
Contributor Author

Hey @danielharbor I contributed this fix a long time a go. Im currently not in the position to fix the static code analysis issue in the cicd so if the maintainers can address that its LGTM and shippable

@danielharbor
Copy link

Hey @danielharbor I contributed this fix a long time a go. Im currently not in the position to fix the static code analysis issue in the cicd so if the maintainers can address that its LGTM and shippable

Thanks @crunchie84, we'll try to push it along.

In serverless v2.7.0 schema validation of the serverless.yml has been
implemented (see serverless/serverless#8422 (comment))

Based on this you can trigger errors when parameters are referenced which
do not exist. This plugin relies on some custom properties in the serverless.yml
which caused warnings.

This commit appends the JSON schema as described in the README.md of this
plugin
@crunchie84 crunchie84 force-pushed the feature/fix-73_invalid-function-property-warning branch from 7e592f0 to 3300add Compare March 24, 2023 14:43
@crunchie84
Copy link
Contributor Author

@danielharbor I've rebased the PR on the current state of master and locally made the linting issues pass. Lemme know if that makes it easy for you to merge this PR

@danielharbor
Copy link

@danielharbor I've rebased the PR on the current state of master and locally made the linting issues pass. Lemme know if that makes it easy for you to merge this PR

This is great to hear @crunchie84. Thank you 馃. I'll also try to see if I can find a way to get the last item checked off:

  • Test that when using this version of the plugin the console warning is no longer rendered

@mluhovyi I do see that you've done quite a bit of work in this repo recently. Please any assistance you can offer in terms of getting the unchecked item above verified and/or reviewing the pull request would be much appreciated.

@rddimon rddimon merged commit 7ce8869 into amplify-education:master Dec 26, 2023
@danielharbor
Copy link

FYI @g-bartoszek

@danielharbor
Copy link

Thanks, @rddimon!

@rddimon
Copy link
Contributor

rddimon commented Dec 26, 2023

Hi @danielharbor!

It's not released yet
I'm still working on some improvements
But I hope we will release it this year

@danielharbor
Copy link

Hi @danielharbor!

It's not released yet I'm still working on some improvements But I hope we will release it this year

Thanks for the heads up, @rddimon. We'll try to keep an eye out for the new release but please, in case you remember, we would appreciate an update here as well. Thanks so much!

@rddimon rddimon mentioned this pull request Jan 17, 2024
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.

Invalid configuration encountered: unrecognized property 'logForwarding'
3 participants