HYPERFLEET-1016 - refactor: Replace Ready condition references with Reconciled#110
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change replaces the Kubernetes condition type "Ready" with "Reconciled" across the repository. It renames captured fields and template variables (e.g., Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/config-loader/config_criteria_integration_test.go`:
- Around line 69-70: Update the misleading test text/messages that claim the
expected reconciled condition is "True" (or say “condition False”) to accurately
reflect the fixture value reconciledConditionStatus == "False"; specifically
change the comment near ctx.Set("reconciledConditionStatus", "False") and the
assertion/failure messages referenced around the block covering lines ~154–177
so they state that the precondition expects "False" (or say “condition False in
fixture”) to avoid confusing failure output. Ensure the text changes reference
the same test case and keep the actual assertion logic (which checks
reconciledConditionStatus == "False") 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 0b3f7268-46e2-4a99-bfa4-1b591032ec80
📒 Files selected for processing (25)
charts/examples/maestro-two-resources/adapter-task-config.yamlcharts/examples/maestro/README.mdcharts/examples/maestro/adapter-task-config.yamlconfigs/adapter-task-config-template.yamldocs/adapter-authoring-guide.mdinternal/configloader/README.mdinternal/configloader/validator_test.gointernal/criteria/README.mdinternal/criteria/cel_evaluator_test.gointernal/criteria/evaluator_test.gointernal/executor/README.mdtest/integration/config-loader/config_criteria_integration_test.gotest/integration/config-loader/loader_template_test.gotest/integration/config-loader/testdata/adapter-config-template.yamltest/integration/config-loader/testdata/templates/cluster-status-payload.yamltest/integration/executor/executor_integration_test.gotest/integration/executor/executor_k8s_integration_test.gotest/integration/testutil/mock_api_server.gotest/testdata/dryrun/cel-showcase/dryrun-cel-showcase-api-responses.jsontest/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yamltest/testdata/dryrun/dryrun-api-responses.jsontest/testdata/dryrun/dryrun-delete-api-responses.jsontest/testdata/dryrun/kubernetes/dryrun-kubernetes-task-config.yamltest/testdata/dryrun/maestro/dryrun-maestro-adapter-task-config.yamltest/testdata/task-config.yaml
| t.Run("preconditions fail with Reconciled condition Unknown", func(t *testing.T) { | ||
| ctx := criteria.NewEvaluationContext() | ||
| ctx.Set("readyConditionStatus", "Unknown") // Not matching expected "True" | ||
| ctx.Set("reconciledConditionStatus", "Unknown") // Not matching expected "True" |
There was a problem hiding this comment.
Nit: Comment consistency (non-blocking)
Minor inconsistency in the comment.
The loaded test config expects reconciledConditionStatus == "False":
// NOTE: reconciledConditionStatus must match the condition in the template (False)
For consistency, this could arguably be:
ctx.Set("reconciledConditionStatus", "Unknown") // Does not match expected "False"
There was a problem hiding this comment.
Thanks for flagging this @ldornele . Line 177 is actually correct — the template expects "True" (line 153 in adapter-config-template.yaml), so "Unknown" does not match expected "True". The inconsistency was at line 69 where I incorrectly changed (True) to (False). Fixed that and removed the redundant comment at line 156 as well
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene 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 |
openshift-merge-bot
Bot
merged commit 5d6f1d4
into
openshift-hyperfleet:main
3 participants
Summary
c.type == "Ready"toc.type == "Reconciled"in production configs, helm examples, and test fixturesreadyConditionStatus→reconciledConditionStatus,clusterNotReady→clusterNotReconciled, etc.)The API now emits both
Ready(deprecated backward-compat shim) andReconciled(HYPERFLEET-853). This aligns adapter configs and tests with the canonical condition type.Scope
configs/adapter-task-config-template.yamlcharts/examples/maestro/,maestro-two-resources/configs and READMEtest/testdata/YAML and JSON filescel_evaluator_test.goandevaluator_test.go(6 + 4 lines only — genericctx.Set("status", "Ready")tests left unchanged)adapter-authoring-guide.md,configloader/README.md,criteria/README.md,executor/README.mdNot changed
resources.?namespace0,resources.?myResource) — these reference conditions on spoke cluster CRDs, not HyperFleet conditions"Ready"as an arbitrary test stringclusterPhase == "Ready"references — phase is separate from condition typereadyReplicas,minReadySeconds,/readyz)NamespaceReady/NamespaceNotReadyadapter status stringsAlreadyExists/already deletederror handlingTest plan
make testpasses (unit tests)make test-integrationpassesmake dryrunvalidates CEL expressions parse correctly with ReconciledSummary by CodeRabbit
Bug Fixes
Documentation
Tests