bool flag caused a panic

[carolynvs]

Describe the bug

I passed an empty bool flag to a mixin and it caused a panic when it executed.

To Reproduce

- az:
      description: "Remove application's resource group"
      arguments:
        - group
        - delete
      flags:
        name: "{{ bundle.parameters.resource-group }}"
        yes:

This also panics:

      yes: ""

Expected behavior

Definitely not a panic, at least an error if I should have used different formatting. Ideally it should pass my flag appropriately, e.g. --yes.

Porter Command and Output

porter uninstall

Version

porter v0.27.2 (aee93e9)

The code on this line used to use fmt.Sprintf("%v"....) shenanigans here to convert the key to a string to avoid yaml's typing of keys as int and bools. What's happening is that yaml is interpreting "yes" or "y" as a bool, not a string. I was able to force it to a string in the yaml by changing it to

    "yes":

We should go back to the old hack and comment why it's there with a test so that I'm not tempted to remove it again. 😊

[dev-drprasad]

We should go back to the old hack and comment why it's there with a test so that I'm not tempted to remove it again. 😊

I couldn't understand what this statement saying. may be lack of context ??

Yaml 1.2 dropped boolean representations (y, yes, 'off' etc) in favour of true/false values. Unfortunately, go-yaml@v2 parses them as booleans for backward compatibility. Luckily go-yaml@v3 supports yaml1.2. Migrating to go-yaml@v3, will fix this issue.

ref: go-yaml/yaml#214

[carolynvs]

line 89 is casting an untyped yaml key unconditionally to a string, when yaml keys can also be integers or booleans. I believe even after upgrading to v3, someone could make a key using true/false or a number and the line below would panic, right?

https://github.com/deislabs/porter/blob/ed1e19f815839ccd0401aee7cf360dcd9b2231ff/pkg/exec/builder/flags.go#L89

Previously I had a workaround that would force the string representation of the key, using fmt.Sprintf("%v"....) and I accidentally removed it because I forgot why I was doing that instead of just casting it and "simplified the code". So my comment above was that we should go back to the original workaround for yaml allowing non-string keys and put a comment in the code so that future devs understand why we are using that instead of a cast.

I am still quite interested in upgrading to go-yaml v3 though for this other issue #1019, as it will allow us to get line numbers for our linter. But I don't think the upgrade will make this part of the code safe from panics. Let me know if you see it differently though! 😀

[dev-drprasad]

But I don't think the upgrade will make this part of the code safe from panics

Agreed.

If we need keys as strings, then why not just use map[string]interface{}{} instead of map[interface{}]interface{}{}. go-yaml is smart enough to convert keys to strings automatically. We don't need to do it explicitly