HYPERFLEET-853 - feat: Add Reconciled to OpenAPI spec#34
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces two new condition types to the API schema and updates the API version from 1.0.7 to 1.0.8. A new mandatory Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (1)
schemas/core/openapi.yaml (1)
171-189:⚠️ Potential issue | 🟠 MajorDelete examples contradict the new
Reconciledsemantics.Both delete response examples increment
generationto2but showReconciled: Truewithobserved_generation: 1. That conflicts with the documented meaning (“fully reconciled … at the current generation”) and can cause clients to treat deletion as already reconciled.Proposed fix (example direction)
generation: 2 status: conditions: - type: Reconciled - status: 'True' - reason: All adapters reported Reconciled True for the current generation - message: All adapters reported Reconciled True for the current generation - observed_generation: 1 + status: 'False' + reason: ReconciliationPending + message: Desired state for generation 2 is not yet fully reconciled + observed_generation: 2Also applies to: 370-392, 1185-1187, 1551-1553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/core/openapi.yaml` around lines 171 - 189, The delete response examples show generation: 2 but Reconciled type with observed_generation: 1, which contradicts the Reconciled semantics; update those examples so Reconciled is only true when observed_generation equals generation—either set observed_generation: 2 to match generation or change the Reconciled condition to status: 'False' (and keep observed_generation: 1) for the delete examples; apply the same fix to the other occurrences referenced (around the blocks at the other locations).
🧹 Nitpick comments (3)
services/clusters.tsp (1)
41-41: Clarify mandatory-condition wording to match enforceable schema.Line 41 says all three conditions are mandatory, but the models currently allow
@minItems(2). That mismatch can confuse client expectations and generated docs. Consider wording this as “initially includes”/“expected conditions” until the schema can enforce 3 in a breaking version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/clusters.tsp` at line 41, Update the text that states "status.conditions will include mandatory 'Available', 'Ready' and 'Reconciled' conditions" to reflect the current schema by changing it to phrasing like "status.conditions initially includes/is expected to include 'Available', 'Ready' and 'Reconciled' conditions" and add a short parenthetical noting the models currently use `@minItems`(2) so enforcement of three conditions is not yet in the schema; reference the status.conditions field and the `@minItems`(2) constraint in your note so readers understand the mismatch until a breaking schema change is made.models/nodepools/model.tsp (1)
33-34: Avoid enforceability ambiguity in mandatory-condition docs.Lines 33-34 describe three mandatory conditions, while
conditionsremains@minItems(2)(Line 39). Consider rephrasing to “expected/present on creation” to keep docs aligned with what schema validators can enforce today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/nodepools/model.tsp` around lines 33 - 34, The doc text claims three "mandatory" conditions but the schema for the conditions array is `@minItems`(2), creating ambiguity; update the prose around the `conditions` field to avoid enforceability language (e.g., change "mandatory" to "expected/present on creation" or similar) and clarify that `type: "Ready"` is deprecated in favor of `type: "Reconciled"`, without implying the schema enforces three items; edit the comment block that lists `type: "Ready"` and `type: "Reconciled"` and the surrounding description of `conditions` to reflect the two-item minimum and that additional conditions may be present but are not strictly enforced by the schema.models/clusters/model.tsp (1)
36-37: Keep ClusterStatus wording consistent with schema constraints.Lines 36-37 call out three mandatory conditions, but
@minItems(2)(Line 42) does not enforce that. Rewording to “expected at creation” would reduce client-contract ambiguity until a stricter schema change is possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/clusters/model.tsp` around lines 36 - 37, Update the ClusterStatus conditions wording so it no longer claims three conditions are "mandatory"; change the phrasing around the `Ready` and `Reconciled` condition descriptions to indicate they are "expected at creation" (or similar) to match the current schema constraint `@minItems(2)` on the conditions array. Locate the ClusterStatus documentation block that names `type: "Ready"` and `type: "Reconciled"` and reword those lines to remove "mandatory" language and explicitly note the schema currently requires at least two conditions (`@minItems(2)`), reducing client-contract ambiguity until the schema is tightened.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@schemas/core/openapi.yaml`:
- Around line 171-189: The delete response examples show generation: 2 but
Reconciled type with observed_generation: 1, which contradicts the Reconciled
semantics; update those examples so Reconciled is only true when
observed_generation equals generation—either set observed_generation: 2 to match
generation or change the Reconciled condition to status: 'False' (and keep
observed_generation: 1) for the delete examples; apply the same fix to the other
occurrences referenced (around the blocks at the other locations).
---
Nitpick comments:
In `@models/clusters/model.tsp`:
- Around line 36-37: Update the ClusterStatus conditions wording so it no longer
claims three conditions are "mandatory"; change the phrasing around the `Ready`
and `Reconciled` condition descriptions to indicate they are "expected at
creation" (or similar) to match the current schema constraint `@minItems(2)` on
the conditions array. Locate the ClusterStatus documentation block that names
`type: "Ready"` and `type: "Reconciled"` and reword those lines to remove
"mandatory" language and explicitly note the schema currently requires at least
two conditions (`@minItems(2)`), reducing client-contract ambiguity until the
schema is tightened.
In `@models/nodepools/model.tsp`:
- Around line 33-34: The doc text claims three "mandatory" conditions but the
schema for the conditions array is `@minItems`(2), creating ambiguity; update the
prose around the `conditions` field to avoid enforceability language (e.g.,
change "mandatory" to "expected/present on creation" or similar) and clarify
that `type: "Ready"` is deprecated in favor of `type: "Reconciled"`, without
implying the schema enforces three items; edit the comment block that lists
`type: "Ready"` and `type: "Reconciled"` and the surrounding description of
`conditions` to reflect the two-item minimum and that additional conditions may
be present but are not strictly enforced by the schema.
In `@services/clusters.tsp`:
- Line 41: Update the text that states "status.conditions will include mandatory
'Available', 'Ready' and 'Reconciled' conditions" to reflect the current schema
by changing it to phrasing like "status.conditions initially includes/is
expected to include 'Available', 'Ready' and 'Reconciled' conditions" and add a
short parenthetical noting the models currently use `@minItems`(2) so enforcement
of three conditions is not yet in the schema; reference the status.conditions
field and the `@minItems`(2) constraint in your note so readers understand the
mismatch until a breaking schema change is made.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 68c7cc2d-f4f1-4328-ad92-1050a3d487f5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
main.tspmodels-core/cluster/example_cluster.tspmodels-core/nodepool/example_nodepool.tspmodels-gcp/cluster/example_cluster.tspmodels-gcp/nodepool/example_nodepool.tspmodels/clusters/model.tspmodels/common/model.tspmodels/nodepools/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/clusters.tsp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
schemas/core/openapi.yaml (1)
1194-1195: Add an explicit backward-compatibility note forminItems: 2.The prose now says three conditions are mandatory, but schema validation still allows two (
minItems: 2). Consider documenting this exception directly in these descriptions to avoid consumer confusion.Proposed doc tweak
**Mandatory conditions**: - `type: "Ready"` *(deprecated — use Reconciled)*: Whether all adapters report successfully at the current generation. - `type: "Reconciled"`: Whether the resource's desired state has been fully reconciled by all adapters at the current generation. - `type: "Available"`: Aggregated adapter result for a common observed_generation. + Note: `minItems` remains `2` for backward compatibility with existing clients.Also applies to: 1560-1561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/core/openapi.yaml` around lines 1194 - 1195, The documentation text says three conditions are mandatory but the OpenAPI schema still allows two via "minItems: 2"; update the descriptions for the condition list (the prose entries for `type: "Ready"` and `type: "Reconciled"`) to include a short backward-compatibility note that explains "minItems: 2" is accepted for compatibility (and new clients should expect three conditions), and apply the same explanatory sentence to the analogous description block referenced around the other occurrence (the lines noted in the review). Ensure you mention the exact schema constraint `minItems: 2` in the note so consumers see why the prose and schema differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@schemas/core/openapi.yaml`:
- Around line 182-189: The delete example's Reconciled condition is
inconsistent: resource `generation` is set to 2 but the Reconciled entry shows
observed_generation: 1 while the reason/message says "at the current
generation"; update the Reconciled entries (the block with type: Reconciled,
observed_generation, created_time, last_updated_time, last_transition_time) to
use observed_generation: 2 and adjust timestamps to match the delete example's
current generation, or alternatively change the reason/message to indicate the
observed generation is 1—apply the same fix to both occurrences (the Reconciled
blocks around lines with observed_generation: 1, including the one referenced at
186 and the similar block at 389).
---
Nitpick comments:
In `@schemas/core/openapi.yaml`:
- Around line 1194-1195: The documentation text says three conditions are
mandatory but the OpenAPI schema still allows two via "minItems: 2"; update the
descriptions for the condition list (the prose entries for `type: "Ready"` and
`type: "Reconciled"`) to include a short backward-compatibility note that
explains "minItems: 2" is accepted for compatibility (and new clients should
expect three conditions), and apply the same explanatory sentence to the
analogous description block referenced around the other occurrence (the lines
noted in the review). Ensure you mention the exact schema constraint `minItems:
2` in the note so consumers see why the prose and schema differ.
🪄 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: c0c14637-9ca8-4dd0-be39-db992db7b277
📒 Files selected for processing (7)
models/common/model.tspmodels/statuses/example_adapter_status.tspmodels/statuses/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yaml
✅ Files skipped from review due to trivial changes (2)
- models/statuses/model.tsp
- models/statuses/example_adapter_status.tsp
🚧 Files skipped from review as they are similar to previous changes (3)
- schemas/gcp/openapi.yaml
- schemas/core/swagger.yaml
- schemas/gcp/swagger.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
schemas/core/swagger.yaml (1)
1363-1379: Clarify the “mandatory” wording vsminItems: 2contract.The descriptions now declare three mandatory conditions, but the schema still validates arrays with only 2 items. That creates a contract ambiguity for consumers and generated validators. If
minItemsmust stay 2 for compatibility, please explicitly document that this is a backward-compatibility allowance.Suggested non-breaking wording tweak
- **Mandatory conditions**: + **Mandatory conditions** (semantic requirement; schema keeps `minItems: 2` for backward compatibility):Also applies to: 1750-1766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/core/swagger.yaml` around lines 1363 - 1379, The docs list three "Mandatory conditions" (types "Ready", "Reconciled", "Available") but the schema property still has minItems: 2, causing ambiguity; update the swagger.yaml text near the array schema (the array with items: $ref: '#/definitions/ResourceCondition' and minItems: 2) to clarify the backward-compatibility allowance by explicitly stating that although three condition types are recommended/expected, minItems remains 2 for compatibility with older clients, and add a short note which of the three may be omitted in older contracts (or instruct consumers to expect the third condition when supported).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@schemas/core/swagger.yaml`:
- Around line 1363-1379: The docs list three "Mandatory conditions" (types
"Ready", "Reconciled", "Available") but the schema property still has minItems:
2, causing ambiguity; update the swagger.yaml text near the array schema (the
array with items: $ref: '#/definitions/ResourceCondition' and minItems: 2) to
clarify the backward-compatibility allowance by explicitly stating that although
three condition types are recommended/expected, minItems remains 2 for
compatibility with older clients, and add a short note which of the three may be
omitted in older contracts (or instruct consumers to expect the third condition
when supported).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c774788f-d010-4319-a54b-ee1213cd621c
📒 Files selected for processing (9)
aliases.tspmodels-core/cluster/example_cluster.tspmodels-core/nodepool/example_nodepool.tspmodels-gcp/cluster/example_cluster.tspmodels-gcp/nodepool/example_nodepool.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yaml
✅ Files skipped from review due to trivial changes (1)
- aliases.tsp
🚧 Files skipped from review as they are similar to previous changes (5)
- models-gcp/nodepool/example_nodepool.tsp
- models-core/nodepool/example_nodepool.tsp
- models-gcp/cluster/example_cluster.tsp
- schemas/core/openapi.yaml
- schemas/gcp/openapi.yaml
…xamples (removed aliases inclusion
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 (1)
models-gcp/nodepool/example_nodepool.tsp (1)
141-181:⚠️ Potential issue | 🟠 MajorDon't move create-time success conditions onto the delete generation.
After these entries are advanced to
observed_generation: 2, the delete example still says things likeNodePool has 3 nodes runningandAll validations passed. That cannot describe the same generation as the new delete-sideReconciled=Trueexample. Either keep those conditions on the prior generation until deletion is reported, or update their reason/message to deletion/finalization semantics so the example matches the current generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models-gcp/nodepool/example_nodepool.tsp` around lines 141 - 181, The example advances create-time success conditions (e.g., the entries with type "ValidationSuccessful", "NodePoolSuccessful", ConditionType.Available and their messages like "NodePool has 3 nodes running" and "All validations passed") to observed_generation: 2 which is the delete-generation and conflicts with the new delete-side ConditionType.Reconciled=True; either move those create-time conditions back to the prior generation (set their observed_generation to the previous generation number) or change their reason/message and type to deletion/finalization semantics (e.g., update reason to a deletion/finalizer-specific value and message to reflect teardown) so the status entries consistently correspond to the same observed_generation and accurately reflect delete-time state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models-core/nodepool/example_nodepool.tsp`:
- Around line 98-101: The example's observed_generation (observed_generation: 2)
and the conditions block are inconsistent for a delete/finalization scenario;
either revert observed_generation to 1 or update the conditions (the entries
under conditions/Reconciled) to reflect deletion semantics (adjust reason and
message fields and timestamps such as last_transition_time/last_updated_time) so
the Reconciled condition no longer describes a successful active reconcile but a
delete/finalize state; locate the example_nodepool.tsp snippet with
observed_generation, conditions and Reconciled and make the timestamps and
reason/message consistent with the delete lifecycle.
In `@models-gcp/cluster/example_cluster.tsp`:
- Around line 132-145: The example's Reconciled conditions
(ConditionType.Reconciled with ResourceConditionStatus.True, using
ExampleReconciledReason/ExampleReconciledMessage) incorrectly use
adapter-availability wording for a deleted resource; update those entries to
reflect finalization/deletion semantics by either keeping observed_generation at
the prior generation (so adapter success refers to the previous generation) or
change the adapter reason/message to indicate Finalized/Deletion (e.g., replace
ExampleReconciledReason/ExampleReconciledMessage with finalization-oriented
text) for both the block around ConditionType.Reconciled and the similar block
at the later section (the blocks that currently show adapters "are available").
---
Outside diff comments:
In `@models-gcp/nodepool/example_nodepool.tsp`:
- Around line 141-181: The example advances create-time success conditions
(e.g., the entries with type "ValidationSuccessful", "NodePoolSuccessful",
ConditionType.Available and their messages like "NodePool has 3 nodes running"
and "All validations passed") to observed_generation: 2 which is the
delete-generation and conflicts with the new delete-side
ConditionType.Reconciled=True; either move those create-time conditions back to
the prior generation (set their observed_generation to the previous generation
number) or change their reason/message and type to deletion/finalization
semantics (e.g., update reason to a deletion/finalizer-specific value and
message to reflect teardown) so the status entries consistently correspond to
the same observed_generation and accurately reflect delete-time state.
🪄 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: 0161d27e-e620-4943-92d9-977a8c987ac4
📒 Files selected for processing (8)
models-core/cluster/example_cluster.tspmodels-core/nodepool/example_nodepool.tspmodels-gcp/cluster/example_cluster.tspmodels-gcp/nodepool/example_nodepool.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- models-core/cluster/example_cluster.tsp
| observed_generation: 2, | ||
| created_time: "2021-01-01T10:00:00Z", | ||
| last_updated_time: "2021-01-01T10:00:00Z", | ||
| last_transition_time: "2021-01-01T10:00:00Z", |
There was a problem hiding this comment.
Generation 2 still reads like the pre-delete nodepool state.
These conditions now claim to reflect generation 2, but the example still describes the old success path instead of deletion/finalization. That makes the delete example internally inconsistent with the new Reconciled semantics. Please either leave those stale conditions on generation 1 or rewrite their reason/message for the delete lifecycle.
Also applies to: 103-111, 118-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models-core/nodepool/example_nodepool.tsp` around lines 98 - 101, The
example's observed_generation (observed_generation: 2) and the conditions block
are inconsistent for a delete/finalization scenario; either revert
observed_generation to 1 or update the conditions (the entries under
conditions/Reconciled) to reflect deletion semantics (adjust reason and message
fields and timestamps such as last_transition_time/last_updated_time) so the
Reconciled condition no longer describes a successful active reconcile but a
delete/finalize state; locate the example_nodepool.tsp snippet with
observed_generation, conditions and Reconciled and make the timestamps and
reason/message consistent with the delete lifecycle.
| observed_generation: 2, | ||
| created_time: "2021-01-01T10:00:00Z", | ||
| last_updated_time: "2021-01-01T10:00:00Z", | ||
| last_transition_time: "2021-01-01T10:00:00Z", | ||
| }, | ||
| #{ | ||
| type: ConditionType.Reconciled, | ||
| status: ResourceConditionStatus.True, | ||
| reason: ExampleReconciledReason, | ||
| message: ExampleReconciledMessage, | ||
| observed_generation: 2, | ||
| created_time: "2021-01-01T10:00:00Z", | ||
| last_updated_time: "2021-01-01T10:00:00Z", | ||
| last_transition_time: "2021-01-01T10:00:00Z", |
There was a problem hiding this comment.
The delete example still reports adapter availability instead of finalization.
These conditions now reflect observed_generation: 2, but the adapter messages still say the adapters are available. For a deleted resource, the new Reconciled semantics are based on required adapters reporting Finalized=True at the current generation. Either keep those adapter-success conditions on the prior generation or update their reason/message to deletion/finalization wording.
Also applies to: 152-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models-gcp/cluster/example_cluster.tsp` around lines 132 - 145, The example's
Reconciled conditions (ConditionType.Reconciled with
ResourceConditionStatus.True, using
ExampleReconciledReason/ExampleReconciledMessage) incorrectly use
adapter-availability wording for a deleted resource; update those entries to
reflect finalization/deletion semantics by either keeping observed_generation at
the prior generation (so adapter success refers to the previous generation) or
change the adapter reason/message to indicate Finalized/Deletion (e.g., replace
ExampleReconciledReason/ExampleReconciledMessage with finalization-oriented
text) for both the block around ConditionType.Reconciled and the similar block
at the later section (the blocks that currently show adapters "are available").
openshift-merge-bot
Bot
merged commit 31fbfe1
into
openshift-hyperfleet:main
2 participants
Summary
Reconciledcondition type to the OpenAPI spec alongsideReady(backward-compatible)Finalizedcondition to adapter status documentation and examplesReadyas deprecated in ClusterStatus and NodePoolStatus descriptionsContext
Supports HYPERFLEET-853 in the hyperfleet-api repo. The
Reconciledcondition replacesReadywith broader semantics — it means "the desired state has been fully reconciled" which works for both normal lifecycle (all adapters Available) and deletion lifecycle (all adapters Finalized).Finalizedis an adapter-reported condition indicating cleanup completion during deletion. It is already supported by the adapter framework (hyperfleet-adapter#104) and consumed by the API to computeReconciled.Changes
Reconciled:
ReconciledtoConditionTypeunion inmodels/common/model.tspExampleReconciledReason,ExampleReconciledMessage)Readyas deprecated in both descriptionsservices/clusters.tspminItemsremains at 2 to avoid breaking existing clientsFinalized:
Finalizedto AdapterCondition and AdapterStatus.conditions descriptionsExampleAdapter1FinalizedReason,ExampleAdapter1FinalizedMessage)Test plan
npm run build:allsucceedsschemas/core/openapi.yamlincludes Reconciled and Finalized changesschemas/gcp/openapi.yamlincludes Reconciled and Finalized changesaliases.tspsymlink not modified in commitSummary by CodeRabbit
Release Notes
New Features
Deprecations
General