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

Alerting: add new type to support both primitive and string ints #9840

Closed
wants to merge 3 commits into from

Conversation

titolins
Copy link
Contributor

@titolins titolins commented Nov 5, 2024

Context

  • Helm doesn't handle big ints well - we should support using strings
  • Some tenants are already hitting the new config limits we've added but now we can't increase them using configmaps because helm will convert them to scientific notation
  • For flags this is not an issue
  • I've merged this PR earlier, but it caused AM pods to crash since the go-yaml doesn't handle the string conversion automatically: https://github.com/grafana/deployment_tools/pull/189045
  • IntString should allow us to use both a string or an integer
  • Use that for config/state size limits
  • Tested the flag parsing is working by running locally (we don't have a unit test for that setup and doing so requires a bit tinkering)
    Screenshot 2024-11-05 at 19 26 47

@titolins titolins requested a review from a team as a code owner November 5, 2024 18:27
@bboreham
Copy link
Contributor

bboreham commented Nov 5, 2024

Consider putting the new type here: https://github.com/grafana/dskit/tree/main/flagext

@titolins titolins force-pushed the titolins/support-string-ints branch from 931ceea to 1d10e72 Compare November 5, 2024 20:07
@titolins
Copy link
Contributor Author

titolins commented Nov 6, 2024

Consider putting the new type here: https://github.com/grafana/dskit/tree/main/flagext

Ah, that's a lot better. Tks 🙏

Converting this to a draft then, will adjust once the new type is added to dskit 👍

@titolins titolins marked this pull request as draft November 6, 2024 09:54
@titolins
Copy link
Contributor Author

titolins commented Nov 8, 2024

superseded by #9845

@titolins titolins closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants