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.

Sign up for GitHub

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

Jump to bottom

Add ClassName attributes to PV and PVC #40013

Merged
merged 2 commits into from
Feb 23, 2017
Merged

Add ClassName attributes to PV and PVC #40013

merged 2 commits into from
Feb 23, 2017

Conversation

jsafrane
Copy link
Member

This just adds new attributes to PV/PVC. Real code that uses the attributes instead of beta annotations will follow when we agree on the attribute names / style.

@jsafrane jsafrane added release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Jan 17, 2017
@jsafrane jsafrane added this to the v1.6 milestone Jan 17, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 17, 2017
@jsafrane jsafrane force-pushed the storage-ga-class-attributes branch 2 times, most recently from 3ef7876 to 8c3ae5f Compare January 17, 2017 14:25
@jsafrane jsafrane mentioned this pull request Jan 17, 2017
Closed
23 tasks
@jsafrane
Copy link
Member Author

Not sure what this bazel error means:

FAIL: //pkg/controller/node:go_default_test (see /root/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/execroot/kubernetes/bazel-out/local-fastbuild/testlogs/pkg/controller/node/go_default_test/test.log)

@jsafrane
Copy link
Member Author

Anyway, it's ready for review, cc: @kubernetes/sig-storage-misc.

bgrant0607
bgrant0607 reviewed Jan 17, 2017
View reviewed changes
pkg/api/v1/types.go Outdated
// Name of StorageClass where this persistent volume belongs. Empty value
// means that this volume does not belong to any StorageClass.
// +optional
ClassName string `json:"className,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Should be storageClassName per conventions (assuming the resource kind is StorageClass):

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#naming-conventions

cc @thockin

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion here, I just follow pvc.Spec.VolumeName pattern (and not Spec.PersistentVolumeName)

@bgrant0607 bgrant0607 assigned thockin and unassigned bgrant0607 Jan 17, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2017
msau42
msau42 reviewed Jan 18, 2017
View reviewed changes
pkg/api/v1/types.go Outdated
// Name of the StorageClass where the desired volume belongs to.
// More info: http://kubernetes.io/docs/user-guide/persistent-volumes#class-1
// +optional
ClassName *string `json:"className,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Why is this field a ptr and the previous one not?

Copy link
Member Author

@jsafrane jsafrane Jan 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Because when PV.Spec.Class is missing it will default to empty string and hence it will be always filled. When PVC.Spec.Class is missing it means something else than empty string and we need to distinguish between these two cases.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2017
@jsafrane jsafrane force-pushed the storage-ga-class-attributes branch 2 times, most recently from 8ee3db5 to d6c26f7 Compare January 25, 2017 09:21
@jsafrane
Copy link
Member Author

sorry about the Jenkins noise, updating API is tricky and fragile :-(

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2017
@kerneltime kerneltime mentioned this pull request Feb 16, 2017
Closed
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 17, 2017
@jsafrane jsafrane mentioned this pull request Feb 17, 2017
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2017
@jsafrane
Copy link
Member Author

@k8s-bot non-cri e2e test this
@k8s-bot gci gce e2e test this
@k8s-bot gce etcd3 e2e test this

@jsafrane
Copy link
Member Author

@k8s-bot gce etcd3 e2e test this

@thockin
Copy link
Member

thockin commented Feb 22, 2017

The only facet of this I am wrestling with is "storageClassName" vs "storageClass". Name is most precise and I guess precedented, it just feels verbose.

@thockin
Copy link
Member

thockin commented Feb 22, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
@thockin
Copy link
Member

thockin commented Feb 22, 2017

/approve

@thockin thockin added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2017
@thockin
Copy link
Member

thockin commented Feb 22, 2017

shouldn't this get a release note?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: jsafrane, thockin

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 23, 2017
@jsafrane jsafrane added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2017
@jsafrane
Copy link
Member Author

restoring lgtm after rebase

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit bb7cc74 into kubernetes:master Feb 23, 2017
@jsafrane jsafrane mentioned this pull request Feb 23, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgrant0607 bgrant0607 bgrant0607 left review comments

@msau42 msau42 msau42 left review comments

@kerneltime kerneltime kerneltime approved these changes

Assignees

@thockin thockin

@bgrant0607 bgrant0607

@msau42 msau42

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jsafrane @k8s-reviewable @kerneltime @k8s-ci-robot @msau42 @thockin @k8s-github-robot @bgrant0607 @brendandburns