OCPCLOUD-2992: refactor: split machineset migration testing into multiple files #390
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughCentralizes e2e initialization, adds AWS and migration helper libraries, introduces two authoritative MachineSet migration test suites (CAPI- and MAPI-authoritative), removes a legacy consolidated migration test, and updates OpenShift Machine API condition type references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as E2E Test
participant Init as InitCommonVariables
participant Helpers as Migration/AWS Helpers
participant K8s as Kubernetes API
participant AWS as AWS EC2 API
Test->>Init: InitCommonVariables()
Init->>K8s: register schemes, create client, get Infrastructure
Init-->>Test: client, ctx, platform, clusterName
Test->>Helpers: createMachineSet / createAWSMachineTemplate
Helpers->>K8s: create/update MachineSet(s) & Templates
Helpers-->>Test: resource refs
Test->>Helpers: switchAuthoritativeAPI(...)
Helpers->>K8s: patch MachineSet.authoritativeAPI & wait for conditions
Helpers-->>Test: updated status
alt AWS instance inspection (AWS platform)
Test->>Helpers: getMAPICreatedInstance / getCAPICreatedInstance
Helpers->>K8s: read providerStatus / secret for creds
Helpers->>AWS: DescribeInstances(...)
AWS-->>Helpers: instance data
Helpers->>Test: comparison result
end
Test->>Helpers: cleanup resources
Helpers->>K8s: delete MachineSets & Templates
Helpers-->>Test: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
e2e/aws_helpers.go (2)
116-129: Consider moving to AWS SDK for Go v2 (tests)aws-sdk-go v1 is EOL/maintenance; v2 is recommended. Not blocking, but plan migration for resilience and support.
Based on learnings
232-234: Optionally fail test on instance mismatchCurrently only logs diffs; consider asserting equality so regressions fail fast.
Apply this minimal change:
- if !cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...) { - GinkgoWriter.Print("Instances created by MAPI and CAPI are not equal\n" + cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...)) - } + Expect(cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...)). + To(BeTrue(), "MAPI and CAPI EC2 instances differ:\n%s", cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...))e2e/machineset_migration_capi_authoritative_test.go (1)
145-149: Avoid variable shadowing of capiMachineSet.Using := creates a new local; downstream assertions may use the stale outer var. Assign instead.
- capiMachineSet := capiframework.GetMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace) + capiMachineSet = capiframework.GetMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace)e2e/machineset_migration_mapi_authoritative_test.go (3)
192-193: Correct log message (“Scaling down” not “Scaling up”).Minor clarity fix.
- By("Scaling up CAPI MachineSet to 1") + By("Scaling down CAPI MachineSet to 1")
352-369: Verify old InfraTemplate is removed (test title says so).Add deletion assertion similar to earlier providerSpec-update test.
By("Verifying the MAPI MachineSet is updated to reflect the new template") mapiMachineSet, _ = mapiframework.GetMachineSet(ctx, cl, mapiMSAuthMAPIName) Eventually(k.Object(mapiMachineSet), capiframework.WaitMedium, capiframework.RetryMedium).Should( HaveField("Spec.Template.Spec.ProviderSpec.Value.Raw", ContainSubstring(newInstanceType)), "MAPI MachineSet providerSpec should be updated to reflect the new InfraTemplate with InstanceType %s", newInstanceType, ) + By("Verifying the old InfraTemplate is deleted") + verifyResourceRemoved(awsMachineTemplate)
120-128: Machine name equality can be brittle across mirrors.If mirroring ever diverges in naming, this will flake. Prefer asserting mirror linkage (ownerRefs/labels) or presence of corresponding mirror objects.
Also applies to: 124-128
e2e/machineset_migration_helpers.go (2)
183-212: Relax exact Synchronized message match to reduce flakiness.Messages can change; use ContainSubstring instead of Equal.
- HaveField("Message", Equal(expectedMessage)), + HaveField("Message", ContainSubstring(expectedMessage)),
328-354: Optionally wait for AWSMachineTemplate deletion.Consider waiting for templates to be fully deleted to avoid residue between tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
e2e/aws_helpers.go(1 hunks)e2e/aws_test.go(1 hunks)e2e/e2e_common.go(1 hunks)e2e/e2e_test.go(1 hunks)e2e/machineset_migration_capi_authoritative_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/migration_common.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/machineset_migration_test.go
🔇 Additional comments (8)
e2e/migration_common.go (1)
10-12: Condition type alignment with MAPI — LGTMUsing mapiv1beta1.ConditionType for Synchronized and Paused is correct for MAPI conditions.
e2e/aws_test.go (1)
29-31: Updated helper signature usage — LGTMSwitch to getDefaultAWSMAPIProviderSpec() without args matches the new helper and simplifies setup.
e2e/e2e_test.go (1)
16-17: Centralized init — LGTMInitCommonVariables() in BeforeSuite cleanly replaces scattered setup.
e2e/machineset_migration_helpers.go (3)
37-49: Confirm external params exist: infraAPIVersion/clusterName.These are referenced here; ensure they’re initialized in e2e_common.go (or similar) before use.
54-81: komega default client dependency.This file calls komega.Get/Object/Update via package-level API. Ensure komega.SetClient(cl) is set in test init; otherwise these Eventually(...) will fail.
99-162: Path for CAPI paused condition.You assert on Status.V1Beta2.Conditions. Verify that clusterv1.MachineSet in your pinned CAPI version exposes this path; otherwise switch to Status.Conditions.
e2e/machineset_migration_capi_authoritative_test.go (1)
18-26: komega default client initialization present
InitCommonVariables (called in e2e_test.go) invokes komega.SetClient and komega.SetContext in e2e/e2e_common.go.e2e/machineset_migration_mapi_authoritative_test.go (1)
21-31: Drop redundant SetClient recommendation Default komega client is already initialized in e2e/e2e_common.go’s setup.Likely an incorrect or invalid review comment.
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
e2e/aws_helpers.go (3)
111-123: Support STS session tokens in AWS credentialsMany clusters use temporary creds. Read aws_session_token from the Secret and pass it to the credentials constructor.
accessKey := secret.Data["aws_access_key_id"] Expect(accessKey).ToNot(BeNil()) secretAccessKey := secret.Data["aws_secret_access_key"] Expect(secretAccessKey).ToNot(BeNil()) + sessionToken := secret.Data["aws_session_token"] // optional awsConfig := &aws.Config{ Region: aws.String(region), - Credentials: credentials.NewStaticCredentials( - string(accessKey), - string(secretAccessKey), - "", - ), + Credentials: credentials.NewStaticCredentials( + string(accessKey), + string(secretAccessKey), + string(sessionToken), + ), }Based on learnings.
166-169: Don’t require exactly one AWSMachine; relax or select deterministicallyHaveLen(1) will break when replicas >1. Prefer Not(BeEmpty()) and select a target (e.g., newest) or filter by OwnerReference/ProviderID match.
233-235: Make instance comparison assertive (or document it’s informational)Currently only logs diffs; tests won’t fail on a mismatch. If this is meant to guard regressions, assert equality.
Example:
Expect(cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...)).To(BeTrue(), "Instances differ:\n%s", cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...))e2e/machineset_migration_capi_authoritative_test.go (1)
35-36: Scope test-only variables narrowlyinstanceType is only used in the first Context; consider moving it inside that Context/BeforeAll to limit scope.
e2e/machineset_migration_mapi_authoritative_test.go (1)
191-194: Align By() message with the actionThe step scales down to 1; message says “Scaling up”.
- By("Scaling up CAPI MachineSet to 1") + By("Scaling down CAPI MachineSet to 1")e2e/machineset_migration_helpers.go (2)
225-227: Prefer json.Unmarshal for ProviderSpec RawProviderSpec.Value.Raw is JSON; using json.Unmarshal avoids YAML round‑tripping.
- providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - Expect(yaml.Unmarshal(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed()) + providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} + Expect(json.Unmarshal(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed())
165-167: Nit: correct error messagesThese refer to “Machine” but the subject is a MachineSet.
- Expect(machineSet).NotTo(BeNil(), "Machine parameter cannot be nil") - Expect(machineSet.GetName()).NotTo(BeEmpty(), "Machine name cannot be empty") + Expect(machineSet).NotTo(BeNil(), "MachineSet parameter cannot be nil") + Expect(machineSet.GetName()).NotTo(BeEmpty(), "MachineSet name cannot be empty")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
e2e/aws_helpers.go(1 hunks)e2e/e2e_common.go(1 hunks)e2e/machineset_migration_capi_authoritative_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/e2e_common.go
🔇 Additional comments (2)
e2e/machineset_migration_capi_authoritative_test.go (1)
272-276: Good: add minimal assertion to prevent false-positive testKeeping the test active with a basic existence check is pragmatic until OCPBUGS-56897 is fixed.
e2e/machineset_migration_helpers.go (1)
37-49: Definitions present for clusterName and infraAPIVersion clusterName and infraAPIVersion are defined in e2e/e2e_common.go; no further changes required.
sunzhaohua2
force-pushed
the
machineset-split
branch
from
1824c13 to
5cfb089
Compare
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
e2e/machineset_migration_mapi_authoritative_test.go (1)
292-301: Correct DeletePolicy matcher.Switching to Equal("Random") on the string field is correct. Nice fix.
e2e/machineset_migration_helpers.go (1)
154-157: LGTM: CAPI v1beta2 conditions path is correct for Paused.Using Status.V1Beta2.Conditions for Paused matches the CAPI v1beta2 condition set discussed earlier.
🧹 Nitpick comments (12)
e2e/aws_test.go (3)
25-31: Guard AWS client region.Ensure mapiDefaultProviderSpec.Placement.Region is non-empty before creating the client; fail early if missing.
- mapiDefaultMS, mapiDefaultProviderSpec = getDefaultAWSMAPIProviderSpec() - awsClient = createAWSClient(mapiDefaultProviderSpec.Placement.Region) + mapiDefaultMS, mapiDefaultProviderSpec = getDefaultAWSMAPIProviderSpec() + Expect(mapiDefaultProviderSpec.Placement.Region).NotTo(BeEmpty(), "region must be set for AWS client") + awsClient = createAWSClient(mapiDefaultProviderSpec.Placement.Region)
33-42: Don’t Skip inside AfterEach; either return or use DeferCleanup.Calling Skip here can mask real outcomes. Prefer a guard return or move cleanup to DeferCleanup in the spec.
Option A (guard):
- AfterEach(func() { - if platform != configv1.AWSPlatformType { - Skip("Skipping AWS E2E tests") - } + AfterEach(func() { + if platform != configv1.AWSPlatformType { + return + } framework.DeleteMachineSets(ctx, cl, machineSet) framework.WaitForMachineSetsDeleted(cl, machineSet) framework.DeleteObjects(ctx, cl, awsMachineTemplate) })Option B (preferred): add DeferCleanup in the It block for awsMachineTemplate/machineSet creation.
44-66: Prefer DeferCleanup scoped to created resources.Keeps lifecycle tied to the spec and avoids reliance on global AfterEach. Also consider unique names to avoid collisions in parallel runs.
- awsMachineTemplate = newAWSMachineTemplate(mapiDefaultProviderSpec) + awsMachineTemplate = newAWSMachineTemplate(mapiDefaultProviderSpec) + DeferCleanup(func() { + framework.DeleteObjects(ctx, cl, awsMachineTemplate) + }) ... - machineSet = framework.CreateMachineSet(ctx, cl, framework.NewMachineSetParams( + machineSet = framework.CreateMachineSet(ctx, cl, framework.NewMachineSetParams( "aws-machineset", ... - )) + )) + DeferCleanup(func() { + framework.DeleteMachineSets(ctx, cl, machineSet) + framework.WaitForMachineSetsDeleted(cl, machineSet) + })Are awsMachineTemplateName/infraAPIVersion initialized by InitCommonVariables at suite startup? Please confirm.
e2e/machineset_migration_capi_authoritative_test.go (2)
142-151: Avoid shadowing capiMachineSet.Using := creates a new local variable and hides the outer capiMachineSet, which is confusing. Reuse the outer variable.
- capiMachineSet := capiframework.GetMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace) + capiMachineSet = capiframework.GetMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace)
195-197: Assert scaling result.ScaleMachineSet returns error; assert success for clearer failures.
- Expect(mapiframework.ScaleMachineSet(mapiMSAuthCAPIName, 1)).To(Succeed(), "should be able to scale down MAPI MachineSet") + Expect(mapiframework.ScaleMachineSet(mapiMSAuthCAPIName, 1)).To(Succeed(), "should be able to scale down MAPI MachineSet")If the helper already returns void, ignore; otherwise add the assertion. Based on learnings.
e2e/machineset_migration_mapi_authoritative_test.go (2)
331-337: Assert scale helper call.Add an assertion on ScaleMachineSet to surface failures immediately.
- mapiframework.ScaleMachineSet(mapiMSAuthMAPIName, 1) + Expect(mapiframework.ScaleMachineSet(mapiMSAuthMAPIName, 1)).To(Succeed(), "scaling MAPI MachineSet should be rejected and restored by controllers")If the helper does not return error, ignore. Based on learnings.
360-369: Optional: avoid re-fetch shadowing and add explicit waits.Consider using the existing komega instance for the MAPI update verification for consistency with earlier assertions, and avoid reassign-with-blank error.
- mapiMachineSet, _ = mapiframework.GetMachineSet(ctx, cl, mapiMSAuthMAPIName) - Eventually(k.Object(mapiMachineSet), capiframework.WaitMedium, capiframework.RetryMedium).Should( + mapiMachineSet, _ = mapiframework.GetMachineSet(ctx, cl, mapiMSAuthMAPIName) + Eventually(k.Object(mapiMachineSet), capiframework.WaitMedium, capiframework.RetryMedium).Should( HaveField("Spec.Template.Spec.ProviderSpec.Value.Raw", ContainSubstring(newInstanceType)), )e2e/machineset_migration_helpers.go (5)
99-132: Paused condition checks for MAPI look fine; ensure reason strings won’t flake.You assert exact Reason strings "AuthoritativeAPIMachineAPI"/"AuthoritativeAPINotMachineAPI". If these ever change upstream, tests will flake. Consider matching on Type+Status and using ContainSubstring on Reason or ignoring it with HaveField(..., Ignore()) to reduce brittleness.
164-181: Minor: naming and message nits in replicas verifier.
- Function name is verifyMachinesetReplicas; prefer verifyMachineSetReplicas for consistency.
- Error strings say “Machine” but this function verifies a MachineSet.
Optional diff (rename locally if no external callers rely on the old name):
-func verifyMachinesetReplicas(machineSet client.Object, replicas int) { - Expect(machineSet).NotTo(BeNil(), "Machine parameter cannot be nil") - Expect(machineSet.GetName()).NotTo(BeEmpty(), "Machine name cannot be empty") +func verifyMachineSetReplicas(machineSet client.Object, replicas int) { + Expect(machineSet).NotTo(BeNil(), "MachineSet parameter cannot be nil") + Expect(machineSet.GetName()).NotTo(BeEmpty(), "MachineSet name cannot be empty")If you adopt the rename, please update call sites.
222-229: Prefer json.Unmarshal for Raw providerSpec.ProviderSpec.Value.Raw is JSON. Using sigs.k8s.io/yaml works, but adds a YAML->JSON conversion step. json.Unmarshal is simpler and faster.
- providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - Expect(yaml.Unmarshal(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed()) + providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} + Expect(json.Unmarshal(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed())
269-280: Tiny log nit: double space in message.“for MAPI” has two spaces.
- By(fmt.Sprintf("Verifying there is a CAPI MachineSet mirror for MAPI MachineSet %s", machineName)) + By(fmt.Sprintf("Verifying there is a CAPI MachineSet mirror for MAPI MachineSet %s", machineName))
328-354: Optional: wait for AWSMachineTemplate deletions to finish.You delete AWSMachineTemplates without waiting. Consider an Eventually-get NotFound loop to reduce leak/ordering flakes in CI when subsequent tests reuse the same prefixes.
If desired, I can draft a small helper using client.Get with apierrors.IsNotFound checks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
e2e/aws_helpers.go(1 hunks)e2e/aws_test.go(1 hunks)e2e/e2e_common.go(1 hunks)e2e/e2e_test.go(1 hunks)e2e/machineset_migration_capi_authoritative_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/migration_common.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/machineset_migration_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/e2e_common.go
- e2e/aws_helpers.go
🔇 Additional comments (6)
e2e/migration_common.go (1)
4-15: Type alignment looks good; verify callers.Switching SynchronizedCondition and MAPIPausedCondition to mapiv1beta1.ConditionType matches the rest of the suite. Ensure all helper matchers/consumers expect mapiv1beta1.ConditionType.
e2e/machineset_migration_capi_authoritative_test.go (2)
19-26: Gating is correct and clear.AWS-only and feature-gate checks upfront are good. LGTM.
267-276: Good minimal assertion to avoid always-pass test.Keeping it active with a basic existence check is a solid middle ground until OCPBUGS-56897 is fixed. LGTM.
e2e/machineset_migration_mapi_authoritative_test.go (1)
21-31: Komega wiring LGTM.Using komega.New(cl) enables Eventually(k.Object(...)) nicely. Good choice.
e2e/e2e_test.go (1)
15-17: Review comment is incorrect—InitCommonVariables() does not return an error.The function signature at e2e/e2e_common.go:54 shows
func InitCommonVariables()with no return type, confirming it returns void. The function already implements fail-fast behavior through internalExpect()assertions (lines 56, 59, 65), which halt test execution on any error. The proposed fix would not compile in Go, as it assumes an error return type that does not exist. The original code is correct and requires no changes.Likely an incorrect or invalid review comment.
e2e/machineset_migration_helpers.go (1)
24-52: Based on the search results, BeforeEach blocks run before It blocks, and when multiple BeforeEach blocks are defined in nested Describe and Context blocks the outermost BeforeEach blocks run first. More importantly, BeforeSuite and AfterSuite are specialized suite setup nodes that must be called at the top-level of the suite and cannot be nested in containers. There can be at most one BeforeSuite node and one AfterSuite node per suite, and it is idiomatic to place the suite setup nodes in the Ginkgo bootstrap suite file.Now I need to verify the actual initialization pattern in e2e_common.go:Globals
clusterNameandinfraAPIVersionare defined and accessible in the e2e package.Based on verification of the codebase, both variables are properly declared in
e2e/e2e_common.go:
infraAPIVersionis initialized at package level as a constant with the literal value"infrastructure.cluster.x-k8s.io/v1beta2"(line 29)clusterNameis declared as a package-level string variable (line 37) and is initialized during test setup at line 67Both are accessible from
e2e/machineset_migration_helpers.gosince they are in the same package. The helper functioncreateCAPIMachineSetreferences these globals directly without parameters. Ginkgo's BeforeEach blocks are run before It blocks, and when multiple BeforeEach blocks are defined in nested Describe and Context blocks the outermost BeforeEach blocks are run first, ensuring that package setup completes before helper functions are called. No compile or initialization errors will occur.
sunzhaohua2
force-pushed
the
machineset-split
branch
from
5cfb089 to
0902a6f
Compare
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
e2e/machineset_migration_helpers.go (4)
35-36: Fix Eventually misuse: pass a retriable action (or use komega.Create).Eventually(cl.Create(...)) evaluates once and doesn’t poll. Use komega.Create or a lambda, or just Expect for a one-shot create.
-Eventually(cl.Create(ctx, awsMachineTemplate), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to create a new awsMachineTemplate %s", awsMachineTemplate.Name) +Eventually(komega.Create(awsMachineTemplate), capiframework.WaitMedium, capiframework.RetryMedium). + Should(Succeed(), "Failed to create a new awsMachineTemplate %s", awsMachineTemplate.Name)-Eventually(cl.Create(ctx, newTemplate), capiframework.WaitMedium, capiframework.RetryMedium).Should( - Succeed(), "Failed to create a new awsMachineTemplate %s", newTemplate.Name) +Eventually(komega.Create(newTemplate), capiframework.WaitMedium, capiframework.RetryMedium). + Should(Succeed(), "Failed to create a new awsMachineTemplate %s", newTemplate.Name)Also applies to: 311-313
223-230: Unmarshal ProviderSpec.Raw with json; drop yaml import.Raw is JSON; json.Unmarshal avoids an extra dependency.
-import ( - ... - yaml "sigs.k8s.io/yaml" -) +import ( + ... +)func getAWSProviderSpecFromMachineSet(mapiMachineSet *mapiv1beta1.MachineSet) *mapiv1beta1.AWSMachineProviderConfig { Expect(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value).ToNot(BeNil()) providerSpec := &mapiv1beta1.AWSMachineProviderConfig{} - Expect(yaml.Unmarshal(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed()) + Expect(json.Unmarshal(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value.Raw, providerSpec)).To(Succeed()) return providerSpec }Also applies to: 21-21
329-355: Optional: wait for AWSMachineTemplate deletion to finish.Delete is async; add a short wait to ensure templates are gone to prevent test flakiness.
Eventually(func() bool { var tmp awsv1.AWSMachineTemplate err := cl.Get(ctx, client.ObjectKey{Name: template.Name, Namespace: template.Namespace}, &tmp) return apierrors.IsNotFound(err) }, capiframework.WaitShort, capiframework.RetryShort).Should(BeTrue(), "AWSMachineTemplate %s should be deleted", template.Name)
164-181: Rename function to use consistent MachineSet casing and update messages for accuracy.Rename
verifyMachinesetReplicastoverifyMachineSetReplicasand update error messages on lines 165–166 to reference "MachineSet" instead of "Machine" (the parameter is a MachineSet, not a Machine). This affects 12 call sites across two test files:
- e2e/machineset_migration_capi_authoritative_test.go (lines 153, 182, 183, 259)
- e2e/machineset_migration_mapi_authoritative_test.go (lines 145, 146, 161, 197, 198, 237, 238, 289, 336)
e2e/machineset_migration_mapi_authoritative_test.go (1)
331-337: Assert ScaleMachineSet result for consistency.Elsewhere you assert scaling succeeds; do the same here to catch failures early.
- mapiframework.ScaleMachineSet(mapiMSAuthMAPIName, 1) + Expect(mapiframework.ScaleMachineSet(mapiMSAuthMAPIName, 1)). + To(Succeed(), "Scaling MAPI MachineSet should be rejected and restored by controller")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
e2e/machineset_migration_capi_authoritative_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/migration_common.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/machineset_migration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/migration_common.go
🔇 Additional comments (2)
e2e/machineset_migration_helpers.go (1)
214-221: Good fix: scope ctx/cl correctly in ProviderSpec verifier.Passing ctx and cl removes the prior out‑of‑scope issue; polling with Eventually is correct.
e2e/machineset_migration_capi_authoritative_test.go (1)
272-276: Nice: add minimal assertion to avoid a vacuous pass.Keeping the test active with a basic existence check is a good interim step pending OCPBUGS-56897.
sunzhaohua2
force-pushed
the
machineset-split
branch
from
0902a6f to
9c4d524
Compare
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/machineset_migration_helpers.go (2)
214-221: Simplify providerSpec verification with komega.Object + Transform.Avoid explicit ctx/cl fetch in the polling loop; komega.Object re-fetches for you and reduces boilerplate.
Example:
-func verifyMAPIMachineSetProviderSpec(ctx context.Context, cl client.Client, mapiMachineSet *mapiv1beta1.MachineSet, matcher types.GomegaMatcher) { - By(fmt.Sprintf("Verifying MAPI MachineSet %s ProviderSpec", mapiMachineSet.Name)) - Eventually(func(g Gomega) *mapiv1beta1.AWSMachineProviderConfig { - freshMachineSet, err := mapiframework.GetMachineSet(ctx, cl, mapiMachineSet.Name) - g.Expect(err).ShouldNot(HaveOccurred()) - return getAWSProviderSpecFromMachineSet(freshMachineSet) - }, capiframework.WaitMedium, capiframework.RetryShort).Should(matcher) -} +func verifyMAPIMachineSetProviderSpec(ctx context.Context, cl client.Client, mapiMachineSet *mapiv1beta1.MachineSet, matcher types.GomegaMatcher) { + By(fmt.Sprintf("Verifying MAPI MachineSet %s ProviderSpec", mapiMachineSet.Name)) + Eventually(komega.Object(mapiMachineSet), capiframework.WaitMedium, capiframework.RetryShort).Should( + WithTransform(getAWSProviderSpecFromMachineSet, matcher), + ) +}
270-272: Minor: fix double space in log message.Cosmetic polish in By() string.
- By(fmt.Sprintf("Verifying there is a CAPI MachineSet mirror for MAPI MachineSet %s", machineName)) + By(fmt.Sprintf("Verifying there is a CAPI MachineSet mirror for MAPI MachineSet %s", machineName))e2e/machineset_migration_mapi_authoritative_test.go (1)
195-197: Add a replica assertion after scaling down to 1.Strengthens the test by verifying the effect.
It("should succeed scaling down CAPI MachineSet to 1, after the switch of AuthoritativeAPI to ClusterAPI", func() { By("Scaling down CAPI MachineSet to 1") capiframework.ScaleCAPIMachineSet(mapiMSAuthMAPIName, 1, capiframework.CAPINamespace) capiframework.WaitForMachineSet(cl, mapiMSAuthMAPIName, capiframework.CAPINamespace) + verifyMachinesetReplicas(capiMachineSet, 1) + verifyMachinesetReplicas(mapiMachineSet, 1) })e2e/machineset_migration_capi_authoritative_test.go (2)
146-149: Avoid shadowing capiMachineSet inside the test.Reusing the outer variable name can confuse readers. Rename the inner local or drop the short declaration.
- capiMachineSet := capiframework.GetMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace) - capiMachine := capiframework.GetNewestMachineFromMachineSet(cl, capiMachineSet) + capiMS := capiframework.GetMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace) + capiMachine := capiframework.GetNewestMachineFromMachineSet(cl, capiMS)
194-197: Add replica assertion after scaling down to 1.Completes the flow validation.
It("should succeed scaling down MAPI MachineSet to 1, after the switch of AuthoritativeAPI to MachineAPI", func() { By("Scaling down MAPI MachineSet to 1 replicas") Expect(mapiframework.ScaleMachineSet(mapiMSAuthCAPIName, 1)).To(Succeed(), "should be able to scale down MAPI MachineSet") + verifyMachinesetReplicas(mapiMachineSet, 1) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
e2e/machineset_migration_capi_authoritative_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/migration_common.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/machineset_migration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/migration_common.go
🔇 Additional comments (1)
e2e/machineset_migration_mapi_authoritative_test.go (1)
171-174: The review comment is based on incorrect assumptions about function behavior—ScaleCAPIMachineSethas built-in error handling.The function already asserts success internally:
Eventually(...).Should(Succeed(), ...)means if the scaling fails, the test fails immediately. It's void and doesn't return an error to assert on.The test at line 173 already has assertions: it verifies the new machine is running and has the correct paused condition. However, unlike the scaling test at line 193, it doesn't explicitly verify the MachineSet replica count—though it does verify the machine exists and is in the correct state, which implicitly confirms the scaling occurred.
Likely an incorrect or invalid review comment.
| Eventually(cl.Create(ctx, awsMachineTemplate), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to create a new awsMachineTemplate %s", awsMachineTemplate.Name) | ||
|
|
There was a problem hiding this comment.
Eventually(Create) is not retried; wrap in a function or use komega.Create.
Passing the immediate result of cl.Create to Eventually evaluates once and won’t retry. Use a closure or komega.Create to actually poll.
Apply:
- Eventually(cl.Create(ctx, awsMachineTemplate), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to create a new awsMachineTemplate %s", awsMachineTemplate.Name)
+ Eventually(komega.Create(awsMachineTemplate), capiframework.WaitMedium, capiframework.RetryMedium).
+ Should(Succeed(), "Failed to create a new awsMachineTemplate %s", awsMachineTemplate.Name)- Eventually(cl.Create(ctx, newTemplate), capiframework.WaitMedium, capiframework.RetryMedium).Should(
- Succeed(), "Failed to create a new awsMachineTemplate %s", newTemplate.Name)
+ Eventually(komega.Create(newTemplate), capiframework.WaitMedium, capiframework.RetryMedium).Should(
+ Succeed(), "Failed to create a new awsMachineTemplate %s", newTemplate.Name)Also applies to: 310-312
🤖 Prompt for AI Agents
In e2e/machineset_migration_helpers.go around lines 34-35 (and similarly at
lines 310-312), the call passes the immediate result of cl.Create into
Eventually which evaluates once and won't retry; wrap the create call in a
closure (e.g. func() error { return cl.Create(ctx, awsMachineTemplate) }) or use
the komega.Create helper so Eventually can poll, and keep the assertion as
.Should(Succeed(), "Failed to create a new awsMachineTemplate %s",
awsMachineTemplate.Name).
There was a problem hiding this comment.
this is not correct, undefined: komega.Create
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the komega package usage in the codebase to understand available functions
rg -n 'komega\.' --type=go -A2 -B2
Length of output: 48228
@sunzhaohua2 You're absolutely correct, and I apologize for the error. The komega.Create function does not exist in the komega package.
Looking at the codebase, I can confirm that komega provides functions like Get, Update, Object, List, etc., but not Create.
The correct solution is to wrap the cl.Create call in a closure so that Eventually can retry it:
Correct fix for lines 34-35:
Eventually(func() error {
return cl.Create(ctx, awsMachineTemplate)
}, capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to create a new awsMachineTemplate %s", awsMachineTemplate.Name)Correct fix for lines 310-312:
Eventually(func() error {
return cl.Create(ctx, newTemplate)
}, capiframework.WaitMedium, capiframework.RetryMedium).Should(
Succeed(), "Failed to create a new awsMachineTemplate %s", newTemplate.Name)The issue with the current code is that cl.Create(ctx, awsMachineTemplate) is evaluated once before Eventually runs, so Eventually just checks the same error result repeatedly instead of retrying the create operation. Wrapping it in a function allows Eventually to call the create operation multiple times until it succeeds or times out.
There was a problem hiding this comment.
I will keep it unchanged here
There was a problem hiding this comment.
@sunzhaohua2 Understood! I'll note your decision to keep the current implementation as-is. Thank you for the clarification.
| machineSetOpenshiftLabelKey = "machine.openshift.io/cluster-api-machineset" | ||
| ) | ||
|
|
||
| func getDefaultAWSMAPIProviderSpec() (*mapiv1beta1.MachineSet, *mapiv1beta1.AWSMachineProviderConfig) { |
There was a problem hiding this comment.
@sunzhaohua2 shall we add comments to helper functions , so that they can be available on IDEs and godocs?
e2e/aws_helpers.go
Outdated
| ID: mapiProviderSpec.AMI.ID, | ||
| }, | ||
| Ignition: &awsv1.Ignition{ | ||
| Version: "3.4", |
There was a problem hiding this comment.
possible to take this version as constant for future maintenance ?
There was a problem hiding this comment.
I deleted this filed, this is deleted in pr #363
sunzhaohua2
force-pushed
the
machineset-split
branch
from
9c4d524 to
f1b24be
Compare
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
e2e/e2e_common.go (1)
33-38: Prefer a cancellable suite context to avoid hangs and enable cleanupUsing context.Background() can leave watchers running after suite completion. Create a suite-scoped context with cancel, set it in komega, and cancel in AfterSuite.
--- a/e2e/e2e_common.go +++ b/e2e/e2e_common.go @@ var ( - cl client.Client - ctx = context.Background() + cl client.Client + ctx context.Context + suiteCancel context.CancelFunc @@ func InitCommonVariables() { - cfg, err := config.GetConfig() + cfg, err := config.GetConfig() Expect(err).ToNot(HaveOccurred()) - cl, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + // Cancellable suite context + ctx, suiteCancel = context.WithCancel(context.Background()) + cl, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).ToNot(HaveOccurred()) @@ komega.SetClient(cl) komega.SetContext(ctx) }Apply this in e2e_test.go to cancel after all specs:
--- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ var _ = BeforeSuite(func() { InitCommonVariables() }) + +var _ = AfterSuite(func() { + if suiteCancel != nil { + suiteCancel() + } +})Also applies to: 55-72
e2e/e2e_test.go (1)
16-16: Centralized init via InitCommonVariables — LGTMKeeps suite bootstrap consistent. If you adopt the cancellable context, add an AfterSuite to invoke suiteCancel() (see e2e_common.go comment).
e2e/machineset_migration_mapi_authoritative_test.go (3)
171-179: Add an explicit wait after scaling CAPI to reduce flakinessAfter ScaleCAPIMachineSet(..., 3), wait for the CAPI MachineSet to reconcile before fetching the newest Machine.
- By("Scaling up CAPI MachineSet to 3") - capiframework.ScaleCAPIMachineSet(mapiMSAuthMAPIName, 3, capiframework.CAPINamespace) + By("Scaling up CAPI MachineSet to 3") + capiframework.ScaleCAPIMachineSet(mapiMSAuthMAPIName, 3, capiframework.CAPINamespace) + capiframework.WaitForMachineSet(cl, mapiMSAuthMAPIName, capiframework.CAPINamespace)
284-301: Strengthen “reject update” checks with Consistently to ensure no driftUse Consistently around replicas/spec assertions to confirm the mirror remains unchanged over a window, not just eventually restored once.
- By("Scaling up CAPI MachineSet to 1 should be rejected") - capiframework.ScaleCAPIMachineSet(mapiMSAuthMAPIName, 1, capiframework.CAPINamespace) - capiMachineSet = capiframework.GetMachineSet(cl, mapiMSAuthMAPIName, capiframework.CAPINamespace) - verifyMachinesetReplicas(capiMachineSet, 0) + By("Scaling up CAPI MachineSet to 1 should be rejected") + capiframework.ScaleCAPIMachineSet(mapiMSAuthMAPIName, 1, capiframework.CAPINamespace) + capiMachineSet = capiframework.GetMachineSet(cl, mapiMSAuthMAPIName, capiframework.CAPINamespace) + Consistently(func() int32 { return *capiMachineSet.Spec.Replicas }, capiframework.WaitShort, capiframework.RetryShort).Should(Equal(int32(0)))- Eventually(k.Object(mapiMachineSet), ...).Should(HaveField("Spec.DeletePolicy", SatisfyAny(BeEmpty(), Equal("Random")))) - Eventually(k.Object(capiMachineSet), ...).Should(HaveField("Spec.DeletePolicy", Equal("Random"))) + Consistently(k.Object(mapiMachineSet), capiframework.WaitShort, capiframework.RetryShort). + Should(HaveField("Spec.DeletePolicy", SatisfyAny(BeEmpty(), Equal("Random")))) + Consistently(k.Object(capiMachineSet), capiframework.WaitShort, capiframework.RetryShort). + Should(HaveField("Spec.DeletePolicy", Equal("Random")))
106-114: Reduce name-collision risk by adding a random suffix to test resource namesIf these suites run in parallel in future, fixed names may collide. Consider adding a short random suffix.
Example:
// helper func uniqueName(base string) string { return fmt.Sprintf("%s-%s", base, rand.String(5)) } // k8s.io/apimachinery/pkg/util/rand // usage var mapiMSAuthMAPIName = uniqueName("ms-authoritativeapi-mapi")Also applies to: 33-40
e2e/machineset_migration_capi_authoritative_test.go (3)
142-151: Add wait after scaling CAPI to 2 to avoid racesWait for the MachineSet reconcile before reading newest machine.
capiframework.ScaleCAPIMachineSet(mapiMSAuthCAPIName, 2, capiframework.CAPINamespace) +capiframework.WaitForMachineSet(cl, mapiMSAuthCAPIName, capiframework.CAPINamespace)
194-197: Verify scale-down resultAdd a wait and assert replicas for both MAPI and CAPI MachineSets to make the step meaningful.
By("Scaling down MAPI MachineSet to 1 replicas") Expect(mapiframework.ScaleMachineSet(mapiMSAuthCAPIName, 1)).To(Succeed(), "should be able to scale down MAPI MachineSet") +mapiframework.WaitForMachineSet(ctx, cl, mapiMSAuthCAPIName) +verifyMachinesetReplicas(mapiMachineSet, 1) +verifyMachinesetReplicas(capiMachineSet, 1)
206-211: Deletion helpers: assert success if function returns an errorIf DeleteMachineSets returns an error, prefer asserting success to surface failures early.
// If signature returns error: Expect(capiframework.DeleteMachineSets(ctx, cl, capiMachineSet)).To(Succeed(), "Should be able to delete test MachineSet")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
e2e/aws_helpers.go(1 hunks)e2e/aws_test.go(1 hunks)e2e/e2e_common.go(1 hunks)e2e/e2e_test.go(1 hunks)e2e/machineset_migration_capi_authoritative_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/migration_common.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/machineset_migration_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/aws_helpers.go
- e2e/machineset_migration_helpers.go
- e2e/aws_test.go
🔇 Additional comments (2)
e2e/migration_common.go (1)
10-12: Condition constants look good; verify string identifiers match controller-emitted typesTyped aliases are correct. Please confirm "Synchronized" and "Paused" exactly match the Condition.Type values your controllers set, to avoid fragile string mismatches across repos.
e2e/e2e_common.go (1)
27-31: Infra API version fixed to v1beta2 — LGTMValue aligns with AWSMachineTemplate v1beta2 usage.
| mapiMachines, err := mapiframework.GetMachinesFromMachineSet(ctx, cl, mapiMachineSet) | ||
| Expect(mapiMachines).ToNot(BeEmpty(), "no MAPI Machines found") | ||
| Expect(err).ToNot(HaveOccurred(), "failed to get MAPI Machines from MachineSet") | ||
|
|
||
| capiMachines := capiframework.GetMachinesFromMachineSet(cl, capiMachineSet) | ||
| Expect(capiMachines).ToNot(BeEmpty(), "no CAPI Machines found") | ||
| Expect(capiMachines[0].Name).To(Equal(mapiMachines[0].Name)) | ||
|
|
There was a problem hiding this comment.
Assert errors before inspecting results
Check err first, then assert on the returned slice to keep failure messages clear and avoid nil-dereference risks.
- mapiMachines, err := mapiframework.GetMachinesFromMachineSet(ctx, cl, mapiMachineSet)
- Expect(mapiMachines).ToNot(BeEmpty(), "no MAPI Machines found")
- Expect(err).ToNot(HaveOccurred(), "failed to get MAPI Machines from MachineSet")
+ mapiMachines, err := mapiframework.GetMachinesFromMachineSet(ctx, cl, mapiMachineSet)
+ Expect(err).ToNot(HaveOccurred(), "failed to get MAPI Machines from MachineSet")
+ Expect(mapiMachines).ToNot(BeEmpty(), "no MAPI Machines found")🤖 Prompt for AI Agents
In e2e/machineset_migration_capi_authoritative_test.go around lines 226 to 233,
the code asserts the returned mapiMachines slice before checking err which can
produce confusing failures or nil-dereference; change the order to assert err
first (Expect(err).ToNot(HaveOccurred())), then assert the slice is not empty
(Expect(mapiMachines).ToNot(BeEmpty())), and only after those checks access
mapiMachines[0].Name to compare with capiMachines[0].Name; ensure any similar
pattern in nearby lines is updated to check errors before inspecting results.
|
@sunzhaohua2: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@damdo @theobarberbany can you help to take a look this pr when you got time? thank you! |
3 participants
@huali9 @miyadav @damdo @theobarberbany Please help to take a look, thanks.
Summary by CodeRabbit
New Features
Tests
Refactor