[stable/nginx-ingress] update to use the correct argument name, fixes #730

[egeland]

Note that this is for the beta version of the nginx-ingress controller

[k8s-ci-robot]

Hi @egeland. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[chancez]

Do you think this could be done in a more backwards compatible way? Perhaps you can not update the image tag since it's still beta, and instead add a condition in the template like:

{{- if (contains "0.9." .Values.controller.image.tag) }}
            - --configmap={{ .Release.Namespace }}/{{ template "controller.fullname" . }}
{{- else }}
            - --nginx-configmap={{ .Release.Namespace }}/{{ template "controller.fullname" . }}
{{- end }}

I think it might be worth it since nginx-ingress is so heavily used, and some users might be running an older version but want the latest version of the chart. Not sure if its worth it, but just a potential option to break less people, and still allow them to upgrade their nginx-ingress chart.

[egeland]

I was thinking almost exactly the same thing, just after submitting the PR. I'll tweak it early next week.

…

On 9 Mar. 2017 1:21 pm, "Chance Zibolski" ***@***.***> wrote: Do you think this could be done in a more backwards compatible way? Perhaps you can not update the image tag since it's still beta, and instead add a condition in the template like: {{- if (contains "0.9." .Values.controller.image.tag) }} - --configmap={{ .Release.Namespace }}/{{ template "controller.fullname" . }} {{- else }} - --nginx-configmap={{ .Release.Namespace }}/{{ template "controller.fullname" . }} {{- end }} I think it might be worth it since nginx is so heavily used, and some might be running an older version but want the latest version of the chart. Not sure if its worth it, but just a potential option to break less people, and still allow them to upgrade their nginx-ingress chart. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#770 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABONY6eFC5YD2FZ05cF60XA0wPHrqHqTks5rj2IQgaJpZM4MXfE6> .

[mgoodness]

@chancez That's an interesting solution. There's an open issue for Helm 2.3 that would add an app_version field to Chart.yaml. The assumption until now has been that a chart version is (loosely) tied to an application version. I think it's worth considering how an approach like this might impact that relationship.

[chancez]

@mgoodness Yeah, I think app_version will be nice, but likely not enough to handle all cases. As intentionally proposed. I chimed in on that issue making my point.

Sometimes an app may be composed of multiple containers, and they each have different versions and thus different compatibility needs. If the chart author wants to maintain compatibility, you would need a version per chart, which I think at this point is fairly introspectable via app.image.tag or something of the sort.

[egeland]

Maybe I'll hold off on this change until we have app_version (or some other semver implementation) in the charts..?

[chancez]

Well, I don't think we necessarily should wait for that (I think it'll be a while). The chart is still pre 1.0, so I think we could either go with what you have, or take a best effort approach to handle 0.8.x and 0.9.x. I don't think its a huge deal either way, but I just wanted to make the suggestion.

[egeland]

I'll make the change you suggested, then - and we can always fine-tune it later, as SemVer becomes available in the templating..

[mgoodness]

@egeland

I took the liberty of adding the conditional that @chancez suggested. Also added conditionals around the tcp & udp arguments to (hopefully - I haven't tested it yet) address kubernetes/ingress-nginx#443. Finally, I'd like to keep the default image tag 0.8.3.

[mgoodness]

@k8s-bot ok to test

[mgoodness]

Can finally confirm this works as expected with 0.8.3 and 0.9.0-beta.3.

[egeland]

Thanks for jumping in - I didn't get around to looking at this (obviously).. 😄