🌱 Fix integration tests and add comprehensive placement API tests #396
Conversation
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
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds six new Ginkgo/Gomega integration test suites under test/integration/api that create, validate, update, and clean up resources for AppliedManifestWork, ClusterManager, Klusterlet, ManagedCluster, ManifestWork, and Placement APIs (v1/v1beta1), exercising creation, status updates, and edge cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
qiujian16
force-pushed
the
fix/integration-tests-and-add-placement-api
branch
from
13d2ac9 to
46bb202
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: 1
🧹 Nitpick comments (24)
test/integration/api/appliedmanifestwork_test.go (3)
23-28: Tighten cleanup: ignore NotFound only, fail on other errors.Currently all errors are silently ignored. Gate only NotFound and surface unexpected failures.
Apply this diff:
ginkgo.AfterEach(func() { - err := hubWorkClient.WorkV1().AppliedManifestWorks().Delete(context.TODO(), appliedManifestWorkName, metav1.DeleteOptions{}) - if err != nil { - // Ignore not found errors in cleanup - } + err := hubWorkClient.WorkV1().AppliedManifestWorks().Delete(context.TODO(), appliedManifestWorkName, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + } })Add missing import:
import ( "context" "fmt" "github.com/onsi/ginkgo" "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" workv1 "open-cluster-management.io/api/work/v1" )
47-61: Test name vs. body mismatch.Test “should handle … applied resources” never exercises status/appliedResources. Either rename or add a minimal status update to validate the field.
80-98: Avoid setting UID in status; it’s controller-owned.UID in AppliedManifestResourceMeta is “not directly settable by a client.” Drop it to align with API semantics and reduce future breakage if server-side validation tightens.
Suggested tweak:
- Version: "v1", - UID: "test-uid-123", + Version: "v1",test/integration/api/klusterlet_test.go (3)
35-41: Use HaveOccurred for error assertions.Prefer
Expect(err).To(HaveOccurred())overNotTo(BeNil())for clarity and consistency.- _, err := operatorClient.OperatorV1().Klusterlets().Create(context.TODO(), klusterlet, metav1.CreateOptions{}) - Expect(err).NotTo(BeNil()) + _, err := operatorClient.OperatorV1().Klusterlets().Create(context.TODO(), klusterlet, metav1.CreateOptions{}) + Expect(err).To(HaveOccurred())
15-33: Add cleanup to avoid leaked Klusterlets in this suite.This Describe creates CRs without deleting them. Add AfterEach with NotFound‑tolerant delete to keep the test env clean.
Example:
AfterEach(func() { err := operatorClient.OperatorV1().Klusterlets().Delete(context.TODO(), klusterlet.Name, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { Expect(err).NotTo(HaveOccurred()) } })And import:
apierrors "k8s.io/apimachinery/pkg/api/errors"
76-125: Add cleanup for HostAlias suite as well.Same leak risk here; mirror the AfterEach pattern to delete the created Klusterlet and ignore NotFound.
test/integration/api/manifestwork_test.go (3)
409-416: Make the “empty propagation policy” test robust against CRD defaulting.Defaulting may set Foreground when DeleteOption is present but policy is omitted, making this test flaky. Force an explicit invalid value to assert validation.
- Spec: workv1.ManifestWorkSpec{ - DeleteOption: &workv1.DeleteOption{}, - }, + Spec: workv1.ManifestWorkSpec{ + DeleteOption: &workv1.DeleteOption{ + PropagationPolicy: workv1.DeletePropagationPolicyType(""), + }, + },
561-613: Minor: avoid []string trick for string pointer.Use a local var for readability.
- StatusFeedbacks: workv1.StatusFeedbackResult{ - Values: []workv1.FeedbackValue{ - { - Name: "status", - Value: workv1.FieldValue{ - Type: workv1.String, - String: &[]string{"Applied"}[0], - }, - }, - }, - }, + StatusFeedbacks: workv1.StatusFeedbackResult{ + Values: []workv1.FeedbackValue{ + { + Name: "status", + Value: func() workv1.FieldValue { + s := "Applied" + return workv1.FieldValue{Type: workv1.String, String: &s} + }(), + }, + }, + },
18-37: Add cleanup for this Describe to prevent resource accumulation.This suite creates ManifestWorks but doesn’t delete them. Add an AfterEach like the enhanced suite below.
Example:
ginkgo.AfterEach(func() { err := hubWorkClient.WorkV1().ManifestWorks(testNamespace).Delete(context.TODO(), manifestWorkName, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { gomega.Expect(err).NotTo(gomega.HaveOccurred()) } })Import if missing:
apierrors "k8s.io/apimachinery/pkg/api/errors"test/integration/api/placement_test.go (1)
159-175: Optional: add a negative NumberOfClusters test (<=0) to lock validation.Covers a common misconfig and guards against regressions in CRD validation.
test/integration/api/clustermanager_test.go (5)
16-35: Add cleanup for this suite to avoid leaked ClusterManagers.Resources created here aren’t deleted. Add AfterEach with NotFound‑tolerant delete.
Example:
AfterEach(func() { err := operatorClient.OperatorV1().ClusterManagers().Delete(context.TODO(), clusterManagerName, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { Expect(err).NotTo(HaveOccurred()) } })Import:
apierrors "k8s.io/apimachinery/pkg/api/errors"
53-123: Also add cleanup for the Hosted mode suite.Same reasoning; add an AfterEach to delete the created CR.
157-177: Typo in test description.“euqually” → “equally”.
- It("should have euqually value after create", func() { + It("should have equally valued fields after create", func() {
180-199: Add cleanup for RegistrationConfiguration suite.Mirror the NotFound‑tolerant deletion pattern to avoid accumulating CRs.
312-331: Add cleanup for WorkConfiguration suite.Same leak risk; add an AfterEach to delete CRs created in this block.
test/integration/api/managedcluster_test.go (9)
4-16: Don't swallow cleanup errors; ignore only NotFound.Cleanup should fail the test on unexpected errors.
Apply:
import ( "context" "fmt" "time" "github.com/onsi/ginkgo" "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" clusterv1 "open-cluster-management.io/api/cluster/v1" )ginkgo.AfterEach(func() { - err := hubClusterClient.ClusterV1().ManagedClusters().Delete(context.TODO(), clusterName, metav1.DeleteOptions{}) - if err != nil { - // Ignore not found errors in cleanup - } + err := hubClusterClient.ClusterV1().ManagedClusters().Delete(context.TODO(), clusterName, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + } })Also applies to: 122-127
348-352: Fix test name vs. patch type mismatch.The test uses JSON Merge Patch, not Strategic Merge Patch (not supported for CRDs).
-ginkgo.It("should support strategic merge patch for taints", func() { +ginkgo.It("should support merge patch for taints", func() {
101-111: Assert the patched taint value to avoid false positives.Verify value changed to "testnew".
_, err = hubClusterClient.ClusterV1().ManagedClusters().Patch( context.TODO(), cluster.Name, types.MergePatchType, []byte(`{"spec":{"taints":[{"key":"test.io/test","value":"testnew","effect":"NoSelect"}]}}`), metav1.PatchOptions{}, ) gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Verify patch effect + cluster, err = hubClusterClient.ClusterV1().ManagedClusters().Get(context.TODO(), cluster.Name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(cluster.Spec.Taints).To(gomega.HaveLen(1)) + gomega.Expect(cluster.Spec.Taints[0].Value).To(gomega.Equal("testnew"))
192-193: Use HaveLen matcher for clarity.- gomega.Expect(len(cluster.Spec.ManagedClusterClientConfigs)).Should(gomega.Equal(2)) + gomega.Expect(cluster.Spec.ManagedClusterClientConfigs).Should(gomega.HaveLen(2))- gomega.Expect(len(cluster.Spec.Taints)).Should(gomega.Equal(2)) + gomega.Expect(cluster.Spec.Taints).Should(gomega.HaveLen(2))Also applies to: 382-383
54-54: Correct the comment: value is optional.- // key, value, effect is required + // key and effect are required; value is optional
319-319: Prefer metav1.Now() over NewTime(time.Now()).-now := metav1.NewTime(time.Now()) +now := metav1.Now()
324-331: Simplify quantities with MustParse and explicit units.- Allocatable: clusterv1.ResourceList{ - "cpu": *resource.NewQuantity(4, resource.DecimalSI), - "memory": *resource.NewQuantity(8*1024*1024*1024, resource.BinarySI), - }, - Capacity: clusterv1.ResourceList{ - "cpu": *resource.NewQuantity(4, resource.DecimalSI), - "memory": *resource.NewQuantity(8*1024*1024*1024, resource.BinarySI), - }, + Allocatable: clusterv1.ResourceList{ + "cpu": resource.MustParse("4"), + "memory": resource.MustParse("8Gi"), + }, + Capacity: clusterv1.ResourceList{ + "cpu": resource.MustParse("4"), + "memory": resource.MustParse("8Gi"), + },
130-130: Rename for accuracy.-ginkgo.It("should accept client config without validation", func() { +ginkgo.It("should store client config URL unchanged", func() {
166-168: Also assert CABundle round-trips.-_, err := hubClusterClient.ClusterV1().ManagedClusters().Create(context.TODO(), managedCluster, metav1.CreateOptions{}) -gomega.Expect(err).ToNot(gomega.HaveOccurred()) +createdCluster, err := hubClusterClient.ClusterV1().ManagedClusters().Create(context.TODO(), managedCluster, metav1.CreateOptions{}) +gomega.Expect(err).ToNot(gomega.HaveOccurred()) +gomega.Expect(createdCluster.Spec.ManagedClusterClientConfigs[0].CABundle).Should(gomega.Equal([]byte("dummy-ca-bundle")))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
test/integration/api/appliedmanifestwork_test.go(1 hunks)test/integration/api/clustermanager_test.go(2 hunks)test/integration/api/klusterlet_test.go(2 hunks)test/integration/api/managedcluster_test.go(2 hunks)test/integration/api/manifestwork_test.go(2 hunks)test/integration/api/placement_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
test/integration/api/appliedmanifestwork_test.go (1)
work/v1/types.go (5)
AppliedManifestWork(668-678)AppliedManifestWorkSpec(681-693)AppliedManifestWorkStatus(696-712)AppliedManifestResourceMeta(449-462)ResourceIdentifier(391-411)
test/integration/api/placement_test.go (2)
cluster/v1beta1/types_placement.go (6)
Placement(41-53)PlacementSpec(58-103)ClusterPredicate(169-178)ClusterSelector(182-194)Toleration(369-400)TolerationOpExists(407-407)cluster/v1/types.go (1)
TaintEffectNoSelect(124-124)
test/integration/api/klusterlet_test.go (4)
operator/v1/types_klusterlet.go (7)
Klusterlet(19-28)KlusterletSpec(31-103)ServerURL(106-115)KlusterletDeployOption(335-346)RegistrationConfiguration(133-189)WorkAgentConfiguration(277-327)KlusterletStatus(349-370)cluster/v1beta1/types_placement.go (2)
Toleration(369-400)TolerationOpExists(407-407)operator/v1/types_clustermanager.go (7)
InstallModeDefault(441-441)FeatureGate(311-324)FeatureGateModeTypeEnable(330-330)WorkConfiguration(260-285)InstallModeHosted(445-445)GenerationStatus(503-527)RelatedResourceMeta(479-499)operator/v1/type_resourcerequirement.go (2)
ResourceRequirement(11-18)ResourceQosClassResourceRequirement(28-28)
test/integration/api/manifestwork_test.go (1)
work/v1/types.go (26)
String(591-591)ManifestWork(18-28)Manifest(60-64)DeleteOption(73-95)DeletePropagationPolicyTypeForeground(373-373)DeletePropagationPolicyTypeOrphan(376-376)ManifestConfigOption(98-120)ResourceIdentifier(391-411)UpdateStrategy(223-242)ServerSideApplyConfig(274-291)FeedbackRule(313-329)JSONPathsType(341-341)JsonPath(344-365)ManifestWorkExecutor(173-177)ManifestWorkExecutorSubject(182-193)ExecutorSubjectTypeServiceAccount(201-201)ManifestWorkSubjectServiceAccount(205-220)ManifestWorkStatus(465-479)WorkApplied(500-500)ManifestResourceStatus(483-492)ManifestCondition(524-536)ManifestResourceMeta(417-445)StatusFeedbackResult(539-545)FeedbackValue(547-559)FieldValue(563-584)ManifestApplied(601-601)
test/integration/api/managedcluster_test.go (1)
cluster/v1/types.go (10)
ManagedCluster(34-44)ManagedClusterSpec(48-77)ClientConfig(81-90)Taint(94-116)TaintEffectNoSelect(124-124)TaintEffectNoSelectIfNew(131-131)ManagedClusterStatus(146-175)ManagedClusterVersion(179-183)ResourceList(249-249)ManagedClusterConditionAvailable(232-232)
test/integration/api/clustermanager_test.go (3)
operator/v1/types_clustermanager.go (15)
ClusterManager(18-29)ClusterManagerSpec(32-84)NodePlacement(87-97)ClusterManagerDeployOption(413-433)InstallModeDefault(441-441)RegistrationHubConfiguration(99-123)FeatureGate(311-324)FeatureGateModeTypeEnable(330-330)WorkConfiguration(260-285)WorkDriverTypeKube(292-292)AddOnManagerConfiguration(299-309)ServerConfiguration(166-176)ClusterManagerStatus(455-476)GenerationStatus(503-527)RelatedResourceMeta(479-499)cluster/v1beta1/types_placement.go (2)
Toleration(369-400)TolerationOpExists(407-407)operator/v1/type_resourcerequirement.go (2)
ResourceRequirement(11-18)ResourceQosClassResourceRequirement(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: verify
Comment on lines
+24
to
+30
| ginkgo.AfterEach(func() { | ||
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| // Ignore not found errors during cleanup | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Cleanup block contradicts comment; it will fail on any error.
You intend to ignore NotFound, but Expect(err).ToNot(HaveOccurred()) runs only when err != nil, causing failure. Gate on NotFound.
Apply this diff:
ginkgo.AfterEach(func() {
err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{})
- if err != nil {
- // Ignore not found errors during cleanup
- gomega.Expect(err).ToNot(gomega.HaveOccurred())
- }
+ if err != nil && !apierrors.IsNotFound(err) {
+ gomega.Expect(err).ToNot(gomega.HaveOccurred())
+ }
})Add missing import:
import (
"context"
"fmt"
"github.com/onsi/ginkgo"
"github.com/onsi/gomega"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/rand"
clusterv1 "open-cluster-management.io/api/cluster/v1"
clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ginkgo.AfterEach(func() { | |
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | |
| if err != nil { | |
| // Ignore not found errors during cleanup | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| } | |
| }) | |
| import ( | |
| "context" | |
| "fmt" | |
| "github.com/onsi/ginkgo" | |
| "github.com/onsi/gomega" | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/util/rand" | |
| clusterv1 "open-cluster-management.io/api/cluster/v1" | |
| clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1" | |
| ) |
Suggested change
| ginkgo.AfterEach(func() { | |
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | |
| if err != nil { | |
| // Ignore not found errors during cleanup | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| } | |
| }) | |
| ginkgo.AfterEach(func() { | |
| err := hubClusterClient.ClusterV1beta1().Placements(testNamespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{}) | |
| if err != nil && !apierrors.IsNotFound(err) { | |
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In test/integration/api/placement_test.go around lines 24 to 30, the cleanup
currently expects no error whenever err != nil which contradicts the comment
about ignoring NotFound; change the logic to treat NotFound as acceptable and
only fail for other errors (i.e., if err != nil and !apierrors.IsNotFound(err)
then Expect(err).ToNot(HaveOccurred())); also add the missing import for k8s API
errors (k8s.io/apimachinery/pkg/api/errors) and reference it as apierrors in the
check.
- Fix ManifestWork propagation policy validation tests to match integration environment behavior - Add required lastTransitionTime fields to condition objects in status updates - Add comprehensive placement API integration tests with creation, validation, and update scenarios - Fix AppliedManifestWork tests to use proper required fields - Update ManagedCluster tests to handle validation environment limitations All 115 integration tests now pass successfully. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
fix/integration-tests-and-add-placement-api
branch
from
46bb202 to
7bc280c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/integration/api/manifestwork_test.go (2)
414-416: Prefer type-based validation over substring matchingUse errors.IsInvalid(err) instead of checking the error string. More robust and future-proof.
- gomega.Expect(err).To(gomega.HaveOccurred()) - gomega.Expect(err.Error()).Should(gomega.ContainSubstring("Unsupported value: \"\"")) + gomega.Expect(errors.IsInvalid(err)).To(gomega.BeTrue())
566-566: Use metav1.Now() and remove the time importSimplifies code and avoids an extra import.
- now := metav1.NewTime(time.Now()) + now := metav1.Now()Also remove the now-unused import:
- "time"test/integration/api/appliedmanifestwork_test.go (1)
65-99: Verify persisted status after UpdateStatusAfter UpdateStatus, GET and assert the applied resources length to ensure the update took effect.
_, err = hubWorkClient.WorkV1().AppliedManifestWorks().UpdateStatus(context.TODO(), appliedManifestWork, metav1.UpdateOptions{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + got, err := hubWorkClient.WorkV1().AppliedManifestWorks().Get(context.TODO(), appliedManifestWork.Name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(len(got.Status.AppliedResources)).Should(gomega.Equal(1))test/integration/api/managedcluster_test.go (2)
349-349: Fix test title: this uses JSON Merge Patch, not Strategic Merge PatchKeep the title accurate to avoid confusion.
- ginkgo.It("should support strategic merge patch for taints", func() { + ginkgo.It("should support JSON merge patch for taints", func() {
320-320: Use metav1.Now() and remove the time importSimplifies the timestamp assignment and avoids a dependency.
- now := metav1.NewTime(time.Now()) + now := metav1.Now()Also remove the now-unused import:
- "time"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
test/integration/api/appliedmanifestwork_test.go(1 hunks)test/integration/api/clustermanager_test.go(2 hunks)test/integration/api/klusterlet_test.go(2 hunks)test/integration/api/managedcluster_test.go(2 hunks)test/integration/api/manifestwork_test.go(2 hunks)test/integration/api/placement_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/api/placement_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
test/integration/api/manifestwork_test.go (1)
work/v1/types.go (28)
String(591-591)ManifestWork(18-28)ManifestWorkSpec(37-57)ManifestsTemplate(67-71)Manifest(60-64)DeleteOption(73-95)DeletePropagationPolicyTypeForeground(373-373)DeletePropagationPolicyTypeOrphan(376-376)ManifestConfigOption(98-120)ResourceIdentifier(391-411)UpdateStrategy(223-242)UpdateStrategyTypeServerSideApply(259-259)ServerSideApplyConfig(274-291)FeedbackRule(313-329)JsonPath(344-365)ManifestWorkExecutor(173-177)ManifestWorkExecutorSubject(182-193)ExecutorSubjectTypeServiceAccount(201-201)ManifestWorkSubjectServiceAccount(205-220)ManifestWorkStatus(465-479)WorkApplied(500-500)ManifestResourceStatus(483-492)ManifestCondition(524-536)ManifestResourceMeta(417-445)StatusFeedbackResult(539-545)FeedbackValue(547-559)FieldValue(563-584)ManifestApplied(601-601)
test/integration/api/managedcluster_test.go (1)
cluster/v1/types.go (10)
ManagedCluster(34-44)ManagedClusterSpec(48-77)ClientConfig(81-90)Taint(94-116)TaintEffectNoSelect(124-124)TaintEffectNoSelectIfNew(131-131)ManagedClusterStatus(146-175)ManagedClusterVersion(179-183)ResourceList(249-249)ManagedClusterConditionAvailable(232-232)
test/integration/api/appliedmanifestwork_test.go (1)
work/v1/types.go (5)
AppliedManifestWork(668-678)AppliedManifestWorkSpec(681-693)AppliedManifestWorkStatus(696-712)AppliedManifestResourceMeta(449-462)ResourceIdentifier(391-411)
test/integration/api/klusterlet_test.go (4)
operator/v1/types_klusterlet.go (7)
Klusterlet(19-28)KlusterletSpec(31-103)ServerURL(106-115)KlusterletDeployOption(335-346)RegistrationConfiguration(133-189)WorkAgentConfiguration(277-327)KlusterletStatus(349-370)vendor/k8s.io/api/core/v1/types.go (1)
TaintEffectNoSchedule(3822-3822)operator/v1/types_clustermanager.go (6)
InstallModeDefault(441-441)FeatureGateModeTypeEnable(330-330)WorkConfiguration(260-285)InstallModeHosted(445-445)GenerationStatus(503-527)RelatedResourceMeta(479-499)operator/v1/type_resourcerequirement.go (2)
ResourceRequirement(11-18)ResourceQosClassResourceRequirement(28-28)
test/integration/api/clustermanager_test.go (3)
operator/v1/types_clustermanager.go (15)
ClusterManager(18-29)ClusterManagerSpec(32-84)NodePlacement(87-97)ClusterManagerDeployOption(413-433)InstallModeDefault(441-441)RegistrationHubConfiguration(99-123)FeatureGate(311-324)FeatureGateModeTypeEnable(330-330)WorkConfiguration(260-285)WorkDriverTypeKube(292-292)AddOnManagerConfiguration(299-309)ServerConfiguration(166-176)ClusterManagerStatus(455-476)GenerationStatus(503-527)RelatedResourceMeta(479-499)vendor/k8s.io/api/core/v1/types.go (1)
TaintEffectNoSchedule(3822-3822)operator/v1/type_resourcerequirement.go (2)
ResourceRequirement(11-18)ResourceQosClassResourceRequirement(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: verify
Comment on lines
+424
to
+429
| AfterEach(func() { | ||
| err := operatorClient.OperatorV1().ClusterManagers().Delete(context.TODO(), clusterManagerName, metav1.DeleteOptions{}) | ||
| if !apierrors.IsForbidden(err) { | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
AfterEach should ignore NotFound as well
Avoid failing cleanup when the resource wasn’t created.
AfterEach(func() {
err := operatorClient.OperatorV1().ClusterManagers().Delete(context.TODO(), clusterManagerName, metav1.DeleteOptions{})
- if !apierrors.IsForbidden(err) {
- Expect(err).ToNot(HaveOccurred())
- }
+ if err != nil && !apierrors.IsForbidden(err) && !apierrors.IsNotFound(err) {
+ Expect(err).ToNot(HaveOccurred())
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AfterEach(func() { | |
| err := operatorClient.OperatorV1().ClusterManagers().Delete(context.TODO(), clusterManagerName, metav1.DeleteOptions{}) | |
| if !apierrors.IsForbidden(err) { | |
| Expect(err).ToNot(HaveOccurred()) | |
| } | |
| }) | |
| AfterEach(func() { | |
| err := operatorClient.OperatorV1().ClusterManagers().Delete(context.TODO(), clusterManagerName, metav1.DeleteOptions{}) | |
| - if !apierrors.IsForbidden(err) { | |
| - Expect(err).ToNot(HaveOccurred()) | |
| if err != nil && !apierrors.IsForbidden(err) && !apierrors.IsNotFound(err) { | |
| Expect(err).ToNot(HaveOccurred()) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In test/integration/api/clustermanager_test.go around lines 424 to 429, the
AfterEach cleanup currently ignores Forbidden errors but will fail if the
ClusterManager was never created; update the error handling to also ignore
NotFound errors by checking apierrors.IsNotFound(err) alongside
apierrors.IsForbidden(err) (i.e., treat nil, IsForbidden, or IsNotFound as
non-fatal) so the test cleanup does not fail when the resource does not exist.
Comment on lines
+224
to
+229
| AfterEach(func() { | ||
| err := operatorClient.OperatorV1().Klusterlets().Delete(context.TODO(), klusterletName, metav1.DeleteOptions{}) | ||
| if !apierrors.IsForbidden(err) { | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
AfterEach should ignore NotFound as well
Current cleanup fails if the resource was not created. Tolerate both Forbidden and NotFound.
AfterEach(func() {
err := operatorClient.OperatorV1().Klusterlets().Delete(context.TODO(), klusterletName, metav1.DeleteOptions{})
- if !apierrors.IsForbidden(err) {
- Expect(err).ToNot(HaveOccurred())
- }
+ if err != nil && !apierrors.IsForbidden(err) && !apierrors.IsNotFound(err) {
+ Expect(err).ToNot(HaveOccurred())
+ }
})🤖 Prompt for AI Agents
In test/integration/api/klusterlet_test.go around lines 224 to 229, the
AfterEach cleanup currently only tolerates Forbidden errors but fails when the
resource was never created (NotFound). Update the check soAfter deleting the
klusterlet you ignore both apierrors.IsForbidden(err) and
apierrors.IsNotFound(err) — i.e. only call Expect(err).ToNot(HaveOccurred())
when err is non-nil and is neither Forbidden nor NotFound — so the cleanup
tolerates both cases.
Member
|
/lgtm |
openshift-merge-bot
bot
merged commit 46de1ae
into
open-cluster-management-io:main
12 checks passed
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.
All 115 integration tests now pass successfully.
🤖 Generated with Claude Code
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit