-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 the customization of the storage class for the PVC of pipelines #1148
Conversation
This commit enables to use storage classes other than the cluster-wide default storage class for the PVC of pipelines. The customization can be defined in the config map config-artifact-pvc.yaml - similar to how the size of the PVC can already be defined. By default, it will fallback to the cluster-wide default storage class. fixes tektoncd#1101
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this.
This functionality might go away, but unless we manage to remove before next release, I think it's worth getting this in, as it will help folks running Tekton on clouds where a non-default storageClass is important.
pvcSpec := GetPVCSpec(pr, pvcSize) | ||
var pvcStorageClassName *string | ||
if pvcStorageClassNameStr == "" { | ||
pvcStorageClassName = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this goes to nil
if it's not defined, it will work fine on existing (or new) clusters that do not include the storageClass in the config map.
/ok-to-test |
The following is the coverage report on pkg/.
|
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only feedback is that I'd like the install docs to include a bit more info on how to configure this - I don't know much about storage classes myself so I'd be a bit lost in knowing what kind of values I can use! I don't want to block the PR on that tho so @Fabian-K if you are able to add that to the install docs in a follow up PR that would be awesome 🙏 but no worries if not! Thanks for updating the install docs anyway :D
Like @afrittoli said this functionality will likely be going away - but we also need to make sure that #1087 (which will replace this in the long run!) uses this same configuration!
/approve
/meow space
@@ -109,6 +109,7 @@ The PVC option can be configured using a ConfigMap with the name | |||
`config-artifact-pvc` and the following attributes: | |||
|
|||
- size: the size of the volume (5Gi by default) | |||
- storageClassName: the storage class of the volume (default storage class by default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe link to some docs here on what kind of values can be used?
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish, Fabian-K The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…of pipelines This commit adds a link to the k8s documentation for storage classes as a follow up for tektoncd#1148.
…of pipelines This commit adds a link to the k8s documentation for storage classes as a follow up for #1148.
Changes
This commit enables to use storage classes other than the cluster-wide default storage class for the PVC of pipelines. The customization can be defined in the config map config-artifact-pvc.yaml - similar to how the size of the PVC can already be defined. By default, it will fallback to the cluster-wide default storage class. fixes #1101
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes