Skip to content
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

HELM add variabels for runInterval & timeout #424

Merged
merged 5 commits into from Apr 20, 2020

Conversation

NissesSenap
Copy link
Contributor

Fixing #422 also added the possibility to enable & disable checkReaper, I think this will be nice when doing e2e tests.

@@ -1,3 +1,5 @@
{{- if .Values.checkReaper.enabled }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm hesitant to make check-reaper configurable in helm... Only concern being that if folks are using Kuberhealthy 2.1.0+, checks will not get cleaned up. If we were to make it configurable in Helm, it would be definitely have to be enabled by default and customers should be warned about disabling it

image:
repository: kuberhealthy/pod-status-check
tag: v1.2.2
extraEnvs:
nodeSelector: {}

checkReaper:
enabled: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mentioned already in the above comment, but a warning would be nice here if this is disabled! Also wondering if this should be contingent on what Kuberhealthy version they're using. I say that if they're using Kuberhealthy 2.1.0+, this should be enabled!

@joshulyne
Copy link
Collaborator

thanks for addressing your own issue :) added a comment about the checkReaper but other than that, lgtm!

@jonnydawg @chadbitzer, i know you guys did most of the helm configurations, so would be nice to get your reviews!

@NissesSenap
Copy link
Contributor Author

No worries.

I added a extra varning in the values file as you pointed out @joshulyne.
Hope you think that is enough.

Will put in a seperate PR instead so I also update the rest of the
deployment files.
Copy link
Collaborator

@jonnydawg jonnydawg left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for doing this!

podSpec:
containers:
- name: deployment
image: {{ .Values.check.deployment.image.repository }}:{{ .Values.check.deployment.image.tag }}
imagePullPolicy: IfNotPresent
env:
- name: CHECK_TIME_LIMIT
value: *deployment_check_timeout
value: {{ .Values.check.deployment.timeout }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I was wondering if there was a better way to do this (yaml anchors and nodes) -- it seems like helm is a perfect solution!

Copy link
Collaborator

@joshulyne joshulyne left a comment

Choose a reason for hiding this comment

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

lgtm! merging :) @NissesSenap thanks!

@joshulyne joshulyne merged commit 4052744 into kuberhealthy:master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants