You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/09/25 13:46:07 UTC

[GitHub] [pulsar-helm-chart] Imunhatep opened a new issue #66: Helm charts are managing namespace

Imunhatep opened a new issue #66:
URL: https://github.com/apache/pulsar-helm-chart/issues/66


   **Describe the bug**
   Helm charts are managing namespace.  
   
   helm does not position it self as a namespace manager, as namespaces in kubernetes are considered as a higher control structure that is not part of the application. 
   
   **To Reproduce**
   Use helm for installation without prior syncing namespaces in chart values and installation cmd:
   
   ```
   kubectl create ns pulsartest
   helm upgrade --install pulsar -n pulsartest streamnative/pulsar
   Error: namespaces "pulsar" not found
   ```
   even with `namespaceCreate: false`
   
   **Expected behavior**
   Do not manage kubernetes namespaces in helm charts.
   
   * in case I failed to convince you, at least use consistent variables to refer to a proper namespace e.g. `{ .Release.Namespace }`
   
   **Desktop (please complete the following information):**
    - Linux


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar-helm-chart] Imunhatep commented on issue #66: Helm charts are managing namespace

Posted by GitBox <gi...@apache.org>.
Imunhatep commented on issue #66:
URL: https://github.com/apache/pulsar-helm-chart/issues/66#issuecomment-723197761


   This issue is not about usage of namespace refs in rbac manifests or other kinds.. This issue is about managing `kind: namespace` , i.e. create, update, delete e.t.c  with HELM chart. 
   
   I reported same issue for other pulsar chart: https://github.com/streamnative/charts/issues/140


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar-helm-chart] jeantil commented on issue #66: Helm charts are managing namespace

Posted by GitBox <gi...@apache.org>.
jeantil commented on issue #66:
URL: https://github.com/apache/pulsar-helm-chart/issues/66#issuecomment-723224112


   I am confused, in your initial issue you provided the following reproduction steps:
   ```
   kubectl create ns pulsartest
   helm upgrade --install pulsar -n pulsartest apache/pulsar
   Error: namespaces "pulsar" not found
   ```
   the chart **does not** try to create/update/delete the namespace and the chart will succeed if you configure it correclty (pass `--set namespace=pulsartest`) still without creating/updating/deleting a namespace.So you are forced to repeat the namespace information which is suboptimal.
   
   The helm's best practice indicate that to avoid this repetition chart maintainers are not supposed to embbed namespace definitions in their templates. 
   The problem is that making the chart follow the official helm conventions ( which is easy)  will fail in some common helm/k8s workflows because of the rbac/FQDN issues i mention in the referenced comment.
   
   In the stream native chart they had already applied the other alternative I mentioned  : 
   > I am considering using defaulting instead of fully removing the namespace mentions
   > ```{{ .Values.namespace | default .Release.Namespace }}```
   
   They had left a default value in the values file which required you to pass `--set namespace=null` to get the desired behavior. The fix they applied to your issue was to remove the default value and align the documentation.
   
   I'm hoping one of the maintainers for the apache chart will comment on what they would consider the correct option for their chart :D


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar-helm-chart] sijie closed issue #66: Helm charts are managing namespace

Posted by GitBox <gi...@apache.org>.
sijie closed issue #66:
URL: https://github.com/apache/pulsar-helm-chart/issues/66


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar-helm-chart] jeantil commented on issue #66: Helm charts are managing namespace

Posted by GitBox <gi...@apache.org>.
jeantil commented on issue #66:
URL: https://github.com/apache/pulsar-helm-chart/issues/66#issuecomment-723193163


   I started an attempt at implementing helm's 3 best practices regarding namespace management as laid out by one helm's core maintainer in [this comment](https://github.com/helm/helm/issues/5465#issuecomment-473942223)
   
   However after trying to apply the namespace-less recommendation I encoutered at  [2 issues](https://github.com/helm/helm/issues/5465#issuecomment-723182962) which made me doubt that the official best practice was sound 
   
   I am considering using defaulting instead of fully removing the namespace mentions 
   ```
   {{ .Values.namespace | default .Release.Namespace }}
   ```
   This would make the default behaviour align with the recommendations while allowing users to pin the namespace through value
   I wonder if people in the team have an opinion ...


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar-helm-chart] Imunhatep commented on issue #66: Helm charts are managing namespace

Posted by GitBox <gi...@apache.org>.
Imunhatep commented on issue #66:
URL: https://github.com/apache/pulsar-helm-chart/issues/66#issuecomment-726040127


   Hey, thank you for follow up.
   
   Here I see chart is managing NS and in fact will try to create/delete it: [https://github.com/apache/pulsar-helm-chart/blob/master/charts/pulsar/templates/namespace.yaml]()
   
   While it seems convenient to be able to create namespace by configuring the chart, but the whole goal of mentioned helm issues, is to point out missing tooling in helm to properly manage NS. 
   
   At the end, it's up to maintainers to decide how to handle charts.. e.g. streamnative decided that updating docs is the way, well.. ok :)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org