Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Add rocket.chat #752

Merged
merged 10 commits into from
Jul 5, 2017
Merged

Add rocket.chat #752

merged 10 commits into from
Jul 5, 2017

Conversation

pierreozoux
Copy link
Contributor

No description provided.

@k8s-ci-robot
Copy link
Contributor

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 @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 3, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 3, 2017
@pierreozoux pierreozoux force-pushed the rocket.chat branch 2 times, most recently from 99934fc to 7fe1313 Compare March 13, 2017 10:27
name: {{ template "fullname" . }}
key: mongo-uri
{{ else }}
value: mongodb://{{ template "mongodb.fullname" . }}:27017/meteor
Copy link
Collaborator

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
Copy link
Collaborator

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?

Copy link
Contributor

@linki linki Mar 16, 2017

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).

@linki
Copy link
Contributor

linki commented Mar 16, 2017

@pierreozoux can you have another look at this?

@geekgonecrazy Thanks for your thoughts.

@pierreozoux
Copy link
Contributor Author

@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
Copy link
Contributor

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.

@pierreozoux
Copy link
Contributor Author

Is there anything I can do to get this merged?

@engelgabriel
Copy link

The Rocket.Chat team is looking forward for this!

@geekgonecrazy
Copy link
Collaborator

geekgonecrazy commented Apr 19, 2017

@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:

name: INSTANCE_IP
valueFrom:
  fieldRef:
    fieldPath: status.podIP

This way if scaled the instances can know how to talk to each other over the overlay network

@pierreozoux
Copy link
Contributor Author

@geekgonecrazy hum, do you have the link to the documentation?

If the person uses S3, it would scale automatically out of the box.
Let's think about this INSTANCE_IP (which is a better way I guess) for another PR, once this got merged?

@geekgonecrazy
Copy link
Collaborator

geekgonecrazy commented Apr 26, 2017

@pierreozoux
Here's a snippet directly out of one of our deployments:
testdeploy.yaml

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    DeploymentDomain: testdeploy.rocket.chat
    DeploymentName: testdeploy
  name: testdeploy
  namespace: <namespace here>
spec:
  replicas: 2
  revisionHistoryLimit: 1
  selector:
    matchLabels:
      DeploymentDomain: testdeploy.rocket.chat
      DeploymentName: testdeploy
  strategy:
    rollingUpdate:
      maxSurge: 1
      maxUnavailable: 1
    type: RollingUpdate
  template:
    metadata:
      creationTimestamp: null
      labels:
        DeploymentDomain: testdeploy.rocket.chat
        DeploymentName: testdeploy
    spec:
      containers:
      - env:
        - name: INSTANCE_IP
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: status.podIP
        - <another 10-20 ommited for briefness>
        image: rocketchat/rocket.chat:0.55.1
        imagePullPolicy: IfNotPresent
        name: testdeploy
        ports:
        - containerPort: 3000
          protocol: TCP
Its for sure something that could be added in later.

@viglesiasce
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 5, 2017

@pierreozoux: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
Jenkins Charts e2e c11f5277d86c42457d0eca1b4c8dc2423deca273 link @k8s-bot e2e test this

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" . -}}
Copy link
Contributor

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 -}}

Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 29, 2017
@geekgonecrazy
Copy link
Collaborator

Helping @pierreozoux get things finished here.

@unguiculus @viglesiasce I think i've now addressed all feedback.

  • Added maintainers. I went ahead and left our org and email for contact, and added two maintainers to the list.
  • Fixed Notes so the output will suggest how to get site going at http://chat.yourdomain.com. Fixed a few selectors in the queries it says to run as well as the port forward port
  • Now can be installed with as little as: helm install stable/rocketchat brings up mongodb and rocket.chat

I've also signed the CLA

@geekgonecrazy
Copy link
Collaborator

@viglesiasce @unguiculus @linki would any of you guys be able to take a look at this?

@viglesiasce
Copy link
Contributor

@geekgonecrazy I'll take a look today.

@geekgonecrazy
Copy link
Collaborator

@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.

@viglesiasce
Copy link
Contributor

Worked a treat! Thanks!

@viglesiasce viglesiasce merged commit 496c968 into helm:master Jul 5, 2017
@geekgonecrazy
Copy link
Collaborator

Perfect! Thanks @viglesiasce !

@pierreozoux pierreozoux deleted the rocket.chat branch July 6, 2017 11:27
@pierreozoux
Copy link
Contributor Author

\o/ Thanks!

lachie83 added a commit to lachie83/charts that referenced this pull request Jul 10, 2017
* 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)
  ...
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
* 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
@sathieu
Copy link
Contributor

sathieu commented Nov 29, 2017

Hello,

It is not on https://kubeapps.com/. Can someone push it?

@sathieu
Copy link
Contributor

sathieu commented Nov 29, 2017

and there is no README...

@unguiculus
Copy link
Member

@prydonius Could you check why the chart is not listed?

@geekgonecrazy
Copy link
Collaborator

Would be glad to help make what ever changes are needed to make it show up.

@sathieu
Copy link
Contributor

sathieu commented Nov 30, 2017

Maybe this is the README missing?

@beranPro
Copy link

Hi @pierreozoux @viglesiasce ,
rocket.chat is still not available at kubeapps.
Other projects on helm have Readme.md's and they are listet on kubeapps and the readme is displayed.
Have anybode more information? How can we fix this?

Thanks a lot!

@geekgonecrazy
Copy link
Collaborator

@beranPro @sathieu is it just a matter of missing readme? If you could point me to what ever docs related to this i'll gladly put together

@beranPro
Copy link

Hi @geekgonecrazy ,
sadly i couldn't find some docs about this. i only compared the wordpress filestructure with the rocket-chat structure. Maybe someone can add a Readme.md. I could copy'n'paste the radme from the project but i think this doesn't make any sense.

@shinebayar-g
Copy link

Please maintain your helm chart up to date.

@geekgonecrazy
Copy link
Collaborator

we are aware of an issue created by an update to the helm chart here #7636

The updated mongo dependency introduced an automatic password generation that wasn't present before. Passing: --set mongodb.usePassword=false helm install temporarily solves, we have a fix incoming soon.

cc: @LuluGO

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changes needed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.