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/11/06 20:53:50 UTC
[GitHub] [pulsar-helm-chart] jeantil opened a new pull request #80: Use `.Release.Namespace` by default to handle namespaces
jeantil opened a new pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80
It remains possible to override the current release namespace by setting
the `namespace` value though this may lead to having the helm metadata
and the pulsar components in different namespaces
fixes #66
Fixes #<xyz>
### Motivation
*Explain here the context, and why you're making that change. What is the problem you're trying to solve.*
### Modifications
*Describe the modifications you've done.*
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
----------------------------------------------------------------
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 pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-725622231
I'm not familiar with charts linting, it appears the error is
```
✖︎ pulsar => (version: "2.6.1-3", path: "charts/pulsar") > Error validating maintainer 'The Apache Pulsar Team': 404 Not Found
```
and I don't think I touched that :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] jeantil commented on pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-738278716
@sijie @wolfstudy Sorry to bother you, it's been a couple weeks and I am worndering if there is more work I need to do on the PR to get it merged (and to fix the CI in the process) ? thanks !
----------------------------------------------------------------
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 pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-725615956
I switched to the template function and rebased on master
----------------------------------------------------------------
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 commented on a change in pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#discussion_r521568693
##########
File path: charts/pulsar/templates/bookkeeper-configmap.yaml
##########
@@ -22,7 +22,7 @@ apiVersion: v1
kind: ConfigMap
metadata:
name: "{{ template "pulsar.fullname" . }}-{{ .Values.bookkeeper.component }}"
- namespace: {{ .Values.namespace }}
+ namespace: {{ .Values.namespace | default .Release.Namespace }}
Review comment:
Can you implement this using a template function?
Like what we did in https://github.com/streamnative/charts/blob/master/charts/pulsar/templates/_helpers.tpl#L21
----------------------------------------------------------------
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 merged pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80
----------------------------------------------------------------
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 commented on pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-726487226
@jeantil Can you rebase this pull request to the latest master?
----------------------------------------------------------------
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 a change in pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#discussion_r521904247
##########
File path: charts/pulsar/Chart.yaml
##########
@@ -21,11 +21,11 @@ apiVersion: v1
appVersion: "2.6.1"
description: Apache Pulsar Helm chart for Kubernetes
name: pulsar
-version: 2.6.1-2
+version: 2.6.1-3
home: https://pulsar.apache.org
sources:
-- https://github.com/apache/pulsar
+ - https://github.com/apache/pulsar
Review comment:
sorry about that, It's from the prettier plugin in my vscode install, I was pretty sure I had it disabled for this workspace and was careless when adding the modifications. I have reverted the indentation changes
----------------------------------------------------------------
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 commented on pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-738543056
@jeantil sorry that I missed your comment.
----------------------------------------------------------------
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 pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-739242498
Thank you @sijie !
----------------------------------------------------------------
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 a change in pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#discussion_r521587737
##########
File path: charts/pulsar/templates/bookkeeper-configmap.yaml
##########
@@ -22,7 +22,7 @@ apiVersion: v1
kind: ConfigMap
metadata:
name: "{{ template "pulsar.fullname" . }}-{{ .Values.bookkeeper.component }}"
- namespace: {{ .Values.namespace }}
+ namespace: {{ .Values.namespace | default .Release.Namespace }}
Review comment:
sure, see updated commit
----------------------------------------------------------------
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 commented on pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-725598100
regarding the CI, can you rebase your pull request to latest master to include the fix in #76 ?
----------------------------------------------------------------
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 commented on a change in pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#discussion_r521624691
##########
File path: charts/pulsar/Chart.yaml
##########
@@ -21,11 +21,11 @@ apiVersion: v1
appVersion: "2.6.1"
description: Apache Pulsar Helm chart for Kubernetes
name: pulsar
-version: 2.6.1-2
+version: 2.6.1-3
home: https://pulsar.apache.org
sources:
-- https://github.com/apache/pulsar
+ - https://github.com/apache/pulsar
Review comment:
Any ideas on these changes?
##########
File path: charts/pulsar/values.yaml
##########
@@ -372,8 +373,8 @@ bookkeeper:
##
resources:
# requests:
- # memory: 4Gi
Review comment:
Any ideas on these indention changes?
----------------------------------------------------------------
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] wolfstudy commented on pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-726104988
@jeantil After https://github.com/apache/pulsar-helm-chart/pull/83 is merged, please merge the latest master code to ensure that ci passes. Thanks.
----------------------------------------------------------------
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 pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-723298365
@wolfstudy I assume that the ci fails because I don't belong to a specific organization, not sure how I can fix it
```
Digest: sha256:b0d5e8220d7f0594952dc1bd452ce3c9c0a724484320645077eccf610648b88b
Status: Downloaded newer image for gcr.io/google-containers/kube-scheduler:v1.14.10
gcr.io/google-containers/kube-scheduler:v1.14.10
ERROR: no nodes found for cluster "pulsar-ci-1e799b03-a506-4be2-a1e3-117912596fe9"
```
----------------------------------------------------------------
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 pull request #80: Use `.Release.Namespace` by default to handle namespaces
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #80:
URL: https://github.com/apache/pulsar-helm-chart/pull/80#issuecomment-726654784
@sijie @wolfstudy I rebased on master and incremented the version to 2.6.2-2
The lint task is green but I think it actually failed, the following comes up as a warning in the action
>Unexpected input(s) 'command', valid inputs are ['version']
The rest of the CI chain seemed stuck, after digging in a bit I realized it depended on the namespace being defined in the default values file (not overriden in the `.ci/cluster/` value files and didn't pass the target namespace to helm.
Because of my patch the deployed containers ended up in the `default` namespace but the verification commands using kubectl explicitely queried the `pulsar` namespace.\
I have updated the ci script to explicitely pass the target namespace to helm. let me know if you prefer it another way ...
----------------------------------------------------------------
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