Skip to content

Conversation

@mayorova
Copy link
Contributor

@mayorova mayorova commented May 7, 2025

@mayorova mayorova requested review from a team as code owners May 7, 2025 13:52
@@ -1,5 +1,5 @@
{
"$schema": "http://apicast.io/poolicy-v1/schema#manifest#",
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

According to #522 and #565 the manifest and schema is mostly used for the UI. And from #646, we only run validation with busted test. So as long as the test passes, I'm happy.

@mayorova with porta changing to json_schemer, do we still validate each policy schema against the manifest?

Copy link
Contributor Author

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.

Copy link
Contributor

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

@tkan145
Copy link
Contributor

tkan145 commented May 9, 2025

Also, if a user has a custom policy and performs upgrade with the old schema, would that cause any problems?

@mayorova
Copy link
Contributor Author

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 #manifest, see here. There is also a test here

@mayorova mayorova force-pushed the update-apicast-schema-manifest branch from 0e10caf to 8405560 Compare May 12, 2025 12:40
@tkan145
Copy link
Contributor

tkan145 commented May 14, 2025

@mayorova LGTM, but please don't merge it yet, I need to release version 2.16 with the old scheme first.

@tkan145
Copy link
Contributor

tkan145 commented Jun 10, 2025

@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

@mayorova mayorova force-pushed the update-apicast-schema-manifest branch from ba92521 to c7b4516 Compare June 26, 2025 14:24
@mayorova mayorova merged commit cd3517a into master Jul 8, 2025
13 of 16 checks passed
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.

3 participants