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: fix error when containerPort misses protocol #32

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

YBeaugnon
Copy link

Using helm chart with ports, without protocol specified (which default to TCP), cause error during

mergeValuesByKey = attrMergeKey: listMergeKeys: values:
evaluation.

This hack use TCP as a default when protocol key is not defined.

I think a cleaner way could be to give default to option, based on k8s default options.

@YBeaugnon YBeaugnon marked this pull request as draft July 19, 2023 11:29
@YBeaugnon YBeaugnon changed the title Draft: fix error when containerPort misses protocol Fix: fix error when containerPort misses protocol Jul 19, 2023
@hall
Copy link
Owner

hall commented Aug 26, 2023

Hi @YBeaugnon, thanks for flagging this! I agree it's a pain. It was introduced in #21 as an unfortunate side effect with the hope that it could be solved down the line.

What I'd like to do is add some testing here that can validate that this (and future changes) don't break existing functionality.

@thorstenweber83
Copy link

I think the same issue also arises with the ServicePort spec.
I encountered it when trying to import the dashboard yaml from https://raw.githubusercontent.com/kubernetes/dashboard/v2.7.0/aio/deploy/recommended.yaml
where the container's ports specify "protocol" but the service's ports do not.

@felixscheinost
Copy link
Contributor

What would be an acceptable fix for this issue? Would it make sense to mkDefault a value of "TCP" instead of null?

Is mkDefault a mkOverride 1002 in our case?

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.

4 participants