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

Support ingress annotations (kuma.io/ingress-public-address and kuma.… #1796

Merged
merged 3 commits into from
Apr 13, 2021

Conversation

bdecoste
Copy link
Contributor

…io/ingress-public-port) in helm (#1795)

Summary

Support ingress annotations (kuma.io/ingress-public-address and kuma.io/ingress-public-port) in helm

Full changelog

  • [Implement ingress annotations]

Issues resolved

Fix #1795

Documentation

@bdecoste bdecoste requested a review from a team as a code owner April 10, 2021 01:02
@CLAassistant
Copy link

CLAassistant commented Apr 10, 2021

CLA assistant check
All committers have signed the CLA.

…io/ingress-public-port) in helm (kumahq#1795)

Signed-off-by: Bill DeCoste <bdecoste@gmail.com>
Signed-off-by: Bill DeCoste <bdecoste@gmail.com>
@austince
Copy link
Contributor

Looks great, thanks for the contribution @bdecoste ! I think you've found almost all the gotchas, just need to run: make generate/kumactl/install/k8s/control-plane and commit those results as well.

Signed-off-by: Bill DeCoste <bdecoste@gmail.com>
Copy link
Contributor

@austince austince left a comment

Choose a reason for hiding this comment

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

Change looks good to me, though all e2e-type tests are having seemingly unrelated issues, cc' @nickolaev: https://app.circleci.com/pipelines/github/kumahq/kuma/6825/workflows/8a43ec13-c47b-4e2d-b294-b500016c57ec/jobs/81156

I'd say we merge this, but would like to understand what's going on with the tests.

Comment on lines +25 to +29
{{- if .Values.ingress.annotations }}
{{- range $key, $value := .Values.ingress.annotations }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, and more of a follow-up note for myself since I know this is done elsewhere, but I don't think we should be silently supporting invalid annotations (by quoting each one). Instead, it'd be better to:

Suggested change
{{- if .Values.ingress.annotations }}
{{- range $key, $value := .Values.ingress.annotations }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
{{- with .Values.ingress.annotations }}
{{ . | toYaml | nindent 8 }}
{{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

I will merge it as it is and will let you submit another PR if you want to cover all the relevant places in the charts.

@austince
Copy link
Contributor

Looks like the failing tests are part of a larger problem/ outage with Bintray's planned maintenance: https://status.bintray.com/incidents/4mzph0y8l6hk

@nickolaev nickolaev merged commit 309e035 into kumahq:master Apr 13, 2021
mergify bot pushed a commit that referenced this pull request Apr 13, 2021
#1796)

* Support ingress annotations (kuma.io/ingress-public-address and kuma.io/ingress-public-port) in helm (#1795)

Signed-off-by: Bill DeCoste <bdecoste@gmail.com>
(cherry picked from commit 309e035)

# Conflicts:
#	app/kumactl/pkg/install/k8s/control-plane/helmtemplates_vfsdata.go
nickolaev pushed a commit that referenced this pull request Apr 13, 2021
…… (backport #1796) (#1809)

* Support ingress annotations (kuma.io/ingress-public-address and kuma.… (#1796)

* Support ingress annotations (kuma.io/ingress-public-address and kuma.io/ingress-public-port) in helm (#1795)

Signed-off-by: Bill DeCoste <bdecoste@gmail.com>
(cherry picked from commit 309e035)

# Conflicts:
#	app/kumactl/pkg/install/k8s/control-plane/helmtemplates_vfsdata.go

* fix(*) make check

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Co-authored-by: Bill DeCoste <bdecoste@gmail.com>
Co-authored-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
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.

Support ingress annotations (kuma.io/ingress-public-address and kuma.io/ingress-public-port) in helm
4 participants