-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 ClusterClass: add condition for references with outdated apiVersions #7259
🌱 ClusterClass: add condition for references with outdated apiVersions #7259
Conversation
I chose to add a separate condition for validation. Failure state: status:
conditions:
- lastTransitionTime: "2022-09-21T13:56:46Z"
message: infrastructure.cluster.x-k8s.io/v1alpha4, Kind=DockerClusterTemplate
should be infrastructure.cluster.x-k8s.io/v1beta1, Kind=DockerClusterTemplate,
infrastructure.cluster.x-k8s.io/v1alpha4, Kind=DockerMachineTemplate should
be infrastructure.cluster.x-k8s.io/v1beta1, Kind=DockerMachineTemplate, infrastructure.cluster.x-k8s.io/v1alpha4,
Kind=DockerMachineTemplate should be infrastructure.cluster.x-k8s.io/v1beta1,
Kind=DockerMachineTemplate
reason: OutdatedAPIVersionsInReferences
severity: Warning
status: "False"
type: ClusterClassValidationSucceeded Success state: status:
conditions:
- lastTransitionTime: "2022-09-21T13:56:46Z"
status: "true"
type: ClusterClassValidationSucceeded There are a few alternatives:
Opinions? @fabriziopandini @vincepri @fabriziopandini @killianmuldoon P.S. Some prior art: CRDs have some validation conditions: https://github.com/kubernetes/kubernetes/blob/e3f2d237fdb225e4835cd0985104bf01cabf6cd3/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types.go#L282-L301 (but they are afaik not all compliant with our conventions, e.g. |
8565d9a
to
7157169
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass
WRt to question above:
- I'm +1 to use Using a more specific condition like APIVersionsUpToDate, or simply ReferenceUpToDate (I prefer the second one)
- I'm -1 to use Ready condition because it is reserved for being used as a summary of all conditions; however, I will not add ready at the current stage (it is optional in the policy)
Thx good point, adjusted accordingly |
087208a
to
6734e3c
Compare
6734e3c
to
48a6931
Compare
Should be ready for another round of review now |
@fabriziopandini When you have some time, would be good to get your input on condition / reason names |
48a6931
to
d284deb
Compare
@fabriziopandini @vincepri constants adjusted, PTAL |
/test pull-cluster-api-e2e-full-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/assign @vincepri |
clusterv1.ClusterClassRefVersionsUpToDateCondition, | ||
clusterv1.ClusterClassOutdatedRefVersionsReason, | ||
clusterv1.ConditionSeverityWarning, | ||
strings.Join(msg, ", "), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using a different seperator like ;
(or something else). GVK when converted to string already has ,
in it. The output string could be confusing.
Also, the message only uses the GVK of the ref which does not include the name. If a few DockerMachineTemplate refs are outdated that tt could be confusing if the mesage just repeats that a DockerMachineTemplate ref is outdated without pointing to the correct ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to use a quoted string. Kept using ,
as separater seems slight more grammatically correct. The condition now looks like this:
status:
conditions:
- lastTransitionTime: "2022-10-07T10:04:22Z"
message: Ref "infrastructure.cluster.x-k8s.io/v1alpha4, Kind=DockerClusterTemplate
default/quick-start-my-cluster" should be "infrastructure.cluster.x-k8s.io/v1beta1,
Kind=DockerClusterTemplate default/quick-start-my-cluster", Ref "infrastructure.cluster.x-k8s.io/v1alpha4,
Kind=DockerMachineTemplate default/quick-start-control-plane" should be "infrastructure.cluster.x-k8s.io/v1beta1,
Kind=DockerMachineTemplate default/quick-start-control-plane", Ref "infrastructure.cluster.x-k8s.io/v1alpha4,
Kind=DockerMachineTemplate default/quick-start-docker-worker-machinetemplate"
should be "infrastructure.cluster.x-k8s.io/v1beta1, Kind=DockerMachineTemplate
default/quick-start-docker-worker-machinetemplate"
reason: OutdatedRefVersions
severity: Warning
status: "False"
type: RefVersionsUpToDate
It's a bit verbose, but also very explicit which is probably a good thing
Signed-off-by: Stefan Büringer [email protected]
d284deb
to
c14e9fc
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #7212