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

[overlay] improve overlay mismatch error message by specifying left/right side context #95

Open
cppforlife opened this issue Feb 20, 2020 · 3 comments
Labels
discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution error msg improvement

Comments

@cppforlife
Copy link
Contributor

some error message improvements to indicate left vs right nodes

Document on line demo.yml:10 [matched demo.yml:1]
  Map item key 'clients' on line demo.yml:11 [matched demo.yml:2]
    Map item key 'client1' on line demo.yml:12 [did not match]
      Expected number of matched left map items to be 1, but was 0

vs

left=demo.yml right=overlay.yml
left=:1 right=:10 [matched]  Document
left=:2 right=:11 [matched]    Map item key 'clients'
left=?  right=:12 [error]        Map item key 'client1'
                                   Expected number of matched left map items to be 1, but was 0

vs

left=demo.yml:1 right=demo.yml:10 [matched]  Document
left=demo.yml:2 right=demo.yml:11 [matched]    Map item key 'clients'
left=?          right=demo.yml:12 [error]        Map item key 'client1'
                                                   Expected number of matched left map items to be 1, but was 0

originated from #94

/cc @jtigger

@jtigger
Copy link
Contributor

jtigger commented Feb 20, 2020

(more sketching...)

Given:

test.yml
 1: ---
 2: clients:
 3: - client1:
 4:     secret: blah1
 5: - client2:
 6:     secret: blah2
 7:
 8: #@ load("@ytt:overlay", "overlay")
 9: #@overlay/match by=overlay.all
10: ---
11: clients:
12: #@overlay/match by=overlay.all,expects="1+"
13: - client1:
14:     secret: foo

... yields ...

left:              right:
test.yml           test.yml
 1: ---          ✓ 10: Document — by=overlay.all
 2: clients:     ✓ 11:   Map item — by=<key equality> (default)
 5: -            ✓ 13:     Array item — by=overlay.all,expects="1+"
 5:   client2:   ✗ 13:       Map item — by=<key equality> (default)

✗ Map item (key 'client1') on line test.yml:13: Expected number of matched map items (by key equality) to be 1, but was 0
  • render left in "document" format — ytt keeps you thinking in terms of structure.
  • explicitly notate what matcher was applied to which node.
  • (as you suggested verbally) name concrete node type (here, map item)

@jtigger
Copy link
Contributor

jtigger commented Aug 24, 2020

Made some progress on this, this weekend.

Thinking we could get significant value with mere newlines, without indentation:

tpl.yml

---
clients:
- client1:
    secret: blah1
- client2:
    secret: blah2

overlay.yml

#@ load("@ytt:overlay", "overlay")
#@overlay/match by=overlay.all
---
clients:
#@overlay/match by=overlay.all,expects="1+"
- client1:
    secret: foo

yields:

Overlaying (in following order: overlay.yml):
overlay.yml:4: Document:
overlay.yml:5: Map item 'clients' (on Map @ tpl.yml:2):
overlay.yml:7: Array item [0]: (on Array @ tpl.yml:5):
overlay.yml:7: Map item 'client1' (on Map @ tpl.yml:5):
Expected number of matched nodes to be 1, but was 0

... and also surfacing the fact that *-items from overlays are being applied to the entire corresponding collection on the left (a source of confusion for some).

Thoughts?

@StevenLocke StevenLocke added the discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution label Aug 28, 2020
@StevenLocke
Copy link
Contributor

@ewrenn8 and I spent some time on this.

We're really good at showing the "too many" matches cases because we build up that collection of matches and then check its desired length, it's the "less than expected" matches that we're struggling to represent. Especially once we get into the larger scenarios: with 100 items, expecting 10 of them to match on kind: StatefulSet, but only 5 do, do we want to output 95 locations that don't match? We as the tool don't have any context about where the overlays should be matching. Or do we output only the last location that was checked? (which would then require running ytt multiple times). Plus, if the last location really shouldn't be matching, the error message is no longer helpful.

This is made harder because the code is building up error messages recursively, so outputting a location seems to require outputting the entire parent stack, like we do with the overlay in the current error message. Displaying the whole path to the node that didn't match may be more straightforward, but with multiple matches we may end up with some strange output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution error msg improvement
Projects
Status: To Triage
Development

No branches or pull requests

3 participants