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

Fix any Node configuration that is of numerical type #1407

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

RafalKorepta
Copy link
Contributor

For example crash_loop_limit can change value type from integer to string while template engine converts it due to toYaml function usage.

Reference

https://docs.redpanda.com/current/reference/properties/broker-properties/#crash_loop_limit

For example crash_loop_limit can change value type from integer to string while
template engine converts it due to `toYaml` function usage.

Reference

https://docs.redpanda.com/current/reference/properties/broker-properties/#crash_loop_limit
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

How did this make it passed chart-testing in CI?

@RafalKorepta
Copy link
Contributor Author

How did this make it passed chart-testing in CI?

Maybe Redpanda is not strict about it. Redpanda apparently could work with string conversion to integer (I verified that manually).

I found out by yaml.Unmarshall using RPK go struct that represent Redpanda Node Configuration.

As migration to go based Redpanda configuration is done, any further differences
should not be handled in test case.
@RafalKorepta RafalKorepta enabled auto-merge (rebase) July 3, 2024 17:53
@RafalKorepta RafalKorepta merged commit d2444af into main Jul 3, 2024
41 checks passed
@RafalKorepta RafalKorepta deleted the rk/fix-crash-loop-limit branch July 3, 2024 18:31
@chrisseto
Copy link
Contributor

oooooooh, fascinating. I remember stumbling across the C++ YAML parsing and noting how different it was. Seems like it's more permissive.

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

Successfully merging this pull request may close these issues.

None yet

2 participants