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

Use CSI spec v1.5.0 #312

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Conversation

chrishenzie
Copy link
Contributor

@chrishenzie chrishenzie commented Jun 14, 2021

What type of PR is this?
/kind feature

What this PR does / why we need it:
Updates the CSI spec version to v1.5.0, which includes support for SINGLE_NODE_SINGLE_WRITER and SINGLE_NODE_MULTI_WRITER access modes.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Updates container-storage-interface dependency to v1.5.0

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 14, 2021
@k8s-ci-robot k8s-ci-robot requested review from gnufied and jingxu97 June 14, 2021 17:33
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 14, 2021
@chrishenzie
Copy link
Contributor Author

/assign @msau42
/assign @bswartz

@xing-yang
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrishenzie, xing-yang

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 Jun 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9193818 into kubernetes-csi:master Jun 14, 2021
@chrishenzie
Copy link
Contributor Author

/remove-kind feature
/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jun 14, 2021
@chrishenzie chrishenzie deleted the csi-spec-v1.5.0 branch June 14, 2021 21:04
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 4, 2021
@chrishenzie
Copy link
Contributor Author

Relabeling, categorizing this as a feature for this sidecar because it enables new functionality.

/remove-kind cleanup
/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 10, 2021
@pohly
Copy link
Contributor

pohly commented Aug 10, 2021

Relabeling, categorizing this as a feature for this sidecar because it enables new functionality.

Which functionality is that?

I don't see any code changes that depend on the new CSI version.

@chrishenzie
Copy link
Contributor Author

chrishenzie commented Aug 10, 2021

@pohly
Copy link
Contributor

pohly commented Aug 10, 2021

But "enabling the use" is not the same as "using". Unless I am missing something, this PR causes no functional change in external-attacher. That will change in a future PR where the new definitions from CSI 1.5 are actually used.

@chrishenzie
Copy link
Contributor Author

chrishenzie commented Aug 10, 2021

I think because I'm retroactively fixing the label on this PR it's easy to get the tenses confused. This new access mode code is now in use, but at the time of this PR it was not.

I have been labeling PRs that update dependencies across sidecars as feature if that code will be exercised as part of a future feature, that way readers can see both the dependency bump and feature addition (the mapping logic) in the same section of the changelog since the feature is dependent on the dependency bump.

If you don't think this is an appropriate use of this label I can change this going forward. What label would you suggest instead? cleanup? Alternatively I can package dependency bumps with the specific feature, however in the past I've received feedback to handle these separately. Or I can not tag these at all.

@pohly
Copy link
Contributor

pohly commented Aug 11, 2021

I think because I'm retroactively fixing the label on this PR it's easy to get the tenses confused. This new access mode code is now in use, but at the time of this PR it was not.

Then the PR which put that new access mode into use should be marked as "feature", but not this PR here. Look at it from the perspective of a user who sees a new feature listed in the changelog with a link to this PR here: there's simple nothing here that explains what that new feature is.

If you don't think this is an appropriate use of this label I can change this going forward. What label would you suggest instead? cleanup?

I would use cleanup and only use feature or bug if that update is known to enable new features or fix some bug, in which case the change note should mention those and not just "update foo to x.y.z".

Alternatively I can package dependency bumps with the specific feature, however in the past I've received feedback to handle these separately.

A separate PR is useful because typically we want to merge dependency updates regardless of whether the new feature gets merged.

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants