search

operator-framework / operator-courier

Build, verify and push operators
Apache License 2.0
41 stars 53 forks source link

Start using JSON Schema for validation #83

Closed csomh closed 2 years ago

csomh commented 5 years ago

Refactor CRD and CSV validation, and use JSON Schema instead of nested if-s.

This should make future validation easier.

TODO:

openshift-ci-robot commented 5 years ago

Hi @csomh. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
csomh commented 5 years ago

This should resolve #17 and #39.

kevinrizza commented 5 years ago

@csomh Just wanted to say good work on this! It seems like it satisfies a lot of the basic cases, and allows us to just codify the more complex comparisons. I would like to take some time and ensure that all of the use cases that were covered previously are still working here -- but in general I do like this approach.

One thing I want to point out is that this does change the formatting of most/all of the log messages, which to me is a breaking change -- so we would have to either update this PR to retain the old format of the logs or we would need to bump the major version. I would prefer the former.

scoheb commented 5 years ago

FYI @kevinrizza as one of the consumers of courier...CVP does not rely on the format of the log messages.

csomh commented 5 years ago

@kevinrizza thanks!

As for the format of log messages: would it be possible to sync releasing this change with the future fix for #74, which will probably also change the format of the log messages?

I'm asking this, because keeping the old format of the messages requires parsing the error.message from jsonschema and reconstructing the current error messages. But then, the current error messages resulting from validation issues will probably not match in style the new error messages that will pop up due to fine tuning the schemas.

I would rather stick to the current _concatenate error.absolute_path with error.message_ approach, as it's short, simple and provides a consistent style on the long run.

kevinrizza commented 5 years ago

@csomh Agreed, if we are going to make format changes to all of the log messages, my preference would be to make a more sweeping change that would give more detail as to what csv or crd is causing the error. I'm not opposed to the changes to the logger in principle, just to the timing of them. If that is directly tied to this PR because of technical reasons, then lets hold this pr until we make that change as well.

openshift-ci-robot commented 5 years ago

@csomh: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
csomh commented 2 years ago

I think this isn't relevant anymore :slightly_smiling_face: Closing.