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

[stable/mattemost-team-edition] Add initial charts for Mattermost Team Edition #5987

Merged
merged 3 commits into from
Sep 19, 2018
Merged

[stable/mattemost-team-edition] Add initial charts for Mattermost Team Edition #5987

merged 3 commits into from
Sep 19, 2018

Conversation

cpanato
Copy link
Member

@cpanato cpanato commented Jun 7, 2018

What this PR does / why we need it:
Add Mattermost Team Edition to Helm charts

Special notes for your reviewer:
Please let me know if need any clarifications

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 7, 2018
Copy link
Contributor

@jwilander jwilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just a few minor comments

"DataRetentionSettings": {
"Enable": false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to remove any of the enterprise related settings so it's a little cleaner as those won't be needed in the chart

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# Mattermost configuration:
config:
SiteUrl: "http://mattermost.example.com"
SiteName: "MMTE"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe default this to just Mattermost

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


## Introduction

This chart creates a [Mattermost](https://mattermost.com/) deployment on a [Kubernetes](http://kubernetes.io)
Copy link
Contributor

@jwilander jwilander Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to stress that it's team edition so:

This chart creates a Mattermost deployment...

to

This chart creates a Mattermost Team Edition deployment...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@cpanato
Copy link
Member Author

cpanato commented Jun 7, 2018

/assign @lachie83

Copy link
Contributor

@jwilander jwilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpanato cpanato changed the title Add initial charts for Mattermost Team Edition [incubator/mattemost-team-edition] Add initial charts for Mattermost Team Edition Jun 11, 2018
@cpanato
Copy link
Member Author

cpanato commented Jun 22, 2018

hello
@lachie83 @linki I know you guys are busy, but when we expect some reviews for this pr? 😃

@clkao
Copy link
Contributor

clkao commented Jun 26, 2018

Thanks for the chart! I wonder if it is possible to define a configmap generated by the config key in values, which would override arbitrary keys in config.json (maybe with jq's object merge). With that we don't need to change the chart for new configurable options, for example gitlab SSO integration.

subPath: config.json
resources:
{{ toYaml .Values.resources | indent 12 }}
volumes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be an option for /mattermost/data be persistent with a pvc?

Copy link
Member Author

@cpanato cpanato Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the data we are using minio

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, sorry minio is for the other edition. Will add the persistence for that 😄 thanks for the catch!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@clkao clkao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For people using hosted sql, it'd be useful if this chart provides extraContainers and extraVolumes to configure cloud proxy. See the keycloak chart for example

FilesAccessKey:
FilesSecretKey:
FileBucketName:
SMTPHost:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seem this should be SMTPServer according to config.tpl

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@cpanato
Copy link
Member Author

cpanato commented Jul 2, 2018

hello @clkao thanks for all review and feedback!
this part is not clear to me what need to be done, can you help me on that?

I wonder if it is possible to define a configmap generated by the config key in values, which would override arbitrary keys in config.json (maybe with jq's object merge). With that we don't need to change the chart for new configurable options, for example gitlab SSO integration.

@cpanato
Copy link
Member Author

cpanato commented Jul 2, 2018

@clkao for this request:

For people using hosted sql, it'd be useful if this chart provides extraContainers and extraVolumes to configure cloud proxy.

I think it is better to add in a follow up, we need to change how we handle our configuration and this change will allow better manipulation. For now I will put this as TODO. is that ok for you?

@clkao
Copy link
Contributor

clkao commented Jul 2, 2018

I've added a few things including postgresql support, recreate on config changes, and the required hooks for cloud sql proxy. Let me push somewhere for you to see if the changes can be incorporated.

@cpanato
Copy link
Member Author

cpanato commented Jul 5, 2018

@clkao Hello, I was reading more about the cloud sql proxy that you mentioned, and the problem for that it is limited only to Google cloud i thought that was more generic.

@clkao
Copy link
Contributor

clkao commented Jul 10, 2018

Hi @cpanato I made a few improvements in separate commits so it should be easier for you to review and cherry-pick from: pull/5987/head...clkao:mattermost

@cpanato
Copy link
Member Author

cpanato commented Aug 10, 2018

hi @clkao sorry for the delay I was in vacations. I cherry pick all suggestions thanks!!

Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a consistent list indentation style throughout the chart. Either use zero or two spaces.

Why does the chart have dependencies on kube-lego and nginx-ingress? You don't install these on a per-chart basis. You usually have multiple services use the same ingress controller. Also, the ingress that's included is tied to nginx-ingress. What if someone wants to use a different ingress controller? kube-lego is deprecated. I would rather use cert-manager, but again, not on a per-chart basis. I'd suggest you remove these dependencies.

protocol: TCP
name: {{ template "mattermost-team-edition.name" . }}
selector:
app: {{ template "mattermost-team-edition.name" . }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add release label to selector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my fault added only in the label.

apiVersion: v1
kind: ConfigMap
metadata:
name: {{template "mattermost-team-edition.fullname" .}}-tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add standard labels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{{- if .Values.persistence.data.enabled }}
kind: PersistentVolumeClaim
apiVersion: v1
metadata:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add standard labels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

name: {{template "mattermost-team-edition.fullname" .}}-config-json
labels:
chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}"
app: {{ template "mattermost-team-edition.fullname" . }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add release and heritage labels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

kind: Service
metadata:
name: {{ template "mattermost-team-edition.name" . }}
labels:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add release and heritage labels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}"
spec:
selector:
app: {{ template "mattermost-team-edition.name" . }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add release label to selector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

apiVersion: v1
kind: Pod
metadata:
name: "{{template "mattermost-team-edition.fullname" .}}-test-{{ randAlphaNum 5 | lower }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add standard labels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@unguiculus
Copy link
Member

/assign

@cpanato
Copy link
Member Author

cpanato commented Aug 10, 2018

@unguiculus thanks so much for your review and the ingress/kubelego makes sense. removed.

ptal

Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please fix list indentation as mentioned above.

description: Mattermost Team Edition server.
name: mattermost-team-edition
version: 0.1.0
appVersion: 5.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image used is 5.1.1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


# Mattermost configuration:
config:
SiteUrl: "http://mattermost.example.com"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convention is to use lower camel case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

volumes:
- name: config-json
configMap:
name: {{template "mattermost-team-edition.fullname" .}}-config-json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be consistent. Add a space after {{ am before }}. Apply throughout the chart.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

app: {{ template "mattermost-team-edition.fullname" . }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
annotations:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't set nginx-ingress specific annotations by default. Instead provide an option to configure annotations. You may add these defaults to values.yaml and comment them out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks!

heritage: {{ .Release.Service }}
spec:
selector:
app: {{ template "mattermost-team-edition.name" . }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add release label to selector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

protocol: TCP
name: {{ template "mattermost-team-edition.name" . }}
selector:
app: {{ template "mattermost-team-edition.name" . }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it?

enabled: false
path: /
annotations:
kubernetes.io/ingress.class: nginx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be commented out

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mattfarina mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@cpanato cpanato changed the title [incubator/mattemost-team-edition] Add initial charts for Mattermost Team Edition [stable/mattemost-team-edition] Add initial charts for Mattermost Team Edition Sep 7, 2018
@ey-bot ey-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). and removed Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Sep 7, 2018
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 11, 2018
Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please fix list indentation.

@@ -0,0 +1,76 @@
apiVersion: apps/v1beta2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use apps/v1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mysqlRootPassword: root_password
mysqlUser: mmuser
mysqlPassword: mmpasswd
mysqlDatabase: mattermost
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't set a default password.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

image:
repository: mattermost/mattermost-team-edition
tag: 5.2.1
imagePullPolicy: Always
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IfNotPresent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

## Default: volume.alpha.kubernetes.io/storage-class: default
##
# storageClass:
accessMode: ReadWriteOnce
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I run multiple replicas? How would Mattermost behave?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For team edition, you can run just one instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made note in the Readme.md as limitation.

@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 14, 2018
Signed-off-by: cpanato <ctadeu@gmail.com>
Signed-off-by: cpanato <ctadeu@gmail.com>
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 14, 2018
Signed-off-by: cpanato <ctadeu@gmail.com>
@unguiculus
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 19, 2018
@unguiculus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, unguiculus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2018
@k8s-ci-robot k8s-ci-robot merged commit ba3b3d8 into helm:master Sep 19, 2018
@cpanato cpanato deleted the add_mattermost-te branch September 20, 2018 12:16
@cpanato
Copy link
Member Author

cpanato commented Sep 20, 2018

@unguiculus thanks for all the review!

@consideRatio
Copy link
Contributor

consideRatio commented Sep 20, 2018

@cpanato wieeeeeeeeeeeeeeee! :D I'm excited about this! Thank you! ❤️

@clkao you appear all over =D

legal90 added a commit to volvo-cars/helm-charts that referenced this pull request Sep 21, 2018
* superset-annotations: (27 commits)
  [stable/superset] Bump version to 0.1.3
  [stable/superset] Add service annotations support
  [stable/drupal] Fix chart not being upgradable (helm#7825)
  [stable/parse] Fix chart not being upgradable (helm#7824)
  [stable/jasperreports] Fix chart not being upgradable (helm#7818)
  [stable/osclass] Fix chart not being upgradable (helm#7817)
  [stable/phpbb] Fix chart not being upgradable (helm#7820)
  [stable/ghost] Fix chart not being upgradable (helm#7814)
  [stable/suitecrm] Fix chart not being upgradable (helm#7816)
  [stable/phpmyadmin] Fix chart not being upgradable (helm#7830)
  [stable/magento] Release 2.0.6 (helm#7810)
  [stable/wordpress] Fix chart not being upgradable (helm#7831)
  [stable/mongodb] Use .Values.existingSecret for standalone deployments (helm#7839)
  [incubator/kafka]: add reassignPartitions to topic configuration (helm#7623)
  Changed syntax error in custom-metrics-apiserver-service and secret (label to labels) (helm#7295)
  [stable/spinnaker] Changing email of dwardu89 (helm#7838)
  fix: mongo init issues (helm#7772)
  [stable/mattemost-team-edition] Add initial charts for Mattermost Team Edition (helm#5987)
  Fixed TLS Ingress, updated mongo dep requirements and app to latest version (helm#7636)
  Adding dwardu89 to owners and chart maintainers of stable/spinnaker (helm#7751)
  ...
jicowan pushed a commit to jicowan/charts that referenced this pull request Oct 2, 2018
…m Edition (helm#5987)

* Add initial charts for Mattermost Team Edition

Signed-off-by: cpanato <ctadeu@gmail.com>

* update based on feedback

Signed-off-by: cpanato <ctadeu@gmail.com>

* update dependencies

Signed-off-by: cpanato <ctadeu@gmail.com>
Signed-off-by: jenkin-x <jicowan@hotmail.com>
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
…m Edition (helm#5987)

* Add initial charts for Mattermost Team Edition

Signed-off-by: cpanato <ctadeu@gmail.com>

* update based on feedback

Signed-off-by: cpanato <ctadeu@gmail.com>

* update dependencies

Signed-off-by: cpanato <ctadeu@gmail.com>
Signed-off-by: Jakob Niggel <info@jakobniggel.de>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
…m Edition (helm#5987)

* Add initial charts for Mattermost Team Edition

Signed-off-by: cpanato <ctadeu@gmail.com>

* update based on feedback

Signed-off-by: cpanato <ctadeu@gmail.com>

* update dependencies

Signed-off-by: cpanato <ctadeu@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants