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

Fix tolerations iteration issue for pg-db cluster template #369

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kv-dhyey-moliya
Copy link

Currently the cluster template incorrectly takes only single value (object) for tolerations instead of accepting an array of objects.

There is already an issue for this : #333
This PR fixes that issue by updating the template code that Instead of replacing with single value, replaces array of tolerations for pg-db cluster template.

Instead of replacing with single value, read array of tolerations for pg-db cluster template.
Copy link
Member

@tplavcic tplavcic 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 for the contribution! Please bump the chart version.

@kv-dhyey-moliya
Copy link
Author

kv-dhyey-moliya commented Sep 9, 2024

This fix might break the existing setup of users as the workaround was to put single map under tolerations, and it would work. But the fix makes the chart use range.

What should be ideal version bump in this case? Do the charts use semantic versioning? Then it might require to increase major version due to incompatibility with older versions. Changing major version seems like an overkill.

Or else we can resort to conditional logic if the user passes array, use the array, else use the direct map values as shown below.

tolerations:
{{- $tolerations := .Values.mapItems }}
{{- if eq (kindOf $tolerations) "map" }}
  - key1: {{ $tolerations.key1 }}
    key2: {{ $tolerations.key2 }}
{{- else }}
{{- range .Values.mapItems }}
  - key1: {{ .key1 }}
    key2: {{ .key2 }}
{{- end }}
{{- end }}

Please suggest.

@tplavcic
Copy link
Member

tplavcic commented Sep 9, 2024

@kv-dhyey-moliya Usually we bump the "patch" version, but if it's something that breaks compatibility then we merge it on the next operator release when the "minor" version is bumped. At the current moment we do not bump "major" version at all. Basically our helm chart versions follow the operator version except that we can bump the "patch" version if there's a small fix just for the helm chart.
So I propose for your change that we merge it on the next PG operator release.

@kv-dhyey-moliya
Copy link
Author

kv-dhyey-moliya commented Sep 9, 2024

@tplavcic thank you for the update. Then, should I update the chart version to a minor version for this PR? Or leave it as it is for now?

Additionally, As per my other suggestion, we can make it backwards compatible if we do conditional check, then we can update with a patch version, but again that will add unnecessary conditional code into the codebase. Ping me if you feel it's ok for now to add the conditional code, then I'll update it accordingly with a patch version.

@tplavcic
Copy link
Member

tplavcic commented Sep 9, 2024

I think just leave it for now, when we will work on the next release we will merge your fix to the release branch and make any additional changes needed.

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

Successfully merging this pull request may close these issues.

3 participants