Add chart elasticsearch-exporter #2525
Conversation
k8s-ci-robot
added
needs-ok-to-test
|
can someone please review this PR? |
|
Looking forward for this PR to be merged |
|
/assign @mattfarina any idea how this review process can be continued? |
There was a problem hiding this comment.
Thanks for the PR. Please make sure you read our review guidelines. https://github.com/unguiculus/charts/blob/master/REVIEW_GUIDELINES.md
| - elasticsearch | ||
| - monitoring | ||
| maintainers: | ||
| - name: Sven Mueller |
There was a problem hiding this comment.
Use Github username and add appVersion.
|
|
||
| ## Prerequisites | ||
|
|
||
| - Kubernetes 1.5+ with Beta APIs enabled |
There was a problem hiding this comment.
1.8+
We officially support the current and previous minor version of Kubernetes.
| {{/* | ||
| Expand the name of the chart. | ||
| */}} | ||
| {{- define "name" -}} |
There was a problem hiding this comment.
Use namespaced templates. Creating a chart with helm create would give you a good basis with current best practices applied.
| name: {{ template "fullname" . }}-cert | ||
| labels: | ||
| chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}" | ||
| app: {{ template "fullname" . }} |
There was a problem hiding this comment.
Shoudl be app: {{ template "name" . }} on all resources.
| app: {{ template "fullname" . }} | ||
| release: "{{ .Release.Name }}" | ||
| heritage: "{{ .Release.Service }}" | ||
| component: elasticsearch-exporter |
There was a problem hiding this comment.
You don't have multiple components. I'd remove this label.
| @@ -0,0 +1,95 @@ | |||
| apiVersion: extensions/v1beta1 | |||
| ports: | ||
| - name: http-service | ||
| port: {{ .Values.service.externalPort }} | ||
| targetPort: {{ .Values.service.externalPort }} |
There was a problem hiding this comment.
This is not a good idea. You'd break the chart if you change the external port.
|
Thx for the feedback. I will apply the requested changes ASAP. |
|
@unguiculus Here we go. The requested changes are done. Thx again for reviewing 👍 |
|
/assign @mattfarina |
| ports: | ||
| - name: http-service | ||
| port: {{ .Values.service.externalPort }} | ||
| targetPort: {{ .Values.service.internalPort }} |
There was a problem hiding this comment.
Remove service.internalPort. It doesn't make sense to have it configurable. The container port is hard-coded anyways. Just reference the named port (http). You can the rename service.externalPort to just service.port.
|
@unguiculus thx for the the helpful feedback 👍 i commited the requested changes. |
|
/assign @lachie83 |
|
@unguiculus can you please have another look? |
| tag: 1.0.2 | ||
| pullPolicy: IfNotPresent | ||
|
|
||
| resources: |
There was a problem hiding this comment.
Defaults for resources should be commented out.
resources: {}
# requests:
# cpu: 100m
# memory: 128Mi
# limits:
# cpu: 100m
# memory: 128MiThere was a problem hiding this comment.
@unguiculus not sure if i can follow you here. if i comment the values out as requested by you, then there are no default values defined anymore. can you please explain?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thx for the clarification 👍
There was a problem hiding this comment.
I removed the default resources as suggested in the review guide lines.
| appVersion: 1.0.2 | ||
| sources: | ||
| - https://github.com/justwatchcom/elasticsearch_exporter | ||
| keywords: |
|
Looks good now. I'd suggest you move it to stable before we merge it. |
|
@unguiculus Thx again for the review. I applied the latest changes (no default values for resources + moved helm chart to stable folder) |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@mattfarina who can merge this PR? |
unguiculus
changed the title
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: svenmueller, unguiculus The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
Add helm chart for managing Prometheus Elasticsearch-Exporter instances (https://github.com/justwatchcom/elasticsearch_exporter).