HYPERFLEET-1017 - refactor: rename aggregated Available condition to LastKnownReconciled#127
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new aggregated condition Type Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant Agg as AggregationService
participant Adapters as Adapters (status snapshots)
participant Store as StatusStore (prev conditions)
Client->>Agg: Request aggregated status
Agg->>Adapters: Read adapter status snapshots
Adapters-->>Agg: Adapter conditions (Available, ObservedGeneration, timestamps)
Agg->>Store: Read previous aggregated conditions
Store-->>Agg: Prev conditions (may include LastKnownReconciled, Reconciled, Ready)
Agg->>Agg: computeReconciled(current adapter reports)
Agg->>Agg: computeLastKnownReconciled(current reports, prev LastKnownReconciled)
Agg-->>Client: Return reconciled, lastKnownReconciled, adapterConditions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openapi/openapi.yaml (1)
1195-1204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the schema cardinality to match the new mandatory condition set.
These descriptions now say
Ready,Reconciled, andLastKnownReconciledare all mandatory, but both schemas still allowminItems: 2. That leaves the public contract inconsistent and lets generated validators accept payloads that this PR says are invalid.Suggested fix
ClusterStatus: type: object required: - conditions properties: conditions: type: array items: $ref: '#/components/schemas/ResourceCondition' - minItems: 2 + minItems: 3NodePoolStatus: type: object required: - conditions properties: conditions: type: array items: $ref: '#/components/schemas/ResourceCondition' - minItems: 2 + minItems: 3Also applies to: 1557-1570
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 1195 - 1204, The schema's conditions array currently uses "minItems: 2" but the description lists three mandatory condition types ("Ready", "Reconciled", "LastKnownReconciled"); update the schema(s) that define the cluster/status conditions array (the entries containing "minItems: 2" and the description block listing those three types) to use "minItems: 3" so the OpenAPI contract enforces all three required conditions; apply the same change to the second matching schema mentioned in the comment (the other block that currently has minItems: 2 at the later location).pkg/services/aggregation.go (1)
477-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferent false states are all reported as
AdaptersNotAtSameGeneration.
computeLastKnownReconciledStatusreturnsFalsefor missing reports and explicit adapter failures as well, but this function always emits the same false reason/message. That makes the new canonical condition misleading for dashboards and operators during the rollout, because “adapter failed” and “adapter hasn’t reported yet” are indistinguishable from a mixed-generation case.Please branch the false
reason/messageby root cause before building the condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 477 - 514, computeLastKnownReconciled currently emits the same false reason/message; change it to pick the false reason/message based on the root cause by inspecting sameGenerationAllTrue(required, byAdapter), the individual adapter states in byAdapter and whether any required adapter reports Available=False or is missing a report: if status==api.ConditionTrue keep the existing "All required..." reason/message; if any required adapter explicitly reports Available=False set reason like "AdapterReportedNotAvailable" with a message naming that root cause; else if required adapters are missing reports set reason like "AdaptersMissingReports" with an explanatory message; otherwise (mixed generations) keep "AdaptersNotAtSameGeneration". Use the existing helper calls sameGenerationAllTrue, computeLastKnownReconciledStatus and the byAdapter map to determine which branch to use and assign reason/message before returning the api.ResourceCondition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 1195-1204: The schema's conditions array currently uses "minItems:
2" but the description lists three mandatory condition types ("Ready",
"Reconciled", "LastKnownReconciled"); update the schema(s) that define the
cluster/status conditions array (the entries containing "minItems: 2" and the
description block listing those three types) to use "minItems: 3" so the OpenAPI
contract enforces all three required conditions; apply the same change to the
second matching schema mentioned in the comment (the other block that currently
has minItems: 2 at the later location).
In `@pkg/services/aggregation.go`:
- Around line 477-514: computeLastKnownReconciled currently emits the same false
reason/message; change it to pick the false reason/message based on the root
cause by inspecting sameGenerationAllTrue(required, byAdapter), the individual
adapter states in byAdapter and whether any required adapter reports
Available=False or is missing a report: if status==api.ConditionTrue keep the
existing "All required..." reason/message; if any required adapter explicitly
reports Available=False set reason like "AdapterReportedNotAvailable" with a
message naming that root cause; else if required adapters are missing reports
set reason like "AdaptersMissingReports" with an explanatory message; otherwise
(mixed generations) keep "AdaptersNotAtSameGeneration". Use the existing helper
calls sameGenerationAllTrue, computeLastKnownReconciledStatus and the byAdapter
map to determine which branch to use and assign reason/message before returning
the api.ResourceCondition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4f7f6aee-c198-432b-a2e6-76055dfae55c
📒 Files selected for processing (6)
openapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster_test.gopkg/services/node_pool_test.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
7c6c332 to
da7f311
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api-operator-guide.md (1)
147-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
Reconciledin this response example.The implementation still persists three top-level synthetic conditions —
Ready,Reconciled, andLastKnownReconciled— so this example now under-documents the actual response shape. That’s easy for operators to copy into broken jq filters or dashboards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 147 - 167, Update the example response under "status" to reflect the actual persisted synthetic conditions by adding a third condition object for "Reconciled" (matching the existing "type", "status", "observed_generation", and "last_transition_time" fields used for "Ready" and "LastKnownReconciled"); ensure the conditions array contains objects with "type": "Reconciled", a "status" value (e.g., "True"/"False"), "observed_generation": 1 and a realistic "last_transition_time" timestamp so the example matches the real API shape that includes Ready, Reconciled, and LastKnownReconciled.
🧹 Nitpick comments (2)
pkg/services/aggregation.go (1)
110-138: ⚡ Quick winRename the aggregated
availablevalue tolastKnownReconciled.This function is now the semantic boundary between adapter
Availableand aggregatedLastKnownReconciled. Keeping the old identifier here makes those two concepts easy to confuse again in callers and follow-up changes.Suggested rename
func AggregateResourceStatus(ctx context.Context, in AggregateResourceStatusInput) ( - reconciled, available api.ResourceCondition, adapterConditions []api.ResourceCondition, + reconciled, lastKnownReconciled api.ResourceCondition, adapterConditions []api.ResourceCondition, ) { prevReconciled, prevAvail, prevAdapterByType := parsePrevConditions(ctx, in.PrevConditionsJSON) reports := normalizeAdapterReportsForAggregation(ctx, in.AdapterStatuses, in.RequiredAdapters, in.ResourceGeneration) reconciled = computeReconciled( in.ResourceGeneration, in.RefTime, in.DeletedTime, prevReconciled, in.RequiredAdapters, reports, in.HasChildResources, ) - available = computeLastKnownReconciled( + lastKnownReconciled = computeLastKnownReconciled( in.RefTime, prevAvail, in.RequiredAdapters, reports, ) adapterConditions = computeAdapterConditions(in.RequiredAdapters, reports, prevAdapterByType, in.RefTime) - return reconciled, available, adapterConditions + return reconciled, lastKnownReconciled, adapterConditions }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 110 - 138, The aggregated variable named available in AggregateResourceStatus should be renamed to lastKnownReconciled to reflect that this function produces the aggregated LastKnownReconciled (distinct from adapter Available); update the local variable declaration and assignment (currently assigned from computeLastKnownReconciled) and the function's returned identifier list so the third return remains adapterConditions and the second return is lastKnownReconciled, and update any internal references and the function comment to use lastKnownReconciled instead of available (references: AggregateResourceStatus, computeLastKnownReconciled, adapterConditions).pkg/api/status_types.go (1)
31-37: Add an explicit consumer migration note.Resource-level
status.conditions.Availableis being replaced here, while adapter-levelconditions[type=Available]stays unchanged. A release note or upgrade note calling out that distinction would reduce Sentinel/dashboard regressions during rollout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/status_types.go` around lines 31 - 37, Add an explicit upgrade/migration note that resource-level status.conditions.Available (referenced by ConditionTypeAvailable in pkg/api/status_types.go) is being replaced while adapter-level conditions[type=Available] remain unchanged; update the release notes/upgrade docs to call out this distinction, mention any expected dashboards/Sentinel impacts and recommended actions (e.g., remap watchers or dashboards to the new resource-level condition name or keep using adapter-level condition where appropriate), and include an example mapping to avoid regressions during rollout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/openapi.yaml`:
- Line 53: Docs state three mandatory status conditions ("LastKnownReconciled",
"Ready", "Reconciled") but the schemas for ClusterStatus.conditions and
NodePoolStatus.conditions still use minItems: 2; update both schema definitions
to minItems: 3 and update their descriptions/examples to explicitly list the
three mandatory condition names so generated clients/validators enforce the
3-item minimum (locate the ClusterStatus.conditions and
NodePoolStatus.conditions entries in openapi.yaml and change minItems: 2 ->
minItems: 3 and adjust the human-readable description accordingly).
---
Outside diff comments:
In `@docs/api-operator-guide.md`:
- Around line 147-167: Update the example response under "status" to reflect the
actual persisted synthetic conditions by adding a third condition object for
"Reconciled" (matching the existing "type", "status", "observed_generation", and
"last_transition_time" fields used for "Ready" and "LastKnownReconciled");
ensure the conditions array contains objects with "type": "Reconciled", a
"status" value (e.g., "True"/"False"), "observed_generation": 1 and a realistic
"last_transition_time" timestamp so the example matches the real API shape that
includes Ready, Reconciled, and LastKnownReconciled.
---
Nitpick comments:
In `@pkg/api/status_types.go`:
- Around line 31-37: Add an explicit upgrade/migration note that resource-level
status.conditions.Available (referenced by ConditionTypeAvailable in
pkg/api/status_types.go) is being replaced while adapter-level
conditions[type=Available] remain unchanged; update the release notes/upgrade
docs to call out this distinction, mention any expected dashboards/Sentinel
impacts and recommended actions (e.g., remap watchers or dashboards to the new
resource-level condition name or keep using adapter-level condition where
appropriate), and include an example mapping to avoid regressions during
rollout.
In `@pkg/services/aggregation.go`:
- Around line 110-138: The aggregated variable named available in
AggregateResourceStatus should be renamed to lastKnownReconciled to reflect that
this function produces the aggregated LastKnownReconciled (distinct from adapter
Available); update the local variable declaration and assignment (currently
assigned from computeLastKnownReconciled) and the function's returned identifier
list so the third return remains adapterConditions and the second return is
lastKnownReconciled, and update any internal references and the function comment
to use lastKnownReconciled instead of available (references:
AggregateResourceStatus, computeLastKnownReconciled, adapterConditions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2baa6cf6-a79c-4b58-b84c-61a89fb7ef82
📒 Files selected for processing (9)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster_test.gopkg/services/node_pool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api-resources.md
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
da7f311 to
964de33
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/services/cluster.go (1)
225-240: ⚡ Quick winUpdate stale method comment that still references
Available.The implementation now aggregates
lastKnownReconciled(Line 225 / Line 240), but the method comment above still says “Reconciled, Available.” Please align that comment to prevent future confusion between adapter-levelAvailableand aggregatedLastKnownReconciled.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/cluster.go` around lines 225 - 240, The comment above AggregateResourceStatus is stale: it still references "Reconciled, Available" while the implementation now aggregates lastKnownReconciled and uses ready := reconciled / lastKnownReconciled; update the method comment to reflect the current outputs (e.g., "Reconciled, LastKnownReconciled" or similar), and clarify that adapter-level Available remains distinct from the aggregated lastKnownReconciled produced by AggregateResourceStatus and returned in the adapterConditions/lastKnownReconciled values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-operator-guide.md`:
- Line 332: Update the docs line for LastKnownReconciled to match runtime logic:
clarify that LastKnownReconciled=True requires adapters to report Available=True
for a common reconciled generation or that the controller's "sticky-true"
conditions have already been met; mixed-generation Available=True reports alone
do not necessarily set LastKnownReconciled=True unless the sticky-true path in
the aggregation logic (see the aggregation/availability handling in
pkg/services/aggregation.go, e.g., the sticky-true / aggregate status
evaluation) is satisfied.
In `@docs/api-resources.md`:
- Around line 56-57: Update all status examples and the status-fields note to
include the required synthesized condition "Reconciled" alongside "Ready" and
"LastKnownReconciled"; search for the examples that show only
"LastKnownReconciled" and "Ready" (occurrences near the examples around the
swapped snippets corresponding to the symbols Ready, Reconciled,
LastKnownReconciled) and add the "Reconciled" condition with appropriate "type"
and "status" entries so every example reflects the API contract's three
mandatory conditions.
---
Nitpick comments:
In `@pkg/services/cluster.go`:
- Around line 225-240: The comment above AggregateResourceStatus is stale: it
still references "Reconciled, Available" while the implementation now aggregates
lastKnownReconciled and uses ready := reconciled / lastKnownReconciled; update
the method comment to reflect the current outputs (e.g., "Reconciled,
LastKnownReconciled" or similar), and clarify that adapter-level Available
remains distinct from the aggregated lastKnownReconciled produced by
AggregateResourceStatus and returned in the
adapterConditions/lastKnownReconciled values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 25db7dd3-db39-4c52-8f37-d736014035e4
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
964de33 to
8445ee0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/services/cluster.go (1)
191-192: 💤 Low valueStale comment references old condition name.
The comment says "Reconciled, Available, and per-adapter conditions" but should reference
LastKnownReconciledto match the implementation.📝 Suggested comment update
-// UpdateClusterStatusFromAdapters recomputes aggregated Reconciled, Available, and per-adapter conditions +// UpdateClusterStatusFromAdapters recomputes aggregated Reconciled, LastKnownReconciled, and per-adapter conditions // from stored adapter rows and persists them to the cluster's status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/cluster.go` around lines 191 - 192, Update the stale comment above the UpdateClusterStatusFromAdapters function: replace the mention of "Reconciled" with the current condition name "LastKnownReconciled" so the comment reads that it recomputes aggregated LastKnownReconciled, Available, and per-adapter conditions from stored adapter rows and persists them to the cluster's status; ensure the comment text matches the implemented condition name exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/services/cluster.go`:
- Around line 191-192: Update the stale comment above the
UpdateClusterStatusFromAdapters function: replace the mention of "Reconciled"
with the current condition name "LastKnownReconciled" so the comment reads that
it recomputes aggregated LastKnownReconciled, Available, and per-adapter
conditions from stored adapter rows and persists them to the cluster's status;
ensure the comment text matches the implemented condition name exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 989a98bf-34b5-4c8d-8b81-aee2e44b2f33
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/cluster_test.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
2 times, most recently
from
d29226e to
72a1271
Compare
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
72a1271 to
f9e175b
Compare
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
225c50e to
f08a379
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/services/aggregation_test.go (1)
1207-1230:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest may not be testing the intended behavior after migration removal.
This test uses
api.ConditionTypeAvailableinprevConds, but per the past review discussion, the backward-compatibility migration that mappedAvailable→LastKnownReconciledinparsePrevConditionswas removed. The test still passes (since adapter "b" reportsFalse, resulting inavail.Status == False), but it's not actually exercising the "sticky-break" logic it claims — the prev condition won't be recognized asprevAvail.Consider updating this test to use
api.ConditionTypeLastKnownReconciledinprevCondsto properly test the sticky-break behavior.🔧 Suggested fix
t.Run("Available False at tracked gen breaks prev=True sticky", func(t *testing.T) { t.Parallel() required := []string{"a", "b"} prevConds := encodePrev( api.ResourceCondition{ - Type: api.ConditionTypeAvailable, Status: api.ConditionTrue, ObservedGeneration: 1, + Type: api.ConditionTypeLastKnownReconciled, Status: api.ConditionTrue, ObservedGeneration: 1, CreatedTime: aggT0, LastUpdatedTime: aggT0, LastTransitionTime: aggT0, }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation_test.go` around lines 1207 - 1230, The test "Available False at tracked gen breaks prev=True sticky" is constructing prevConds with api.ConditionTypeAvailable but the compatibility migration was removed, so the prev condition isn't treated as prevAvail; update the test to use api.ConditionTypeLastKnownReconciled when calling encodePrev so the prior sticky reconciliation state is recognized by AggregateResourceStatus (keep the rest of the setup the same), referencing the test helper encodePrev, the condition constant api.ConditionTypeLastKnownReconciled, and the function under test AggregateResourceStatus to ensure the test actually exercises the sticky-break logic.openapi/openapi.yaml (1)
1262-1276:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
minItems: 3for ClusterStatus and NodePoolStatusconditionsarrays.Both ClusterStatus (line 1266) and NodePoolStatus (line 1632) document three mandatory conditions (
Ready,Reconciled,LastKnownReconciled) but validate withminItems: 2. Update both tominItems: 3to enforce the documented contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 1262 - 1276, The ClusterStatus and NodePoolStatus schema definitions declare three mandatory conditions but currently set conditions.minItems to 2; update the schemas named ClusterStatus and NodePoolStatus by changing the conditions array property minItems from 2 to 3 so the OpenAPI contract enforces the documented three mandatory ResourceCondition entries (the property to update is conditions under each of the ClusterStatus and NodePoolStatus components).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 1262-1276: The ClusterStatus and NodePoolStatus schema definitions
declare three mandatory conditions but currently set conditions.minItems to 2;
update the schemas named ClusterStatus and NodePoolStatus by changing the
conditions array property minItems from 2 to 3 so the OpenAPI contract enforces
the documented three mandatory ResourceCondition entries (the property to update
is conditions under each of the ClusterStatus and NodePoolStatus components).
In `@pkg/services/aggregation_test.go`:
- Around line 1207-1230: The test "Available False at tracked gen breaks
prev=True sticky" is constructing prevConds with api.ConditionTypeAvailable but
the compatibility migration was removed, so the prev condition isn't treated as
prevAvail; update the test to use api.ConditionTypeLastKnownReconciled when
calling encodePrev so the prior sticky reconciliation state is recognized by
AggregateResourceStatus (keep the rest of the setup the same), referencing the
test helper encodePrev, the condition constant
api.ConditionTypeLastKnownReconciled, and the function under test
AggregateResourceStatus to ensure the test actually exercises the sticky-break
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 56668f77-5dad-493a-aae9-716c73edd894
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
|
Thanks for catching these!
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/aggregation_test.go (1)
89-101: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider renaming
mkPrevAvailto match its actual behavior.The helper function name
mkPrevAvailsuggests it creates an "Available" condition, but it now creates aLastKnownReconciledcondition (line 94). Consider renaming tomkPrevLastKnownReconciledormkPrevLKRfor consistency.♻️ Suggested rename
-// mkPrevAvail builds a LastKnownReconciled ResourceCondition for use as a prev fixture. -func mkPrevAvail( +// mkPrevLastKnownReconciled builds a LastKnownReconciled ResourceCondition for use as a prev fixture. +func mkPrevLastKnownReconciled( status api.ResourceConditionStatus, obsGen int32, lastTransition, lastUpdated time.Time, ) *api.ResourceCondition {Then update all call sites accordingly (lines 730, 759, 792, 889, 920, 935, 979, 993, 1004).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation_test.go` around lines 89 - 101, Rename the helper mkPrevAvail to a name that reflects it builds a LastKnownReconciled condition (suggested mkPrevLastKnownReconciled or mkPrevLKR) and update all call sites that reference mkPrevAvail (occurrences near the tests around lines with uses at roughly the blocks referenced: callers at the locations noted in the review) to use the new name; ensure the function signature and returned api.ResourceCondition remain unchanged so tests compile, and update any comments/docstrings to match the new name (function identifier: mkPrevAvail -> mkPrevLastKnownReconciled or mkPrevLKR).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/services/aggregation_test.go`:
- Around line 89-101: Rename the helper mkPrevAvail to a name that reflects it
builds a LastKnownReconciled condition (suggested mkPrevLastKnownReconciled or
mkPrevLKR) and update all call sites that reference mkPrevAvail (occurrences
near the tests around lines with uses at roughly the blocks referenced: callers
at the locations noted in the review) to use the new name; ensure the function
signature and returned api.ResourceCondition remain unchanged so tests compile,
and update any comments/docstrings to match the new name (function identifier:
mkPrevAvail -> mkPrevLastKnownReconciled or mkPrevLKR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 37fe71fe-2273-4a49-896b-93e2ba736815
📒 Files selected for processing (1)
pkg/services/aggregation_test.go
|
/retest |
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
90028f8 to
d25fdfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
openapi/openapi.yaml (1)
1262-1275:⚠️ Potential issue | 🟠 Major
conditionsstill permits fewer entries than the documented mandatory set.
ClusterStatus.conditionsandNodePoolStatus.conditionsstill useminItems: 2even though these same sections now sayReady,Reconciled, andLastKnownReconciledare always present. Generated clients/validators will accept payloads that violate the documented contract. Since this file is synced, the fix likely needs to happen inhyperfleet-api-specbefore the next regeneration.As per coding guidelines, "Cluster/NodePool status mandatory conditions in the schema/descriptions include: Ready (deprecated), Reconciled, LastKnownReconciled" and "Condition array sizing constraint in the generated schema exists (see minItems on resource condition arrays)."
Also applies to: 1628-1641
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 1262 - 1275, The conditions arrays for ClusterStatus and NodePoolStatus currently have minItems: 2 but the description mandates three mandatory conditions (Ready, Reconciled, LastKnownReconciled); update the source spec (hyperfleet-api-spec) so the schema for ClusterStatus.conditions and NodePoolStatus.conditions sets minItems: 3 (and regenerate the OpenAPI file), ensuring the ResourceCondition array constraint matches the documented mandatory set and that the descriptions remain consistent with the schema.pkg/services/aggregation.go (1)
147-177:⚠️ Potential issue | 🟠 MajorKeep the legacy aggregated
Availablefallback until old rows are rewritten.
parsePrevConditionsno longer recognizes the pre-rename aggregateAvailable, so the first recompute after rollout loses the previous sticky state for existing resources. A resource persisted asAvailable=Truecan now incorrectly dropLastKnownReconciledtoFalseduring mixed-generation transitions until all adapters report again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 147 - 177, parsePrevConditions currently ignores the legacy aggregate name "Available", causing old rows with ConditionTypeAvailable to be dropped; update the switch in parsePrevConditions to treat api.ConditionTypeAvailable the same as api.ConditionTypeLastKnownReconciled (set prevAvail = &c) so the legacy Available condition is preserved as the LastKnownReconciled fallback during mixed-generation transitions, leaving existing logic for Reconciled/Ready/other adapter types unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-operator-guide.md`:
- Line 111: Update the aggregated-condition docs paragraph to include the
missing "Reconciled" condition and mark "Ready" as its deprecated alias:
explicitly list the three mandatory aggregated conditions as Reconciled,
LastKnownReconciled, and Ready; add a short sentence that Reconciled is the
primary/current aggregated condition, and Ready remains a deprecated alias tied
to current-generation adapter reports; ensure the same change is applied to the
other referenced section (lines around 332-355) so examples and the public
status contract consistently show Reconciled alongside LastKnownReconciled and
Ready.
In `@docs/api-resources.md`:
- Around line 486-488: The docs incorrectly state that
ResourceCondition.last_updated_time is taken from AdapterStatus.last_report_time
for aggregated conditions; update the ResourceCondition section to document the
synthesized last_updated_time semantics used for aggregated conditions: explain
that for aggregated conditions (e.g., Reconciled and LastKnownReconciled)
last_updated_time is computed as the oldest valid adapter report time within the
relevant generation bucket (not the latest AdapterStatus.last_report_time),
describe how adapters’ last_report_time is filtered by generation and validity
before selecting the minimum timestamp, and reference the condition names
Reconciled, LastKnownReconciled, ResourceCondition.last_updated_time, and
AdapterStatus.last_report_time so readers can locate the behavior exactly.
In `@openapi/openapi.yaml`:
- Around line 527-563: Update the external docs to reflect the new PUT /statuses
contract: locate the operationId putNodePoolStatuses in the OpenAPI (the PUT
path that accepts AdapterStatusCreateRequest and returns AdapterStatus on 201)
and update the linked architecture/adapter docs
(openshift-hyperfleet/architecture and openshift-hyperfleet/hyperfleet-adapter)
to describe this PUT behavior (purpose, request/response schema, status codes
201/400/404/409, and BearerAuth requirement) so adapter authors are aware of the
new public surface; ensure the same changes are applied for the other PUT
occurrences noted (lines ~638-668).
In `@pkg/services/cluster_test.go`:
- Around line 1049-1050: The test fixture seeds legacy ConditionTypeAvailable
but the code now checks for api.ConditionTypeLastKnownReconciled, so update the
test's initialConditions to use api.ConditionTypeLastKnownReconciled (replace
any ConditionTypeAvailable entries) so parsePrevConditions sees an existing LKR
entry; specifically modify the initialConditions array(s) referenced by the test
and any createdConds setup (where createdAvailable is assigned) so they use
ConditionTypeLastKnownReconciled, ensuring the timestamp carry-forward code in
parsePrevConditions is exercised and the comparison between refTime and fixedNow
remains valid.
---
Duplicate comments:
In `@openapi/openapi.yaml`:
- Around line 1262-1275: The conditions arrays for ClusterStatus and
NodePoolStatus currently have minItems: 2 but the description mandates three
mandatory conditions (Ready, Reconciled, LastKnownReconciled); update the source
spec (hyperfleet-api-spec) so the schema for ClusterStatus.conditions and
NodePoolStatus.conditions sets minItems: 3 (and regenerate the OpenAPI file),
ensuring the ResourceCondition array constraint matches the documented mandatory
set and that the descriptions remain consistent with the schema.
In `@pkg/services/aggregation.go`:
- Around line 147-177: parsePrevConditions currently ignores the legacy
aggregate name "Available", causing old rows with ConditionTypeAvailable to be
dropped; update the switch in parsePrevConditions to treat
api.ConditionTypeAvailable the same as api.ConditionTypeLastKnownReconciled (set
prevAvail = &c) so the legacy Available condition is preserved as the
LastKnownReconciled fallback during mixed-generation transitions, leaving
existing logic for Reconciled/Ready/other adapter types unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ca2b6062-24df-41e1-8118-70ec9b4d77fc
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
| put: | ||
| operationId: putNodePoolStatuses | ||
| summary: Adapter creates or updates resource nodepool status | ||
| parameters: | ||
| - name: cluster_id | ||
| in: path | ||
| required: true | ||
| description: Cluster ID | ||
| schema: | ||
| type: string | ||
| - name: nodepool_id | ||
| in: path | ||
| required: true | ||
| description: Nodepool ID | ||
| schema: | ||
| type: string | ||
| responses: | ||
| '201': | ||
| description: The request has succeeded and a new resource has been created as a result. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatus' | ||
| '400': | ||
| description: The server could not understand the request due to invalid syntax. | ||
| '404': | ||
| description: The server cannot find the requested resource. | ||
| '409': | ||
| description: The request conflicts with the current state of the server. | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatusCreateRequest' | ||
| security: | ||
| - BearerAuth: [] |
There was a problem hiding this comment.
@rafabene, understood — thanks for the context. Since the PUT /statuses endpoints were introduced in a prior PR and the architecture/adapter doc updates are tracked separately, this comment no longer applies here. I'll note this for future reference.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
d25fdfd to
8e05139
Compare
| put: | ||
| operationId: putNodePoolStatuses | ||
| summary: Adapter creates or updates resource nodepool status | ||
| parameters: | ||
| - name: cluster_id | ||
| in: path | ||
| required: true | ||
| description: Cluster ID | ||
| schema: | ||
| type: string | ||
| - name: nodepool_id | ||
| in: path | ||
| required: true | ||
| description: Nodepool ID | ||
| schema: | ||
| type: string | ||
| responses: | ||
| '201': | ||
| description: The request has succeeded and a new resource has been created as a result. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatus' | ||
| '400': | ||
| description: The server could not understand the request due to invalid syntax. | ||
| '404': | ||
| description: The server cannot find the requested resource. | ||
| '409': | ||
| description: The request conflicts with the current state of the server. | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatusCreateRequest' | ||
| security: | ||
| - BearerAuth: [] |
There was a problem hiding this comment.
@rafabene, understood — thanks for the context. Since the PUT /statuses endpoints were introduced in a prior PR and the architecture/adapter doc updates are tracked separately, this comment no longer applies here. I'll note this for future reference.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/api-resources.md (1)
9-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the new
PUTstatus endpoints or mark them internal-only.This reference still lists only
GET/POST, but the PR addsPUT /clusters/{cluster_id}/statusesandPUT /clusters/{cluster_id}/nodepools/{nodepool_id}/statusesin OpenAPI. Leaving the endpoint tables unchanged makes the public docs drift from the shipped contract and doesn't clarify the POST-vs-PUT split for clients.As per coding guidelines, "Validate changes against HyperFleet architecture standards from the linked architecture repository."
Also applies to: 274-281
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-resources.md` around lines 9 - 15, Update the API reference table so it matches the OpenAPI changes by adding the new PUT endpoints or marking them internal-only: include "PUT /api/hyperfleet/v1/clusters/{cluster_id}/statuses" and "PUT /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses" (or explicitly annotate them as internal-only) and clarify the POST vs PUT semantics for statuses; ensure the same edits are applied to the duplicate section noted (lines 274-281) and reference the HyperFleet architecture validation guidelines when deciding visibility/semantics.pkg/services/cluster.go (1)
266-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the legacy
Availablefallback during the upgrade window.
PrevConditionsJSONstill comes from persisted status rows. IfparsePrevConditionsno longer maps the legacy aggregatedAvailableentry toLastKnownReconciled, pre-upgrade resources lose their previous LKR state on the first recompute. That breaks the documented sticky-true/timestamp carry-forward semantics and is externally visible to consumers like Sentinel. Please keep the fallback until stored rows are migrated/backfilled.As per coding guidelines, "LastKnownReconciled is the stateful cross-generation “best effort” condition computed with sticky-true behavior during generation transitions; do not treat “Available” as the aggregated condition type."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/cluster.go` around lines 266 - 274, PrevConditionsJSON currently may contain legacy aggregated "Available" entries and must still map that legacy type to LastKnownReconciled to preserve sticky-true/timestamp carry-forward; update the parsing path (e.g., parsePrevConditions or wherever PrevConditionsJSON is decoded before calling AggregateResourceStatus) to detect legacy "Available" aggregated condition and populate/translate it into a LastKnownReconciled condition entry so AggregateResourceStatus receives the legacy fallback; ensure the translation preserves the original timestamp and truthiness and keep this fallback in place until stored rows are migrated/backfilled.pkg/services/aggregation.go (1)
642-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fall back to
refTimewhen the tracked generation already has reports.For
LastKnownReconciled=False, this helper only uses adapter timestamps when an adapter is explicitlyAvailable=FalseatobservedGen. If the tracked generation has reports but they are allAvailable=True(mixed-generation case) or some adapters are simply missing, it falls back torefTime, which makeslast_updated_timenewer than the actual evidence.Proposed fix
if status == api.ConditionFalse { - x := observedGen - hasFalseAtX := false - for _, name := range required { - s, ok := byAdapter[name] - if !ok { - continue - } - if s.observedGeneration == x && !s.availableTrue { - hasFalseAtX = true - break - } - } - if hasFalseAtX { - var atX []time.Time - for _, name := range required { - s, ok := byAdapter[name] - if !ok { - continue - } - if s.observedGeneration == x { - atX = append(atX, s.observedTime) - } - } - if len(atX) > 0 { - return minTime(atX) - } - } + atX := make([]time.Time, 0, len(required)) + for _, name := range required { + s, ok := byAdapter[name] + if !ok || s.observedGeneration != observedGen { + continue + } + atX = append(atX, s.observedTime) + } + if len(atX) > 0 { + return minTime(atX) + } }As per coding guidelines, "docs/api-operator-guide.md": "
last_updated_timefor aggregated conditions uses the oldest valid adapter report time for the relevant generation bucket (and for LKR, it’s kept unchanged in some mixed-generation cases)..."
♻️ Duplicate comments (1)
docs/api-operator-guide.md (1)
404-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCarry
Reconciledthrough the remaining operator-guide sections.These sections still document only
ReadyandLastKnownReconciled. SinceReconciledis now a mandatory synthesized condition andReadyis only its deprecated alias, the example payload and creation-time rules are still incomplete and can mislead operators about which condition is authoritative.As per coding guidelines, "Status aggregation model: resources expose synthetic aggregated conditions Reconciled, LastKnownReconciled, and deprecated alias Ready" and "
ClusterStatus.conditionsandNodePoolStatus.conditionsdefine mandatory aggregated conditions present on creation:Ready,Reconciled, andLastKnownReconciled."Also applies to: 509-510
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 404 - 407, Documentation currently shows only Ready and LastKnownReconciled in examples and creation rules; update the remaining operator-guide sections to include the synthesized Reconciled condition as mandatory on creation. Modify the example payloads and any textual rules (where Ready and LastKnownReconciled are mentioned) to list Reconciled, LastKnownReconciled and the deprecated alias Ready as the aggregated conditions, and update ClusterStatus.conditions and NodePoolStatus.conditions documentation to state that Reconciled is mandatory (with Ready marked as deprecated) so the authoritative synthesized condition is clear. Ensure occurrences referenced around the given snippet and at the additional lines noted (509-510) are changed consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/openapi.yaml`:
- Around line 192-195: Replace the prose reason/message in the
LastKnownReconciled example with the exact literals returned by
computeLastKnownReconciled(): set reason to "AllAdaptersReconciled" and message
to "All required adapters report Available=True for the tracked generation";
update the same pattern in the other example blocks (the similar
LastKnownReconciled examples present elsewhere in the file) so the OpenAPI
example payloads match the service’s aggregated condition output.
- Around line 527-563: The OpenAPI spec adds PUT operations putClusterStatuses
and putNodePoolStatuses but there are no corresponding handlers or router
registrations; either remove these PUT entries from openapi.yaml or implement
them by adding Put/Update handler methods to ClusterStatusHandler and
NodePoolStatusHandler (implementing the AdapterStatusCreateRequest ->
AdapterStatus flow and appropriate status codes), and register the routes in the
HTTP router in plugins/clusters/plugin.go (add .Methods(http.MethodPut) for the
cluster and nodepool paths) so generated clients have working endpoints.
---
Outside diff comments:
In `@docs/api-resources.md`:
- Around line 9-15: Update the API reference table so it matches the OpenAPI
changes by adding the new PUT endpoints or marking them internal-only: include
"PUT /api/hyperfleet/v1/clusters/{cluster_id}/statuses" and "PUT
/api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses" (or
explicitly annotate them as internal-only) and clarify the POST vs PUT semantics
for statuses; ensure the same edits are applied to the duplicate section noted
(lines 274-281) and reference the HyperFleet architecture validation guidelines
when deciding visibility/semantics.
In `@pkg/services/cluster.go`:
- Around line 266-274: PrevConditionsJSON currently may contain legacy
aggregated "Available" entries and must still map that legacy type to
LastKnownReconciled to preserve sticky-true/timestamp carry-forward; update the
parsing path (e.g., parsePrevConditions or wherever PrevConditionsJSON is
decoded before calling AggregateResourceStatus) to detect legacy "Available"
aggregated condition and populate/translate it into a LastKnownReconciled
condition entry so AggregateResourceStatus receives the legacy fallback; ensure
the translation preserves the original timestamp and truthiness and keep this
fallback in place until stored rows are migrated/backfilled.
---
Duplicate comments:
In `@docs/api-operator-guide.md`:
- Around line 404-407: Documentation currently shows only Ready and
LastKnownReconciled in examples and creation rules; update the remaining
operator-guide sections to include the synthesized Reconciled condition as
mandatory on creation. Modify the example payloads and any textual rules (where
Ready and LastKnownReconciled are mentioned) to list Reconciled,
LastKnownReconciled and the deprecated alias Ready as the aggregated conditions,
and update ClusterStatus.conditions and NodePoolStatus.conditions documentation
to state that Reconciled is mandatory (with Ready marked as deprecated) so the
authoritative synthesized condition is clear. Ensure occurrences referenced
around the given snippet and at the additional lines noted (509-510) are changed
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c892a6ad-8d30-421b-85c6-42d016b1fedc
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
8e05139 to
b65b8ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/api-resources.md (1)
56-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync these example reason/message values with the generated conditions.
The examples still show synthesized values the service no longer emits.
Reconciledand deprecatedReadyare produced from the same struct, soReadyshould mirror theReconciledreason/message, and the initialLastKnownReconciled=Falsecase is now driven by the missing-reports path rather thanAwaitingAdapters. As written, consumers copying these examples will key off nonexistent codes likeAwaitingAdaptersandResourceReady.Also applies to: 121-148, 327-354, 394-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-resources.md` around lines 56 - 83, The example condition blocks use outdated synthesized reason/message values (e.g., "AwaitingAdapters" and "ResourceReady") that no longer match the service's emitted conditions; update the example entries for the condition types "Reconciled", "LastKnownReconciled", and deprecated "Ready" so their reason/message fields mirror the actual generated values (make "Ready" mirror "Reconciled"'s reason/message and change the initial LastKnownReconciled=False example to reflect the missing-reports path message/code), ensuring all occurrences of those example blocks use the current emitted reason/message strings rather than the old "AwaitingAdapters"/"ResourceReady" values.pkg/services/aggregation_test.go (1)
147-176: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd an explicit regression case for legacy
Available.These updates prove that
LastKnownReconciledis parsed correctly, but they no longer pin the other half of the contract:parsePrevConditionsshould not treat legacyConditionTypeAvailableas the synthetic aggregated condition anymore. A small negative case here would lock in the intentional migration removal from this PR.Suggested test addition
t.Run("only LastKnownReconciled", func(t *testing.T) { t.Parallel() r, a, _ := parsePrevConditions(context.Background(), encode(availCond)) if r != nil { t.Fatalf("expected nil Reconciled, got %v", r) } if a == nil || a.Type != api.ConditionTypeLastKnownReconciled { t.Fatalf("expected LastKnownReconciled, got %v", a) } }) + + t.Run("legacy Available is not treated as LastKnownReconciled", func(t *testing.T) { + t.Parallel() + legacy := api.ResourceCondition{Type: api.ConditionTypeAvailable, Status: api.ConditionTrue} + r, a, _ := parsePrevConditions(context.Background(), encode(legacy)) + if r != nil || a != nil { + t.Fatalf("expected legacy Available to be ignored as a synthetic condition, got (%v,%v)", r, a) + } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation_test.go` around lines 147 - 176, Add a negative regression test to ensure parsePrevConditions no longer treats legacy ConditionTypeAvailable as the synthetic LastKnownReconciled: create a test (e.g., "legacy Available not treated as LastKnownReconciled") that calls parsePrevConditions with encode(legacyAvailableCond) where legacyAvailableCond.Type == api.ConditionTypeAvailable and assert that both returned r (Reconciled) and a (LastKnownReconciled) are nil (or at least that a.Type != api.ConditionTypeLastKnownReconciled); place it alongside the existing tests in pkg/services/aggregation_test.go so the contract that legacy Available is not mapped to LastKnownReconciled is locked in.
♻️ Duplicate comments (1)
pkg/services/aggregation.go (1)
159-174:⚠️ Potential issue | 🟠 MajorRestore legacy
Availableparsing here during rollout.
status_conditionsare persisted JSON. Existing rows written before this rename can still carry the old aggregatedtype=="Available", and this switch now sends that entry throughdefault, soprevAvailis lost on the first recompute after upgrade. That breaks the sticky-true path incomputeLastKnownReconciledand can incorrectly flip a previously reconciled resource toFalseuntil a fresh full-generation quorum arrives.Suggested fix
for i := range conditions { c := conditions[i] switch c.Type { case api.ConditionTypeReady: if prevReconciled == nil { prevReconciled = &c } case api.ConditionTypeReconciled: prevReconciled = &c + case api.ConditionTypeAvailable: + if prevAvail == nil { + prevAvail = &c + } case api.ConditionTypeLastKnownReconciled: prevAvail = &c default: prevAdapterByType[c.Type] = &c } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 159 - 174, The loop over conditions currently maps only ConditionTypeLastKnownReconciled into prevAvail and treats legacy "Available" as default; restore legacy parsing by adding a case for api.ConditionTypeAvailable that sets prevAvail = &c (just like api.ConditionTypeLastKnownReconciled), keeping the existing logic that Reconciled (api.ConditionTypeReconciled) takes precedence and Ready is used only if Reconciled is absent; update the switch in the for i := range conditions loop (where prevReconciled and prevAvail are assigned) to handle api.ConditionTypeAvailable explicitly so old rows keep their prevAvail value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-operator-guide.md`:
- Around line 111-112: The lifecycle example still shows an empty status
(`status: {conditions: []}`) after Create even though the API eagerly
synthesizes `Reconciled`, `LastKnownReconciled`, and `Ready` conditions on
create; update the example immediately below this paragraph to show the
synthesized conditions produced at creation (include `Reconciled`,
`LastKnownReconciled`, and `Ready` with realistic initial values) instead of an
empty `conditions` array so the docs reflect the actual `Create` behavior.
In `@pkg/services/cluster_test.go`:
- Around line 1442-1456: The test must also assert the ObservedGeneration on the
sticky LastKnownReconciled condition to ensure it remains anchored to the prior
reconciled generation; update the block that finds
api.ConditionTypeLastKnownReconciled (variables: conds, reconciled,
lastKnownReconciled) to add an assertion that
lastKnownReconciled.ObservedGeneration equals reconciled.ObservedGeneration (or
the expected generation value, e.g. 1) after the delete bump so the test fails
if ObservedGeneration was incorrectly advanced.
---
Outside diff comments:
In `@docs/api-resources.md`:
- Around line 56-83: The example condition blocks use outdated synthesized
reason/message values (e.g., "AwaitingAdapters" and "ResourceReady") that no
longer match the service's emitted conditions; update the example entries for
the condition types "Reconciled", "LastKnownReconciled", and deprecated "Ready"
so their reason/message fields mirror the actual generated values (make "Ready"
mirror "Reconciled"'s reason/message and change the initial
LastKnownReconciled=False example to reflect the missing-reports path
message/code), ensuring all occurrences of those example blocks use the current
emitted reason/message strings rather than the old
"AwaitingAdapters"/"ResourceReady" values.
In `@pkg/services/aggregation_test.go`:
- Around line 147-176: Add a negative regression test to ensure
parsePrevConditions no longer treats legacy ConditionTypeAvailable as the
synthetic LastKnownReconciled: create a test (e.g., "legacy Available not
treated as LastKnownReconciled") that calls parsePrevConditions with
encode(legacyAvailableCond) where legacyAvailableCond.Type ==
api.ConditionTypeAvailable and assert that both returned r (Reconciled) and a
(LastKnownReconciled) are nil (or at least that a.Type !=
api.ConditionTypeLastKnownReconciled); place it alongside the existing tests in
pkg/services/aggregation_test.go so the contract that legacy Available is not
mapped to LastKnownReconciled is locked in.
---
Duplicate comments:
In `@pkg/services/aggregation.go`:
- Around line 159-174: The loop over conditions currently maps only
ConditionTypeLastKnownReconciled into prevAvail and treats legacy "Available" as
default; restore legacy parsing by adding a case for api.ConditionTypeAvailable
that sets prevAvail = &c (just like api.ConditionTypeLastKnownReconciled),
keeping the existing logic that Reconciled (api.ConditionTypeReconciled) takes
precedence and Ready is used only if Reconciled is absent; update the switch in
the for i := range conditions loop (where prevReconciled and prevAvail are
assigned) to handle api.ConditionTypeAvailable explicitly so old rows keep their
prevAvail value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9f2cd226-c6f8-43e9-9bb8-84f4a1efb5bd
📒 Files selected for processing (10)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
b65b8ea to
2d47acc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/api-operator-guide.md (1)
129-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLifecycle example still shows empty conditions after Create.
The narrative at line 111 states that Create yields synthesized conditions, but the example at line 131 shows
status: {conditions: []}. This contradicts the actual API behavior where conditions are computed on create. This was flagged in a past review but appears to still be inconsistent.📝 Suggested fix
Update line 131 to show the initial synthesized conditions:
→ API stores: - generation: 1 (initial) - - status: {conditions: []} (empty, no adapter reports yet) + - status: {conditions: [{type: "Reconciled", status: "False", ...}, {type: "LastKnownReconciled", status: "False", ...}, {type: "Ready", status: "False", ...}]}Or simplify to:
- - status: {conditions: []} (empty, no adapter reports yet) + - status: {conditions: [Reconciled=False, LastKnownReconciled=False, Ready=False]} (synthesized, awaiting adapter reports)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 129 - 131, The example under "API stores" incorrectly shows an empty conditions array after Create; update the example so the initial synthesized conditions are present (e.g., replace `status: {conditions: []}` with a realistic initial conditions block such as a Ready condition set to False/Unknown with observedGeneration and a reason like "Initializing" or simplify to a note indicating synthesized conditions are present). Locate the example string `→ API stores: - generation: 1 (initial) - status: {conditions: []}` and change the `status` value to reflect the actual synthesized conditions produced on Create (or an explicit comment that conditions are synthesized).pkg/services/aggregation_test.go (1)
112-217: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd one negative case for legacy
Availableparsing.This block now proves the positive
LastKnownReconciledpaths, but it no longer locks in the intentional removal ofAvailable -> LastKnownReconciledmigration. A single negative test here would prevent that compatibility path from being reintroduced silently later.Suggested test case
+ t.Run("legacy Available is not promoted to LastKnownReconciled", func(t *testing.T) { + t.Parallel() + legacy := api.ResourceCondition{Type: api.ConditionTypeAvailable, Status: api.ConditionTrue} + r, a, m := parsePrevConditions(context.Background(), encode(legacy)) + if r != nil || a != nil { + t.Fatalf("expected legacy Available to stay out of synthesized slots, got (%v,%v)", r, a) + } + if _, ok := m[api.ConditionTypeAvailable]; !ok { + t.Fatalf("expected legacy Available to remain non-synthesized, got %v", m) + } + })As per coding guidelines, use the aggregated condition set
{Reconciled, LastKnownReconciled, Ready(deprecated alias of Reconciled)}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation_test.go` around lines 112 - 217, Add a negative test in TestParsePrevConditions to assert that the legacy "Available" condition does NOT get migrated to LastKnownReconciled: call parsePrevConditions with an encoded api.ResourceCondition{Type: api.ConditionTypeAvailable, Status: ...} (alongside/within the aggregated condition set {Reconciled, LastKnownReconciled, Ready} as appropriate) and assert that the returned "a" (prevAvailable/LastKnownReconciled) is nil and that no LastKnownReconciled entry exists in the adapter map; place this new subtest near the other single-condition cases and reference parsePrevConditions and api.ConditionTypeAvailable in the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/api-operator-guide.md`:
- Around line 129-131: The example under "API stores" incorrectly shows an empty
conditions array after Create; update the example so the initial synthesized
conditions are present (e.g., replace `status: {conditions: []}` with a
realistic initial conditions block such as a Ready condition set to
False/Unknown with observedGeneration and a reason like "Initializing" or
simplify to a note indicating synthesized conditions are present). Locate the
example string `→ API stores: - generation: 1 (initial) - status: {conditions:
[]}` and change the `status` value to reflect the actual synthesized conditions
produced on Create (or an explicit comment that conditions are synthesized).
In `@pkg/services/aggregation_test.go`:
- Around line 112-217: Add a negative test in TestParsePrevConditions to assert
that the legacy "Available" condition does NOT get migrated to
LastKnownReconciled: call parsePrevConditions with an encoded
api.ResourceCondition{Type: api.ConditionTypeAvailable, Status: ...}
(alongside/within the aggregated condition set {Reconciled, LastKnownReconciled,
Ready} as appropriate) and assert that the returned "a"
(prevAvailable/LastKnownReconciled) is nil and that no LastKnownReconciled entry
exists in the adapter map; place this new subtest near the other
single-condition cases and reference parsePrevConditions and
api.ConditionTypeAvailable in the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 93fa795e-e5e3-4745-81b7-924e908a0c0d
📒 Files selected for processing (10)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
2d47acc to
c5a171d
Compare
rafabene
changed the title
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
c5a171d to
0ed7961
Compare
|
Depends on: openshift-hyperfleet/hyperfleet-api-spec#43 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/cluster_test.go (1)
779-801: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert
Reconciledin this transition test too.
getSynth()now findsapi.ConditionTypeReconciledon Line 794, but it still returns onlyLastKnownReconciledandReady. That leaves the primary current-generation condition untested here, so this case would still pass ifReadystayed correct whileReconciledregressed.As per coding guidelines,
docs/api-operator-guide.md:Use aggregated conditions Reconciled, LastKnownReconciled (stateful), and Ready (deprecated alias of Reconciled) in status.conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/cluster_test.go` around lines 779 - 801, The test helper getSynth currently finds api.ConditionTypeReconciled but only returns LastKnownReconciled and Ready, leaving Reconciled unverified; update getSynth to assert reconciled is non-nil (it already sets reconciled) and include reconciled in the returned values (change signature/returns of getSynth and all call sites to accept/handle the third api.ResourceCondition) so the test verifies the primary current-generation condition (api.ConditionTypeReconciled) alongside LastKnownReconciled and Ready.
♻️ Duplicate comments (3)
docs/api-operator-guide.md (1)
404-407:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Reconciledis still under-documented in this guide.The resource example still only shows
Ready+LastKnownReconciled, and these rules sections describe timing/observed_generationonly forReadyandLastKnownReconciled. SinceReconciledis now the primary aggregated condition andReadyis its deprecated alias, this leaves the operator guide internally inconsistent.As per coding guidelines,
status.conditionsshould use the aggregated conditions “Reconciled,LastKnownReconciled, andReady(deprecated alias ofReconciled)”.Also applies to: 509-510, 521-526
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 404 - 407, Update the guide to include the aggregated condition "Reconciled" everywhere the doc currently only shows "Ready" + "LastKnownReconciled": modify the resource example to list status.conditions as Reconciled, LastKnownReconciled, and Ready (marked deprecated) and add a sample Reconciled entry with status/reason/message; then update the rules/timing sections (the paragraphs that currently describe timing/observed_generation for Ready and LastKnownReconciled) to instead (or additionally) describe the behavior for Reconciled—how it aggregates adapter Availability and observedGeneration—while noting Ready remains a deprecated alias of Reconciled so readers see the full canonical set: Reconciled, LastKnownReconciled, Ready.pkg/services/aggregation.go (1)
169-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the legacy aggregated
Availablevalue during rollout.Line 171 only restores previous sticky state from
LastKnownReconciled. Existingstatus_conditionsrows can still contain the old top-level"Available"entry, so the first post-upgrade aggregation can lose the prior sticky-true state and incorrectly flipLastKnownReconciledtoFalseuntil another adapter report arrives.Suggested fix
- case api.ConditionTypeLastKnownReconciled: + case api.ConditionTypeLastKnownReconciled, api.ConditionTypeAvailable: prevAvail = &c🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 169 - 172, The switch in aggregation.go only restores prior sticky state from api.ConditionTypeLastKnownReconciled, which misses legacy top-level "Available" rows; update the switch handling (in the function that declares prevReconciled and prevAvail) to also set prevAvail when encountering api.ConditionTypeAvailable (i.e., add a case api.ConditionTypeAvailable: prevAvail = &c) so prior "Available" values are preserved during rollout in addition to api.ConditionTypeLastKnownReconciled; keep the existing case api.ConditionTypeReconciled that sets prevReconciled unchanged.openapi/openapi.yaml (1)
1312-1315:⚠️ Potential issue | 🟠 MajorSchema still under-enforces the three mandatory status conditions.
These descriptions now require
Ready,Reconciled, andLastKnownReconciled, but Line 1308 and Line 1674 still keep both arrays atminItems: 2. Generated clients and validators can therefore accept responses that the contract here declares invalid.Suggested spec fix
ClusterStatus: @@ conditions: @@ - minItems: 2 + minItems: 3 @@ NodePoolStatus: @@ conditions: @@ - minItems: 2 + minItems: 3As per coding guidelines,
docs/api-resources.md:On create, status.conditions initially includes Reconciled=False, LastKnownReconciled=False, and Ready=False until adapters report.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 1312 - 1315, The OpenAPI schema currently allows only two items for status.conditions in two places, so update both status.conditions array schemas to minItems: 3 (replace minItems: 2 with minItems: 3) and ensure the item schema enforces the three mandatory condition types by including an enum or pattern restricting the condition.type values to "Ready", "Reconciled", and "LastKnownReconciled" (apply this change to both occurrences of the status.conditions array to match the docs/api-resources expectation that initial status includes those three conditions).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-resources.md`:
- Around line 56-67: Update the example condition blocks in
docs/api-resources.md to use the current aggregated-condition contract: replace
stale reason/message values like "AwaitingAdapters" and "ResourceReady" with the
new machine-readable reasons and their corresponding messages used by the
Reconciled/LastKnownReconciled conditions; document that "Ready" is deprecated
and show it as the same payload type as "Reconciled" (i.e., keep the Reconciled
payload but note Ready maps to it), and apply these changes consistently to the
other example ranges mentioned (around the blocks at 121-135, 327-338, 394-404)
so examples and tests/readers reflect the current implementation.
---
Outside diff comments:
In `@pkg/services/cluster_test.go`:
- Around line 779-801: The test helper getSynth currently finds
api.ConditionTypeReconciled but only returns LastKnownReconciled and Ready,
leaving Reconciled unverified; update getSynth to assert reconciled is non-nil
(it already sets reconciled) and include reconciled in the returned values
(change signature/returns of getSynth and all call sites to accept/handle the
third api.ResourceCondition) so the test verifies the primary current-generation
condition (api.ConditionTypeReconciled) alongside LastKnownReconciled and Ready.
---
Duplicate comments:
In `@docs/api-operator-guide.md`:
- Around line 404-407: Update the guide to include the aggregated condition
"Reconciled" everywhere the doc currently only shows "Ready" +
"LastKnownReconciled": modify the resource example to list status.conditions as
Reconciled, LastKnownReconciled, and Ready (marked deprecated) and add a sample
Reconciled entry with status/reason/message; then update the rules/timing
sections (the paragraphs that currently describe timing/observed_generation for
Ready and LastKnownReconciled) to instead (or additionally) describe the
behavior for Reconciled—how it aggregates adapter Availability and
observedGeneration—while noting Ready remains a deprecated alias of Reconciled
so readers see the full canonical set: Reconciled, LastKnownReconciled, Ready.
In `@openapi/openapi.yaml`:
- Around line 1312-1315: The OpenAPI schema currently allows only two items for
status.conditions in two places, so update both status.conditions array schemas
to minItems: 3 (replace minItems: 2 with minItems: 3) and ensure the item schema
enforces the three mandatory condition types by including an enum or pattern
restricting the condition.type values to "Ready", "Reconciled", and
"LastKnownReconciled" (apply this change to both occurrences of the
status.conditions array to match the docs/api-resources expectation that initial
status includes those three conditions).
In `@pkg/services/aggregation.go`:
- Around line 169-172: The switch in aggregation.go only restores prior sticky
state from api.ConditionTypeLastKnownReconciled, which misses legacy top-level
"Available" rows; update the switch handling (in the function that declares
prevReconciled and prevAvail) to also set prevAvail when encountering
api.ConditionTypeAvailable (i.e., add a case api.ConditionTypeAvailable:
prevAvail = &c) so prior "Available" values are preserved during rollout in
addition to api.ConditionTypeLastKnownReconciled; keep the existing case
api.ConditionTypeReconciled that sets prevReconciled unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 01d6bc82-9903-4741-b421-e17cb911d6fc
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
0ed7961 to
83e5577
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
openapi/openapi.yaml (1)
60-60:⚠️ Potential issue | 🟠 MajorThe schema still permits dropping one “mandatory” condition.
The descriptions now say
Ready,Reconciled, andLastKnownReconciledare always present, but bothClusterStatus.conditionsandNodePoolStatus.conditionsstill useminItems: 2. Generated validators/clients will therefore accept payloads that violate the documented status contract.Also applies to: 1304-1317, 1670-1683
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` at line 60, The OpenAPI schema claims the conditions array always includes "LastKnownReconciled", "Ready" and "Reconciled" but the array constraints (ClusterStatus.conditions and NodePoolStatus.conditions, and the other identical blocks around the ranges referenced) still have minItems: 2; update those conditions array schemas to minItems: 3 so generated validators/clients require all three mandatory conditions. Locate the conditions arrays referenced as ClusterStatus.conditions and NodePoolStatus.conditions (and the duplicate blocks at the other indicated ranges) and change minItems from 2 to 3, leaving the existing item/schema definitions and descriptions intact.pkg/services/aggregation.go (1)
159-173:⚠️ Potential issue | 🟠 MajorKeep the legacy aggregated
Availablestate during rollout.Existing
status_conditionsJSON can still contain the old aggregated"Available"entry. In the current switch that falls intodefault, so the first recomputation drops the prior sticky state and can incorrectly flipLastKnownReconciledtoFalse. Treat legacyConditionTypeAvailableas the previousLastKnownReconcileduntil old rows have been rewritten.Suggested fix
for i := range conditions { c := conditions[i] switch c.Type { case api.ConditionTypeReady: if prevReconciled == nil { prevReconciled = &c } case api.ConditionTypeReconciled: prevReconciled = &c + case api.ConditionTypeAvailable: + if prevAvail == nil { + prevAvail = &c + } case api.ConditionTypeLastKnownReconciled: prevAvail = &c default: prevAdapterByType[c.Type] = &c } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 159 - 173, The switch over conditions currently ignores the legacy api.ConditionTypeAvailable and drops the old aggregated Available sticky state; update the switch inside the loop that iterates "for i := range conditions" so that case api.ConditionTypeAvailable is treated the same as api.ConditionTypeLastKnownReconciled (i.e., set prevAvail = &c) to preserve legacy Available as the previous LastKnownReconciled until rows are rewritten; keep existing precedence for Reconciled and Ready unchanged.docs/api-operator-guide.md (2)
111-173:⚠️ Potential issue | 🟠 MajorThe create walkthrough still shows impossible initial status.
This section says the API synthesizes
Reconciled/LastKnownReconciled/Ready, but the walkthrough still says create storesstatus.conditions: []and then immediately shows all three asTruebefore any adapter reports. With configured required adapters, those conditions are synthesized on create but startFalseuntil reports arrive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 111 - 173, The walkthrough's create example incorrectly shows status.conditions as empty then immediately all three synthesized conditions (Reconciled, LastKnownReconciled, Ready) as True; instead update the "Resource Lifecycle Example" so that on create the API stores synthesized conditions for Reconciled, LastKnownReconciled, and Ready with status "False" (observed_generation set to the new generation and last_transition_time populated) rather than an empty array, and ensure the narrative clarifies these synthesized conditions start as False until adapter reports arrive.
327-333:⚠️ Potential issue | 🟠 Major
Reconciledis still missing from later contract examples/rules.The table says there are three synthetic conditions, but the JSON example and the lifecycle bullets still only model
ReadyandLastKnownReconciled. SinceReconciledis mandatory andReadyis only its deprecated alias, these sections should includeReconciledanywhere they describe the synthesized set.Also applies to: 343-350, 404-408, 509-516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 327 - 333, The documentation's synthesized conditions list and examples omit the mandatory "Reconciled" condition in later sections; update all JSON examples and lifecycle bullet lists that currently show "Ready" and "LastKnownReconciled" to include "Reconciled" (and if you keep the deprecated alias, show "Ready" as an alias of "Reconciled"). Specifically, add the "Reconciled" entry wherever the synthesized set is described (including the JSON condition arrays and lifecycle bullets that reference "Ready" and "LastKnownReconciled"), ensure the semantics match the table (Reconciled = all adapters report Available=True at current resource.spec.generation), and mark "Ready" explicitly as a deprecated alias of "Reconciled" when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-resources.md`:
- Around line 486-488: Update the docs to treat Ready as a deprecated alias of
Reconciled: change the bullet for Ready to read "Ready (deprecated — alias of
Reconciled)" and update the example query that currently uses
status.conditions.Ready='True' to use status.conditions.Reconciled='True' (or
show both with Ready noted as deprecated). Ensure references to Reconciled,
LastKnownReconciled, and status.conditions.Ready='True' are adjusted so Ready is
explicitly marked deprecated and users are guided to use Reconciled instead.
- Around line 486-488: The API docs rename of the aggregated condition from
"Available" to "Reconciled" (and the related "LastKnownReconciled" and
deprecation of "Ready") must be mirrored in the architecture docs so both repos
stay consistent; update the openshift-hyperfleet/architecture documentation to
replace references to the "Available" aggregated condition with "Reconciled",
add or update mentions of "LastKnownReconciled", and mark "Ready" as deprecated
(or remove if the architecture text follows the API doc change), ensuring all
occurrences and any diagrams or headings referencing "Available" use the new
names.
---
Duplicate comments:
In `@docs/api-operator-guide.md`:
- Around line 111-173: The walkthrough's create example incorrectly shows
status.conditions as empty then immediately all three synthesized conditions
(Reconciled, LastKnownReconciled, Ready) as True; instead update the "Resource
Lifecycle Example" so that on create the API stores synthesized conditions for
Reconciled, LastKnownReconciled, and Ready with status "False"
(observed_generation set to the new generation and last_transition_time
populated) rather than an empty array, and ensure the narrative clarifies these
synthesized conditions start as False until adapter reports arrive.
- Around line 327-333: The documentation's synthesized conditions list and
examples omit the mandatory "Reconciled" condition in later sections; update all
JSON examples and lifecycle bullet lists that currently show "Ready" and
"LastKnownReconciled" to include "Reconciled" (and if you keep the deprecated
alias, show "Ready" as an alias of "Reconciled"). Specifically, add the
"Reconciled" entry wherever the synthesized set is described (including the JSON
condition arrays and lifecycle bullets that reference "Ready" and
"LastKnownReconciled"), ensure the semantics match the table (Reconciled = all
adapters report Available=True at current resource.spec.generation), and mark
"Ready" explicitly as a deprecated alias of "Reconciled" when present.
In `@openapi/openapi.yaml`:
- Line 60: The OpenAPI schema claims the conditions array always includes
"LastKnownReconciled", "Ready" and "Reconciled" but the array constraints
(ClusterStatus.conditions and NodePoolStatus.conditions, and the other identical
blocks around the ranges referenced) still have minItems: 2; update those
conditions array schemas to minItems: 3 so generated validators/clients require
all three mandatory conditions. Locate the conditions arrays referenced as
ClusterStatus.conditions and NodePoolStatus.conditions (and the duplicate blocks
at the other indicated ranges) and change minItems from 2 to 3, leaving the
existing item/schema definitions and descriptions intact.
In `@pkg/services/aggregation.go`:
- Around line 159-173: The switch over conditions currently ignores the legacy
api.ConditionTypeAvailable and drops the old aggregated Available sticky state;
update the switch inside the loop that iterates "for i := range conditions" so
that case api.ConditionTypeAvailable is treated the same as
api.ConditionTypeLastKnownReconciled (i.e., set prevAvail = &c) to preserve
legacy Available as the previous LastKnownReconciled until rows are rewritten;
keep existing precedence for Reconciled and Ready unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2e2954de-dcc8-47fa-aef3-0185240f595f
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
83e5577 to
5dcb421
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/services/aggregation.go (1)
642-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the observed-generation bucket for
LastKnownReconciled.last_updated_timein all false cases.Line 642 only recomputes
last_updated_timewhen aFalseadapter exists atobservedGen. IfLastKnownReconciled=Falsecomes from missing or mixed-generation reports, this leaves the timestamp pinned toprev.LastUpdatedTime/refTimeeven though adapters have already reported in the current bucket. That breaks the documented timestamp contract and can skew stale-status queries/polling.Suggested fix
if status == api.ConditionFalse { x := observedGen - hasFalseAtX := false + atX := make([]time.Time, 0, len(required)) for _, name := range required { s, ok := byAdapter[name] if !ok { continue } - if s.observedGeneration == x && !s.availableTrue { - hasFalseAtX = true - break + if s.observedGeneration == x { + atX = append(atX, s.observedTime) } } - if hasFalseAtX { - var atX []time.Time - for _, name := range required { - s, ok := byAdapter[name] - if !ok { - continue - } - if s.observedGeneration == x { - atX = append(atX, s.observedTime) - } - } - if len(atX) > 0 { - return minTime(atX) - } + if len(atX) > 0 { + return minTime(atX) } }As per coding guidelines,
last_updated_timefor aggregated conditions is “oldest valid adapter report time” for the relevant generation bucket (not the latest adapter report time).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 642 - 675, The current logic only returns the observed-generation bucket time when a False adapter exists with observedGeneration == observedGen; change it so that whenever LastKnownReconciled.Status == api.ConditionFalse you compute the observed-generation bucket (x := observedGen) across required adapters (using byAdapter and required), collect all s.observedTime where s.observedGeneration == x, and if any exist return minTime(atX); do this regardless of s.availableTrue so the false case always uses the observed-generation bucket for LastKnownReconciled.last_updated_time instead of falling back to prev.LastUpdatedTime or refTime.openapi/openapi.yaml (2)
1741-1747:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
ResourceCondition.last_updated_timesemantics are now incorrect for aggregated conditionsLine 1744–1747 currently describes adapter-only behavior (
AdapterStatus.last_report_time) for all resource conditions. That is now incorrect for aggregatedReconciled/LastKnownReconciled/Ready, where the value is derived from the oldest valid adapter report time in the relevant generation bucket.Suggested spec wording update
last_updated_time: type: string format: date-time description: |- - When the corresponding adapter last reported (API-managed) - Updated every time the adapter POSTs, even if condition status hasn't changed - Copied from AdapterStatus.last_report_time + API-managed update timestamp for this condition. + For per-adapter synthesized conditions, copied from AdapterStatus.last_report_time. + For aggregated conditions (Reconciled, LastKnownReconciled, Ready), computed as the oldest valid adapter report time within the relevant generation bucket.As per coding guidelines,
docs/api-resources.md: "last_updated_time semantics for aggregated conditions: Must be described as computed from the oldest valid adapter report time within the relevant generation bucket (not the newest report time)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 1741 - 1747, Update the OpenAPI description for ResourceCondition.last_updated_time so it no longer describes adapter-only behavior (AdapterStatus.last_report_time); instead, state the correct semantics for aggregated conditions (Reconciled, LastKnownReconciled, Ready): last_updated_time is computed from the oldest valid adapter report time within the relevant generation bucket (not the newest report), and is updated according to the aggregation rules rather than individual adapter POSTs. Locate the description text currently referencing AdapterStatus.last_report_time and replace it with this aggregated-generation-bucket wording, keeping examples/context about when it changes.
807-810:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSearch parameter example still promotes deprecated
ReadyThe
SearchParamsexample still usesstatus.conditions.Ready='True'(Line 809). PreferLastKnownReconciled(orReconciled) in examples so new consumers don’t anchor on deprecated condition names.As per coding guidelines,
docs/search.md: "Use status.conditions.LastKnownReconciled='True' instead of any older Available-based query."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 807 - 810, Update the TSL examples in the SearchParams description to stop using the deprecated condition name "status.conditions.Ready='True'": replace it with "status.conditions.LastKnownReconciled='True'" (or "status.conditions.Reconciled='True'" per guidelines) so the OpenAPI example and any references to TSL queries reflect the recommended condition name; ensure the examples string that also shows name/labels remains unchanged except for this condition name swap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/search.md`:
- Around line 101-108: The docs mention status.conditions.LastKnownReconciled
but linked repos still use status.conditions.Available; update all example
queries and CEL consumers that reference status.conditions.Available (and any
other old condition names) to the new PascalCase condition name
status.conditions.LastKnownReconciled, or explicitly mark those snippets as
adapter-local if they remain different; search for references to
status.conditions.Available in the related examples (e.g., snippets used by the
adapter and architecture examples) and either rename them to
status.conditions.LastKnownReconciled or add a clear comment noting the
adapter-local vs aggregated contract difference so consumers know which name to
use.
---
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 1741-1747: Update the OpenAPI description for
ResourceCondition.last_updated_time so it no longer describes adapter-only
behavior (AdapterStatus.last_report_time); instead, state the correct semantics
for aggregated conditions (Reconciled, LastKnownReconciled, Ready):
last_updated_time is computed from the oldest valid adapter report time within
the relevant generation bucket (not the newest report), and is updated according
to the aggregation rules rather than individual adapter POSTs. Locate the
description text currently referencing AdapterStatus.last_report_time and
replace it with this aggregated-generation-bucket wording, keeping
examples/context about when it changes.
- Around line 807-810: Update the TSL examples in the SearchParams description
to stop using the deprecated condition name "status.conditions.Ready='True'":
replace it with "status.conditions.LastKnownReconciled='True'" (or
"status.conditions.Reconciled='True'" per guidelines) so the OpenAPI example and
any references to TSL queries reflect the recommended condition name; ensure the
examples string that also shows name/labels remains unchanged except for this
condition name swap.
In `@pkg/services/aggregation.go`:
- Around line 642-675: The current logic only returns the observed-generation
bucket time when a False adapter exists with observedGeneration == observedGen;
change it so that whenever LastKnownReconciled.Status == api.ConditionFalse you
compute the observed-generation bucket (x := observedGen) across required
adapters (using byAdapter and required), collect all s.observedTime where
s.observedGeneration == x, and if any exist return minTime(atX); do this
regardless of s.availableTrue so the false case always uses the
observed-generation bucket for LastKnownReconciled.last_updated_time instead of
falling back to prev.LastUpdatedTime or refTime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5043d7e0-02b1-455b-a91a-e71c9e30d763
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
…LastKnownReconciled - Rename the aggregated Available condition to LastKnownReconciled across service layer, tests, docs, and OpenAPI spec - Sync OpenAPI spec from upstream hyperfleet-api-spec v1.0.11 (includes new PUT endpoints for cluster/nodepool statuses) - Rename Reconciled condition reason constants from Ready* prefix to Reconciled* prefix to match their actual usage - Fix doc examples: correct Reconciled condition reason values to match code constants (ReconciledAll, AllAdaptersReconciled)
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
5dcb421 to
5718842
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/services/aggregation.go (1)
635-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive
LastKnownReconciled.last_updated_timefrom the tracked generation bucket.When adapters are all
Available=Truebut split across generations,ObservedGenerationadvances tomaxG, but this code falls back toprev.LastUpdatedTime/refTimebecausehasFalseAtXis false. That makeslast_updated_timeunrelated to the tracked bucket and breaks stale-condition queries during partial rollouts.Suggested fix
if status == api.ConditionFalse { x := observedGen - hasFalseAtX := false + hasFalseAtX := false + atX := make([]time.Time, 0, len(required)) for _, name := range required { s, ok := byAdapter[name] if !ok { continue } - if s.observedGeneration == x && !s.availableTrue { - hasFalseAtX = true - break - } - } - if hasFalseAtX { - var atX []time.Time - for _, name := range required { - s, ok := byAdapter[name] - if !ok { - continue - } - if s.observedGeneration == x { - atX = append(atX, s.observedTime) - } + if s.observedGeneration != x { + continue + } + atX = append(atX, s.observedTime) + if !s.availableTrue { + hasFalseAtX = true } - if len(atX) > 0 { - return minTime(atX) - } + } + if len(atX) > 0 && (hasFalseAtX || (allTrue && mixed)) { + return minTime(atX) } }As per coding guidelines,
docs/api-operator-guide.md:last_updated_timefor aggregated conditions “represents the oldest valid adapter report time within the relevant generation bucket, not the newest report time”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 635 - 675, When allTrue && mixed && status == api.ConditionTrue, instead of immediately returning prev.LastUpdatedTime or refTime, derive LastUpdatedTime from the tracked generation bucket: collect s.observedTime for adapters in required where byAdapter[name].observedGeneration == observedGen (use the same observedGen variable used later) and return minTime(of those times) if any exist; otherwise fall back to prev.LastUpdatedTime or refTime. Update the logic around the if allTrue && mixed && status == api.ConditionTrue block (references: byAdapter, required, observedGen, s.observedGeneration, s.observedTime, minTime, prev, refTime) so last_updated_time represents the oldest report in the relevant generation bucket.docs/api-operator-guide.md (1)
150-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe examples now show a never-observed initial status.
AggregateResourceStatusinitializes the three aggregated conditions asFalseuntil required adapters report, but this walkthrough now shows them asTrueimmediately after create. The later resource JSON example also still omitsReconciled, so both examples diverge from the actual/public contract.As per coding guidelines,
docs/api-resources.md: "initial synthesized status must includeReconciled=False,LastKnownReconciled=False, andReady=Falseuntil adapters report."Also applies to: 173-173, 404-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-operator-guide.md` around lines 150 - 166, The documentation examples show aggregated conditions as True immediately and omit Reconciled in later JSON, which conflicts with AggregateResourceStatus initialization; update all examples (including the initial synthesized status block and the later resource JSON) to reflect the required initial state: Reconciled=False, LastKnownReconciled=False, Ready=False until adapters report, and ensure Reconciled is present in the later resource JSON sample so the docs match the public contract and the behavior of AggregateResourceStatus.
♻️ Duplicate comments (1)
pkg/services/aggregation.go (1)
164-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the legacy aggregated
Availablecondition during rollout.
parsePrevConditionsnow ignores stored"Available"entries. Existing rows that have not been recomputed yet will lose the previous sticky state, so the first mixed-generation update can incorrectly flipLastKnownReconciledfromTruetoFalse.Suggested fix
for i := range conditions { c := conditions[i] switch c.Type { case api.ConditionTypeReady: if prevReconciled == nil { prevReconciled = &c } case api.ConditionTypeReconciled: prevReconciled = &c + case api.ConditionTypeAvailable: + if prevAvail == nil { + prevAvail = &c + } case api.ConditionTypeLastKnownReconciled: prevAvail = &c default: prevAdapterByType[c.Type] = &c } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/aggregation.go` around lines 164 - 173, parsePrevConditions currently ignores stored ConditionTypeAvailable which causes legacy "Available" state to be lost; update parsePrevConditions to handle api.ConditionTypeAvailable (similar to api.ConditionTypeLastKnownReconciled) by capturing the condition into prevAvail (or the existing variable used for previous availability) so the legacy aggregated Available condition is preserved during rollout and mixed-generation updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-operator-guide.md`:
- Around line 327-333: Docs were updated to use the aggregated condition name
LastKnownReconciled but related ADRs and adapter-framework docs still reference
Available; coordinate a rename across repos by updating the architecture ADRs
and adapter-framework docs to use LastKnownReconciled (and mark Ready as
deprecated/alias of Reconciled) so all published contracts match; search for
symbols/terms "LastKnownReconciled", "Reconciled", "Ready", and "Available" in
the openshift-hyperfleet/architecture ADRs and adapter-framework docs, change
wording and examples to the new names, update any API/contract examples and
tests to reference LastKnownReconciled, and validate the changes against
HyperFleet architecture standards before merging.
---
Outside diff comments:
In `@docs/api-operator-guide.md`:
- Around line 150-166: The documentation examples show aggregated conditions as
True immediately and omit Reconciled in later JSON, which conflicts with
AggregateResourceStatus initialization; update all examples (including the
initial synthesized status block and the later resource JSON) to reflect the
required initial state: Reconciled=False, LastKnownReconciled=False, Ready=False
until adapters report, and ensure Reconciled is present in the later resource
JSON sample so the docs match the public contract and the behavior of
AggregateResourceStatus.
In `@pkg/services/aggregation.go`:
- Around line 635-675: When allTrue && mixed && status == api.ConditionTrue,
instead of immediately returning prev.LastUpdatedTime or refTime, derive
LastUpdatedTime from the tracked generation bucket: collect s.observedTime for
adapters in required where byAdapter[name].observedGeneration == observedGen
(use the same observedGen variable used later) and return minTime(of those
times) if any exist; otherwise fall back to prev.LastUpdatedTime or refTime.
Update the logic around the if allTrue && mixed && status == api.ConditionTrue
block (references: byAdapter, required, observedGen, s.observedGeneration,
s.observedTime, minTime, prev, refTime) so last_updated_time represents the
oldest report in the relevant generation bucket.
---
Duplicate comments:
In `@pkg/services/aggregation.go`:
- Around line 164-173: parsePrevConditions currently ignores stored
ConditionTypeAvailable which causes legacy "Available" state to be lost; update
parsePrevConditions to handle api.ConditionTypeAvailable (similar to
api.ConditionTypeLastKnownReconciled) by capturing the condition into prevAvail
(or the existing variable used for previous availability) so the legacy
aggregated Available condition is preserved during rollout and mixed-generation
updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3d9424d9-094d-431c-b3cc-e82ff76cd955
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
| HyperFleet API aggregates the condition values reported by adapters associated with a resource and adds three synthetic aggregated conditions. | ||
|
|
||
| | Condition | Meaning | When True | | ||
| |-----------|---------|-----------| | ||
| | **Ready** | Resource is fully reconciled at current spec | All registered adapters report `Available=True` at the **current** `resource.spec.generation` | | ||
| | **Available** | Resource is operational at any known good configuration | All registered adapters report `Available=True` (at any generation) | | ||
| | **Reconciled** | Resource is fully reconciled at current spec | All registered adapters report `Available=True` at the **current** `resource.spec.generation` | | ||
| | **LastKnownReconciled** | Resource is operational at any known good configuration | All registered adapters report `Available=True` for a common `observed_generation`, or sticky-true is preserved when adapters are transitioning to a new generation | | ||
| | **Ready** *(deprecated — use Reconciled)* | Alias of Reconciled | Same as Reconciled | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Coordinate this rename with the architecture repo too.
This guide is now aligned on LastKnownReconciled, but the linked openshift-hyperfleet/architecture ADRs and adapter-framework docs still describe aggregated Available. A companion update there will avoid publishing two different contracts to adapter authors.
As per coding guidelines, "Validate changes against HyperFleet architecture standards from the linked architecture repository."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/api-operator-guide.md` around lines 327 - 333, Docs were updated to use
the aggregated condition name LastKnownReconciled but related ADRs and
adapter-framework docs still reference Available; coordinate a rename across
repos by updating the architecture ADRs and adapter-framework docs to use
LastKnownReconciled (and mark Ready as deprecated/alias of Reconciled) so all
published contracts match; search for symbols/terms "LastKnownReconciled",
"Reconciled", "Ready", and "Available" in the openshift-hyperfleet/architecture
ADRs and adapter-framework docs, change wording and examples to the new names,
update any API/contract examples and tests to reference LastKnownReconciled, and
validate the changes against HyperFleet architecture standards before merging.
There was a problem hiding this comment.
Out of scope for this PR. The architecture repo update will be coordinated as a follow-up after both this PR and the spec PR (#43) merge.
There was a problem hiding this comment.
@rafabene, understood — the architecture repo update will be a follow-up once this PR and the upstream spec PR (#43) are merged.
Would you like me to open a follow-up GitHub issue to track the architecture/adapter-framework documentation update (ADRs, adapter-framework docs) to align with the LastKnownReconciled rename?
2 participants
Summary
Availableresource condition toLastKnownReconciledacross code, tests, docs, and OpenAPI specReconciledcondition to documentation alongsideLastKnownReconciledandReady(deprecated)last_updated_timeaggregated semantics inapi-resources.mdOpenAPI spec update
The
openapi/openapi.yamlwas regenerated from the TypeSpec repo (hyperfleet-api-spec) to reflect:Available→LastKnownReconciledin examples and descriptionsAllAdaptersReconciled,ReconciledAll) in example payloadsTest plan
make verify-all— go vet + gofmt + unit tests (885 passing)ConditionTypeLastKnownReconciledreferences verified in test fixturesObservedGenerationassertion added to cascade delete test