Skip to content

✨ Simplify status condition reasons #1241

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 10, 2024
Merged

✨ Simplify status condition reasons #1241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 8 additions & 23 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,19 +425,11 @@ const (
TypeBundleDeprecated = "BundleDeprecated"
TypeUnpacked = "Unpacked"

ReasonErrorGettingClient = "ErrorGettingClient"
ReasonBundleLoadFailed = "BundleLoadFailed"

ReasonInstallationFailed = "InstallationFailed"
ReasonResolutionFailed = "ResolutionFailed"

ReasonSuccess = "Success"
ReasonDeprecated = "Deprecated"
ReasonUpgradeFailed = "UpgradeFailed"

ReasonUnpackSuccess = "UnpackSuccess"
ReasonUnpackFailed = "UnpackFailed"
ReasonSuccess = "Succeeded"
ReasonDeprecated = "Deprecated"
ReasonFailed = "Failed"

ReasonErrorGettingClient = "ErrorGettingClient"
ReasonErrorGettingReleaseState = "ErrorGettingReleaseState"

ReasonUnverifiable = "Unverifiable"
Expand All @@ -460,15 +452,10 @@ func init() {
)
// TODO(user): add Reasons from above
conditionsets.ConditionReasons = append(conditionsets.ConditionReasons,
ReasonResolutionFailed,
ReasonInstallationFailed,
ReasonSuccess,
ReasonDeprecated,
ReasonUpgradeFailed,
ReasonBundleLoadFailed,
ReasonFailed,
ReasonErrorGettingClient,
ReasonUnpackSuccess,
ReasonUnpackFailed,
ReasonErrorGettingReleaseState,
ReasonUnverifiable,
)
Expand Down Expand Up @@ -508,11 +495,9 @@ type ClusterExtensionStatus struct {
// - "Unpacked", represents whether or not the bundle contents have been successfully unpacked
//
// The current set of reasons are:
// - "ResolutionFailed", this reason is set on the "Resolved" condition when an error has occurred during resolution.
// - "InstallationFailed", this reason is set on the "Installed" condition when an error has occurred during installation
// - "Success", this reason is set on the "Resolved" and "Installed" conditions when resolution and installation/upgrading is successful
// - "UnpackSuccess", this reason is set on the "Unpacked" condition when unpacking a bundle's content is successful
// - "UnpackFailed", this reason is set on the "Unpacked" condition when an error has been encountered while unpacking the contents of a bundle
// - "Success", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when unpacking a bundle's content, resolution and installation/upgrading is successful
// - "Failed", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when an error has occurred while unpacking the contents of a bundle, during resolution or installation.
//
//
// +patchMergeKey=type
// +patchStrategy=merge
Expand Down
7 changes: 2 additions & 5 deletions config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,11 +484,8 @@ spec:
- "Unpacked", represents whether or not the bundle contents have been successfully unpacked

The current set of reasons are:
- "ResolutionFailed", this reason is set on the "Resolved" condition when an error has occurred during resolution.
- "InstallationFailed", this reason is set on the "Installed" condition when an error has occurred during installation
- "Success", this reason is set on the "Resolved" and "Installed" conditions when resolution and installation/upgrading is successful
- "UnpackSuccess", this reason is set on the "Unpacked" condition when unpacking a bundle's content is successful
- "UnpackFailed", this reason is set on the "Unpacked" condition when an error has been encountered while unpacking the contents of a bundle
- "Success", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when unpacking a bundle's content, resolution and installation/upgrading is successful
- "Failed", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when an error has occurred while unpacking the contents of a bundle, during resolution or installation.
items:
description: Condition contains details for one aspect of the current
state of this API Resource.
Expand Down
2 changes: 1 addition & 1 deletion docs/refs/api/operator-controller-api-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ _Appears in:_
| --- | --- | --- | --- |
| `install` _[ClusterExtensionInstallStatus](#clusterextensioninstallstatus)_ | | | |
| `resolution` _[ClusterExtensionResolutionStatus](#clusterextensionresolutionstatus)_ | | | |
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#condition-v1-meta) array_ | conditions is a representation of the current state for this ClusterExtension.<br />The status is represented by a set of "conditions".<br /><br />Each condition is generally structured in the following format:<br /> - Type: a string representation of the condition type. More or less the condition "name".<br /> - Status: a string representation of the state of the condition. Can be one of ["True", "False", "Unknown"].<br /> - Reason: a string representation of the reason for the current state of the condition. Typically useful for building automation around particular Type+Reason combinations.<br /> - Message: a human readable message that further elaborates on the state of the condition<br /><br />The current set of condition types are:<br /> - "Installed", represents whether or not the package referenced in the spec.packageName field has been installed<br /> - "Resolved", represents whether or not a bundle was found that satisfies the selection criteria outlined in the spec<br /> - "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types.<br /> - "PackageDeprecated", represents whether or not the package specified in the spec.packageName field has been deprecated<br /> - "ChannelDeprecated", represents whether or not the channel specified in spec.channel has been deprecated<br /> - "BundleDeprecated", represents whether or not the bundle installed is deprecated<br /> - "Unpacked", represents whether or not the bundle contents have been successfully unpacked<br /><br />The current set of reasons are:<br /> - "ResolutionFailed", this reason is set on the "Resolved" condition when an error has occurred during resolution.<br /> - "InstallationFailed", this reason is set on the "Installed" condition when an error has occurred during installation<br /> - "Success", this reason is set on the "Resolved" and "Installed" conditions when resolution and installation/upgrading is successful<br /> - "UnpackSuccess", this reason is set on the "Unpacked" condition when unpacking a bundle's content is successful<br /> - "UnpackFailed", this reason is set on the "Unpacked" condition when an error has been encountered while unpacking the contents of a bundle | | |
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#condition-v1-meta) array_ | conditions is a representation of the current state for this ClusterExtension.<br />The status is represented by a set of "conditions".<br /><br />Each condition is generally structured in the following format:<br /> - Type: a string representation of the condition type. More or less the condition "name".<br /> - Status: a string representation of the state of the condition. Can be one of ["True", "False", "Unknown"].<br /> - Reason: a string representation of the reason for the current state of the condition. Typically useful for building automation around particular Type+Reason combinations.<br /> - Message: a human readable message that further elaborates on the state of the condition<br /><br />The current set of condition types are:<br /> - "Installed", represents whether or not the package referenced in the spec.packageName field has been installed<br /> - "Resolved", represents whether or not a bundle was found that satisfies the selection criteria outlined in the spec<br /> - "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types.<br /> - "PackageDeprecated", represents whether or not the package specified in the spec.packageName field has been deprecated<br /> - "ChannelDeprecated", represents whether or not the channel specified in spec.channel has been deprecated<br /> - "BundleDeprecated", represents whether or not the bundle installed is deprecated<br /> - "Unpacked", represents whether or not the bundle contents have been successfully unpacked<br /><br />The current set of reasons are:<br /> - "Success", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when unpacking a bundle's content, resolution and installation/upgrading is successful<br /> - "Failed", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when an error has occurred while unpacking the contents of a bundle, during resolution or installation. | | |


#### PreflightConfig
Expand Down
6 changes: 3 additions & 3 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
setInstallStatus(ext, nil)
setResolutionStatus(ext, nil)
setResolvedStatusConditionFailed(ext, err.Error())
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, err.Error())
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error())
return ctrl.Result{}, err
}
if finalizeResult.Updated || finalizeResult.StatusUpdated {
Expand All @@ -220,7 +220,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
setInstallStatus(ext, nil)
setResolutionStatus(ext, nil)
setResolvedStatusConditionFailed(ext, err.Error())
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, err.Error())
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error())
return ctrl.Result{}, err
}

Expand Down Expand Up @@ -262,7 +262,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
switch unpackResult.State {
case rukpaksource.StatePending:
setStatusUnpackFailed(ext, unpackResult.Message)
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonUnpackFailed, "unpack pending")
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, "unpack pending")
return ctrl.Result{}, nil
case rukpaksource.StateUnpacked:
setStatusUnpacked(ext, fmt.Sprintf("unpack successful: %v", unpackResult.Message))
Expand Down
20 changes: 10 additions & 10 deletions internal/controllers/clusterextension_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestClusterExtensionResolutionFails(t *testing.T) {
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
require.NotNil(t, cond)
require.Equal(t, metav1.ConditionFalse, cond.Status)
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
require.Equal(t, ocv1alpha1.ReasonFailed, cond.Reason)
require.Equal(t, fmt.Sprintf("no package %q found", pkgName), cond.Message)

verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
Expand Down Expand Up @@ -166,7 +166,7 @@ func TestClusterExtensionResolutionSucceeds(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionFalse, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackFailed, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonFailed, unpackedCond.Reason)

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}
Expand Down Expand Up @@ -242,7 +242,7 @@ func TestClusterExtensionUnpackFails(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionFalse, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackFailed, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonFailed, unpackedCond.Reason)

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}
Expand Down Expand Up @@ -320,7 +320,7 @@ func TestClusterExtensionUnpackUnexpectedState(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionFalse, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackFailed, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonFailed, unpackedCond.Reason)

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}
Expand Down Expand Up @@ -402,7 +402,7 @@ func TestClusterExtensionUnpackSucceeds(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionTrue, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackSuccess, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonSuccess, unpackedCond.Reason)

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}
Expand Down Expand Up @@ -484,13 +484,13 @@ func TestClusterExtensionInstallationFailedApplierFails(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionTrue, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackSuccess, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonSuccess, unpackedCond.Reason)

t.Log("By checking the expected installed conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
require.NotNil(t, installedCond)
require.Equal(t, metav1.ConditionFalse, installedCond.Status)
require.Equal(t, ocv1alpha1.ReasonInstallationFailed, installedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonFailed, installedCond.Reason)

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}
Expand Down Expand Up @@ -575,7 +575,7 @@ func TestClusterExtensionManagerFailed(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionTrue, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackSuccess, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonSuccess, unpackedCond.Reason)

t.Log("By checking the expected installed conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
Expand Down Expand Up @@ -675,7 +675,7 @@ func TestClusterExtensionManagedContentCacheWatchFail(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionTrue, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackSuccess, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonSuccess, unpackedCond.Reason)

t.Log("By checking the expected installed conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
Expand Down Expand Up @@ -772,7 +772,7 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) {
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
require.NotNil(t, unpackedCond)
require.Equal(t, metav1.ConditionTrue, unpackedCond.Status)
require.Equal(t, ocv1alpha1.ReasonUnpackSuccess, unpackedCond.Reason)
require.Equal(t, ocv1alpha1.ReasonSuccess, unpackedCond.Reason)

t.Log("By checking the expected installed conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
Expand Down
8 changes: 4 additions & 4 deletions internal/controllers/common_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func setResolvedStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
Type: ocv1alpha1.TypeResolved,
Status: metav1.ConditionFalse,
Reason: ocv1alpha1.ReasonResolutionFailed,
Reason: ocv1alpha1.ReasonFailed,
Message: message,
ObservedGeneration: ext.GetGeneration(),
})
Expand All @@ -61,7 +61,7 @@ func setInstalledStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
Type: ocv1alpha1.TypeInstalled,
Status: metav1.ConditionFalse,
Reason: ocv1alpha1.ReasonInstallationFailed,
Reason: ocv1alpha1.ReasonFailed,
Message: message,
ObservedGeneration: ext.GetGeneration(),
})
Expand All @@ -72,7 +72,7 @@ func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) {
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
Type: ocv1alpha1.TypeUnpacked,
Status: metav1.ConditionFalse,
Reason: ocv1alpha1.ReasonUnpackFailed,
Reason: ocv1alpha1.ReasonFailed,
Message: message,
ObservedGeneration: ext.GetGeneration(),
})
Expand All @@ -82,7 +82,7 @@ func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) {
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
Type: ocv1alpha1.TypeUnpacked,
Status: metav1.ConditionTrue,
Reason: ocv1alpha1.ReasonUnpackSuccess,
Reason: ocv1alpha1.ReasonSuccess,
Message: message,
ObservedGeneration: ext.GetGeneration(),
})
Expand Down
10 changes: 5 additions & 5 deletions test/e2e/cluster_extension_install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ func TestClusterExtensionInstallRegistry(t *testing.T) {
return
}
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonUnpackSuccess, cond.Reason)
assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
assert.Contains(ct, cond.Message, "unpack successful")
}, pollDuration, pollInterval)

Expand Down Expand Up @@ -324,7 +324,7 @@ func TestClusterExtensionInstallRegistryMultipleBundles(t *testing.T) {
return
}
assert.Equal(ct, metav1.ConditionFalse, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
assert.Equal(ct, ocv1alpha1.ReasonFailed, cond.Reason)
assert.Contains(ct, cond.Message, "in multiple catalogs with the same priority [operatorhubio test-catalog]")
assert.Nil(ct, clusterExtension.Status.Resolution)
}, pollDuration, pollInterval)
Expand Down Expand Up @@ -393,7 +393,7 @@ func TestClusterExtensionBlockInstallNonSuccessorVersion(t *testing.T) {
if !assert.NotNil(ct, cond) {
return
}
assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
assert.Equal(ct, ocv1alpha1.ReasonFailed, cond.Reason)
assert.Equal(ct, "error upgrading from currently installed version \"1.0.0\": no bundles found for package \"prometheus\" matching version \"1.2.0\"", cond.Message)
assert.Empty(ct, clusterExtension.Status.Resolution)
}, pollDuration, pollInterval)
Expand Down Expand Up @@ -848,7 +848,7 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes
return
}
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonUnpackSuccess, cond.Reason)
assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
assert.Contains(ct, cond.Message, "unpack successful")
}, pollDuration, pollInterval)

Expand All @@ -860,7 +860,7 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes
return
}
assert.Equal(ct, metav1.ConditionFalse, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonInstallationFailed, cond.Reason)
assert.Equal(ct, ocv1alpha1.ReasonFailed, cond.Reason)
assert.Contains(ct, cond.Message, "forbidden")
}, pollDuration, pollInterval)

Expand Down
Loading