-
Notifications
You must be signed in to change notification settings - Fork 808
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
feat: Add option to provision StorageClasses #697
Conversation
Welcome @gazal-k! |
Hi @gazal-k. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Similar to kubernetes-sigs/aws-efs-csi-driver#298 |
Pull Request Test Coverage Report for Build 1547
💛 - Coveralls |
@@ -98,3 +98,7 @@ serviceAccount: | |||
create: true | |||
name: ebs-snapshot-controller | |||
annotations: {} | |||
|
|||
storageClasses: [] |
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.
It'd be really nice if we include the options here similar to the efs PR.
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.
You mean like an example? I do have an example storage class entry in comments just like the efs PR. Just that ebs doesn't really need much explicit config. I can provide a more detailed entry I suppose.
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.
I've added reference parameters
now. Should we also mention other options like volumeBindingMode
& reclaimPolicy
?
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.
Sorry for the delay here.
Yeah that would be great. Ideally we should note if they're optional and default value if they have one.
/ok-to-test |
0788526
to
c9c603d
Compare
c9c603d
to
3e76d8e
Compare
3e76d8e
to
e5ca54e
Compare
/lgtm thanks! |
/retest |
e5ca54e
to
8288f88
Compare
/retest |
I thought rebasing on master would fix the tests 🤦. So, please approve again @ayberk |
Sorry for the back and forth, but we also need to update the chart version. Looks like I missed it when I was reviewing it. I'd wait for the 0.9.0 PR to be merged and than update the chart version to 0.9.1. |
8288f88
to
1cf1c14
Compare
No worries, that's what PRs are all about anyway 🙂. I've bumped it to 0.8.2, so this can go in before 0.9.0 if required. Or else, I'll just rebase & bump the version later |
Most application charts don't usually create `StorageClass`es. They create PVCs.
1cf1c14
to
42dfe27
Compare
/lgtm Thank you!! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ayberk, gazal-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 |
not sure what happened here. the chart release seems to have failed; https://github.com/kubernetes-sigs/aws-ebs-csi-driver/runs/1835265236. It's trying to publish |
Yeah interesting it's still 0.9.0 on master but in the diff I see it's 0.9.1. |
Ah we updated the appVersion not the chart version, that's why. |
oops, let me fix that |
amending issue introduced by kubernetes-sigs#697
@anilkumarpasupuleti @DeVysh @pradeesh-varghese @mfaridk you can start using 0.9.1 of |
Is this a bug fix or adding new feature?
New feature in the helm chart
What is this PR about? / Why do we need it?
Most application charts don't usually create
StorageClass
es. They create PVCs.What testing is done?
Installed the chart with the following custom values:
Then tried creating a PVC and a pod that used it:
Verified the following:
PV was provisioned.
New EBS gp3 volume was created as expected.