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

patch/consistent-naming-ingressClassName #278

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zenon-was-here
Copy link
Contributor

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.

  • Closes #N/A

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.

Make ingress.className to ingressClassName to be consistent with ingress.yaml file
Make ingress.className to ingressClassName to be consistent with ingress.yaml
@zenon-was-here zenon-was-here requested a review from a team as a code owner July 19, 2023 14:50
Copy link
Contributor

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@zenon-was-here thank you for bringing this to our attention. There are actually a couple issues with our use of ingress class name spread out between the files.

First, ingress.yaml uses .Values.ingressClassName. This equates to a root level field in the values.yaml named ingressClassName. But the default values.yaml has an ingress section, which is where all ingress configuration should live. So .Values.ingress.className is the most appropriate.

Second, ingress-grpc.yaml uses .Values.ingress.className, but there is a grpcIngress section in the values.yaml that it should be using.

What I think we should do:

  1. Update ingress.yaml to use .Values.ingress.className.
  2. Update ingress-grpc.yaml to use .Values.grpcIngress.className.
  3. Update NOTES.txt to fail if .Values.ingressClassName is set.

@zenon-was-here
Copy link
Contributor Author

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

Revert change to ingress-beta.yaml
Update ingress-grpc based on PR feedback
Update based on PR feedback
Fail if ingressClassName is set, per PR comment
@TylerHelmuth
Copy link
Contributor

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

Add empty string `className:` fields to ingress and grpcIngress values
@TylerHelmuth
Copy link
Contributor

@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.

@zenon-was-here
Copy link
Contributor Author

@TylerHelmuth , understood, thanks for the feedback

@TylerHelmuth
Copy link
Contributor

@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.

TylerHelmuth added a commit that referenced this pull request May 6, 2024
Prepares an "internal" release of the chart for us to test Refinery
3.0.0.

> ⚠️ We do not recommend any customer use this version of the
chart and we do not support it.

Changes:
- The commits from #278
(thanks @zenon-was-here!)
- Updated default refinery configuration
- Updated resource values
- A new Statefulset Redis deployment option

---------

Co-authored-by: zenon-was-here <[email protected]>
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.

2 participants