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: clean up logic for autodelete pvcs #6042

Merged
merged 3 commits into from
Jan 4, 2024
Merged

helm: clean up logic for autodelete pvcs #6042

merged 3 commits into from
Jan 4, 2024

Conversation

captncraig
Copy link
Contributor

@captncraig captncraig commented Jan 3, 2024

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@captncraig captncraig requested a review from rfratto January 3, 2024 18:55
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I'm not sure about adding a Kube version setting since other charts don't seem to do the same thing (see comment) and it seems like we could get away with a more lax check with documentation warning users about when it's safe to enable.

Comment on lines 192 to 193
# -- The StatefulSetAutoDeletePVC feature gate went into alpha status with k8s 1.27. If you have it enabled on an earlier version, you can lower this value as far as 1.23.
minK8sVersionForAutoDelete: ">= 1.27"
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at what other Helm charts do here to remove the need for another option. It looks like the Loki chart only checks for 1.23. I see a similar check in the Consul Helm chert.

Ideally we'd avoid a new config option here, and rely on documentation on enableStatefulSetAutoDeletePVC, where we mention it's a feature gate from 1.23 to 1.26, and generally available afterwards.

@captncraig captncraig merged commit 50d1620 into main Jan 4, 2024
8 checks passed
@captncraig captncraig deleted the cmp_gates branch January 4, 2024 18:47
hainenber pushed a commit to hainenber/agent that referenced this pull request Jan 6, 2024
* helm: clean up logic for autodelete pvcs

* remove flag

* changelog
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
* helm: clean up logic for autodelete pvcs

* remove flag

* changelog
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants