carvel-dev / ytt

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

Overlay'ed data values set to Starlark expressions are schema-type compatible #342

Open pivotaljohn opened 3 years ago

pivotaljohn commented 3 years ago

As a Configuration Consumer I want to overlay my Data Values with using an @overlay/replace function and have the result be schema type-checkable So that such configuration documents can properly benefit from schema-enforced checks

Context

As detailed in the comments, below, currently when an overlay includes an @overlay/replace via=... the result of that function is converted to its Go equivalent and not AST (i.e. composed of instances of Node). Specifically:

Schema expects to check against a ytt AST and fails when type-checking against these values.

Acceptance Criteria

🟢 Starlark expressions that conform to schema

Given A schema

#! schema.yml
#@schema/match data_values=True
---
db_conn:
- hostname: ""

And a conforming data value that includes YAML inserted from a Starlark expression

#! values.yml
#@data/values
---
#@overlay/replace via=lambda l,r: [{"hostname": "db.example.com"}]
db_conn:

When I run ytt Then The result passes schema checks

$ ytt -f schema.yml -f values.yml --enable-experiment-schema --data-values-inspect
db_conn:
- hostname: db.example.com

🔴 Starlark expressions that do not conform to schema

Given A schema

#! schema.yml
#@schema/match data_values=True
---
db_conn:
- hostname: ""

And a non-conforming data value that includes YAML inserted from a Starlark expression

#! values.yml
#@data/values
---
#@overlay/replace via=lambda l,r: [{"hostname": 42}]
db_conn:

When I run ytt Then The result fails schema checks

$ ytt -f schema.yml -f values.yml --enable-experiment-schema --data-values-inspect
ytt: Error: Overlaying data values (in following order: values.yml):
values.yml:4 | db_conn:
             |
             | TYPE MISMATCH - the value of this item is not what schema expected:
             |      found: integer
             |   expected: string (by schema.yml:4)
gcheadle-vmware commented 3 years ago

this may be about the following invariant: regardless of source, a YAML tree in ytt must be composed of only AST nodes.

In the overlay package in op.gp, we convert an interface to AST nodes. However, the #@overlay/replace via=lambda l,r: [{"hostname": "db.example.com"}] is stored in the metas of the node in the AST and is not converted to YAML until later on. Once the overlay apply finishes, we then have an orderedmap.Map in the AST, which is not an AST node.

I did find that adding leftObj = yamlmeta.NewASTFromInterface(leftObj) after the overlay apply made the happy case test pass.

cari-lynn commented 3 years ago

We talked about splitting this issue up into more than one in order to keep the scope small based on either

  1. where the starlark code is evaluated (ie: #@overlay/replace via=lambda l,r: [{"hostname": 42}] or just in `#@ ["foo"]
  2. What type is return in the starlark code (ie: support collections, then maps)

I also consider this a more advanced use case than other schema stories so far. Data values files most often do not need to have starlark evaluated when setting a value, although there are plenty of cases where they do (such as base64 encoding a value). So it makes sense to wait to complete this work until we have a solid foundation of schema features.

pivotaljohn commented 3 years ago

Per @cari-lynn's note, moved to next Schema epic to reflect reprioritization.

pivotaljohn commented 3 years ago

@gcheadle-vmware noted:

I did find that adding leftObj = yamlmeta.NewASTFromInterface(leftObj) after the overlay apply made the happy case test pass.

I'm nearly positive that the only overlay operation that injects values like this is @overlay/replace... would it be enough to ensure that the result from the via function is AST?