New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cloudflare-datadog chart #1324

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@metadave
Contributor

metadave commented Jun 20, 2017

see also: #780

@so0k since you did most of the work on this, do you want me to add your name and email to the maintainers?

cc @jgmize @glogiotatidis

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jun 20, 2017

Contributor

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

I understand the commands that are listed here.

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.

Contributor

k8s-ci-robot commented Jun 20, 2017

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

I understand the commands that are listed here.

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.

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Jun 21, 2017

Contributor

@metadave - it's fine, sorry I didn't have time to work on my previous PR - Thanks for taking this over

Contributor

so0k commented Jun 21, 2017

@metadave - it's fine, sorry I didn't have time to work on my previous PR - Thanks for taking this over

@metadave

This comment has been minimized.

Show comment
Hide comment
@metadave

metadave Jun 21, 2017

Contributor

@so0k excellent, added!

Contributor

metadave commented Jun 21, 2017

@so0k excellent, added!

@viglesiasce

This comment has been minimized.

Show comment
Hide comment
@viglesiasce

viglesiasce Jul 5, 2017

Contributor

@k8s-bot ok to test

Contributor

viglesiasce commented Jul 5, 2017

@k8s-bot ok to test

replicaCount: 1
image:
repository: mozmeao/cloudflare-datadog
tag: latest

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

How about not using latest and using IfNotPresent?

@unguiculus

unguiculus Jul 20, 2017

Member

How about not using latest and using IfNotPresent?

@@ -0,0 +1,16 @@
apiVersion: v1

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

Can you add an icon and appVersion?

Also, please use Github usernames instead of real names.

@unguiculus

unguiculus Jul 20, 2017

Member

Can you add an icon and appVersion?

Also, please use Github usernames instead of real names.

heritage: {{ .Release.Service | quote }}
release: {{ .Release.Name | quote }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
app: {{ template "fullname" . }}

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

Please apply best practices for lablels.
https://github.com/kubernetes/helm/tree/master/docs/chart_best_practices
The app label should be {{ template "name" . }}.

@unguiculus

unguiculus Jul 20, 2017

Member

Please apply best practices for lablels.
https://github.com/kubernetes/helm/tree/master/docs/chart_best_practices
The app label should be {{ template "name" . }}.

template:
metadata:
labels:
heritage: {{ .Release.Service | quote }}

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

The heritage label can be removed here.

@unguiculus

unguiculus Jul 20, 2017

Member

The heritage label can be removed here.

chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
app: {{ template "fullname" . }}
spec:
replicas: {{default 1 .Values.replicaCount}}

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

It's better to add defaults to values.yaml only. Don't specify them here if not necessary.

Use consistent formatting: Space after {{ and before }}.

@unguiculus

unguiculus Jul 20, 2017

Member

It's better to add defaults to values.yaml only. Don't specify them here if not necessary.

Use consistent formatting: Space after {{ and before }}.

repository: mozmeao/cloudflare-datadog
tag: latest
pullPolicy: Always
resources:

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

We started recommending not to specify defaults for resources. This should be a conscious choice of the user.

@unguiculus

unguiculus Jul 20, 2017

Member

We started recommending not to specify defaults for resources. This should be a conscious choice of the user.

{{/*
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).

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

24 -> 63

@unguiculus

unguiculus Jul 20, 2017

Member

24 -> 63

## Pre Requisites:
* Requires (and tested with) helm `v2.1.2` or above.

This comment has been minimized.

@unguiculus

unguiculus Jul 20, 2017

Member

v2.1.2 is kind of dated.

@unguiculus

unguiculus Jul 20, 2017

Member

v2.1.2 is kind of dated.

@unguiculus

This comment has been minimized.

Show comment
Hide comment
@unguiculus

unguiculus Jul 28, 2017

Member

Marking this as stale. Please up date within the next week.

Member

unguiculus commented Jul 28, 2017

Marking this as stale. Please up date within the next week.

@metadave metadave closed this Jul 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment