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

Adding content for allowVolumeExpansion field in StorageClass #15604

Merged
merged 1 commit into from Sep 3, 2019
Merged

Adding content for allowVolumeExpansion field in StorageClass #15604

merged 1 commit into from Sep 3, 2019

Conversation

savitharaghunathan
Copy link
Member

@savitharaghunathan savitharaghunathan commented Jul 31, 2019

This PR is in reference to issue - #15587. I have added allowVolumeExpansion in the example and have written a paragraph to explain what it is. I have added supported types and a note telling that it cannot be used to shrink volumes. Please review and help me correct if any of the content is not up-to date and if anything else is missing. Thanks!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jul 31, 2019
@netlify
Copy link

netlify bot commented Jul 31, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 94c92c9

https://deploy-preview-15604--kubernetes-io-master-staging.netlify.com

@savitharaghunathan
Copy link
Member Author

/assign @Rajakavitha1

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 31, 2019
@poothia
Copy link
Contributor

poothia commented Aug 1, 2019

thanks for fixing this.

lgtm

note: please squash the commits into one.

@savitharaghunathan
Copy link
Member Author

savitharaghunathan commented Aug 1, 2019

@poothia I have squashed all the commits related to allowVolumeExpansion into one. Should I squash all the commits that are showing up in this PR into one?

Apologies for a lot of commit and amends, this is my second PR and I am still learning.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 1, 2019
@thecrudge
Copy link
Contributor

/lgtm

thanks @snathan13

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019
@msau42
Copy link
Member

msau42 commented Aug 1, 2019

/assign @gnufied
To check the list of supported drivers

@thecrudge
Copy link
Contributor

good catch, thanks @msau42

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019
content/en/docs/concepts/storage/storage-classes.md Outdated Show resolved Hide resolved
content/en/docs/concepts/storage/storage-classes.md Outdated Show resolved Hide resolved
The following types of volumes support volume expansion, when the underlying
Storage Class has the field `allowVolumeExpansion` set to true.

* gcePersistentDisk
Copy link
Member

Choose a reason for hiding this comment

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

@gnufied, can you verify which version all of these plugins were implemented in?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @gnufied for confirmation :)

Copy link
Member

Choose a reason for hiding this comment

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

here is support/version matrix of various volume types look like:

  • EBS (1.11)
  • Cinder (1.11)
  • Glusterfs (1.11)
  • GCE PD (1.11)
  • Portworx (1.11)
  • Azure File (1.11)
  • RBD (1.11)
  • Azure Disk(1.11)
  • Flex Volume (1.13)
  • CSI (1.14, 1.15 - Alpha, 1.16 beta)

Copy link
Member

Choose a reason for hiding this comment

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

It might be worth updating this document and explicitly specify which version of k8s the volume plugin type supported expansion.

@poothia
Copy link
Contributor

poothia commented Aug 5, 2019

@poothia I have squashed all the commits related to allowVolumeExpansion into one. Should I squash all the commits that are showing up in this PR into one?

@snathan13, Sorry for the late reply. Yes you should squash all the commits for this change and mention all the changes you made in one commit. As it will help in keeping git history concise and clean. We should make multiple commits only when there are a lot of changes in multiple files to make reviewing easy.

Ref:https://softwareengineering.stackexchange.com/questions/263164/why-squash-git-commits-for-pull-requests

Apologies for a lot of commit and amends, this is my second PR and I am still learning.

Please don't apologise, Even I am still learning!
Happy Contributing!! :)

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@snathan13 Thanks for this PR! ✨ You're off to a good start. Be sure to respond to reviewer feedback and squash your commits. 👍

content/en/docs/concepts/storage/storage-classes.md Outdated Show resolved Hide resolved
@msau42
Copy link
Member

msau42 commented Aug 16, 2019

This lgtm but would like @gnufied to confirm the releases where each of the plugins were implemented.

@dims
Copy link
Member

dims commented Sep 3, 2019

it's been 19+ days. let's merge and iterate

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2019
@savitharaghunathan
Copy link
Member Author

/assign @Rajakavitha1

@jimangel
Copy link
Member

jimangel commented Sep 3, 2019

Agree with @dims, let's merge this and iterate...
/approve
/cc @msau42 @gnufied

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jimangel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2019
@k8s-ci-robot k8s-ci-robot merged commit dc5179c into kubernetes:master Sep 3, 2019
@savitharaghunathan
Copy link
Member Author

I will create another PR with changes that @gnufied mentioned in the comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet