carvel-dev / ytt

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

overlay/remove added key with null value #343

Closed Birdrock closed 3 years ago

Birdrock commented 3 years ago

What steps did you take: Modified the ytt playground example-multiple-data-values as follows:

config.yml

#! it can be useful to split data values
#! into multiple files, then combine them
#! before templating, for example,
#! to split default and production values

#@ load("@ytt:data", "data")

keys: #@ data.values.keys
names: #@ data.values.names

values-defaults.yml

#@data/values
---
names:
- default.example.com

values-production.yml

#@ load("@ytt:overlay", "overlay")
#@data/values
---
#@overlay/match missing_ok=True
keys:
  #@overlay/remove
  key1:
  key2: val2-new
  key3: val3
names:
#@overlay/match by=overlay.subset("default.example.com")
#@overlay/remove
-
- production.example.com

What happened:

keys:
  key1: null
  key2: val2-new
  key3: val3
names:
- production.example.com

keys.key1 was inserted with a null value despite #@overlay/remove.

What did you expect:

keys:
  key2: val2-new
  key3: val3
names:
- production.example.com

Environment:

gcheadle-vmware commented 3 years ago

Hi @Birdrock,

The #@overlay/remove action will match and remove the node on the left side (values-defaults.yml), based on the right side (values-production.yml). Since it looks like you deleted the keys: map and its children in values-defaults.yml, the #@overlay/remove action in your overlay will not match any nodes in values-defaults.yml, thus no action will be taken. If you want the result:

keys:
  key2: val2-new
  key3: val3
names:
- production.example.com

then you could simply delete the #@overlay/remove ... key1: from the values-production.yml.

Let me know if this makes sense to you; I'm happy to try and answer any further questions you have.

aaronshurley commented 3 years ago

Hey @Birdrock, did @gcheadle-vmware's comment above resolve your issue?

cppforlife commented 3 years ago

per related slack thread...

we have an item in https://github.com/vmware-tanzu/carvel-ytt/issues/112

traverse value for merge/replace/append/insert (since they may include overlay directives) when short circuiting

that would solve this problem. in summary, ytt should not interpret chunk of yaml as plain yaml once it decided that key is missing and it needs to add chunk of yaml. instead it should interpret it as an overlay which would account for annotations such as overlay/remove.

pivotaljohn commented 3 years ago

Note: referred to Slack thread = https://kubernetes.slack.com/archives/CH8KCCKA5/p1617742603193700

So, I'm reading:

For the first part: looks resolved. As such, I'll close this issue.

For the second part: #112 is effectively an Epic... before we migrated to ZenHub.

I'll:

  1. convert #112 to a ZenHub epic
  2. convert the to do item @cppforlife referenced into a story #371 attached to that epic so that it can be prioritized.
    • reference this issue in that story.

@Birdrock I think this part mates up with your expectations of how the feature should work.

Birdrock commented 3 years ago

:+1:

pivotaljohn commented 3 years ago

For those keeping score at home, tracking this class of symptoms as a "bug" first reported in #308.

The work to fix this is in #371.