HYPERFLEET-568- ci: Fix linting errors caused by the OpenAPI schema update#16
Conversation
WalkthroughThis PR replaces phase-based readiness checks with condition-based checks across helpers and e2e tests. New waiter functions were added: WaitForClusterCondition, WaitForAdapterCondition, WaitForAllAdapterConditions, and WaitForNodePoolCondition; they inspect resource Status.Conditions (and adapter-specific conditions) for matching (Type, Status) pairs. Tests were updated to use the new condition-based waiters; an initial cluster phase assertion was deferred inside a TODO block and an unused openapi import removed. A new exported constant Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as rgba(6,86,147,0.5) Test
participant Helper as rgba(0,153,68,0.5) Helper
participant API as rgba(153,51,255,0.5) API Server
participant Resource as rgba(204,102,0,0.5) Cluster/NodePool
participant Adapter as rgba(220,20,60,0.5) Adapter Status
Test->>Helper: WaitForClusterCondition(clusterID, "Ready", expectedStatus, timeout)
Helper->>API: GET /clusters/{clusterID}
API-->>Helper: cluster.Status (Conditions[])
alt matching condition found
Helper-->>Test: return nil
else no match
loop until timeout
Helper->>API: GET /clusters/{clusterID}
API-->>Helper: cluster.Status (Conditions[])
end
Helper-->>Test: timeout error
end
Test->>Helper: WaitForAllAdapterConditions(clusterID, "Applied", expectedStatus, timeout)
Helper->>API: GET /clusters/{clusterID}/statuses
API-->>Helper: [adapterStatuses]
Helper->>Adapter: inspect each adapter.Status.Conditions for (Type, Status)
alt all adapters match
Helper-->>Test: return nil
else
Helper-->>Test: timeout/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
yingzhanredhat
changed the title
yingzhanredhat
force-pushed
the
ying-test
branch
2 times, most recently
from
94b54a6 to
aa54863
Compare
yingzhanredhat
force-pushed
the
ying-test
branch
from
aa54863 to
f9c3754
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helper/wait.go (1)
41-74:⚠️ Potential issue | 🔴 CriticalReplace non-existent
HasConditioncalls withHasAdapterCondition.
The code callsh.HasCondition()which does not exist as a method on Helper. OnlyHasAdapterCondition(conditions []openapi.AdapterCondition, condType string, status openapi.AdapterConditionStatus) boolis defined. This causes compile-time errors.🔧 Required changes
- hasCondition := h.HasCondition(status.Conditions, condType, expectedStatus) + hasCondition := h.HasAdapterCondition(status.Conditions, condType, expectedStatus)- hasCondition := h.HasCondition(adapterStatus.Conditions, condType, expectedStatus) + hasCondition := h.HasAdapterCondition(adapterStatus.Conditions, condType, expectedStatus)
🧹 Nitpick comments (2)
e2e/nodepool/creation.go (1)
35-52: Useclient.ConditionTypeReadyinstead of"Ready"literals.
Keeps condition types centralized and avoids drift.♻️ Suggested refactor
- err = h.WaitForClusterCondition(ctx, clusterID, "Ready", openapi.ResourceConditionStatusTrue, h.Cfg.Timeouts.Cluster.Ready) + err = h.WaitForClusterCondition(ctx, clusterID, client.ConditionTypeReady, openapi.ResourceConditionStatusTrue, h.Cfg.Timeouts.Cluster.Ready)- err = h.WaitForNodePoolCondition(ctx, clusterID, nodepoolID, "Ready", openapi.ResourceConditionStatusTrue, h.Cfg.Timeouts.NodePool.Ready) + err = h.WaitForNodePoolCondition(ctx, clusterID, nodepoolID, client.ConditionTypeReady, openapi.ResourceConditionStatusTrue, h.Cfg.Timeouts.NodePool.Ready)- hasReady := h.HasResourceCondition(finalNodePool.Status.Conditions, client.ConditionTypeReady, openapi.ResourceConditionStatusTrue) + hasReady := h.HasResourceCondition(finalNodePool.Status.Conditions, client.ConditionTypeReady, openapi.ResourceConditionStatusTrue)Also applies to: 81-83
e2e/cluster/creation.go (1)
36-73: Replace the commented-out phase checks with condition-based readiness.
The TODO block disables readiness assertions, so this test can pass even if the cluster never becomes Ready. Consider using the new condition-based helpers to restore coverage.✅ Suggested replacement (condition-based)
- /** <TODO> - Expect(cluster.Status.Phase).To(Equal(openapi.NotReady), "cluster should be in NotReady phase initially") - - Cluster final status depends on all deployed adapter result, this is still in progress. - Will update this part once adapter scope is finalized. - ginkgo.By("monitoring cluster status - waiting for phase transition to Ready") - err = h.WaitForClusterPhase(ctx, clusterID, openapi.Ready, h.Cfg.Timeouts.Cluster.Ready) - Expect(err).NotTo(HaveOccurred(), "cluster should reach Ready phase") - - ginkgo.By("verifying all adapter conditions via /clusters/{id}/statuses endpoint") - const expectedAdapterCount = 1 // GCP cluster expects 1 adapter - Eventually(func(g Gomega) { - statuses, err := h.Client.GetClusterStatuses(ctx, clusterID) - g.Expect(err).NotTo(HaveOccurred(), "failed to get cluster statuses") - g.Expect(statuses.Items).To(HaveLen(expectedAdapterCount), - "expected %d adapter(s), got %d", expectedAdapterCount, len(statuses.Items)) - - for _, adapter := range statuses.Items { - hasApplied := h.HasCondition(adapter.Conditions, client.ConditionTypeApplied, openapi.True) - g.Expect(hasApplied).To(BeTrue(), - "adapter %s should have Applied=True", adapter.Adapter) - - hasAvailable := h.HasCondition(adapter.Conditions, client.ConditionTypeAvailable, openapi.True) - g.Expect(hasAvailable).To(BeTrue(), - "adapter %s should have Available=True", adapter.Adapter) - - hasHealth := h.HasCondition(adapter.Conditions, client.ConditionTypeHealth, openapi.True) - g.Expect(hasHealth).To(BeTrue(), - "adapter %s should have Health=True", adapter.Adapter) - } - }, h.Cfg.Timeouts.Adapter.Processing, h.Cfg.Polling.Interval).Should(Succeed()) - - ginkgo.By("verifying final cluster state") - finalCluster, err := h.Client.GetCluster(ctx, clusterID) - Expect(err).NotTo(HaveOccurred(), "failed to get final cluster state") - Expect(finalCluster.Status).NotTo(BeNil(), "cluster status should be present") - Expect(finalCluster.Status.Phase).To(Equal(openapi.Ready), "cluster phase should be Ready") - **/ + ginkgo.By("waiting for cluster to become Ready") + err = h.WaitForClusterCondition(ctx, clusterID, client.ConditionTypeReady, openapi.ResourceConditionStatusTrue, h.Cfg.Timeouts.Cluster.Ready) + Expect(err).NotTo(HaveOccurred(), "cluster should have Ready condition set to True")📦 Import updates needed
import ( "context" "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" //nolint:staticcheck // dot import for test readability + "github.com/openshift-hyperfleet/hyperfleet-e2e/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-e2e/pkg/client" "github.com/openshift-hyperfleet/hyperfleet-e2e/pkg/helper" "github.com/openshift-hyperfleet/hyperfleet-e2e/pkg/labels" )If you want, I can draft a fuller condition-based adapter check block (matching the new adapter condition helpers).
yingzhanredhat
force-pushed
the
ying-test
branch
from
f9c3754 to
c094c66
Compare
|
@yasun1 Would you help review it?Thanks |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yasun1 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 fa76284
into
openshift-hyperfleet:main
2 participants
Summary by CodeRabbit
Refactor
Tests
New Features
✏️ Tip: You can customize this high-level summary in your review settings.