[Helm] Add POD_NAMESPACE environment variable into Che server pod #17773
Conversation
mmorhun
requested review from
sparkoo,
skabashnyuk,
tolusha,
mshaposhnik,
AndrienkoAleksandr and
flacatus
mmorhun
requested review from
l0rd,
nickboldt and
sleshchenko
as code owners
che-bot
added
status/code-review
|
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
| @@ -72,7 +72,8 @@ spec: | |||
| valueFrom: | |||
| fieldRef: | |||
| fieldPath: metadata.namespace | |||
|
|
|||
| - name: POD_NAMESPACE | |||
There was a problem hiding this comment.
I think it is better to set KUBERNETES_NAMESPACE
https://github.com/eclipse/che/blob/master/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/CheInstallationLocation.java#L47
There was a problem hiding this comment.
I've already seen that code. I used POD_NAMESPACE because this environment variable is already used in the Chart unlike KUBERNETES_NAMESPACE. Also, in my opinion, pod namespace is more clear in context of deploying a pod than kubernetes namesapce (which is more like a term to me).
There was a problem hiding this comment.
operator is setting KUBERNETES_NAMESPACE
| @@ -72,7 +72,8 @@ spec: | |||
| valueFrom: | |||
| fieldRef: | |||
| fieldPath: metadata.namespace | |||
|
|
|||
| - name: POD_NAMESPACE | |||
There was a problem hiding this comment.
why not reference it with metadata.namespace as done on many places in this file?
There was a problem hiding this comment.
I've tried that approach first, the result:
› Error: Error: Unable to execute helm command helm upgrade --install che --force --namespace che --set global.cheDomain=192.168.39.227.nip.io --set global.tls.useSelfSignedCerts=true
› --set global.ingressDomain=192.168.39.227.nip.io --set cheImage=quay.io/eclipse/che-server:nightly --set che.disableProbes=false -f
› /home/user/.cache/chectl/templates/kubernetes/helm/che/values/tls.yaml /home/user/.cache/chectl/templates/kubernetes/helm/che/ / Error: YAML parse error on
› che/templates/deployment.yaml: error converting YAML to JSON: yaml: line 52: mapping values are not allowed in this context
› Installation failed, check logs in '/tmp/chectl-logs/1599033986714'
Using of Release.Namespace is safe as it returns the namespace in which the chart is being deployed.
There was a problem hiding this comment.
@sparkoo if you know how to overcome that error, I'll use the reference to metadata.namespace as it is already used in other places.
There was a problem hiding this comment.
@mmorhun looks strange. I don't know how to fix it and I'm ok with keeping it as is.
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
|
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
che-bot
removed
the
status/code-review
Signed-off-by: Mykola Morhun mmorhun@redhat.com
What does this PR do?
Adds
POD_NAMESPACEenvironment variable with value of Che server namespace name into Che server container in Helm chart.What issues does this PR fix or reference?
#17766