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

Add ability to configure StatefulSet PVC retention policy #517

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

sam-mcbr
Copy link
Contributor

What this PR does:
Adds values allowing users to configure the persistentVolumeClaimRetentionPolicy on StatefulSets deployed by the chart.

Which issue(s) this PR fixes:
Fixes #516

Checklist

  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX], [DEPENDENCY]

Comment on lines 24 to 29
{{- if and (semverCompare ">=1.23-0" .Capabilities.KubeVersion.Version) (.Values.alertmanager.persistentVolume.enableRetentionPolicy) }}
persistentVolumeClaimRetentionPolicy:
whenDeleted: {{ .Values.alertmanager.persistentVolume.whenDeleted }}
whenScaled: {{ .Values.alertmanager.persistentVolume.whenScaled }}
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we get any benefit from having the bool enableRetentionPolicy to enable this? To me it seems simpler to just use the values if they are defined, but I get that this is a bit of a style thing.

It could be like this:

Suggested change
{{- if and (semverCompare ">=1.23-0" .Capabilities.KubeVersion.Version) (.Values.alertmanager.persistentVolume.enableRetentionPolicy) }}
persistentVolumeClaimRetentionPolicy:
whenDeleted: {{ .Values.alertmanager.persistentVolume.whenDeleted }}
whenScaled: {{ .Values.alertmanager.persistentVolume.whenScaled }}
{{- end }}
{{- if (semverCompare ">=1.23-0" .Capabilities.KubeVersion.Version) }}
{{- with .Values.alertmanager.persistentVolume.retentionPolicy }}
persistentVolumeClaimRetentionPolicy:
{{- toYaml . | nindent 4}}
{{- end }}
{{- end }}

The values.yaml would be like:

alertmanager:
  persistentVolume:
    retentionPolicy: {}
    # whenDeleted: Retain
    # whenScaled: Retain

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 fine with both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kd7lxl I like that suggestion, I'll go ahead and make the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, made the changes

@nschad nschad merged commit d8768ba into cortexproject:master Jan 16, 2025
2 checks passed
@sam-mcbr sam-mcbr deleted the pvcretpol branch January 16, 2025 16:54
@nschad nschad mentioned this pull request Jan 17, 2025
1 task
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.

Allow for configuration of the persistentVolumeClaimRetentionPolicy field on deployed StatefulSets
3 participants