Skip to content

HYPERFLEET-707 - doc: update E2E adapter configs to add time-based stability precondition#43

Open
86254860 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
86254860:HYPERFLEET-707
Open

HYPERFLEET-707 - doc: update E2E adapter configs to add time-based stability precondition#43
86254860 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
86254860:HYPERFLEET-707

Conversation

@86254860
Copy link
Contributor

@86254860 86254860 commented Mar 12, 2026

Summary by CodeRabbit

  • New Features

    • Added a TTL-based self-healing check (300s) that gates validations.
    • Validation now succeeds when a resource is non-ready or when the TTL has elapsed.
    • Applies to deployments, jobs, namespaces, and configmaps.
  • Refactor

    • Readiness capture logic renamed to reflect non-ready semantics and consolidated into the new validation flow.

@openshift-ci openshift-ci bot requested review from jsell-rh and rh-amarin March 12, 2026 10:07
@openshift-ci
Copy link

openshift-ci bot commented Mar 12, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign xueli181114 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2026

Walkthrough

The PR replaces readyConditionStatus captures with boolean-style captures named clusterNotReady/nodepoolNotReady that evaluate whether a Ready condition is absent or not "True". It adds TTL-based preconditions (clusterReadyTTL / nodepoolReadyTTL) computing seconds since the Ready condition's last_transition_time and comparing to 300 seconds. A new validationCheck precondition (nodepool/cluster) combines the non-ready boolean or TTL expiry (e.g., clusterNotReady || clusterReadyTTL). Structured conditions blocks referencing readyConditionStatus were removed and validationCheck was added to public preconditions.

Sequence Diagram(s)

(removed)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: updating E2E adapter configs to add time-based stability preconditions across multiple configuration files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@testdata/adapter-configs/cl-job/adapter-task-config.yaml`:
- Around line 52-63: The validationCheck expression currently calls
timestamp(readyConditionLastTransitionTime) even when
readyConditionLastTransitionTime == "", causing runtime errors; update the
validationCheck (and the equivalent matching expressions in sibling configs) to
guard the TTL comparison by first ensuring readyConditionLastTransitionTime is
non-empty (e.g., require readyConditionLastTransitionTime != "" before computing
timestamp(...)) so the duration/timestamp logic only runs when a valid
transition time exists; reference the symbols readyConditionLastTransitionTime,
validationCheck, readyConditionStatus, currentTime, and TTL_EXPIRY_THRESHOLD
when locating and updating the expressions.

In `@testdata/adapter-configs/np-configmap/adapter-task-config.yaml`:
- Around line 55-56: The inline comment next to the resource named
"validationCheck" incorrectly refers to the "cluster" response; update the
wording to refer to the "nodepool" response instead so the documentation matches
the actual precondition logic (e.g., change "cluster is NOT Ready OR if cluster
is Ready and stable..." to "nodepool is NOT Ready OR if nodepool is Ready and
stable...") for the validationCheck entry.
- Around line 41-43: The block-scalar expression for the template named
currentTime uses YAML-escaped quotes (\"...) which are preserved by the '|'
block scalar and end up in the rendered template; remove the escaped quotes so
the expression matches the inline observed_time template (i.e. use unescaped
quotes within the scalar or switch to an inline string) in the expression field
for currentTime across all adapter configs; update the expression value under
the currentTime name (the block-scalar '|', the expression key, and the
currentTime identifier) so the produced RFC3339 timestamp has actual quotes
rather than backslash-escaped ones.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 647cc15f-90fe-4299-8df6-c90ca164fd38

📥 Commits

Reviewing files that changed from the base of the PR and between 10d9732 and 8419f90.

📒 Files selected for processing (4)
  • testdata/adapter-configs/cl-deployment/adapter-task-config.yaml
  • testdata/adapter-configs/cl-job/adapter-task-config.yaml
  • testdata/adapter-configs/cl-namespace/adapter-task-config.yaml
  • testdata/adapter-configs/np-configmap/adapter-task-config.yaml

@86254860 86254860 force-pushed the HYPERFLEET-707 branch 2 times, most recently from 398462c to 235306c Compare March 13, 2026 06:28
@86254860 86254860 changed the title HYPERFLEET-707 - doc: update E2E adapter configs to add time-based precondition for resource deletion recovery HYPERFLEET-707 - doc: update E2E adapter configs to add time-based stability precondition Mar 13, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
testdata/adapter-configs/cl-namespace/adapter-task-config.yaml (1)

44-53: Reduce duplicated Ready-condition filtering to improve maintainability.

The same status.conditions.filter(c, c.type == "Ready") logic is repeated multiple times across captures. Consider capturing the Ready condition once and reusing it in both clusterNotReady and clusterReadyTTL to keep this easier to maintain and less error-prone.
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 the current code and only fix it if needed.

In `@testdata/adapter-configs/cl-namespace/adapter-task-config.yaml` around lines
44 - 53, Extract the repeated ready-condition filter into a single local capture
and reuse it in both clusterNotReady and clusterReadyTTL: create a temporary
binding (e.g., readyConds or readyCondition) that evaluates
status.conditions.filter(c, c.type == "Ready") once, then replace subsequent
uses in the clusterNotReady expression and the clusterReadyTTL expression to
reference that binding (using readyConds.size() > 0 and readyConds[0].status /
last_transition_time as appropriate) so the Ready logic is computed once and
maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@testdata/adapter-configs/cl-namespace/adapter-task-config.yaml`:
- Around line 44-53: Extract the repeated ready-condition filter into a single
local capture and reuse it in both clusterNotReady and clusterReadyTTL: create a
temporary binding (e.g., readyConds or readyCondition) that evaluates
status.conditions.filter(c, c.type == "Ready") once, then replace subsequent
uses in the clusterNotReady expression and the clusterReadyTTL expression to
reference that binding (using readyConds.size() > 0 and readyConds[0].status /
last_transition_time as appropriate) so the Ready logic is computed once and
maintained in one place.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 582160a6-330c-4f59-9129-762f1f580103

📥 Commits

Reviewing files that changed from the base of the PR and between 57c8f77 and 254d6b9.

📒 Files selected for processing (4)
  • testdata/adapter-configs/cl-deployment/adapter-task-config.yaml
  • testdata/adapter-configs/cl-job/adapter-task-config.yaml
  • testdata/adapter-configs/cl-namespace/adapter-task-config.yaml
  • testdata/adapter-configs/np-configmap/adapter-task-config.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
  • testdata/adapter-configs/cl-job/adapter-task-config.yaml
  • testdata/adapter-configs/np-configmap/adapter-task-config.yaml
  • testdata/adapter-configs/cl-deployment/adapter-task-config.yaml

@rafabene
Copy link
Contributor

The JIRA acceptance criteria says to update the k8s related adapters in hyperfleet-e2e — was cl-maestro intentionally excluded?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants