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

toToml filter converts int to float #12987

Open
dud225 opened this issue Apr 26, 2024 · 5 comments
Open

toToml filter converts int to float #12987

dud225 opened this issue Apr 26, 2024 · 5 comments

Comments

@dud225
Copy link

dud225 commented Apr 26, 2024

Given the following data structure in values.yaml:

config:
  bar: 9

And this in my configmap template:

data:
  telegraf-local.conf: |-
{{ .Values.config | toToml | indent 4 }}

I get this:

data:
  telegraf-local.conf: |-
    foo = 9.0

Even casting the variable via bar: !!int 9 doesn't change the behavior, can't reproduce using the toJson filter either.

I'm running Helm version.BuildInfo{Version:"v3.14.4", GitCommit:"81c902a123462fd4052bc5e9aa9c513c4c8fc142", GitTreeState:"clean", GoVersion:"go1.21.9"}

@gjenkins8
Copy link
Contributor

toToml uses github.com/BurntSushi/toml to encode:

	e := toml.NewEncoder(b)
	err := e.Encode(v)

helm/pkg/engine/funcs.go

Lines 125 to 133 in c6beb16

func toTOML(v interface{}) string {
b := bytes.NewBuffer(nil)
e := toml.NewEncoder(b)
err := e.Encode(v)
if err != nil {
return err.Error()
}
return b.String()
}

Either Helm is representing the value internally as float, and toToml is rendering that. Or the design decision/issue with rendering ints as floats lies upstream with github.com/BurntSushi/toml. The former should be determined first I think.

@johanfleury
Copy link

IIRC, the internal representation is float. #6010 was made to use json.Number but was then reverted.

It seems like the toml module supports json.Number: https://github.com/BurntSushi/toml/blob/77ce8589413d1fc280be524c8992599c42027c61/encode.go#L259-L272

@CalvinKrist
Copy link
Contributor

CalvinKrist commented May 7, 2024

I made a draft PR to demonstrate this (just an easy way to share code), but adding unit tests for the toToml function that test the int and float conversion makes it appear that BurntSushi is handling this correctly and the issue is likely elsewhere #13012. These tests pass locally for me.

@CalvinKrist
Copy link
Contributor

CalvinKrist commented May 7, 2024

That said, I did repro this issue. I did helm create to make a simple helm chart, added the following config map:

data:
  {{ .Values | toToml }}

copied their values:

bar: 9

and then helm template . returns:

---
# Source: mychart/templates/cm.yaml
data:
    bar = 9.0

I added this repro to my demo PR as a failing test in the template_test.go file.

@CalvinKrist
Copy link
Contributor

CalvinKrist commented May 7, 2024

I was able to trace this to the function yaml.Unmarshal from the sigs.k8s.io/yaml package, used on this line. When debugging the failing test I added to template_test.go, this loads the value as a flaot64 even though we can see it should be an int. Not sure how fixable this is since it's from an external dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants