-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Conversation
Hi @pierreozoux. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. I understand the commands that are listed here. |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
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. I understand the commands that are listed here. |
99934fc
to
7fe1313
Compare
name: {{ template "fullname" . }} | ||
key: mongo-uri | ||
{{ else }} | ||
value: mongodb://{{ template "mongodb.fullname" . }}:27017/meteor |
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.
To increase consistency with other Rocket.Chat documentation maybe use rocketchat as the db name here?
name: {{ template "fullname" . }} | ||
annotations: | ||
kubernetes.io/tls-acme: "true" | ||
kubernetes.io/ingress.class: nginx |
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.
This automatically assumes they have an nginx ingress running. Is this safe?
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 should be commented out by default. Similar to https://github.com/kubernetes/charts/blob/e80d1cfb8857cc71096b3b8a4638a789b378db88/stable/grafana/values.yaml#L23
This will allow the Chart to work by default. People that use the nginx-based ingress controller can either opt-in by setting this to "nginx" or they configure their cluster/ingress-controller to pick up Ingresses without annotations, too (like GKE does).
15e6ad2
to
053b406
Compare
@pierreozoux can you have another look at this? @geekgonecrazy Thanks for your thoughts. |
@linki I addressed all the comments already (I didn't comment, I just fixed it to avoid noise). But yeah, thanks @geekgonecrazy for your comments, I fixed them all :) Let me know if there is anything more needed? |
@@ -0,0 +1 @@ | |||
.git |
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.
This is not the standard .gitignore
. Let's wait on the outcome of https://github.com/kubernetes/charts/pull/485/files#r106305261
Looks like we prefer the "canonical" helmignore from now on but many charts still have just this. It probably makes more sense to sync them up in a separate PR across all charts.
053b406
to
c11f527
Compare
Is there anything I can do to get this merged? |
The Rocket.Chat team is looking forward for this! |
@pierreozoux one thing to maybe add here is the environment variables to populate the INSTANCE_IP variable with the pod ip. Here is a snippet I have for adding this to the schema:
This way if scaled the instances can know how to talk to each other over the overlay network |
@geekgonecrazy hum, do you have the link to the documentation? If the person uses S3, it would scale automatically out of the box. |
@pierreozoux
|
@k8s-bot ok to test |
@pierreozoux: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
@@ -0,0 +1,65 @@ | |||
{{- if include "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.
What is this line trying to do? Shouldnt it be:
{{- if .Values.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 linter is also complaining about it and I the pod is crashing with the following when it is not set:
/app/bundle/programs/server/boot.js:356
}).run();
^
Error: $ROOT_URL, if specified, must be an URL
at Error (native)
at packages/meteor.js:1218:13
at packages/meteor.js:1233:4
at packages/meteor.js:1380:3
at /app/bundle/programs/server/boot.js:303:34
at Array.forEach (native)
at Function._.each._.forEach (/app/bundle/programs/server/node_modules/underscore/underscore.js:79:11)
at /app/bundle/programs/server/boot.js:128:5
at /app/bundle/programs/server/boot.js:352:5
at Function.run (/app/bundle/programs/server/profile.js:510:12)
If this is a required field look at what ghost and gitlab-ce have done for this use case.
Helping @pierreozoux get things finished here. @unguiculus @viglesiasce I think i've now addressed all feedback.
I've also signed the CLA |
@viglesiasce @unguiculus @linki would any of you guys be able to take a look at this? |
@geekgonecrazy I'll take a look today. |
@viglesiasce thank you! I've also joined in the slack #helm-users channel and glad to chat and address what ever concerns if any still remain. |
Worked a treat! Thanks! |
Perfect! Thanks @viglesiasce ! |
\o/ Thanks! |
* upstream/master: (67 commits) Fix json whitespace (helm#1458) Use consistent whitespace in template placeholders (helm#1437) [stable/selenium] Make hub readiness probe timeout configurable (helm#1391) [stable/kube2iam]: add rbac support (helm#1286) [stable/traefik] Allow enabling traefik access logs (helm#1302) Add Stash chart (helm#1420) Add Gearman G2 chart (helm#1421) add option to include tolerations to daemonset (helm#1364) Moved Artifactory to stable and updated version to 5.3.2 (helm#1314) Concourse postgres conditional dependency (helm#1390) Typo in helm install command for dask-distributed (helm#1413) [stable/fluent-bit] Fluent Bit v0.11.12 (helm#1417) fixed cassandra chart's persistence bug (helm#1245) Prometheus: modify config to support k8s 1.6 by default (helm#1080) Add rocket.chat (helm#752) Fix influxdb deployment (helm#1424) feat(stable/etcd-operator): add support for supplying additional command args (helm#1418) add configurable service annotations helm#1234 (helm#1244) [stable/prometheus] extra environment variable for alert manager (helm#1237) [stable/heapster] Default service name to Heapster (helm#1266) ...
* Add rocket.chat * Add additional maintainers beyond the github org * Fix mongo dependency * Improved Labels, Chart Defaults, Notes * Add labels to pod for proper kubectl selection * Allow chart to be installed with no values set * Improved output notes * add helm chart to suggested install command * Add deploy platform environment variable * Add Instance IP to pod env variable to make scaling easier * Switch app name to use app's name * Update to latest Rocket.Chat version * Remove else from ROOT_URL, its not needed
Hello, It is not on https://kubeapps.com/. Can someone push it? |
and there is no README... |
@prydonius Could you check why the chart is not listed? |
Would be glad to help make what ever changes are needed to make it show up. |
Maybe this is the README missing? |
Hi @pierreozoux @viglesiasce , Thanks a lot! |
Hi @geekgonecrazy , |
Please maintain your helm chart up to date. |
No description provided.