-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
helm: 'bpf.ctTcpMax' and 'bpf.ctAnyMax' need to be strings, not integers #14021
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why lbMapMax
does not require the integer to be quoted for example?
indeed:
I assume that the agent is not happy about the scientific notation. Could be related to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for consistency we should quote all numbers? @mvisonneau
I tried to find a way to update the chart templating logic to be able to leverage numbers but I did not manage to get it to work yet. I believe that it would be interesting to stick to the actually expected type by default but at the same time, current implementation prevents it. Lets perhaps merge this one and create a follow-up issue to figure out what to do with this problem 🤔 |
@aanm what do you think of @mvisonneau 's proposal above? If we're fine to merge this as-is, it will need a rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joestringer LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 just needs to be rebased.
b2feb51
to
a2a5c03
Compare
rebased! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helm doc failure from the CI pipeline is legit, @mvisonneau please run make -C install/kubernetes docs
.
Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
a2a5c03
to
eee55bd
Compare
ah right! I rebased again and ran the formatting command 👍, sorry about that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[EDIT: deleted - sorry, missclick]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvisonneau Thanks! Marking this as ready-to-merge
as it doesn't warrant a full CI run.
Make it more evident for end users by amending the comment regarding how to configure
bpf.ctTcpMax
andbpf.ctAnyMax
Helm release would succeed if integers are provided but the Cilium agents would then crash.