Add support to ingressClassName #25
Conversation
There was a problem hiding this comment.
Thanks for working on this @caiohasouza! I just had one minor naming comment
charts/seq/templates/ingress.yaml
Outdated
| @@ -27,6 +27,9 @@ spec: | |||
| secretName: {{ .secretName }} | |||
| {{- end }} | |||
| {{- end }} | |||
| {{- if .Values.ingress.ingressClassName }} | |||
| ingressClassName: {{ .Values.ingress.ingressClassName | quote }} | |||
There was a problem hiding this comment.
Since this lives under the ingress object what do you think about just calling this className?
There was a problem hiding this comment.
Hi @KodrAus,
I normally use the same Kubernetes Object name on Helm variables to be clear and easy to understand, this practice is adopted on many public helm charts, like Grafana, Jenkins, etc. But you do you think that className is better, Ok, i change. just confirm please.
Regards
There was a problem hiding this comment.
I don't think that's a practice we've really followed so far so I think we should go with className for now. At some point in the future we can do a complete pass over the API we've evolved here and try make it consistent with patterns used by popular charts.
There was a problem hiding this comment.
Ok, no problems, change done!
|
Thanks @caiohasouza! We do need to try follow established patterns better here, if you've got any other charts in mind besides the few you've mentioned that you think set good precedents we should look to I'd be keen to check them out! |
|
Perfect, thank you @KodrAus |
|
@KodrAus , This feature is not released yet on 2021.4.7192 helm chart version, right? I'm trying to use but the changes do not take effetc. If yes it's possible generate a new helm chart version to include this feature? Regards |
Fix #24