honeycombio / helm-charts

Helm repository and charts for Honeycomb
Apache License 2.0
30 stars 39 forks source link

patch/consistent-naming-ingressClassName #278

Open evans-zebra opened 1 year ago

evans-zebra commented 1 year ago

Which problem is this PR solving?

The ingress-beta and ingress-grpc Helm templates use one naming convention for an Ingress Class Name: ingress.className, whereas the ingress template uses a different convention: ingressClassName.

The problem solved is to adopt a consistent naming convention. I've opted to the latter one, ingressClassName, as it's used in Kubernetes example docs.

Otherwise, users have unexpected inconsistent behavior when trying to specify the Ingress Class Name for the different ingresses. And, populating the grpcIngress's Ingress Class Name currently would have to happen within the ingress: block in the values, which is unintuitive.

Short description of the changes

Changes the ingress-beta.yaml and ingress-grpc.yaml ingress.className to ingressClassName to be consistent with ingress.yaml and Kubernetes convention.

How to verify that this has the expected result

Specify an ingressClassName in a values.yaml file and dry-run. No tests because it's a minimal change that seems to fit the "small and obvious" acceptance criteria laid out in the lifecycle-and-practices doc.

evans-zebra commented 1 year ago

Thanks for the feedback, I'd be glad to make these changes.

TylerHelmuth commented 1 year ago

@zenon-was-here Can you also add className with an empty string value to both the ingress and grpcIngress sections in the values.yaml

TylerHelmuth commented 1 year ago

@zenon-was-here thanks for making these changes. These are breaking changes, so we'll talking internally about how to handle this. I haven't been able to think yet of a clever way to handle the grpc breaking change.

evans-zebra commented 1 year ago

@TylerHelmuth , understood, thanks for the feedback

TylerHelmuth commented 6 months ago

@zenon-was-here thanks again for working on this, I am going to integrate your changes into an upcoming v3.0.0 version of the chart.