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

Allow to use selectors with volumeClaimTemplates #286

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

claudio-vellage
Copy link
Contributor

@claudio-vellage claudio-vellage commented Aug 30, 2022

Motivation

Currently it's not possible to use selectors with volumeClaimTemplates which makes it hard/impossible to bind statically provisioned PVs.

Modifications

Added (optional) selectors to volumeClaimTemplates and documented in values file.

Verifying this change

  • Make sure that the change passes the CI checks.

@claudio-vellage
Copy link
Contributor Author

@lhotari is there anything left to do for me? Can this be assigned to someone for review? It's a rather small change which doesn't affect existing deployments, only adds additional functionality. It allows that selectors can be added to volumeClaimTemplates, which will allow to easily create PVCs which bind to existing PVs using selectors. This allows to reuse the same PVs even if all pulsar resources are deleted. This will be more robust for production deployments and avoid having to restore the content of the PVs manually in case of disaster recovery.

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

@claudio-vellage - thank you for your contribution. Sorry for the delayed review here. Once we get the values.yaml cleaned up, I plan to merge this. Thanks.

charts/pulsar/values.yaml Outdated Show resolved Hide resolved
charts/pulsar/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@michaeljmarshall
Copy link
Member

@claudio-vellage - are you able to resolve the merge conflicts? Thanks!

@michaeljmarshall
Copy link
Member

Closing and reopening to get the latest test changes. (I am pretty sure we don't need to merge master into this branch to get the fix I just merge here #315, but that might be necessary.)

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