HYPERFLEET-707 - doc: update adapter configs to add time-based stability precondition#20
Merged
xueli181114 merged 3 commits intoopenshift-hyperfleet:mainfrom Mar 18, 2026
Merged
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:
WalkthroughTwo Helm adapter task configs replace a Ready string-status check with a boolean not-ready precondition and a TTL-based precondition: Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/adapter3/adapter-task-config.yaml`:
- Around line 64-67: Update the inline comment above the precondition expression
to reflect the actual comparison used in the expression: change the text to
indicate ">= TTL_EXPIRY_THRESHOLD seconds" (i.e., passing when stable for
greater than or equal to the threshold) so the comment matches the logic using
readyCondition, readyCondition.last_transition_time and TTL_EXPIRY_THRESHOLD in
the expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fa61e23-328c-4483-9b6c-bb7dd38ec389
📒 Files selected for processing (2)
helm/adapter1/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/adapter1/adapter-task-config.yaml`:
- Around line 42-53: clusterNotReady and clusterReadyTTL assume
status.conditions and last_transition_time exist; make the CEL expressions
null-safe by first checking status.conditions != null and that the filtered list
has >0 elements before indexing, and for clusterReadyTTL also check the
existence of last_transition_time and fall back to now() if missing. Update the
clusterNotReady expression used in the "clusterNotReady" rule to:
status.conditions != null && status.conditions.filter(c, c.type ==
"Ready").size() > 0 ? status.conditions.filter(c, c.type == "Ready")[0].status
!= "True" : true; and update "clusterReadyTTL" to compute the timestamp using a
guarded last_transition_time: (timestamp(now()) - timestamp(status.conditions !=
null && status.conditions.filter(c, c.type == "Ready").size() > 0 &&
status.conditions.filter(c, c.type == "Ready")[0].last_transition_time != null ?
status.conditions.filter(c, c.type == "Ready")[0].last_transition_time :
now())).getSeconds() >= 300 so accesses in clusterReadyTTL and clusterNotReady
(status.conditions and last_transition_time) are protected.
In `@helm/adapter3/adapter-task-config.yaml`:
- Around line 47-58: The CEL expressions for nodepoolNotReady and
nodepoolReadyTTL assume status.conditions and last_transition_time exist; make
them defensive by first checking status and conditions are present and non-empty
and that last_transition_time is present before indexing or calling timestamp.
Update the nodepoolNotReady expression to use a guard like status != null &&
status.conditions != null && status.conditions.size() > 0 ?
status.conditions.filter(c, c.type == "Ready")[0].status != "True" : true, and
update nodepoolReadyTTL to compute the reference time only when
status/conditions and last_transition_time exist (fallback to now() otherwise),
e.g. check status != null && status.conditions != null &&
status.conditions.size() > 0 && status.conditions.filter(c, c.type ==
"Ready")[0].last_transition_time != null ? timestamp(status.conditions.filter(c,
c.type == "Ready")[0].last_transition_time) : timestamp(now()), then compare
getSeconds() >= 300; apply these guards in the nodepoolNotReady and
nodepoolReadyTTL expressions to avoid runtime errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6898181-f2c9-48b0-9b31-0822173bc160
📒 Files selected for processing (2)
helm/adapter1/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
…dition for resource deletion recovery
…gs for more readable
86254860
force-pushed
the
HYPERFLEET-707
branch
from
04cff6b to
6ddc7c9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
helm/adapter3/adapter-task-config.yaml (1)
45-54:⚠️ Potential issue | 🟠 MajorAdd null-safe guards in CEL readiness checks.
Line 45 and Line 51 assume
status.conditionsexists, and Line 52 assumeslast_transition_timeexists. Missing fields can make precondition evaluation fail at runtime instead of safely falling back.Suggested hardening
- name: "nodepoolNotReady" expression: | - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].status != "True" + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].status != "True" : true - name: "nodepoolReadyTTL" expression: | (timestamp(now()) - timestamp( - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 && + has(status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time) + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time : now() )).getSeconds() > TTL_EXPIRY_THRESHOLDAs 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 the current code and only fix it if needed. In `@helm/adapter3/adapter-task-config.yaml` around lines 45 - 54, The CEL expressions in the nodepoolReadyTTL check assume status.conditions and each condition.last_transition_time exist; make them null-safe by first testing that status.conditions is defined and non-empty and that the Ready condition and its last_transition_time are present before accessing them (used in the expression for nodepoolReadyTTL and the Ready readiness check), and fall back to now() or true when those fields are missing; update the expressions referencing status.conditions, the Ready condition lookup, and last_transition_time so they short-circuit on null/empty values and continue to compare against TTL_EXPIRY_THRESHOLD safely.helm/adapter1/adapter-task-config.yaml (1)
40-49:⚠️ Potential issue | 🟠 MajorHarden
clusterNotReady/clusterReadyTTLagainst partial status payloads.Line 40, Line 46, and Line 47 access nested readiness fields without null-safe guards. If
status.conditionsorlast_transition_timeis missing, CEL evaluation can fail and block the task.Suggested hardening
- name: "clusterNotReady" expression: | - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].status != "True" + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].status != "True" : true - name: "clusterReadyTTL" expression: | (timestamp(now()) - timestamp( - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 && + has(status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time) + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time : now() )).getSeconds() > TTL_EXPIRY_THRESHOLDAs 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 the current code and only fix it if needed. In `@helm/adapter1/adapter-task-config.yaml` around lines 40 - 49, The expressions for clusterNotReady and clusterReadyTTL access nested fields like status.conditions and last_transition_time without null-safe checks, risking failures if these fields are missing. Update these expressions in the adapter-task-config.yaml to include null or existence checks before accessing nested properties, ensuring the CEL evaluation does not fail on partial or missing status data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@helm/adapter1/adapter-task-config.yaml`:
- Around line 40-49: The expressions for clusterNotReady and clusterReadyTTL
access nested fields like status.conditions and last_transition_time without
null-safe checks, risking failures if these fields are missing. Update these
expressions in the adapter-task-config.yaml to include null or existence checks
before accessing nested properties, ensuring the CEL evaluation does not fail on
partial or missing status data.
In `@helm/adapter3/adapter-task-config.yaml`:
- Around line 45-54: The CEL expressions in the nodepoolReadyTTL check assume
status.conditions and each condition.last_transition_time exist; make them
null-safe by first testing that status.conditions is defined and non-empty and
that the Ready condition and its last_transition_time are present before
accessing them (used in the expression for nodepoolReadyTTL and the Ready
readiness check), and fall back to now() or true when those fields are missing;
update the expressions referencing status.conditions, the Ready condition
lookup, and last_transition_time so they short-circuit on null/empty values and
continue to compare against TTL_EXPIRY_THRESHOLD safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44ced8de-ee11-43ce-ad95-b942940698c6
📒 Files selected for processing (2)
helm/adapter1/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
86254860
force-pushed
the
HYPERFLEET-707
branch
from
dfd0a4d to
3781b96
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
helm/adapter3/adapter-task-config.yaml (1)
37-48:⚠️ Potential issue | 🟠 MajorAdd null-safe guards in readiness CEL expressions to prevent runtime failures.
Line 39 and Line 45/46 index into readiness data without guarding missing fields, which can break precondition evaluation on partial responses.
Suggested fix
- name: "nodepoolNotReady" expression: | - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].status != "True" + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].status != "True" : true - name: "nodepoolReadyTTL" expression: | (timestamp(now()) - timestamp( - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 && + has(status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time) + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time : now() )).getSeconds() > 300As 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 the current code and only fix it if needed. In `@helm/adapter3/adapter-task-config.yaml` around lines 37 - 48, The CEL expressions for the conditions "nodepoolNotReady" and "nodepoolReadyTTL" index into readiness arrays without null-safe guards; update both expressions so they first verify the Ready-condition element exists and its fields are non-null before indexing: for "nodepoolNotReady" ensure size() > 0 and that the selected element has a non-null status before comparing to "True", and for "nodepoolReadyTTL" ensure size() > 0 and that the selected element has a non-null last_transition_time (falling back to now() only if missing) before computing timestamp(now()) - timestamp(...); apply these guards to avoid runtime evaluation failures when fields are missing.helm/adapter1/adapter-task-config.yaml (1)
32-43:⚠️ Potential issue | 🟠 MajorHarden CEL preconditions against missing
status.conditions/last_transition_time.Line 34 and Line 40/41 assume fields always exist; partial API payloads can fail precondition evaluation instead of falling back safely.
Suggested fix
- name: "clusterNotReady" expression: | - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].status != "True" + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].status != "True" : true - name: "clusterReadyTTL" expression: | (timestamp(now()) - timestamp( - status.conditions.filter(c, c.type == "Ready").size() > 0 - ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time + status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 && + has(status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time) + ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time : now() )).getSeconds() > 300As 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 the current code and only fix it if needed. In `@helm/adapter1/adapter-task-config.yaml` around lines 32 - 43, The CEL preconditions for clusterNotReady and clusterReadyTTL assume status.conditions and last_transition_time exist; update both expressions in the rules named "clusterNotReady" and "clusterReadyTTL" to null-safe check status.conditions (e.g., ensure status.conditions != null and is a list or use a size() guard) before filtering, and when reading the Ready condition use a fallback if last_transition_time is missing (e.g., treat missing last_transition_time as now()); in short, add explicit existence checks for status.conditions and last_transition_time in the clusterNotReady and clusterReadyTTL expressions so evaluation falls back safely instead of failing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@helm/adapter1/adapter-task-config.yaml`:
- Around line 32-43: The CEL preconditions for clusterNotReady and
clusterReadyTTL assume status.conditions and last_transition_time exist; update
both expressions in the rules named "clusterNotReady" and "clusterReadyTTL" to
null-safe check status.conditions (e.g., ensure status.conditions != null and is
a list or use a size() guard) before filtering, and when reading the Ready
condition use a fallback if last_transition_time is missing (e.g., treat missing
last_transition_time as now()); in short, add explicit existence checks for
status.conditions and last_transition_time in the clusterNotReady and
clusterReadyTTL expressions so evaluation falls back safely instead of failing.
In `@helm/adapter3/adapter-task-config.yaml`:
- Around line 37-48: The CEL expressions for the conditions "nodepoolNotReady"
and "nodepoolReadyTTL" index into readiness arrays without null-safe guards;
update both expressions so they first verify the Ready-condition element exists
and its fields are non-null before indexing: for "nodepoolNotReady" ensure
size() > 0 and that the selected element has a non-null status before comparing
to "True", and for "nodepoolReadyTTL" ensure size() > 0 and that the selected
element has a non-null last_transition_time (falling back to now() only if
missing) before computing timestamp(now()) - timestamp(...); apply these guards
to avoid runtime evaluation failures when fields are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01e02e1c-f6a5-4c3b-b000-9994760e96e0
📒 Files selected for processing (4)
helm/adapter1/adapter-task-config.yamlhelm/adapter1/values.yamlhelm/adapter3/adapter-task-config.yamlhelm/adapter3/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/adapter1/values.yaml
Contributor
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Configuration Updates
Improvements