-
Notifications
You must be signed in to change notification settings - Fork 16.8k
[stable/jenkins] Make chart compatible with Helm 3 #20624
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: helfper The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @helfper. Thanks for your PR. I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
status: | ||
ingress: [] |
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.
We should not include a status in the generated manifests.
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.
Adding status
is needed to solve the problem that this PR is trying to address, that is being able to create this route using Helm 3. You can see that in the error I pasted and in the issue linked in the PR description.
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.
My feeling is that we would fix it at the wrong location. For me it's rather an upstream issue that those fields are marked as required. Introducing a fix here would also mean to remove it later.
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.
While I completely agree that the status should have been made optional in the spec. As it stands, I'd argue that this chart is wrong by creating a resource that doesn't follow its specification.
@@ -18,7 +18,8 @@ metadata: | |||
{{- end }} | |||
name: {{ template "jenkins.fullname" . }} | |||
spec: | |||
host: {{ .Values.master.route.path }} | |||
host: "{{ .Values.master.route.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.
Why do we need a different value host
can't we re-use the already existing hostName
?
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.
I feel it would be rather confusing to use master.ingress.hostName
as the route's host, when we are not using anything else from master.ingress.*
in the route definition. Unless it was changed to master.hostName
.
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.
Agree that it's not ideal. The issue I see is that the hostname is also used to determine the Jenkins URL which is configured in the config map. Here is the template code:
{{/*
Returns the Jenkins URL
*/}}
{{- define "jenkins.url" -}}
{{- if .Values.master.jenkinsUrl }}
{{- .Values.master.jenkinsUrl }}
{{- else }}
{{- if .Values.master.ingress.hostName }}
{{- if .Values.master.ingress.tls }}
{{- default "https" .Values.master.jenkinsUrlProtocol }}://{{ .Values.master.ingress.hostName }}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- else }}
{{- default "http" .Values.master.jenkinsUrlProtocol }}://{{ .Values.master.ingress.hostName }}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- end }}
{{- else }}
{{- default "http" .Values.master.jenkinsUrlProtocol }}://{{ template "jenkins.fullname" . }}:{{.Values.master.servicePort}}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- end}}
{{- end}}
{{- end -}}
https://github.com/helm/charts/blob/master/stable/jenkins/templates/_helpers.tpl#L51-L68
So it won't be as easy as introducing a new flag...
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.
-
It's seems the same due diligence was not done during the code review when this route was added initially. I don't think that, by any measure, this PR makes that situation worse than it is currently. Notice that the route's host can already be modified by setting
master.route.path
, which is not taken into account whatsoever in thejenkins.url
definition. -
Even ignoring the route for a second, that logic for
jenkins.url
doesn't seem right. If I don't set themaster.jenkinsUrl
value and setmaster.ingress.enabled
to false, thejenkins.url
template might still use the valuemaster.ingress.hostName
. -
The logic for
jenkins.url
can easily be changed to take into account the route values:
{{- define "jenkins.url" -}}
{{- if .Values.master.jenkinsUrl }}
{{- .Values.master.jenkinsUrl }}
{{- else if and .Values.master.ingress.enabled .Values.master.ingress.hostName }}
{{- if .Values.master.ingress.tls }}
{{- default "https" .Values.master.jenkinsUrlProtocol }}://{{ .Values.master.ingress.hostName }}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- else }}
{{- default "http" .Values.master.jenkinsUrlProtocol }}://{{ .Values.master.ingress.hostName }}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- end }}
{{- else if and .Values.master.route.enabled .Values.master.route.host }}
{{- default "http" .Values.master.jenkinsUrlProtocol }}://{{ .Values.master.route.host }}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- else }}
{{- default "http" .Values.master.jenkinsUrlProtocol }}://{{ template "jenkins.fullname" . }}:{{.Values.master.servicePort}}{{ default "" .Values.master.jenkinsUriPrefix }}
{{- end}}
{{- end -}}
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
Signed-off-by: Helder Pereira <helfper@gmail.com>
f2a44f4
to
011ddf6
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
This issue is being automatically closed due to inactivity. |
Signed-off-by: Helder Pereira helfper@gmail.com
What this PR does / why we need it:
Trying to install Jenkins chart with route enabled using Helm 3 fails because of openshift/origin#24060:
This PR adds the missing fields required by the Route v1 spec. Additionally, it also fixes the confusing binding from a value called
master.route.path
to the Route'shost
field.Checklist
[stable/mychartname]
)