From d856af4f8266a6ce4132f48e08837af6a01e0537 Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 29 May 2026 15:54:13 -0500 Subject: [PATCH 1/2] =?UTF-8?q?=E2=9C=85=20Add=20standard=20metav1.Conditi?= =?UTF-8?q?on=20projection=20to=20job=20status?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds []metav1.Condition to ZarfPackageJobStatus and UDSBundleJobStatus. Conditions are derived from the existing phase state machine on every updateStatus call — phases are kept intact as the internal state machine for action chaining and reconcile gating. Ready: True=succeeded / False=failed / Unknown=in-progress Reconciling: True while not in a terminal phase BuildSucceeded / CreateSucceeded / PublishSucceeded / DeploySucceeded: per-operation outcomes derived from OperationStatus.Phase LastTransitionTime is preserved when a condition's status is unchanged. Also fixes updateStatus to merge into existing status rather than replace it entirely, preventing prior operation fields (e.g. buildStatus) from being silently lost when a chained action (e.g. publishStatus) is written. Enables: kubectl wait --for=condition=Ready=True zpj/my-job --- chart/forge/crds/forge.dev_udsbundlejobs.yaml | 63 ++++++++ .../forge/crds/forge.dev_zarfpackagejobs.yaml | 63 ++++++++ pkg/apis/uds/v1alpha3/types.go | 8 + .../uds/v1alpha3/zz_generated.deepcopy.go | 8 + pkg/apis/zarf/v1alpha3/types.go | 8 + .../zarf/v1alpha3/zz_generated.deepcopy.go | 8 + pkg/constants/conditions.go | 41 +++++ pkg/controller/conditions.go | 141 ++++++++++++++++ pkg/controller/conditions_test.go | 153 ++++++++++++++++++ pkg/controller/generic_controller.go | 35 ++-- 10 files changed, 516 insertions(+), 12 deletions(-) create mode 100644 pkg/constants/conditions.go create mode 100644 pkg/controller/conditions.go create mode 100644 pkg/controller/conditions_test.go diff --git a/chart/forge/crds/forge.dev_udsbundlejobs.yaml b/chart/forge/crds/forge.dev_udsbundlejobs.yaml index 07af783..b94e3e8 100644 --- a/chart/forge/crds/forge.dev_udsbundlejobs.yaml +++ b/chart/forge/crds/forge.dev_udsbundlejobs.yaml @@ -1986,6 +1986,69 @@ spec: status: description: UDSBundleJobStatus defines the observed state of a UDSBundleJob properties: + conditions: + description: |- + Conditions is the standard Kubernetes condition array for this job. + The Ready condition reflects the overall outcome; per-operation conditions + (CreateSucceeded, PublishSucceeded, DeploySucceeded) track individual steps. + items: + description: Condition contains details for one aspect of the current + state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map createStatus: description: CreateStatus tracks the create operation status properties: diff --git a/chart/forge/crds/forge.dev_zarfpackagejobs.yaml b/chart/forge/crds/forge.dev_zarfpackagejobs.yaml index a0cd907..40ab9df 100644 --- a/chart/forge/crds/forge.dev_zarfpackagejobs.yaml +++ b/chart/forge/crds/forge.dev_zarfpackagejobs.yaml @@ -1996,6 +1996,69 @@ spec: format: date-time type: string type: object + conditions: + description: |- + Conditions is the standard Kubernetes condition array for this job. + The Ready condition reflects the overall outcome; per-operation conditions + (BuildSucceeded, PublishSucceeded, DeploySucceeded) track individual steps. + items: + description: Condition contains details for one aspect of the current + state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map deployStatus: description: DeployStatus contains deploy operation status properties: diff --git a/pkg/apis/uds/v1alpha3/types.go b/pkg/apis/uds/v1alpha3/types.go index 221b164..b686c3a 100644 --- a/pkg/apis/uds/v1alpha3/types.go +++ b/pkg/apis/uds/v1alpha3/types.go @@ -594,6 +594,14 @@ type UDSBundleJobStatus struct { // +optional Message string `json:"message,omitempty"` + // Conditions is the standard Kubernetes condition array for this job. + // The Ready condition reflects the overall outcome; per-operation conditions + // (CreateSucceeded, PublishSucceeded, DeploySucceeded) track individual steps. + // +optional + // +listType=map + // +listMapKey=type + Conditions []metav1.Condition `json:"conditions,omitempty"` + // CreateStatus tracks the create operation status // +optional CreateStatus *OperationStatus `json:"createStatus,omitempty"` diff --git a/pkg/apis/uds/v1alpha3/zz_generated.deepcopy.go b/pkg/apis/uds/v1alpha3/zz_generated.deepcopy.go index d373408..3116f2d 100644 --- a/pkg/apis/uds/v1alpha3/zz_generated.deepcopy.go +++ b/pkg/apis/uds/v1alpha3/zz_generated.deepcopy.go @@ -7,6 +7,7 @@ package v1alpha3 import ( "github.com/kylegalloway/forge/pkg/apis/common" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -613,6 +614,13 @@ func (in *UDSBundleJobSpec) DeepCopy() *UDSBundleJobSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *UDSBundleJobStatus) DeepCopyInto(out *UDSBundleJobStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]metav1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.CreateStatus != nil { in, out := &in.CreateStatus, &out.CreateStatus *out = new(OperationStatus) diff --git a/pkg/apis/zarf/v1alpha3/types.go b/pkg/apis/zarf/v1alpha3/types.go index 317bcc2..a388a1d 100644 --- a/pkg/apis/zarf/v1alpha3/types.go +++ b/pkg/apis/zarf/v1alpha3/types.go @@ -582,6 +582,14 @@ type ZarfPackageJobStatus struct { // +optional Message string `json:"message,omitempty"` + // Conditions is the standard Kubernetes condition array for this job. + // The Ready condition reflects the overall outcome; per-operation conditions + // (BuildSucceeded, PublishSucceeded, DeploySucceeded) track individual steps. + // +optional + // +listType=map + // +listMapKey=type + Conditions []metav1.Condition `json:"conditions,omitempty"` + // BuildStatus contains build operation status // +optional BuildStatus *OperationStatus `json:"buildStatus,omitempty"` diff --git a/pkg/apis/zarf/v1alpha3/zz_generated.deepcopy.go b/pkg/apis/zarf/v1alpha3/zz_generated.deepcopy.go index 3b3a0ee..9473466 100644 --- a/pkg/apis/zarf/v1alpha3/zz_generated.deepcopy.go +++ b/pkg/apis/zarf/v1alpha3/zz_generated.deepcopy.go @@ -7,6 +7,7 @@ package v1alpha3 import ( "github.com/kylegalloway/forge/pkg/apis/common" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -584,6 +585,13 @@ func (in *ZarfPackageJobSpec) DeepCopy() *ZarfPackageJobSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ZarfPackageJobStatus) DeepCopyInto(out *ZarfPackageJobStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]metav1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.BuildStatus != nil { in, out := &in.BuildStatus, &out.BuildStatus *out = new(OperationStatus) diff --git a/pkg/constants/conditions.go b/pkg/constants/conditions.go new file mode 100644 index 0000000..55ac27c --- /dev/null +++ b/pkg/constants/conditions.go @@ -0,0 +1,41 @@ +package constants + +const ( + // ConditionTypeReady indicates the overall job completed successfully. + // True=succeeded, False=failed terminally, Unknown=in progress. + ConditionTypeReady = "Ready" + + // ConditionTypeReconciling indicates the controller is actively working on the job. + ConditionTypeReconciling = "Reconciling" + + // ConditionTypeBuildSucceeded reflects the build operation outcome. + ConditionTypeBuildSucceeded = "BuildSucceeded" + + // ConditionTypeCreateSucceeded reflects the UDS create operation outcome. + ConditionTypeCreateSucceeded = "CreateSucceeded" + + // ConditionTypePublishSucceeded reflects the publish operation outcome. + ConditionTypePublishSucceeded = "PublishSucceeded" + + // ConditionTypeDeploySucceeded reflects the deploy operation outcome. + ConditionTypeDeploySucceeded = "DeploySucceeded" +) + +const ( + // ConditionReasonSucceeded is used when an operation or job completed successfully. + ConditionReasonSucceeded = "Succeeded" + // ConditionReasonFailed is used when an operation or job terminated with an error. + ConditionReasonFailed = "Failed" + // ConditionReasonProgressing is used while an operation or job is in flight. + ConditionReasonProgressing = "Progressing" + // ConditionReasonSuspended is used when reconciliation is paused via spec.suspend. + ConditionReasonSuspended = "Suspended" +) + +// OperationConditionTypes maps OperationStatus field names to their condition type. +var OperationConditionTypes = map[string]string{ + "buildStatus": ConditionTypeBuildSucceeded, + "createStatus": ConditionTypeCreateSucceeded, + "publishStatus": ConditionTypePublishSucceeded, + "deployStatus": ConditionTypeDeploySucceeded, +} diff --git a/pkg/controller/conditions.go b/pkg/controller/conditions.go new file mode 100644 index 0000000..6286445 --- /dev/null +++ b/pkg/controller/conditions.go @@ -0,0 +1,141 @@ +package controller + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + + "github.com/kylegalloway/forge/pkg/constants" +) + +// DeriveConditions projects the phase state machine into a []metav1.Condition slice. +// existing is the prior conditions slice, used to preserve LastTransitionTime when +// a condition's status has not changed. +func DeriveConditions(phase string, status map[string]interface{}, existing []metav1.Condition, generation int64) []metav1.Condition { + now := metav1.Now() + conditions := make([]metav1.Condition, 0, 6) + + conditions = setCondition(conditions, existing, readyCondition(phase, generation, now)) + conditions = setCondition(conditions, existing, reconcilingCondition(phase, generation, now)) + + for field, condType := range constants.OperationConditionTypes { + opStatus, ok := status[field].(map[string]interface{}) + if !ok { + continue + } + opPhase, ok := opStatus[constants.StatusKeyState].(string) + if !ok || opPhase == "" { + continue + } + conditions = setCondition(conditions, existing, operationCondition(condType, opPhase, generation, now)) + } + + return conditions +} + +func readyCondition(phase string, generation int64, now metav1.Time) metav1.Condition { + c := metav1.Condition{ + Type: constants.ConditionTypeReady, + ObservedGeneration: generation, + LastTransitionTime: now, + } + switch phase { + case constants.PhaseCompleted: + c.Status = metav1.ConditionTrue + c.Reason = constants.ConditionReasonSucceeded + case constants.PhaseFailed: + c.Status = metav1.ConditionFalse + c.Reason = constants.ConditionReasonFailed + default: + c.Status = metav1.ConditionUnknown + c.Reason = constants.ConditionReasonProgressing + } + return c +} + +func reconcilingCondition(phase string, generation int64, now metav1.Time) metav1.Condition { + terminal := phase == constants.PhaseCompleted || phase == constants.PhaseFailed + status := metav1.ConditionTrue + if terminal { + status = metav1.ConditionFalse + } + return metav1.Condition{ + Type: constants.ConditionTypeReconciling, + Status: status, + Reason: constants.ConditionReasonProgressing, + ObservedGeneration: generation, + LastTransitionTime: now, + } +} + +func operationCondition(condType, phase string, generation int64, now metav1.Time) metav1.Condition { + c := metav1.Condition{ + Type: condType, + ObservedGeneration: generation, + LastTransitionTime: now, + } + switch phase { + case constants.PhaseCompleted: + c.Status = metav1.ConditionTrue + c.Reason = constants.ConditionReasonSucceeded + case constants.PhaseFailed: + c.Status = metav1.ConditionFalse + c.Reason = constants.ConditionReasonFailed + default: + c.Status = metav1.ConditionUnknown + c.Reason = constants.ConditionReasonProgressing + } + return c +} + +// setCondition appends or replaces a condition, preserving LastTransitionTime +// when the status is unchanged. +func setCondition(conditions []metav1.Condition, existing []metav1.Condition, c metav1.Condition) []metav1.Condition { + for _, e := range existing { + if e.Type == c.Type && e.Status == c.Status { + c.LastTransitionTime = e.LastTransitionTime + break + } + } + for i, cur := range conditions { + if cur.Type == c.Type { + conditions[i] = c + return conditions + } + } + return append(conditions, c) +} + +// conditionsToUnstructured converts a []metav1.Condition to the []interface{} +// form required for unstructured status writes. +func conditionsToUnstructured(conditions []metav1.Condition) []interface{} { + out := make([]interface{}, 0, len(conditions)) + for i := range conditions { + m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&conditions[i]) + if err != nil { + continue + } + out = append(out, m) + } + return out +} + +// conditionsFromUnstructured reads the conditions slice from a raw status map. +func conditionsFromUnstructured(raw interface{}) []metav1.Condition { + list, ok := raw.([]interface{}) + if !ok { + return nil + } + conditions := make([]metav1.Condition, 0, len(list)) + for _, item := range list { + m, ok := item.(map[string]interface{}) + if !ok { + continue + } + var c metav1.Condition + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(m, &c); err != nil { + continue + } + conditions = append(conditions, c) + } + return conditions +} diff --git a/pkg/controller/conditions_test.go b/pkg/controller/conditions_test.go new file mode 100644 index 0000000..623634b --- /dev/null +++ b/pkg/controller/conditions_test.go @@ -0,0 +1,153 @@ +package controller + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kylegalloway/forge/pkg/constants" +) + +func TestDeriveConditions_Ready(t *testing.T) { + cases := []struct { + phase string + wantReadyStatus metav1.ConditionStatus + wantReconciling metav1.ConditionStatus + }{ + {constants.PhaseCompleted, metav1.ConditionTrue, metav1.ConditionFalse}, + {constants.PhaseFailed, metav1.ConditionFalse, metav1.ConditionFalse}, + {constants.PhaseRunning, metav1.ConditionUnknown, metav1.ConditionTrue}, + {constants.PhasePending, metav1.ConditionUnknown, metav1.ConditionTrue}, + {constants.PhaseRetrying, metav1.ConditionUnknown, metav1.ConditionTrue}, + {constants.PhaseQueued, metav1.ConditionUnknown, metav1.ConditionTrue}, + } + for _, tc := range cases { + t.Run(tc.phase, func(t *testing.T) { + conds := DeriveConditions(tc.phase, map[string]interface{}{}, nil, 1) + ready := findCondition(conds, constants.ConditionTypeReady) + if ready == nil { + t.Fatal("Ready condition missing") + } + if ready.Status != tc.wantReadyStatus { + t.Errorf("Ready status: got %s, want %s", ready.Status, tc.wantReadyStatus) + } + reconciling := findCondition(conds, constants.ConditionTypeReconciling) + if reconciling == nil { + t.Fatal("Reconciling condition missing") + } + if reconciling.Status != tc.wantReconciling { + t.Errorf("Reconciling status: got %s, want %s", reconciling.Status, tc.wantReconciling) + } + }) + } +} + +func TestDeriveConditions_OperationConditions(t *testing.T) { + status := map[string]interface{}{ + "buildStatus": map[string]interface{}{ + constants.StatusKeyState: constants.PhaseCompleted, + }, + "publishStatus": map[string]interface{}{ + constants.StatusKeyState: constants.PhaseRunning, + }, + } + conds := DeriveConditions(constants.PhaseRunning, status, nil, 1) + + build := findCondition(conds, constants.ConditionTypeBuildSucceeded) + if build == nil { + t.Fatal("BuildSucceeded condition missing") + } + if build.Status != metav1.ConditionTrue { + t.Errorf("BuildSucceeded: got %s, want True", build.Status) + } + + publish := findCondition(conds, constants.ConditionTypePublishSucceeded) + if publish == nil { + t.Fatal("PublishSucceeded condition missing") + } + if publish.Status != metav1.ConditionUnknown { + t.Errorf("PublishSucceeded: got %s, want Unknown", publish.Status) + } + + // deployStatus not in status map — condition should not appear + deploy := findCondition(conds, constants.ConditionTypeDeploySucceeded) + if deploy != nil { + t.Error("DeploySucceeded should not be present when deployStatus is absent") + } +} + +func TestDeriveConditions_PreservesLastTransitionTime(t *testing.T) { + past := metav1.NewTime(metav1.Now().Add(-60e9)) + existing := []metav1.Condition{ + { + Type: constants.ConditionTypeReady, + Status: metav1.ConditionUnknown, + LastTransitionTime: past, + Reason: constants.ConditionReasonProgressing, + }, + } + // Same status (Unknown → Unknown): LastTransitionTime must be preserved. + conds := DeriveConditions(constants.PhaseRunning, map[string]interface{}{}, existing, 1) + ready := findCondition(conds, constants.ConditionTypeReady) + if ready == nil { + t.Fatal("Ready condition missing") + } + if !ready.LastTransitionTime.Equal(&past) { + t.Errorf("LastTransitionTime changed when status was unchanged: got %v, want %v", ready.LastTransitionTime, past) + } +} + +func TestDeriveConditions_TransitionsLastTransitionTime(t *testing.T) { + past := metav1.NewTime(metav1.Now().Add(-60e9)) + existing := []metav1.Condition{ + { + Type: constants.ConditionTypeReady, + Status: metav1.ConditionUnknown, + LastTransitionTime: past, + Reason: constants.ConditionReasonProgressing, + }, + } + // Status changes (Unknown → True): LastTransitionTime must be updated. + conds := DeriveConditions(constants.PhaseCompleted, map[string]interface{}{}, existing, 1) + ready := findCondition(conds, constants.ConditionTypeReady) + if ready == nil { + t.Fatal("Ready condition missing") + } + if ready.LastTransitionTime.Equal(&past) { + t.Error("LastTransitionTime should have been updated on status transition") + } +} + +func TestConditionsRoundtrip(t *testing.T) { + original := []metav1.Condition{ + { + Type: constants.ConditionTypeReady, + Status: metav1.ConditionTrue, + Reason: constants.ConditionReasonSucceeded, + ObservedGeneration: 3, + LastTransitionTime: metav1.Now(), + }, + } + roundtripped := conditionsFromUnstructured(conditionsToUnstructured(original)) + if len(roundtripped) != 1 { + t.Fatalf("expected 1 condition after roundtrip, got %d", len(roundtripped)) + } + if roundtripped[0].Type != original[0].Type { + t.Errorf("Type: got %s, want %s", roundtripped[0].Type, original[0].Type) + } + if roundtripped[0].Status != original[0].Status { + t.Errorf("Status: got %s, want %s", roundtripped[0].Status, original[0].Status) + } + if roundtripped[0].ObservedGeneration != original[0].ObservedGeneration { + t.Errorf("ObservedGeneration: got %d, want %d", roundtripped[0].ObservedGeneration, original[0].ObservedGeneration) + } +} + +func findCondition(conditions []metav1.Condition, condType string) *metav1.Condition { + for i := range conditions { + if conditions[i].Type == condType { + return &conditions[i] + } + } + return nil +} diff --git a/pkg/controller/generic_controller.go b/pkg/controller/generic_controller.go index 8f63a79..dff8493 100644 --- a/pkg/controller/generic_controller.go +++ b/pkg/controller/generic_controller.go @@ -644,28 +644,39 @@ func (ctrl *GenericController[T]) updateResourceStatus(ctx context.Context, obj return ctrl.updateStatus(ctx, obj, phase, message, nil) } -// updateStatus updates the resource status +// updateStatus updates the resource status. +// It merges into the existing status (preserving prior operation fields across +// chained actions) and derives standard conditions from the resulting phase. func (ctrl *GenericController[T]) updateStatus(ctx context.Context, obj *unstructured.Unstructured, phase, message string, operationStatus map[string]interface{}) error { name := obj.GetName() namespace := obj.GetNamespace() - // Build status object - status := map[string]interface{}{ - "phase": phase, - "message": message, - "lastUpdateTime": metav1.Now().Format(time.RFC3339), - "observedGeneration": obj.GetGeneration(), + // Start from existing status so chained actions don't lose prior operation fields. + existing, _, err := unstructured.NestedMap(obj.Object, "status") + if err != nil { + return fmt.Errorf("failed to read existing status: %w", err) + } + if existing == nil { + existing = map[string]interface{}{} } - // Add operation-specific status if provided + existingConditions := conditionsFromUnstructured(existing["conditions"]) + + existing["phase"] = phase + existing["message"] = message + existing["lastUpdateTime"] = metav1.Now().Format(time.RFC3339) + existing["observedGeneration"] = obj.GetGeneration() + for key, value := range operationStatus { - status[key] = value + existing[key] = value } - // Update status subresource - obj.Object["status"] = status + conditions := DeriveConditions(phase, existing, existingConditions, obj.GetGeneration()) + existing["conditions"] = conditionsToUnstructured(conditions) + + obj.Object["status"] = existing - _, err := ctrl.dynamicClient.Resource(ctrl.resourceGVR).Namespace(namespace).UpdateStatus(ctx, obj, metav1.UpdateOptions{}) + _, err = ctrl.dynamicClient.Resource(ctrl.resourceGVR).Namespace(namespace).UpdateStatus(ctx, obj, metav1.UpdateOptions{}) if err != nil { if errors.IsNotFound(err) { klog.V(4).InfoS("Resource not found during status update", "name", name, "namespace", namespace) From 3b33c57e057ce7cbe464efd1317bb2b931159e07 Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 29 May 2026 16:12:04 -0500 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=94=A7=20Conditions=20were=20Schr?= =?UTF-8?q?=C3=B6dinger:=20terminal=20but=20still=20Progressing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciling=False with Reason=Progressing was semantically incoherent — like a 'CLOSED' sign that still says 'Be Right Back'. Terminal phases now carry the correct reason (Succeeded/Failed) so kubectl get output doesn't lie to operators. OperationConditionTypes was a map, which in Go means the per-operation conditions appeared in random order on every reconcile. Converted to an ordered slice so the status output is stable across calls. Also stubs ConditionReasonSuspended as an explicit forward declaration, adds klog.ErrorS for the silent drop in conditionsToUnstructured, and covers both fixes with TestDeriveConditions_ReconcilingReason and TestDeriveConditions_DeterministicOrder. --- CHANGELOG.md | 11 ++++++++ pkg/constants/conditions.go | 21 ++++++++++----- pkg/controller/conditions.go | 19 ++++++++----- pkg/controller/conditions_test.go | 45 +++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc17754..b479e73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Dependabot version updates for Go modules and GitHub Actions (`.github/dependabot.yml`), running weekly on Mondays - Scheduled workflow (`.github/workflows/update-tool-versions.yaml`) that runs `scripts/update-tool-versions.sh` every Monday and opens a PR when `ZARF_VERSION` or `UDS_VERSION` change upstream; fills the gap Dependabot leaves since its Docker ecosystem only tracks `FROM` lines, not `ARG` values +### Added +- `TestDeriveConditions_ReconcilingReason`: covers all six phases to verify the `Reconciling` reason is correct for terminal vs in-progress states +- `TestDeriveConditions_DeterministicOrder`: asserts two `DeriveConditions` calls with identical input produce identical condition order + +### Fixed +- `Reconciling` condition `Reason` was always `Progressing` even when `Status=False` (terminal); now correctly reflects the outcome — `Succeeded` when `PhaseCompleted`, `Failed` when `PhaseFailed` +- Per-operation conditions in the status slice had non-deterministic order (map iteration); `OperationConditionTypes` is now an ordered slice so conditions appear in stable order (`build → create → publish → deploy`) + +### Changed +- `ConditionReasonSuspended` comment updated to clearly mark it as a forward declaration for a future `spec.suspend` feature +- `conditionsToUnstructured` now logs via `klog.ErrorS` when a condition cannot be converted rather than silently dropping it ## [0.12.0] - 2026-05-29 diff --git a/pkg/constants/conditions.go b/pkg/constants/conditions.go index 55ac27c..721614f 100644 --- a/pkg/constants/conditions.go +++ b/pkg/constants/conditions.go @@ -28,14 +28,21 @@ const ( ConditionReasonFailed = "Failed" // ConditionReasonProgressing is used while an operation or job is in flight. ConditionReasonProgressing = "Progressing" - // ConditionReasonSuspended is used when reconciliation is paused via spec.suspend. + // ConditionReasonSuspended is reserved for when spec.suspend is added. + // Not yet wired; defined here so the constant is stable when that feature lands. ConditionReasonSuspended = "Suspended" ) -// OperationConditionTypes maps OperationStatus field names to their condition type. -var OperationConditionTypes = map[string]string{ - "buildStatus": ConditionTypeBuildSucceeded, - "createStatus": ConditionTypeCreateSucceeded, - "publishStatus": ConditionTypePublishSucceeded, - "deployStatus": ConditionTypeDeploySucceeded, +// OperationConditionTypes maps OperationStatus JSON field names to condition types. +// Keys must match the json tags on the status types exactly — renaming a status +// field requires updating this list in sync. +// Slice order determines the order conditions appear in the status output. +var OperationConditionTypes = []struct { + Field string + CondType string +}{ + {"buildStatus", ConditionTypeBuildSucceeded}, + {"createStatus", ConditionTypeCreateSucceeded}, + {"publishStatus", ConditionTypePublishSucceeded}, + {"deployStatus", ConditionTypeDeploySucceeded}, } diff --git a/pkg/controller/conditions.go b/pkg/controller/conditions.go index 6286445..a8cce1f 100644 --- a/pkg/controller/conditions.go +++ b/pkg/controller/conditions.go @@ -3,6 +3,7 @@ package controller import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + klog "k8s.io/klog/v2" "github.com/kylegalloway/forge/pkg/constants" ) @@ -17,8 +18,8 @@ func DeriveConditions(phase string, status map[string]interface{}, existing []me conditions = setCondition(conditions, existing, readyCondition(phase, generation, now)) conditions = setCondition(conditions, existing, reconcilingCondition(phase, generation, now)) - for field, condType := range constants.OperationConditionTypes { - opStatus, ok := status[field].(map[string]interface{}) + for _, op := range constants.OperationConditionTypes { + opStatus, ok := status[op.Field].(map[string]interface{}) if !ok { continue } @@ -26,7 +27,7 @@ func DeriveConditions(phase string, status map[string]interface{}, existing []me if !ok || opPhase == "" { continue } - conditions = setCondition(conditions, existing, operationCondition(condType, opPhase, generation, now)) + conditions = setCondition(conditions, existing, operationCondition(op.CondType, opPhase, generation, now)) } return conditions @@ -53,15 +54,20 @@ func readyCondition(phase string, generation int64, now metav1.Time) metav1.Cond } func reconcilingCondition(phase string, generation int64, now metav1.Time) metav1.Condition { - terminal := phase == constants.PhaseCompleted || phase == constants.PhaseFailed status := metav1.ConditionTrue - if terminal { + reason := constants.ConditionReasonProgressing + switch phase { + case constants.PhaseCompleted: + status = metav1.ConditionFalse + reason = constants.ConditionReasonSucceeded + case constants.PhaseFailed: status = metav1.ConditionFalse + reason = constants.ConditionReasonFailed } return metav1.Condition{ Type: constants.ConditionTypeReconciling, Status: status, - Reason: constants.ConditionReasonProgressing, + Reason: reason, ObservedGeneration: generation, LastTransitionTime: now, } @@ -112,6 +118,7 @@ func conditionsToUnstructured(conditions []metav1.Condition) []interface{} { for i := range conditions { m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&conditions[i]) if err != nil { + klog.ErrorS(err, "failed to convert condition to unstructured", "type", conditions[i].Type) continue } out = append(out, m) diff --git a/pkg/controller/conditions_test.go b/pkg/controller/conditions_test.go index 623634b..c399980 100644 --- a/pkg/controller/conditions_test.go +++ b/pkg/controller/conditions_test.go @@ -143,6 +143,51 @@ func TestConditionsRoundtrip(t *testing.T) { } } +func TestDeriveConditions_ReconcilingReason(t *testing.T) { + cases := []struct { + phase string + wantReason string + }{ + {constants.PhaseCompleted, constants.ConditionReasonSucceeded}, + {constants.PhaseFailed, constants.ConditionReasonFailed}, + {constants.PhaseRunning, constants.ConditionReasonProgressing}, + {constants.PhasePending, constants.ConditionReasonProgressing}, + {constants.PhaseRetrying, constants.ConditionReasonProgressing}, + {constants.PhaseQueued, constants.ConditionReasonProgressing}, + } + for _, tc := range cases { + t.Run(tc.phase, func(t *testing.T) { + conds := DeriveConditions(tc.phase, map[string]interface{}{}, nil, 1) + reconciling := findCondition(conds, constants.ConditionTypeReconciling) + if reconciling == nil { + t.Fatal("Reconciling condition missing") + } + if reconciling.Reason != tc.wantReason { + t.Errorf("Reconciling reason: got %s, want %s", reconciling.Reason, tc.wantReason) + } + }) + } +} + +func TestDeriveConditions_DeterministicOrder(t *testing.T) { + status := map[string]interface{}{ + "buildStatus": map[string]interface{}{constants.StatusKeyState: constants.PhaseCompleted}, + "publishStatus": map[string]interface{}{constants.StatusKeyState: constants.PhaseRunning}, + "deployStatus": map[string]interface{}{constants.StatusKeyState: constants.PhaseFailed}, + } + first := DeriveConditions(constants.PhaseRunning, status, nil, 1) + second := DeriveConditions(constants.PhaseRunning, status, nil, 1) + + if len(first) != len(second) { + t.Fatalf("non-deterministic length: %d vs %d", len(first), len(second)) + } + for i := range first { + if first[i].Type != second[i].Type { + t.Errorf("position %d: first=%s second=%s — order is non-deterministic", i, first[i].Type, second[i].Type) + } + } +} + func findCondition(conditions []metav1.Condition, condType string) *metav1.Condition { for i := range conditions { if conditions[i].Type == condType {