✨ Add v1beta2 structs to object status #11234
Conversation
k8s-ci-robot
added
area/api
k8s-ci-robot
added
the
size/XXL
controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go
Outdated
Show resolved
Hide resolved
controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go
Outdated
Show resolved
Hide resolved
|
@vincepri @JoelSpeed this PR add the types/conditions discussed in the proposal for phase1 + conversion tests + fixes some tests, PTAL if you have some time |
k8s-ci-robot
added
the
needs-rebase
fabriziopandini
force-pushed
the
add-v1beta2-structs
branch
from
b4cf95d to
7626629
Compare
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 8916e02b4b62912d93b3c119bd318881dbd4afe4
|
test/infrastructure/inmemory/api/v1alpha1/inmemorycluster_types.go
Outdated
Show resolved
Hide resolved
test/infrastructure/inmemory/api/v1alpha1/inmemorymachine_types.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I didn't go through every file for API changes as it appears to be that a lot of this is duplicated across many files, but I think I've left feedback on all the kinds of API questions I'd have. Please make sure to apply the feedback across the board.
For conditions, how would we feel about conditions existing on our objects that are not known to us? Since we have SSA tags, in theory multiple owners could own the conditions list, but I'm not sure how CAPI handles this today
api/v1beta1/cluster_types.go
Outdated
| // +optional | ||
| DesiredReplicas int32 `json:"desiredReplicas"` |
There was a problem hiding this comment.
Do we need to care about whether this has been observed deliberately or not?
Currently, since this is not a pointer and has no omitempty, when I marshal into json, I will get desiredReplicas: 0 even if I haven't set the field. This could then cause a discrepancy with the desired replicas from the spec, and look like the controller has observed a desire for 0 replicas.
Also, should we not expect the controller to always set this field? Should it actually be required? Same probably applies to lots of the other fields as well
There was a problem hiding this comment.
Made all pointers,
We have to do the same for all the existing counters when we move to v1beta2 (will add to the umbrella issue)
| // +optional | ||
| Replicas int32 `json:"replicas"` | ||
|
|
||
| // upToDateReplicas is the number of up-to-date control plane machines in this cluster. A machine is considered up-to-date when Machine's UpToDate condition is true. |
There was a problem hiding this comment.
When would an UpToDate condition be true, or not? How would a user know what it means for an UpToDate condition to be true or not?
There was a problem hiding this comment.
We have documentation for the UpToDate condition const.
I don't think we should duplicate the same definition of every replica counter, this could easily lead to misalignment and confusion in future.
There was a problem hiding this comment.
One thing to note is that while the documentation is up to date and correct on the conditions constants, that does not get generated into the API schema, so, is not available for users when using kubectl explain or otherwise generating docs from the schema.
I believe it would end up in swagger docs though, not sure if CAPI has generated swagger docs?
There was a problem hiding this comment.
We can think about swagger or about adding something to our book, but out of date comments in our API (or out of date documentation in general) is an issue we are struggling with since a long time, and I think we should really stop doing things that could make this problem worst.
Also, this is not really a problem specific of this PR only and it should be solved in a more structural way.
Also, being very pragmatic, we have time at the end of the cycle/after code freeze to improve documentation, but without go types we can't move on with the actual implementation.
There was a problem hiding this comment.
Will have a think about a good way to solve this in general for the project then, otherwise PR LGTM
test/infrastructure/docker/api/v1beta1/v1beta2_condition_consts.go
Outdated
Show resolved
Hide resolved
test/infrastructure/docker/api/v1beta1/v1beta2_condition_consts.go
Outdated
Show resolved
Hide resolved
test/infrastructure/docker/api/v1beta1/v1beta2_condition_consts.go
Outdated
Show resolved
Hide resolved
test/infrastructure/docker/api/v1beta1/v1beta2_condition_consts.go
Outdated
Show resolved
Hide resolved
k8s-ci-robot
removed
the
lgtm
sbueringer
added
the
tide/merge-method-squash
|
/lgtm I think it's not necesary for ClusterSpec and MachineSpec to be comparable. In my opinion such a requirement would also severly limit our options for API design |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 1c59ad8a4f6857dde29c0d2a5c0a49a89d533f3f
|
k8s-ci-robot
removed
the
lgtm
|
@fabriziopandini: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-cluster-api-e2e-main |
|
Thx! /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 2d6527d5353d9f02d297a640b6ff01268e5cdc5f
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
This PR add v1beta2 structs to object status as defined in https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md
Part of #11105
/area api