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

Support existing persistent volume #105

Merged
merged 20 commits into from
Mar 27, 2023

Conversation

yekibud
Copy link
Contributor

@yekibud yekibud commented Dec 22, 2022

What this PR does / why we need it:

Statefulset volumeClaimTemplates create new volumes and PersistentVolumeClaims which cannot be mutated. This PR adds the ability to declare a persistentVolume as existing and provide the required PV and PVC data for binding a volume to the statefulset. This is especially useful for restoring backup snapshots during disaster recovery efforts, but could also be used for initial data migrations or other scenarios where users would like to deploy this chart with existing CouchDB data.

Which issue this PR fixes

No related issue tracker.

Special notes for your reviewer:

  • I noticed my previous merged PR's version bump isn't being listed by helm search repo for the https://apache.github.io/couchdb-helm repo, but it does appear to be listed on the releases page for this project on github.com. Not sure if there's something I'm doing wrong or something needs to be fixed.

  • I'm still getting the error I got from my previous PR running make test.

     Error: Error linting and installing charts: Error identifying merge base: Error running process: exit status 128
    

    This seems like an unrelated/existing issue, but please let me know if I'm doing something wrong here, as well.

Checklist

  • Chart Version bumped
  • e2e tests pass
  • Variables are documented in the README.md

Copy link
Member

@willholley willholley left a comment

Choose a reason for hiding this comment

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

thanks for the PR @yekibud. The changes look mostly good to me - just a couple of queries / suggestions.

On the issue of why releases aren't showing up in the helm repo index, I believe this is due to the lingering master branch defining the github pages location. We moved the default branch to main some time ago, so I've removed the master branch and hopefully this will fix the issue, though it may take a while to propagate.

docs/index.yaml Outdated
Comment on lines 1 to 6
apiVersion: v1
entries:
couchdb:
- apiVersion: v1
appVersion: 3.2.1
created: "2022-12-20T17:53:44.680236109-08:00"
description: A database featuring seamless multi-master sync, that scales from
big data to mobile, with an intuitive HTTP/JSON API and designed for reliability.
digest: 5a66b7584d72bb2ac9c30e65d751ee374adbea2fe73dac20a058fb20bf1476e2
home: https://couchdb.apache.org/
icon: http://couchdb.apache.org/CouchDB-visual-identity/logo/CouchDB-couch-symbol.svg
keywords:
- couchdb
- database
- nosql
maintainers:
- email: [email protected]
name: kocolosk
- email: [email protected]
name: willholley
name: couchdb
sources:
- https://github.com/apache/couchdb-helm
- https://github.com/apache/couchdb-docker
urls:
- couchdb-3.6.4.tgz
version: 3.6.4
- apiVersion: v1
appVersion: 3.2.1
created: "2022-11-16T10:04:19.187911144-08:00"
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be manually updated - it's now generated automatically on release. We need to update the contributing guide / remove the docs folder...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

couchdb/Chart.yaml Outdated Show resolved Hide resolved
couchdb/templates/persistentvolumeclaim.yaml Outdated Show resolved Hide resolved
@yekibud yekibud force-pushed the support-existingPersistentVolume branch from b6a0735 to 81f4ef7 Compare January 15, 2023 21:36
@yekibud yekibud force-pushed the support-existingPersistentVolume branch from 81f4ef7 to 92bd769 Compare January 15, 2023 23:18
couchdb/README.md Outdated Show resolved Hide resolved
@yekibud
Copy link
Contributor Author

yekibud commented Feb 23, 2023

thanks for the PR @yekibud. The changes look mostly good to me - just a couple of queries / suggestions.

On the issue of why releases aren't showing up in the helm repo index, I believe this is due to the lingering master branch defining the github pages location. We moved the default branch to main some time ago, so I've removed the master branch and hopefully this will fix the issue, though it may take a while to propagate.

Hi @willholley . I made the changes you suggested. Please let me know if you have any further feedback or if it's possible to approve/merge. Thanks.

@willholley
Copy link
Member

thanks @yekibud - no need to update the docs / tarball as these are generated automatically

@yekibud yekibud force-pushed the support-existingPersistentVolume branch from 8dd5b16 to 30e6897 Compare March 22, 2023 00:00
@yekibud
Copy link
Contributor Author

yekibud commented Mar 22, 2023

thanks @yekibud - no need to update the docs / tarball as these are generated automatically

@willholley sorry - I've been using this branch in production and needed it for updating charts. I guess I'd have to point to my own pages/repo in order to do that with the new pattern? In any case, I just kept the docs folder in a fork of my PR branch and removed it from this PR.

Also note that I've added a few more commits to support an array of persistentVolume.existingClaims for cluster scenarios. Let me know what you think.

@willholley
Copy link
Member

thanks @yekibud - lgtm. It just needs a version bump/rebase and then I can merge.

@yekibud
Copy link
Contributor Author

yekibud commented Mar 27, 2023

thanks @yekibud - lgtm. It just needs a version bump/rebase and then I can merge.

@willholley Great! Bumped and resolved conflicts.

@willholley willholley merged commit 4f31b20 into apache:main Mar 27, 2023
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