Addressed review comments

Signed-off-by: Vishal Anarase <iamvishalanarase@gmail.com>

Author: vishalanarase

design/ironic-standalone-operator/reconciliation-events.md

@@ -3,7 +3,7 @@
 ## Summary
 
 Ironic Standalone Operator should emit Kubernetes Events on the `Ironic`
-custom resource to make reconciliation progress and failures visible to
+custom resource to make meaningful state transitions and actionable failures visible to
 cluster users (`kubectl describe ironic ...`), without flooding the cluster
 with repetitive messages.
 
@@ -12,16 +12,20 @@ This document proposes a small, stable set of **Event reasons**, their **types**
 
 ## Goals
 
-- Provide actionable feedback during reconcile (start, progress, ready).
-- Surface user-actionable failures (e.g. referenced Secret/ConfigMap missing).
-- Surface operator/actionable failures (transient reconcile failures).
-- Avoid event spam: emit primarily on transitions or user-actionable changes.
+- Surface **actionable** failures (e.g. invalid version, missing
+  referenced Secret/ConfigMap).
+- Surface **meaningful state transitions** (e.g. ready, version change,
+  operator-created credentials).
+- Avoid event spam: **no** per-reconcile "started/finished" events and **no**
+  transient operational errors (those belong in logs and
+  `status.conditions`).
 - Keep the contract stable so downstream docs/automation can rely on reasons.
 
 ## Non-goals
 
 - Emitting events on every child object create/update (too noisy).
-- Emitting events for every internal step in `pkg/ironic` (implementation detail).s
+- Emitting events for every internal step in `pkg/ironic` (implementation
+  detail).
 
 ## Background
 
@@ -33,60 +37,59 @@ because it is the primary user-facing object.
 
 ### General rules (anti-spam)
 
-- Emit "start" and "fully reconciled" events at most once per reconcile loop.
-- Emit transition events (Ready/NotReady) only when the Ready condition changes.
-- Emit "in progress" events only when the status/conditions meaningfully change
-  (e.g. `status.String()` changes), and ideally rate-limit per Generation.
-- Prefer Warning events only for user-actionable errors or hard failures.
+- Do **not** emit an event at the start or successful end of each reconcile
+  loop (`Reconciling` / `Reconciled` are intentionally omitted).
+- Do **not** emit Warning events for **transient** errors (e.g. API conflicts,
+  temporary network issues, ensure failures that will be retried); use logs
+  and conditions instead.
+- Emit Normal events for **durable, user-visible changes** (e.g. version
+  change request, new API credentials Secret created).
+- Emit events on **Ready condition transitions** (`False → True` / `True →
+  False`) with messages aligned to `status.conditions`, not on every
+  intermediate "in progress" status string.
+- Do **not** emit separate "in progress" or “error without Ready flip” events;
+  represent not-ready and error states through **one** path: the Ready
+  condition and its message (see `IronicReady` / `IronicNotReady` below).
+- Prefer Warning events only for **actionable** or **configuration**
+  problems.
 - Use stable `reason` strings; avoid embedding variable data in `reason`.
 - Include variable detail in the message (e.g. namespaced name, versions).
 
 ### Event reasons
 
 | Reason | Type | When | Message (template) |
 |---|---|---|---|
-| `Reconciling` | Normal | Beginning of reconciliation for an `Ironic` object | `Starting reconciliation` |
-| `ReconcileFailed` | Warning | Reconcile returned error (will be retried) | `Reconciliation failed: <error>` |
-| `Reconciled` | Normal | Reconcile completed with no requeue and no error | `Object has been fully reconciled` |
 | `VersionError` | Warning | Invalid/unsupported version configuration (user action required) | `Failed to process version: <error>` |
 | `DowngradeRejected` | Warning | Downgrade blocked due to external DB constraint | `Downgrade from <installed> to <requested> is not supported with external database` |
-| `VersionChange` | Normal | Requested version changed (upgrade requested) | `Version change requested from <installed> to <requested>` |
+| `VersionChange` | Normal | Requested version changed (upgrade/downgrade requested) | `Version change requested from <installed> to <requested>` |
 | `APISecretCreated` | Normal | Operator generated new API credentials Secret | `Created new API credentials secret: <name>` |
-| `APISecretError` | Warning | Ensuring API secret failed (transient) | `Failed to ensure API secret: <error>` |
 | `SecretNotFound` | Warning | User referenced a Secret that does not exist | `secret <ns>/<name> not found` |
 | `ConfigMapNotFound` | Warning | User referenced a ConfigMap that does not exist | `configmap <ns>/<name> not found` |
-| `EnsureIronicFailed` | Warning | Creating/updating Ironic child resources failed (transient) | `Failed to ensure Ironic resources: <error>` |
-| `IronicReady` | Normal | Ready condition transitions `False → True` | `Ironic deployment is now ready` |
-| `IronicNotReady` | Warning | Ready condition transitions `True → False` | `<status>` |
-| `IronicError` | Warning | Status indicates error (without necessarily flipping Ready) | `<status>` |
-| `IronicInProgress` | Normal | Status indicates progress (not ready, not error) | `<status>` |
+| `IronicReady` | Normal | Ready condition transitions to `True` | `Ironic deployment is now ready` |
+| `IronicNotReady` | Warning | Ready condition transitions to `False` (includes configuration errors and dependency failures surfaced there) | `<message from Ready condition>` |
 
 Notes:
 
-- `SecretNotFound` and `ConfigMapNotFound` are intended to be user-actionable,
-  and should be aligned with setting `IronicReasonFailed` in Conditions.
-- The `<status>` messages should match what is surfaced via Conditions (e.g.
-  `resource: <status>`), so users see consistent messaging between Events and
-  `status.conditions`.
+- `SecretNotFound` and `ConfigMapNotFound` are user-actionable and should stay
+  aligned with setting failure reasons in Conditions.
+- Any “error” or “not ready” situation that does not flip Ready in the same
+  reconcile should still be reflected in Conditions first; when Ready becomes
+  `False`, use **`IronicNotReady`** with the same message users see in
+  `status.conditions` (no separate `IronicError` reason).
 
-## Proposed follow-ups / gaps
+## Deletion, finalizers, and cleanup
 
-The contract above intentionally stays narrow. After the initial rollout, we
-can consider adding **a small number** of additional events if they prove
-valuable and non-noisy, for example:
-
-- `FinalizerAdded` / `FinalizerRemoved`: only on transition, to help explain
-  deletion behavior.
-- `CleanupStarted` / `CleanupFailed`: during finalization and resource teardown.
-
-These should be added only if users commonly debug deletion/cleanup issues and
-need explicit breadcrumbs.
+Debugging deletion, finalizers, and teardown is an **operator** concern.
+Visibility for that belongs in **controller logs**, not in user-facing Events
+on the `Ironic` CR. No additional Event reasons are proposed for cleanup
+flows.
 
 ## Implementation plan (high level)
 
 - Ensure `IronicReconciler` has an `EventRecorder` and it is initialized in
   `cmd/main.go` (controller-runtime manager recorder).
-- Record events using the reasons and conditions above.
+- Record events only for the reasons in the table above, following the general
+  rules.
 - Keep emission in the controller layer; do not sprinkle events throughout
   lower-level helpers unless necessary.
 - Add/verify RBAC permissions for `events` are present in generated RBAC.
@@ -96,9 +99,13 @@ need explicit breadcrumbs.
 - Add unit tests for event emission in the controller using a fake recorder:
    - Ready transition emits `IronicReady` once.
    - Missing Secret/ConfigMap emits `SecretNotFound` / `ConfigMapNotFound`.
-   - Transient reconcile errors emit `ReconcileFailed`.
-- Validate via e2e/functional tests (optional, if already covering these flows):
-   - Deploy an `Ironic` CR and assert at least one `IronicReady` event appears.
+   - Invalid version emits `VersionError` (or equivalent user-error path).
+- Assert that **transient** reconcile errors do **not** emit Warning Events
+  (only logs/conditions).
+- Validate via e2e/functional tests (optional, if already covering these
+  flows):
+   - Deploy an `Ironic` CR and assert a `IronicReady` event appears when Ready
+     becomes `True`.
 
 ## Compatibility and upgrade considerations