Closed leonardoce closed 2 months ago
Hi @leonardoce. Thanks for your PR.
I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
/assign
/ok-to-test
Thank you @xing-yang for reviewing this. I should have taken care of your comments.
/assign @bswartz
Actually after reviewing the change, I'm wondering why use object refs at all, if the PVC ref will always be a PVC and the snapshot ref will always be a VolumeSnapshot. Can't we do like the non-namespaced objects do and specify the object names, but omit their kinds? Namespace can be implied by the namespace of the containing object.
"Name" might be sufficient for now. I'm wondering we may want to keep the namespace just in case in the future there's namespace transfer support.
I replaced ObjectReferences with LocalObjectReferences and updated the code correspondingly. Thank you @bswartz @xing-yang
LocalObjectReference looks good to me. We should probably use the same construct for PV and VolumeSnapshotContent pairs. https://github.com/kubernetes/kubernetes/blob/v1.31.0-alpha.0/pkg/apis/core/types.go#L5460 cc @bswartz
LocalObjectReference looks good to me. We should probably use the same construct for PV and VolumeSnapshotContent pairs.
Nice. I changed the code for this and updated the example in the PR description.
LocalObjectReference looks good to me. We should probably use the same construct for PV and VolumeSnapshotContent pairs. https://github.com/kubernetes/kubernetes/blob/v1.31.0-alpha.0/pkg/apis/core/types.go#L5460 cc @bswartz
Non-namespaced objects definitely should not be using namespaces. Assuming the kind is constant, why not just use an object name?
LocalObjectReference looks good to me. We should probably use the same construct for PV and VolumeSnapshotContent pairs. https://github.com/kubernetes/kubernetes/blob/v1.31.0-alpha.0/pkg/apis/core/types.go#L5460 cc @bswartz
Non-namespaced objects definitely should not be using namespaces. Assuming the kind is constant, why not just use an object name?
@msau42 From API design point of view, are there pros and cons of using LocalObjectReference vs just a string here?
/lgtm /approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: bswartz, leonardoce, xing-yang
The full list of commands accepted by this bot can be found here.
The pull request process is described here
What type of PR is this?
What this PR does / why we need it:
This PR implements an API change proposed by @xing-yang (see https://docs.google.com/document/d/1NdNwFD5Z64K2heQLYOnojt6Ogulg750BuYIJ6W9cEiM/edit?usp=sharing and the relative conversation in #969).
To ease the review process, this PR has been split into two commits:
The information we have in
VolumeGroupSnapshot.status.volumeSnapshotRefList
has been moved intoVolumeGroupSnapshot.status.pvcVolumeSnapshotRefList.volumeSnapshotRef
.The information we have in
VolumeGroupSnapshotContent.status.volumeSnapshotContentRefList
has been moved intoVolumeGroupSnapshotContent.status.volumeSnapshotContentRefList.volumeSnapshotContentName
.The code to set the new fields has already been implemented, and I'm going to open a new PR based on this one. I hope that it will be helpful for the reviewer.
If you prefer a single PR on multiple commits, just tell me.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This is my first API-change PR. I followed the process described in
client/hack/README.md
as carefully as possible, but I encourage you to use special care when reviewing this.Testing
After this patch, a
VolumeGroupSnapshot
will be defined as:And the corresponding VolumeGroupSnapshotContent would be like:
Does this PR introduce a user-facing change?: