HYPERFLEET-1017 - refactor: rename aggregated Available 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
WalkthroughThis PR replaces the aggregated status condition "Available" with "LastKnownReconciled" across API schema, docs, tests, and implementation. It adds api.ConditionTypeLastKnownReconciled, updates aggregation logic to compute LastKnownReconciled (including observed_generation, last_updated_time, last_transition_time and sticky/mixed-generation rules), changes aggregation return values and callers to include LastKnownReconciled, updates cluster/nodepool status builders, updates tests and docs, and bumps the OpenAPI version while adding PUT endpoints for updating cluster and nodepool statuses. Sequence Diagram(s)sequenceDiagram
participant Client
participant AggregationService as Aggregation Service
participant AdapterStatuses as Adapter Status Sources
participant PreviousState as Previous Conditions Store
participant StatusComputation as Status Computation
Client->>AggregationService: Trigger status aggregation / recompute
AggregationService->>AdapterStatuses: Fetch per-adapter snapshots
AdapterStatuses-->>AggregationService: Adapter conditions
AggregationService->>PreviousState: Read previous conditions (incl. LastKnownReconciled / legacy Available)
PreviousState-->>AggregationService: Prev-state map
AggregationService->>StatusComputation: computeReconciled(adapterSnapshots)
StatusComputation-->>AggregationService: Reconciled condition
AggregationService->>StatusComputation: computeLastKnownReconciled(adapterSnapshots, prevState)
StatusComputation-->>AggregationService: LastKnownReconciled condition
AggregationService->>Client: Return aggregated conditions (Reconciled, LastKnownReconciled, Ready, per-adapter)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
There was a problem hiding this comment.
This is a generated file that should not go to the repo
There was a problem hiding this comment.
Agreed — removed openapi.gen.go from tracking. It's already in .gitignore under /pkg/api/openapi/, so it was added accidentally.
| reasonReadyMissingAdapters = "ReadyMissingAdapters" | ||
| reasonReadyWaitingForChildren = "ReadyWaitingForChildren" |
There was a problem hiding this comment.
Should these be Reconciled instead of Ready?
There was a problem hiding this comment.
Good catch — renamed the constants and their string values from Ready* to Reconciled* prefix since they're used in computeReconciled(), not the deprecated Ready condition. Updated aggregation.go, tests, and doc examples.
…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
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
…-break test The test was using ConditionTypeAvailable in prevConds, but parsePrevConditions looks for ConditionTypeLastKnownReconciled. Without this fix the test passed incidentally but didn't exercise the sticky-break logic.
|
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
2 participants
Summary
Availablecondition toLastKnownReconciledacross the entire codebase (service layer, tests, docs)hyperfleet-api-specv1.0.11 (includes new PUT endpoints for cluster/nodepool statuses, updated LKR description)Availablecondition name (not needed since v1.0 hasn't shipped)ReadyAllReconciled,AllAdaptersReconciled)Changes
Service layer (
pkg/services/aggregation.go)ConditionTypeAvailablekept for adapter-level conditions (unchanged)ConditionTypeLastKnownReconciledused for the aggregated conditionparsePrevConditionsthat mapped legacyAvailable→LastKnownReconciledTests (
pkg/services/aggregation_test.go)LastKnownReconciledAvailable→LastKnownReconciledmigrationOpenAPI spec (
openapi/openapi.yaml)hyperfleet-api-specv1.0.11pkg/api/openapi/from updated specDocs (
docs/api-resources.md)Test plan
make test— all unit tests pass (including updated aggregation tests)make test-integration— integration testsmake verify-all— vet + lint + tests