-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Conversation
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 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. |
stable/scalyr/values.yaml
Outdated
# Default values for scalyr. | ||
image: | ||
repository: scalyr/scalyr-docker-agent | ||
tag: latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a fixed version.
metadata: | ||
name: {{ template "fullname" . }} | ||
labels: | ||
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add missing labels. See https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md.
Thanks @unguiculus I addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick changes. I've found a few more things.
apiVersion: v1 | ||
kind: ConfigMap | ||
metadata: | ||
name: {{ template "configuration.fullname" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you name the config map differently? I'd suggest to use {{ template "fullname" . }}
as well.
stable/scalyr/templates/secrets.yaml
Outdated
heritage: "{{ .Release.Service }}" | ||
type: Opaque | ||
data: | ||
api-key: {{ default "MISSING" .Values.scalyr.apiKey | b64enc | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what makes more sense to you, but are you aware of the new required
function? See https://github.com/kubernetes/helm/blob/master/docs/charts_tips_and_tricks.md#know-your-template-functions.
stable/scalyr/values.yaml
Outdated
## | ||
# apiKey: | ||
|
||
resources: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make resources configurable but do not specify defaults. This should be a conscious choice of the user.
metadata: | ||
name: {{ template "fullname" . }} | ||
labels: | ||
app: {{ template "fullname" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{ template "name" . }}
would be a better choice for the app
label. I'd suggest you change it everywhere.
I just tried to deploy the chart and got the following error:
The same happens with |
@pierreozoux ping, can you update to address @unguiculus's comments? |
@prydonius can we keep it on hold, I'm currently working for a client, and I want them to sign the CLA before continuing this task. |
Addressed the comments thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found one more thing.
template: | ||
metadata: | ||
labels: | ||
app: {{ template "fullname" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add release label: release: "{{ .Release.Name }}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done
@k8s-bot ok to test |
@pierreozoux: The following test failed, say
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. |
Sorry for the delay. It should be possible to run install the chart with defaults. |
Closing as stale. Feel free to reopen if/when you decide to work on this again. |
- Borrwed from helm/charts#1066 - Up to date with https://www.scalyr.com/help/install-agent-kubernetes - Still needs ConfigMap support Signed-off-by: Scott Rigby <[email protected]>
@pierreozoux I drew from some of your earlier work for a new Scalyr Helm chart. Check out scalyr/scalyr-agent-2#118 (comment) if you're still interested in this - always up for collaboration 😄 |
No description provided.