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

[stable/suitecrm] adds chart to deploy SuiteCRM #1258

Merged
merged 13 commits into from
Jul 31, 2017

Conversation

sameersbn
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 7, 2017
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 apply best practices for standard labels, especially the app label.

https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md

@unguiculus unguiculus self-assigned this Jun 24, 2017
@unguiculus
Copy link
Member

Marking as stale. Please update within the next week.

@unguiculus
Copy link
Member

Closing as stale. Feel free to re-open if/when you decide to work on this again.

@unguiculus unguiculus closed this Jul 28, 2017
@sameersbn
Copy link
Contributor Author

sorry missed you comments.
updated the chart with the suggested changes.

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.

The app label should be {{ template "name" . }}.


{{/*
Create a default fully qualified app name.
We truncate at 24 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
Copy link
Member

Choose a reason for hiding this comment

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

24 -> 63


{{/*
Create a default fully qualified app name.
We truncate at 24 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
Copy link
Member

Choose a reason for hiding this comment

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

24 -> 63

@sameersbn
Copy link
Contributor Author

updated with the required changes.


{{- if eq .Values.serviceType "ClusterIP" }}

export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app={{ template "fullname" . }}" -o jsonpath="{.items[0].metadata.name}")
Copy link
Member

Choose a reason for hiding this comment

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

"app={{ template "fullname" . }}" -> "app={{ template "name" . }},release={{ .Release.Name }}"

spec:
template:
metadata:
labels:
Copy link
Member

Choose a reason for hiding this comment

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

Only app and release label should be added here.

port: 443
targetPort: https
selector:
app: {{ template "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.

containers:
- name: {{ template "fullname" . }}
image: "{{ .Values.image }}"
imagePullPolicy: {{ default "" .Values.imagePullPolicy | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

No need to add default "" .

- name: SUITECRM_HOST
value: {{ include "host" . | quote }}
- name: SUITECRM_USERNAME
value: {{ default "" .Values.suitecrmUsername | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

No need to add default "" if you add an empty default to values.yml. Apply where applicable in the whole chart.

@unguiculus unguiculus removed the stale label Jul 29, 2017
@sameersbn
Copy link
Contributor Author

updated! thanks for the review.


{{- if eq .Values.serviceType "ClusterIP" }}

export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app={{ template "name" . }}" -o jsonpath="{.items[0].metadata.name}")
Copy link
Member

@unguiculus unguiculus Jul 30, 2017

Choose a reason for hiding this comment

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

"app={{ template "name" . }}" -> "app={{ template "name" . }},release={{ .Release.Name }}"

@sameersbn
Copy link
Contributor Author

updated!

@unguiculus unguiculus added code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed labels Jul 31, 2017
@unguiculus unguiculus merged commit 2cdc360 into helm:master Jul 31, 2017
munnerz added a commit to munnerz/charts-1 that referenced this pull request Feb 5, 2019
* Automated cherry pick of helm#1314 (cert-manager/cert-manager#1315)
* Automated cherry pick of helm#1294 (cert-manager/cert-manager#1296)
* Automated cherry pick of helm#1276 (cert-manager/cert-manager#1277)
* Automated cherry pick of helm#1258 helm#1266 (cert-manager/cert-manager#1273)
* Automated cherry pick of helm#1259 (cert-manager/cert-manager#1260)
* Update Chart.yaml in webhook (cert-manager/cert-manager#1249)
munnerz added a commit to munnerz/charts-1 that referenced this pull request Feb 5, 2019
* Automated cherry pick of helm#1314 (cert-manager/cert-manager#1315)
* Automated cherry pick of helm#1294 (cert-manager/cert-manager#1296)
* Automated cherry pick of helm#1276 (cert-manager/cert-manager#1277)
* Automated cherry pick of helm#1258 helm#1266 (cert-manager/cert-manager#1273)
* Automated cherry pick of helm#1259 (cert-manager/cert-manager#1260)
* Update Chart.yaml in webhook (cert-manager/cert-manager#1249)

Signed-off-by: James Munnelly <james@munnelly.eu>
@sameersbn sameersbn deleted the chart-suitecrm branch July 9, 2019 09:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants