feat: UIPlugin API - new flag for the cluster-health-analyzer #971
Conversation
tremes
force-pushed
the
uiplugin-api-update-2
branch
from
e8b72c3 to
6e2410c
Compare
pkg/apis/uiplugin/v1alpha1/types.go
Outdated
| // Incidents feature flag enablement | ||
| // | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:deprecatedversion:warning="incidents is deprecated, use clusterHealthAnalyzer instead" |
There was a problem hiding this comment.
I'm not sure the deprecatedversion will apply here as this is not a CRD version deprecation but a field in the CRD. We should use a +deprecated annotation and a CEL validation message:
// +kubebuilder:validation:XValidation:rule="!has(self.incidents)",message="field 'incidents' is deprecated and will be removed in v1.5.0; please migrate to 'clusterHealthAnalyzer'",reason="Warning"
There was a problem hiding this comment.
I am affraid that all the reasons (see https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-reason) block the request. So introducing this CEL rule would probably break COO upgrade (if incidents are enabled) and I think we would like to avoid it.
There was a problem hiding this comment.
Ah ok, I thought we could add a CEL rule in a non-blocking way. In that case the only remaining part is to set the +deprecation annotation.
|
@tremes are we adding changes also in the logic to reconcile the incidents features based on the new and old flag? |
|
@jgbernalp yes. There's definitely a plan to update the reconciliation & resources, but it's better to update the API first (since we have two Go modules) and then update the reconcile logic IMO. |
tremes
force-pushed
the
uiplugin-api-update-2
branch
from
6e2410c to
30e07a6
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, tremes 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 |
|
@tremes: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| // ClusterHealthAnalyzer feature flag enablement | ||
| // | ||
| // +kubebuilder:validation:Optional | ||
| ClusterHealthAnalyzer ClusterHealthAnalyzerReference `json:"clusterHealthAnalyzer,omitempty"` |
There was a problem hiding this comment.
it should be a pointer if optional
There was a problem hiding this comment.
yes this was missed and fixed in the consequent PR.
| // | ||
| // +kubebuilder:validation:Optional | ||
| // Deprecated: Use clusterHealthAnalyzer instead | ||
| // +deprecated |
There was a problem hiding this comment.
has the +deprecated marker any meaning? I don't see it mentioned in https://book.kubebuilder.io/reference/markers/crd
There was a problem hiding this comment.
Probably not. Definitely not a kubebuilder marker. I thought this is the "official" way in Go, but it's probably not.
5 participants
This is adding a new
clusterHealthAnalyzerattribute to themonitoringConfigin the UIPlugin API.This also marks the existing
incidentsattribute as deprecated and suggest to use the newclusterHealthAnalyzerinstead.