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

Panics when appending to "empty" #718

Closed
mamachanko opened this issue Aug 21, 2022 · 9 comments · Fixed by #742
Closed

Panics when appending to "empty" #718

mamachanko opened this issue Aug 21, 2022 · 9 comments · Fixed by #742
Labels
bug This issue describes a defect or unexpected behavior

Comments

@mamachanko
Copy link
Contributor

What steps did you take:

#! dreams.yaml
#@ load("@ytt:overlay", "overlay")

#@overlay/match by=overlay.subset({"name": "Sandman", "profession": "King of Dreams"})
#@overlay/append
---
name: Matthew
profession: Raven
ytt -f dreams.yaml

What happened:

panic: runtime error: index out of range [-1]

goroutine 1 [running]:
github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay.Op.appendDocument(...)
        github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay/document.go:197
github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay.Op.applyDocSet({{0x148b260, 0xc0000aed08}, {0x1516700, 0xc0001595e0}, 0xc0001abd60, 0x0}, {0x19d9090, 0x1, 0x0}, 0xc0001595e0, ...)
        github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay/op.go:149 +0x805
github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay.Op.apply({{0x148b260, 0xc0000aed08}, {0x1516700, 0xc0001595e0}, 0xc0001abd60, 0x0}, {0x148b260, 0xc0000aed20}, {0x1516700, 0xc0001595e0}, ...)
        github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay/op.go:55 +0x35c
github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay.Op.Apply({{0x148b260, 0xc0000aed08}, {0x1516700, 0xc0001595e0}, 0xc0001abd60, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/overlay/op.go:27 +0x14f
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.OverlayPostProcessing.Apply({0xc0001b3a58})
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/overlay_post_processing.go:63 +0x716
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*LibraryExecution).Eval(0xc0001b3a58, 0x0, {0x0, 0x0, 0xc0000ae5e8}, {0x0, 0xc0000a1960, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/library_execution.go:183 +0x7b
github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.(*Options).RunWithFiles(0xc0000db540, {{0xc0000b6100, 0x2, 0xc}}, {0x1689890, 0xc0001532f0})
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template/cmd.go:164 +0x570
github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.(*Options).Run(0xc0000db540)
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template/cmd.go:100 +0x4b7
github.com/vmware-tanzu/carvel-ytt/pkg/cmd.NewCmd.func1(0x0, {0xc0000ce9c0, 0x0, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.go:22 +0x1d
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0x0, {0xc0000ce9c0, 0x0, 0x2})
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0x6d
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0xc00013ea00, {0xc0000ce9c0, 0x0, 0x2})
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0x6d
github.com/spf13/cobra.(*Command).execute(0xc00013ea00, {0xc0000a2190, 0x2, 0x2})
        github.com/spf13/cobra@v1.5.0/command.go:872 +0x624
github.com/spf13/cobra.(*Command).ExecuteC(0xc00013ea00)
        github.com/spf13/cobra@v1.5.0/command.go:990 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
        github.com/spf13/cobra@v1.5.0/command.go:918
main.main()
        github.com/vmware-tanzu/carvel-ytt/cmd/ytt/ytt.go:21 +0xd6

What did you expect:

A helpful error message.

Anything else you would like to add:
[Additional information that will assist in solving the issue.]

Environment:

  • ytt version (use ytt --version): ytt version 0.42.0
  • OS (e.g. from /etc/os-release): macOS

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@mamachanko mamachanko added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been triaged for relevance labels Aug 21, 2022
@pivotaljohn
Copy link
Contributor

This is absolutely a bug.

From the docs:
https://carvel.dev/ytt/docs/v0.42.0/lang-ref-ytt-overlay/#overlayappend

Note: This action implies an @overlay/match selecting the last node. Any other @overlay/match annotation is ignored.

@pivotaljohn pivotaljohn added priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. and removed carvel triage This issue has not yet been triaged for relevance labels Aug 24, 2022
@pivotaljohn
Copy link
Contributor

@mamachanko a workaround:

#! dreams.yaml
#@ load("@ytt:overlay", "overlay")
--- _

#@overlay/match by=overlay.subset("_")
#@overlay/replace
---
name: Matthew
profession: Raven

@mamachanko
Copy link
Contributor Author

While trying to write a template test for this, I realised that I can't reproduce it with a *.tpltest:

#! pkg/yamltemplate/filetests/ytt-library/overlay/append-to-nothing.tpltest

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

#@overlay/match by=overlay.subset({"name": "Sandman", "profession": "King of Dreams"})
#@overlay/append
---
name: Matthew
profession: Raven

+++

---
--- FAIL: TestYAMLTemplate (0.10s)
    --- FAIL: TestYAMLTemplate/filetests/ytt-library/overlay/append-to-nothing.tpltest (0.00s)
        filetests.go:131: not equal

            ### result 32 chars:
            >>>name: Matthew
            profession: Raven
            <<<
            ###expected 4 chars:
            >>>---
            <<<

First I thought it maybe already fixed on develop, but that's not the case:

❯ go run cmd/ytt/ytt.go -f dreams.yaml
panic: runtime error: index out of range [-1]

...

So I think the template tests don't fully replicate ytt command's entry point.

Either way, from what I can tell, this is the offending line as per trace and studying the code.

I think it would make sense to append nothing if there are no left-hand docs. The same probably goes for arrays.

@pivotaljohn
Copy link
Contributor

So I think the template tests don't fully replicate ytt command's entry point.

Yes... so the defect here is happening across DocSets.. and the *.tpltests are testing within a DocSet.
Another way I think of it is that there's a direct one-to-one mapping between an input YAML file and the DocSet that's produced by parsing it.

In the case with the unit tests, we're supplying the "YAML file" (i.e. the top-half of that test fixture) and we're testing below the layer of code that combines all the DocSets together.


Either way, from what I can tell, this is the offending line as per trace and studying the code.

bingo.


I think it would make sense to append nothing if there are no left-hand docs. The same probably goes for arrays.

🤔 Can you elaborate, there? I'm concerned we've had a potential blindspot, here... so unpacking this would be very useful.

To date, in the array case, we are appending. The docs are describing what the to-date intended behavior to be: https://carvel.dev/ytt/docs/v0.42.0/lang-ref-ytt-overlay/#overlayappend

An the software matches the documentation in the array item case; just not for the document case.

stuffs: []

#@overlay/match by=lambda i,l,r: True
---
stuffs:
- 3

yields:

stuffs:
- 3

... further, if we were going to be strictly consistent, then @overlay/append ought to be enough to merely add. hmmmmmm.... I'm not certain what the correct behavior ought to be...

@mamachanko
Copy link
Contributor Author

I think it would make sense to append nothing if there are no left-hand docs. The same probably goes for arrays.

🤔 Can you elaborate, there? I'm concerned we've had a potential blindspot, here... so unpacking this would be very useful.

I was thinking that, when the match is empty, i.e. there are is no left-hand side, then nothing should be appended. But now I am not sure any more either.

DocSets and []DocsSets are - in a way, or precisely - arrays of docs. The root of ytt's entire input is a an array of documents. And even if it is empty, I should be able to append to it. That renders by= redundant in the doc case, because there's always an array of documents.

#@ load("@ytt:overlay", "overlay")
---
existing doc
#!                               v--v
#@overlay/match by=lambda i,l,r: True, expects="0+"
#@overlay/append
---
appended doc
#!                               v---v
#@overlay/match by=lambda i,l,r: False, expects="0+"
#@overlay/append
---
appended doc

Yields:

existing doc
---
appended doc
---
appended doc

That's consistent with the idea of always being able to append to the array of documents, even if you don't match.

I think it's getting esoteric. 😆 But by extension, then

#@ load("@ytt:overlay", "overlay")
#!                               v--v
#@overlay/match by=lambda i,l,r: True, expects="0+"
#@overlay/append
---
appended doc
#!                               v---v
#@overlay/match by=lambda i,l,r: False, expects="0+"
#@overlay/append
---
appended doc

should yield:

appended doc
---
appended doc

Or should it not?

@pivotaljohn
Copy link
Contributor

Erroneously closed (an ill-worded comment triggered the workflow in GitHub)

@pivotaljohn pivotaljohn reopened this Oct 3, 2022
@github-actions github-actions bot added the carvel triage This issue has not yet been triaged for relevance label Oct 3, 2022
@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Nov 13, 2022
@vmunishwar vmunishwar reopened this Jun 6, 2023
@vmunishwar vmunishwar removed the stale This issue has had no activity for a while and will be closed soon label Jun 7, 2023
@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Jul 18, 2023
@vmunishwar vmunishwar removed stale This issue has had no activity for a while and will be closed soon carvel triage This issue has not yet been triaged for relevance labels Jul 18, 2023
@vmunishwar
Copy link
Contributor

Looks like this issue was already closed as completed on Nov 17, 2022. Closing it again.

@vmunishwar vmunishwar removed the priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a defect or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants