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

[Jaeger] fix .allinone.replicas helm value #499

Closed

Conversation

micolun
Copy link

@micolun micolun commented Aug 22, 2023

This PR fixes the bug created after PR#494 when helm chart value .allInOne.replicas is 0 but the deployment is still set to 1.

When the below if statement is computed for replicas: 0, the result is false and as a consequence the deployment manifest doesn't contain replicas field, which in turn forces it to use the default value, which is 1.

spec:
  {{- if .Values.allInOne.replicas }}
  replicas: {{ .Values.allInOne.replicas }}
  {{- end }}
  • fixes # Rather than checking value of replicas, we can check if .allInOne.replicas key has been declared. This would create a deployment with the specified number of replicas in the values file.

Checklist

  • DCO signed
  • Commits are GPG signed
  • Chart Version bumped
  • Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
  • README.md has been updated to match version/contain new values

@pavelnikolov
Copy link
Contributor

@Mihail-blip not all commit have been signed. Could you, please, squash all the two commits into one and sign them?

@micolun micolun closed this Aug 22, 2023
@micolun micolun force-pushed the fix-allinone-replicas-helm-value branch from 7e837c9 to 059f49f Compare August 22, 2023 14:21
@micolun micolun reopened this Aug 22, 2023
@micolun
Copy link
Author

micolun commented Aug 22, 2023

One of my first contributions, I accidentally closed the PR.
Think I managed to gpg sign the commits, please review

Signed-off-by: Pavel Nikolov <pavelnikolov@users.noreply.github.com>
pavelnikolov added a commit that referenced this pull request Aug 22, 2023
@pavelnikolov
Copy link
Contributor

The PR is still not DCO signed. Thank you for reporting.
Fixed in 28ab98b

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.

2 participants