KEP-4192: Remove obsolete fields from the API#5597
KEP-4192: Remove obsolete fields from the API#5597k8s-ci-robot merged 4 commits intokubernetes:masterfrom
Conversation
stlaz
commented
Oct 1, 2025
stlaz
commented
- One-line PR description: The API described in the KEP contained several fields that were unused by the implementation. These should not appear in the Beta version of the API.
- Issue link: Move Storage Version Migrator in-tree #4192
- Other comments: There are some minor changes to the KEP structure. You may want to go commit-by-commit to see the actual changes to the API, which are isolated in a separate commit.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
kind/kep
|
/cc @michaelasp |
|
Overall makes sense to me, I agree we don't need the last migrated resource at all since we can just use the RV. |
k8s-ci-robot
added
the
lgtm
Signed-off-by: Stanislav Láznička <[email protected]>
k8s-ci-robot
removed
the
lgtm
| // Standard object metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` |
There was a problem hiding this comment.
Uh why did we drop the proto tags? Since this will be a built-in API, it will support proto...
There was a problem hiding this comment.
They are just inconvenient, I don't feel like they belong to the design. Since it's a built-in API, they should be implied - aren't they also autogenerated?
- move to metav1.Conditions - remove the unused ContinueToken field - rename LastMigratedResourceNameHash to LastMigratedResource Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
|
The result is still serialization compatible for the client requests making it easy to switch, so this lgtm. /lgtm holding in case @enj wants another look and for a squash |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
lgtm
enj
added
the
tide/merge-method-squash
|
/lgtm (labeled so the bot will squash since Standa is OOO) |
k8s-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj, stlaz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| - `.spec.continueToken` - unused in the in-tree implementation | ||
| - `.status.lastMigratedResourceNameHash` - unused in the in-tree implementation | ||
| - **API modifications** | ||
| - `.spec.groupVersionResource` is replaced by `.spec.groupResource` and changes type |
There was a problem hiding this comment.
Looking through this again to get kubernetes/kubernetes#134480 into reviewable, I don't think this is possible unless I'm misunderstanding. We need the version to know what resource to migrate, and it is used here https://github.com/kubernetes/kubernetes/pull/134480/files#diff-71ee1fc95d120f0800d32369be68174335815068609f09e5feafc0fb24e68260L42
Without it, we can't tell which version of a resource to migrate, I think we need to have groupVersionResource. cc @enj
6 participants
