-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(helm): allow cluster administrator to configure ingress host (#804) #804
feat(helm): allow cluster administrator to configure ingress host (#804) #804
Conversation
@@ -91,6 +91,8 @@ spec: | |||
- name: REANA_DEFAULT_QUOTA_DISK_LIMIT | |||
value: {{ .Values.quota.default_disk_limit | default 0 | quote }} | |||
{{- if .Values.quota.enabled }} | |||
- name: REANA_INGRESS_HOST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REANA_INGRESS_HOST
should not be located inside the "if quota..." clause, because it does not depend on quotas. Please move it above, next to where REANA_INGRESS_ANNOTATIONS
lives.
helm/reana/values.yaml
Outdated
@@ -135,6 +135,7 @@ notifications: | |||
# Accessing the cluster from outside world | |||
ingress: | |||
enabled: true | |||
host: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do we need to add a new field ingress.host
for this functionality? I'm wondering because we already have reana_hostname
field defined elsewhere, so if people set it up, we could perhaps reuse that directly. (And if it is not set up, then use the empty string as default value.)
(Another possibility would be to hardcode ""
as default value inside helm/reana/templates/reana-workflow-controller.yaml
, without even playing with reana_hostname
. )
In this way we don't need to introduce the new ingress.host
field.... Thinking of making changes to 0.9.3 as minimal as possible.
1cde2e5
to
1479730
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely locally and on DEV 👍
* feat(helm): allow cluster administrator to configure ingress host (reanahub#804)
* feat(helm): allow cluster administrator to configure ingress host (reanahub#804)
feat(helm): allow cluster administrator to configure ingress host (reanahub#804)
Closes reanahub/reana-workflow-controller#587