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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Schema Validation #331

Merged
merged 11 commits into from
Sep 27, 2022
Merged

Proposal: Schema Validation #331

merged 11 commits into from
Sep 27, 2022

Conversation

pivotaljohn
Copy link
Contributor

No description provided.

@pivotaljohn pivotaljohn force-pushed the 004-schema-validation branch from c53cb45 to 3c3ebb5 Compare January 12, 2022 18:03
@pivotaljohn pivotaljohn force-pushed the 004-schema-validation branch 5 times, most recently from 09c7750 to 67fdd73 Compare January 28, 2022 01:04
@pivotaljohn pivotaljohn force-pushed the 004-schema-validation branch 7 times, most recently from d7876e5 to 8b9479e Compare February 7, 2022 20:11
@pivotaljohn pivotaljohn force-pushed the 004-schema-validation branch from 8b9479e to e8d30b1 Compare February 8, 2022 01:02
@pivotaljohn pivotaljohn marked this pull request as ready for review February 8, 2022 18:35
- exclusiveMaximum (is this useful?)
- exclusiveMinimum (is this useful?)
- [multipleOf](https://datatracker.ietf.org/doc/html/draft-wright-json-schema-validation-00#section-5.1)
- pattern (This string SHOULD be a valid regular expression, according to the ECMA 262 regular - expression dialect)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cppforlife noted:
ECMA 262 != RE2

Q: what approach have others who have implemented this feature (specifically JSON Schema validation keyword pattern) in Go done?

Following discussions with
[gcheadle-vmware](carvel-dev/ytt#604 (comment))
and @cppforlife, it became clear that @assert/validate ought to always
be consumed in a step after overlays are applied for the current
pipeline.

When writing schema, the author intends not for the schema document
_itself_ to be validated, but to validate the _produced_ document (of
which the schema is a basis for).

Authors need a way to declare the intention for a validation to be
attached to the type composite: @schema/validation.
The former is a predicate (expected to return True or False), the later
is an assertion.
@pivotaljohn pivotaljohn force-pushed the 004-schema-validation branch from 744ed61 to 2536c27 Compare February 16, 2022 23:55
It seems quite likely that we'll need such a facility for more advanced
cases. Placed in the "Other approaches considered" means that the ideas
are found without sufficient merit; not the case here.
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

I read through the proposal and it looks very well thought out, AWESOME JOB @pivotaljohn
Added some comments and questions in some of the points.

proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
```python
lib.eval(output_disable_validation=True)
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vrabbi asks:

Any thought on allowing to skip a specific violation?
Not sure what the syntax would be but i can see cases where when consuming third party packages i may want to disable a specific validation that isnt valid in my case.
I don't think its a very common case but it would allow for a break glass solution that is still more secure then disabling all validations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first blush, this starts to make more compelling the need for the ability for overlays to modify metadata in addition to structure (i.e. edit annotations, not just the YAML structure).... 🤔

Curious if others have thoughts about this:

  • can you name specific use cases where this would occur (or speculate as to how frequent this would be)?
  • ideas on a syntax for doing this via the command-line?

(from conversation with @vrabbi; that you Scott!)
Copy link

@jorgemoralespou jorgemoralespou left a comment

Choose a reason for hiding this comment

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

I like the proposal, and looking forward to be able to play with an initial implementation to see if all the use-cases we have can be satisfied. It's difficult to think whether all could work by just reading this proposal. But so happy to see this coming through.

proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
proposals/ytt/004-schema-validation/README.md Show resolved Hide resolved
#@ end

#@data/values-schema
#@schema/validation ("There should be exactly one (1) registry replica if Helm Charts are stored on the filesystem.", one_registry_if_pvc_is_filesystem)

Choose a reason for hiding this comment

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

I think there's going to be a common pattern for this "top-node" functions where sub-nodes on the schema will be validated (or take the value from/if) a top level node is defined. An example is what CNR/TAP is defining as TLK (Top Level Keys).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting...

We should talk more about what you see, here... cooking up some examples might be useful/insightful.

```yaml
#@data/values-schema
---
#@schema/validation ("", valid_service_config), when=lambda v: v.enabled

Choose a reason for hiding this comment

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

Is the lambda v: part required? Could also this be supported?

#@schema/validation ("", valid_service_config), when=this.enabled
service:
  enabled: true

Use of keyword this to denote this node (or similar), or fully reference a value key:

#@schema/validation ("", valid_service_config), when=service.enabled
service:
  enabled: true

I understand the constraint defined above that data.values are not available at pre-process, but definitely syntax is not very user friendly.

Copy link
Contributor Author

@pivotaljohn pivotaljohn Mar 8, 2022

Choose a reason for hiding this comment

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

We need some kind of "callable" that can be invoked in the validation stage: yes.

I like the intention, here.

I think this involves detecting when the argument is not a "callable" and then inject the lambda wrapper...

#@schema/validation ("", valid_service_config), when=this.enabled

is compiled to:

#@schema/validation ("", valid_service_config), when=lambda this: this.enabled

We'll need to make sure that this "auto-wrapping" doesn't accidentally cover-up or make harder-to-debug issues when the expression is invalid for some reason... 🤔

@pivotaljohn pivotaljohn merged commit 3680af8 into develop Sep 27, 2022
@pivotaljohn pivotaljohn deleted the 004-schema-validation branch September 27, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants