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

storage controller: don't | quote split threshold #83

Merged
merged 1 commit into from
May 20, 2024

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented May 20, 2024

Helm (or Go formatting more generally) apparently arbitrarily uses scientific notation for integers above a million when quoting.

Try passing it through without the | quote to see if a simple integer makes it through unscathed.

@jcsp jcsp force-pushed the jcsp/storcon-autosplit branch 2 times, most recently from 7a77021 to ccf75b6 Compare May 20, 2024 08:43
@jcsp jcsp force-pushed the jcsp/storcon-autosplit branch from ccf75b6 to c9c3d2a Compare May 20, 2024 08:47
@jcsp jcsp changed the title Jcsp/storcon autosplit storage controller: don't | quote split threshold May 20, 2024
@jcsp jcsp marked this pull request as ready for review May 20, 2024 08:49
@jcsp jcsp added the bug Something isn't working label May 20, 2024
@jcsp jcsp requested a review from petuhovskiy May 20, 2024 08:50
@petuhovskiy
Copy link
Member

Sounds more like a yaml issue, maybe we should put splitThreshold value in quotes at .github/helm-values/dev-us-east-2-beta.neon-storage-controller.yaml:69?

@petuhovskiy
Copy link
Member

petuhovskiy commented May 20, 2024

This is probably the issue: go-yaml/yaml#730

UPD: looked again, probably a different issue after all

@jcsp
Copy link
Contributor Author

jcsp commented May 20, 2024

maybe we should put splitThreshold value in quotes at .github/helm-values/dev-us-east-2-beta.neon-storage-controller.yaml:69

I have a PR to do that, but I'm interested to see if it's really necessary -- the YAML is allowed to use scientific notation, as long as the string substitution in the helm template handles that as an int rather than string.
So I'd like to try just this PR first, then only merge https://github.com/neondatabase/aws/pull/1358 if necessary.

@petuhovskiy
Copy link
Member

Ok, let's try removing | quote.

But I think that underlying issue is that large numbers in yaml got parsed as float64 by default, and default "to string" implementation for floats is to use scientific notation.

@jcsp jcsp merged commit d541425 into main May 20, 2024
3 checks passed
@jcsp jcsp deleted the jcsp/storcon-autosplit branch May 20, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants