-
Notifications
You must be signed in to change notification settings - Fork 171
Update apicast schema manifest #1550
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
Conversation
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "$schema": "http://apicast.io/poolicy-v1/schema#manifest#", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me think that APIcast never cares about the schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkan145 I am not very familiar with the code, but from here it seems to me that APIcast only validates the configuration of the policy definition, not the complete JSON.
As for porta, it's strange, because I think the validation only applies to the policies definitions that are created manually via API. And yes, the whole policy definition is validated against the manifest.
For the policies fetched from APIcast, I don't think we run any validations, just render them in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayorova yes you are right, I think they used to validate the schema against the manifest manually with this docker image https://github.com/3scale-archive/docker-ajv
|
Also, if a user has a custom policy and performs upgrade with the old schema, would that cause any problems? |
I tried to keep backwards compatibility, so that the old policies stored in the database still keep validating correctly. The aproach is ignoring |
0e10caf to
8405560
Compare
|
@mayorova LGTM, but please don't merge it yet, I need to release version 2.16 with the old scheme first. |
|
@mayorova Sorry for the long delay, we had some issues with luarocks (lua package manager). Can you rebase on top of master branch? Once CI passes we can merge this one |
ba92521 to
c7b4516
Compare
This is to align with the changes proposed in https://github.com/3scale/porta/pull/4069/files#diff-90f30fa09f863d71761b6497bab4b2c299b7b29a5685f84fe7ae42ab792e318a