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.

Sign up for GitHub

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

Jump to bottom

When struct keys are not strings, should error, not panic #764

Open
hmarko opened this issue Nov 15, 2022 · 6 comments
Open

When struct keys are not strings, should error, not panic #764

hmarko opened this issue Nov 15, 2022 · 6 comments
Labels
error msg improvement surprising to users

Comments

@hmarko
Copy link

hmarko commented Nov 15, 2022 •
edited by pivotaljohn
Loading

When a non-string value has been provided for an key in a struct, ytt should report an error

ytt: Error:
- expected string for data value key, was bool
    config.yaml:7 |         on:

rather than panic (see below).

👇🏻 original report 👇🏻

when using on as a key in yaml (see below example) ytt panics. changing it to anything else solves it (just change the on: to on1: for example)

config.yaml:

#@data/values
---
general:
    notifications:
        recipients:
        - admin@test.com
        on:
        - any

data.yaml:

#@ load("@ytt:data", "data")
- name: Delete 
  type: multistep 
  steps: 
  - type: ansible
    inventory: hosts
    #@ if/end hasattr(data.values.general,"notifications"):
    notifications: #@ data.values.general.notifications

ytt panics with the following stack trace:

panic: expected struct key %!s(bool=true) to be string

goroutine 1 [running]:
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue.func1({0x8924a0, 0xd61ba8}, {0x88fee0, 0xc00009b818})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:100 +0x127
github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap.(*Map).Iterate(0xaaaaaaaaaaaaa, 0xc00018cdf8)
        github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap/map.go:71 +0x79
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xe8, 0x0}}, 0xda58e0)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:96 +0xe9
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.asStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xf4, 0x0}}, {0x9004e0, 0xc00009b7e8})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:83 +0x2fc
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue.func1({0x896f20, 0xc00017ab90}, {0x9004e0, 0xc00009b7e8})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:98 +0x98
github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap.(*Map).Iterate(0x0, 0xc00018d008)
        github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap/map.go:71 +0x79
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xd0, 0x0}}, 0xc00018d090)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:96 +0xe9
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.asStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xf4, 0x0}}, {0x9004e0, 0xc00009b7d0})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:83 +0x2fc
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue.func1({0x896f20, 0xc00017ab80}, {0x9004e0, 0xc00009b7d0})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:98 +0x98
github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap.(*Map).Iterate(0x18, 0xc00018d218)
        github.com/vmware-tanzu/carvel-ytt/pkg/orderedmap/map.go:71 +0x79
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.dictAsStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0xb8, 0x0}}, 0xc00018d2f8)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:96 +0xe9
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.asStarlarkValue({{0x9004e0, 0xc00009b7b8}, {0x8, 0x0}}, {0x9004e0, 0xc00009b7b8})
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:83 +0x2fc
github.com/vmware-tanzu/carvel-ytt/pkg/template/core.GoValue.AsStarlarkValue(...)
        github.com/vmware-tanzu/carvel-ytt/pkg/template/core/go_value.go:36
github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary.NewDataModule(0xa84f00, {0xa84f00, 0xc00019daa0})
        github.com/vmware-tanzu/carvel-ytt/pkg/yttlibrary/data.go:27 +0x45
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*TemplateLoader).EvalYAML(0xc0001ae7e0, {0xc0000ba690, 0xc0000ba690}, 0xc0000ba5f0)
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/template_loader.go:198 +0x47c
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*LibraryExecution).eval(0xc00018da58, 0xc0001a82c0, {0x0, 0xc0001969f0, 0xc00018d7a0}, {0x0, 0xa96c88, 0xc00019ff40})
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/library_execution.go:225 +0x91e
github.com/vmware-tanzu/carvel-ytt/pkg/workspace.(*LibraryExecution).Eval(0xc00018da58, 0x0, {0x0, 0x0, 0xc00009a948}, {0x0, 0xc00008b860, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/workspace/library_execution.go:177 +0x46
github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.(*Options).RunWithFiles(0xc0000c7540, {{0xc00008b8b0, 0x2, 0x14}}, {0xa8a930, 0xc000135290})
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template/cmd.go:164 +0x570
github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.(*Options).Run(0xc0000c7540)
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template/cmd.go:100 +0x4b7
github.com/vmware-tanzu/carvel-ytt/pkg/cmd.NewCmd.func1(0x0, {0xc0000b89e0, 0x0, 0x0})
        github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template.go:22 +0x1d
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0x0, {0xc0000b89e0, 0x0, 0x2})
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0x6d
github.com/cppforlife/cobrautil.WrapRunEForCmd.func1.1(0xc000120a00, {0xc0000b89e0, 0x0, 0x2})
        github.com/cppforlife/cobrautil@v0.0.0-20200514214827-bb86e6965d72/misc.go:45 +0x6d
github.com/spf13/cobra.(*Command).execute(0xc000120a00, {0xc00008c160, 0x2, 0x2})
        github.com/spf13/cobra@v1.5.0/command.go:872 +0x624
github.com/spf13/cobra.(*Command).ExecuteC(0xc000120a00)
        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

Environment:
ytt version 0.43.0
CentOS Linux release 7.8.2003 (Core)
@hmarko hmarko added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been triaged for relevance labels Nov 15, 2022
@carvel-bot carvel-bot moved this to To Triage in Carvel Nov 15, 2022
@hmarko
Copy link
Author

hmarko commented Nov 17, 2022

This beheviour happens on words like: on, off, true, false used as yaml keys.

@mamachanko
Copy link
Contributor

@hmarko thanks for reporting this!

Panicking is not great 😢

Treating on as a boolean is YAML as per spec. Unless quoted a string matching y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF is treated as a boolean. See this example on the playground.

To be able to use the key on - as shown in your example - quote it:

#@data/values
---
general:
    notifications:
        recipients:
        - admin@test.com
        "on":
        - any

@hmarko
Copy link
Author

hmarko commented Nov 21, 2022 •
edited
Loading

Thank you ! I think it worth taking care of it to prevent nasty panics from happening. tools like ytt should template this anyway (what will happen if I want to change true to false as per the yaml spec using ytt ?)

@mamachanko
Copy link
Contributor

Thank you ! I think it worth taking care of it to prevent nasty panics from happening.

I agree, the panic isn't right. However, I think the right behavior according to spec is to treat your data values like so:

#@data/values
---
general:
    notifications:
        recipients:
        - admin@test.com
        true: #! note the boolean
        - any

tools like ytt should template this anyway (what will happen if I want to change true to false as per the yaml spec using ytt ?)

I am not sure I follow you here. Can you expound and maybe provide an example?

@hmarko
Copy link
Author

hmarko commented Nov 21, 2022

Ok, I think I got it now :-)
My YAML uses the on: boolean as both key and value in the hierarchy.
I cannot really use it as a key according to the spec.

@pivotaljohn pivotaljohn mentioned this issue Nov 29, 2022
Open
@pivotaljohn
Copy link
Contributor

@hmarko it's not you, this was a "good intention, bad impact" idea that we actually had intended to "fix". But that work has not yet been prioritized by the maintainers. See #407.

What remains, tho, is the better handling. As @mamachanko noted above: panics are suitable for when something goes terribly wrong internally. When the problem arises directly from our inputs, we should error out gracefully with a helpful + actionable message. 👍🏻

I'm going to adjust this issue to capture that scope.

@pivotaljohn pivotaljohn added error msg improvement surprising to users and removed carvel triage This issue has not yet been triaged for relevance labels Nov 29, 2022
@pivotaljohn pivotaljohn changed the title panic: expected struct key %!s(bool=true) to be string When struct keys are not strings, should error, not panic Nov 29, 2022
@pivotaljohn pivotaljohn removed the bug This issue describes a defect or unexpected behavior label Nov 29, 2022
@github-project-automation github-project-automation bot moved this to To Triage in Carvel Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
error msg improvement surprising to users
Projects
Carvel
Status: To Triage
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mamachanko @hmarko @pivotaljohn