carvel-dev / ytt

YAML templating tool that works on YAML structure instead of text
https://carvel.dev/ytt
Apache License 2.0
1.63k stars 137 forks source link

Disallow both "any" and "nullable" types on the same node #400

Closed pivotaljohn closed 2 years ago

pivotaljohn commented 3 years ago

Story

Context

We want to preserve behavior associated with combining @schema/... annotations until a time when we can better appreciate the consequences so as to avoid breaking changes down the line.

In order to do so, we would need to establish that combining these annotation now to be an error case so that users do not rely on them.

πŸ”΄ Node is annotated with both @schema/nullable and @schema/type

#@schema/match data_values=True
---
#@schema/type any=False
#@schema/nullable
foo: 42

(regardless any=False or any=True)

$ ytt -f . --enable-experiement-schema
ytt: Error:
schema.yml:5 | foo: 42
             |
             | INVALID SCHEMA - `@schema/nullable` and `@schema/type` are mutually exclusive; use one or the other

Bonus: include a hint that indicates that @schema/type any=True includes null.


Original discussion

This issue is a place to discuss the pros and cons of limiting the combination of @schema/type any=True and @schema/nullable

Allowing Both

A Configuration Author can offer their users a means to relax Schema type checks for a specific Data Value (or section of Data Values).

#@schema/match data_values=True
---
#@schema/type any=False
#@schema/nullable
foo: 42

means: foo :: null or int

#@schema/match data_values=True
---
#@schema/type any=True
#@schema/nullable
foo: 42

means: foo :: any or null or int

Q: given that Schema are processed before data values, by what means would a Configuration Consumer reasonably provide some kind of input that could have an affect in Schema files?

Disallowing Both

If we prevent this combination, now, it's an error condition to do so. If we later see a live use-case where these two are desired together, we can relax this constraint without creating a breaking change.

cppforlife commented 3 years ago

should we allow schema/type and schema/nullable be present on the same node?

pivotaljohn commented 3 years ago

should we allow schema/type and schema/nullable be present on the same node?

yes. That's exactly the question. πŸ‘πŸΌ

gcheadle-vmware commented 3 years ago

From the Schemas HackMD, the long form of @schema/nullable is:

#@schema/type one_of="null", or_inferred=True
#@schema/default None

So, if we wanted to add to the possible types, for example add 'bool', we would add to the one_of list:

#@schema/type one_of=["null", "bool"], or_inferred=True
#@schema/default None

which, if we resubstitute @schema/nullable, should be equivalent to:

#@schema/type one_of="bool"
#@schema/nullable

Another example where disallowing both might get confusing: We think @schema/type any=True could also be represented as #@schema/type one_of=["null", "bool", "string", "int", "map", "array", "document"] So, should we error in the case of being combined with @schema/nullable?

#@schema/type one_of=["null", "bool", "string", "int", "map", "array", "document"]
#@schema/nullable

cc: @DennisDenuto

aaronshurley commented 2 years ago

To follow up on @gcheadle-vmware's comment, we're planning to take a defensive approach now and disallow "any" and "nullable". We can make changes in the future when one_of is implemented to open things up, if desired.

gcheadle-vmware commented 2 years ago

After discussion with @pivotaljohn about backwards compatibility and the future of the @schema/type annotation, we decided it would be best to allow the case of @schema/type any=False and @schema/nullable being on the same node since the @schema/type annotation is not attributing any information. We are keeping the failure case of the @schema/type any=True and @schema/nullable because @schema/type any=True removes all schema-related type-checking from that node.