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

[velero] probes only when metrics are enabled #471

Merged
merged 1 commit into from Jul 4, 2023

Conversation

maxime1907
Copy link
Contributor

@maxime1907 maxime1907 commented Jun 29, 2023

Special notes for your reviewer:

Fixes #470

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the values.yaml or README.md
  • Title of the PR starts with chart name (e.g. [velero])

qiuming-best
qiuming-best previously approved these changes Jun 30, 2023
Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

I think we should add

{{- if .Values.metrics.enabled }}
{{- end }}

between

{{- with .Values.livenessProbe }}
livenessProbe: {{- toYaml . | nindent 12 }}
{{- end }}
{{- with .Values.readinessProbe }}
readinessProbe: {{- toYaml . | nindent 12 }}
{{- end }}
{{- with .Values.containerSecurityContext }}

@maxime1907
Copy link
Contributor Author

@jenting its done!

@jenting
Copy link
Collaborator

jenting commented Jun 30, 2023

@maxime1907 Given the http-monitoring is valid when .Values.metrics.enabled = true

{{- if .Values.metrics.enabled }}
ports:
- name: http-monitoring
containerPort: 8085
{{- end }}

So, we should change the deployment.yaml to

          {{- if .Values.metrics.enabled }}
          {{- with .Values.livenessProbe }}
          livenessProbe: {{- toYaml . | nindent 12 }}
          {{- end }}
          {{- with .Values.readinessProbe }}
          readinessProbe: {{- toYaml . | nindent 12 }}
          {{- end }}
          {{- end }}

@maxime1907
Copy link
Contributor Author

@jenting is it ok now ?

Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

We don't need to change the values.yaml anymore.

@maxime1907
Copy link
Contributor Author

@jenting done!

Signed-off-by: Maxime Leroy <19607336+maxime1907@users.noreply.github.com>
@maxime1907 maxime1907 changed the title [velero] do not set default probes [velero] probes only when metrics are enabled Jul 4, 2023
Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Thank you 😊

@jenting jenting requested a review from qiuming-best July 4, 2023 11:39
@jenting jenting merged commit 6172515 into vmware-tanzu:main Jul 4, 2023
10 checks passed
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.

Liveness and Readiness Probe PR is broken.
3 participants