OCPCLOUD-3041: Created Cluster-api-tests-ext command for origin e2e #274
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@sunzhaohua2: This pull request references OCPQE-28828 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 spike to target the "4.19.0" version, but no target version was set. DetailsIn 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. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
/retest |
sunzhaohua2
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
@sunzhaohua2: An error was encountered searching for bug OCPQE-28828 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
You do not have the permission to see the specified issue.: request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn 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. |
openshift-ci-robot
removed
the
jira/valid-reference
|
@sunzhaohua2: An error was encountered searching for bug OCPQE-28828 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
You do not have the permission to see the specified issue.: request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn 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
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sunzhaohua2: This pull request references OCPCLOUD-3041 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.20.0" version, but no target version was set. DetailsIn 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
force-pushed
the
extension
branch
2 times, most recently
from
33f1446 to
5e1345b
Compare
| - wrapcheck | ||
|
|
||
| # Exclude some linters from running on tests files. | ||
| - path: e2e/.*|e2e/framework/.* |
There was a problem hiding this comment.
Hmm - do we not want to enforce the same coding standards across our e2es? I can understand excluding gosec.. not as sure about the others.
cc @damdo
| if err != nil { | ||
| panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err.Error())) | ||
| } | ||
|
|
There was a problem hiding this comment.
I see we are having all tests do a check to see what platform it is running on. One idea we are using in MAO is to have tests label their platform and have this binary include/exclude based on platform passed in. Example: https://github.com/openshift/machine-api-operator/blob/e36ec0c1aa5213d3a294bae284a45fa9b2eb8094/cmd/machine-api-tests-ext/main.go#L62
|
Is there any desire for these existing tests to be included in the already defined suites provided by origin? If we label the e2e tests using similar formats as origin (such as using [Suite:openshift/conformance/parallel] or [Suite:openshift/conformance/serial]) they will get included in those suites. We can also add to all these tests any FeatureGate markers so they only run when certain feature gates are enabled. |
I think yes, right? @shellyyang1989 @damdo wdyt? |
We need to be cautious about it I think. Per https://docs.google.com/document/d/1cFZj9QdzW8hbHc3H0Nce-2xrJMtpDJrwAse9H7hLiWk/edit?tab=t.0 the openshift/conformance contain highly reliable, fast running tests that run on a wide variety of configurations. Tests should be as fast as possible; and less than 5 minutes in duration and new tests must pass at >= 99%. Do we really meet the criteria? |
|
@sunzhaohua2 there are many e2e changes that sound not relevant to OTE. Are they intended? |
@shellyyang1989 yes, changing the files name from _test.go to without _test.go resulted in many new linter warnings. so to resolve these warnings, need these changes. |
openshift-merge-robot
added
the
needs-rebase
I agree with @shellyyang1989 here, nice to have them compatible with OTE, but let's be cautious on where we add them for now :) |
|
/assign @mdbooth As you are already reviewing the other OTE PR |
sunzhaohua2
force-pushed
the
extension
branch
from
5e1345b to
14962a9
Compare
openshift-merge-robot
removed
the
needs-rebase
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sunzhaohua2
force-pushed
the
extension
branch
from
14962a9 to
9d8e0b4
Compare
|
/retest-required |
|
@sunzhaohua2: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@chrischdi @theobarberbany @vr4manta thank you for the review, I think I have addressed all the comments, can you help to take a look again? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a new cluster-api tests extension CLI, integrates E2E provider suites (AWS, Azure, BareMetal, GCP, PowerVS), consolidates E2E initialization, updates framework APIs to be context-aware and renames a function, adjusts Makefile/Docker packaging to build and ship the new binary, updates lint rules, and removes a large vendored Azure SDK subtree and old *_test.go files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
e2e/framework/util.go (1)
30-54: Fix undefined ctx, cluster-scoped lookup, and URL handling in GetControlPlaneHostAndPort
- In e2e/framework/util.go (line 31), change the signature to
func GetControlPlaneHostAndPort(ctx context.Context, cl client.Client) (string, int32, error), remove theNamespacefromObjectKey, and add checks for an emptyStatus.APIServerURLplus default‐port logic when none is provided.- Update all call sites to pass the new
ctxparameter:
- e2e/vsphere.go at line 146
- e2e/framework/cluster.go at line 50
e2e/framework/machinetemplate.go (2)
95-116: ctx used but not defined; make function context-aware.
cl.Delete(ctx, ...)references an undefinedctx. Add a context parameter.Apply this diff:
-// DeleteAWSMachineTemplateByPrefix deletes all AWSMachineTemplates with matching name prefix. -func DeleteAWSMachineTemplateByPrefix(cl client.Client, prefix string, namespace string) error { +// DeleteAWSMachineTemplateByPrefix deletes all AWSMachineTemplates with matching name prefix. +func DeleteAWSMachineTemplateByPrefix(ctx context.Context, cl client.Client, prefix string, namespace string) error { if prefix == "" { - return nil + return fmt.Errorf("prefix cannot be empty") }
50-116: Add missingcontext.Contextto DeleteAWSMachineTemplateByPrefix signature
The function body references an undefinedctxbut the signature isfunc DeleteAWSMachineTemplateByPrefix(cl client.Client, prefix string, namespace string) error. Update it to:func DeleteAWSMachineTemplateByPrefix(ctx context.Context, cl client.Client, prefix, namespace string) errorand adjust any callers accordingly.
e2e/framework/machine.go (1)
124-127: Nil dereference bug when building machineNames.If any machine is nil (allowed earlier), appending machine.Name will panic.
Apply this diff:
- machineNames := []string{} - for _, machine := range machines { - machineNames = append(machineNames, machine.Name) - } + machineNames := make([]string, 0, len(machines)) + for _, machine := range machines { + if machine == nil { + continue + } + machineNames = append(machineNames, machine.Name) + }e2e/framework/machineset.go (2)
66-74: APIVersion mismatch for clusterv1.MachineSetYou're creating a CAPI MachineSet but set APIVersion to machine.openshift.io. Use cluster.x-k8s.io/v1beta1 or drop TypeMeta and let the scheme set GVK.
ms := &clusterv1.MachineSet{ TypeMeta: metav1.TypeMeta{ Kind: "MachineSet", - APIVersion: "machine.openshift.io/v1beta1", + APIVersion: "cluster.x-k8s.io/v1beta1", },
233-239: Compile error: undefined ctx in GetNewestMachineFromMachineSetFunction references ctx but doesn't receive it. Add context parameter.
-// GetNewestMachineFromMachineSet returns the new created machine by a given machineSet. -func GetNewestMachineFromMachineSet(cl client.Client, machineSet *clusterv1.MachineSet) (*clusterv1.Machine, error) { - machines, err := GetMachinesFromMachineSet(ctx, cl, machineSet) +// GetNewestMachineFromMachineSet returns the newest machine for a given machineSet. +func GetNewestMachineFromMachineSet(ctx context.Context, cl client.Client, machineSet *clusterv1.MachineSet) (*clusterv1.Machine, error) { + machines, err := GetMachinesFromMachineSet(ctx, cl, machineSet)Follow-up: Update call sites accordingly.
♻️ Duplicate comments (6)
.golangci.yml (1)
138-147: Reassess which linters to disable for E2E; narrow the set.
Echoing prior concern about broad disables for E2E tests; it’s reasonable to relax complexity/length checks, but disablingerr113(and possiblygochecknoglobals) across all of e2e may hide real issues in framework code.Consider re-enabling
err113(and optionallygochecknoglobals) for reusable framework packages while keepingfunlen/gocyclo/dupldisabled for test specs. If you want to proceed, I can propose path-scoped rules or move framework helpers outsidee2e/so they follow stricter linters.e2e/baremetal.go (1)
46-52: InitCommonVariables addressed.InitCommonVariables is now called in BeforeAll; prior concern is resolved.
Dockerfile.rhel (1)
18-18: File should end with a newlinee2e/machine_migration.go (1)
160-163: Consider moving machine deletion check inside DeleteMachines functionThe pattern of checking for nil machines before calling DeleteMachines is repeated. This check could be moved inside the DeleteMachines function for cleaner code.
cmd/cluster-api-tests-ext/main.go (1)
62-74: LGTM: addresses earlier platform-label gating request.This implements platform-based inclusion as suggested previously in review.
e2e/framework/machine.go (1)
25-26: Import alias consistency (optional).Alias awsv1 is fine; ensure consistency across files to satisfy linting preferences raised previously.
🧹 Nitpick comments (45)
.golangci.yml (2)
142-142: Remove trailing whitespace to satisfy YAML linters.
There’s a trailing space after "dupl".Apply this diff:
- - dupl + - dupl
139-146: Redundant path pattern; simplify and tighten the rule.
e2e/framework/.*is already matched bye2e/.*. Also, consider anchoring to avoid accidental matches.Apply this diff:
- - path: e2e/.*|e2e/framework/.* + - path: ^e2e/.*e2e/framework/util.go (1)
56-93: Silence unparam or use the context parameter.
IsMachineAPIMigrationEnableddoesn’t usectxand will be flagged byunparam. Either use it or mark it intentionally unused.Apply this diff to mark it intentionally unused:
-func IsMachineAPIMigrationEnabled(ctx context.Context, cl client.Client) bool { +func IsMachineAPIMigrationEnabled(_ context.Context, cl client.Client) bool {Alternatively, add a
//nolint:unparamif you plan to usectxsoon.e2e/framework/machinetemplate.go (2)
50-66: LGTM on context-aware deletion; add formatting fix for failure message.
Good move to pass context. Also, the Gomega failure message isn’t formatted; usefmt.Sprintf.Apply this diff:
- Eventually(komega.Get(awsMachineTemplate), time.Minute, RetryShort).Should(Succeed(), "Failed to get AWSMachineTemplate %s/%s.", awsMachineTemplate.Namespace, awsMachineTemplate.Name) + Eventually(komega.Get(awsMachineTemplate), time.Minute, RetryShort).Should( + Succeed(), + fmt.Sprintf("Failed to get AWSMachineTemplate %s/%s.", awsMachineTemplate.Namespace, awsMachineTemplate.Name), + )- ), "Delete awsMachineTemplate %s/%s should succeed, or awsMachineTemplate should not be found.", - template.Namespace, template.Name) + ), + fmt.Sprintf("Delete awsMachineTemplate %s/%s should succeed, or awsMachineTemplate should not be found.", template.Namespace, template.Name), + )
101-112: Format Gomega failure message with Sprintf.
Same formatting issue here;%swon’t be expanded.Apply this diff:
- Eventually(komega.List(templateList, client.InNamespace(namespace))).Should(Succeed(), "failed to list AWSMachineTemplates in namespace %s.", namespace) + Eventually(komega.List(templateList, client.InNamespace(namespace))).Should( + Succeed(), + fmt.Sprintf("failed to list AWSMachineTemplates in namespace %s.", namespace), + )e2e/framework/framework.go (3)
32-37: Fix typos in Retry comments.*"rety" → "retry".
Apply:
-// RetryShort defines a short rety duration for test operations. +// RetryShort defines a short retry duration for test operations. -// RetryMedium defines a medium rety duration for test operations. +// RetryMedium defines a medium retry duration for test operations. -// RetryLong defines a long rety duration for test operations. +// RetryLong defines a long retry duration for test operations.
53-59: Make DeleteObjects cleanup resilient to AlreadyDeleted resources.Ignore NotFound to avoid failing teardown on idempotent deletes.
Apply:
- Expect(cl.Delete(ctx, o)).To(Succeed()) + Expect(client.IgnoreNotFound(cl.Delete(ctx, o))).To(Succeed())
61-64: Return the shared package context.This ensures future changes to ctx (timeouts/cancel) propagate through GetContext().
Apply:
-func GetContext() context.Context { - return context.Background() -} +func GetContext() context.Context { + return ctx +}e2e/framework/cluster.go (1)
65-73: Add polling interval to Eventually to reduce flakiness.Use framework.RetryMedium for a consistent poll cadence.
Apply:
- }, WaitMedium).Should(BeTrue()) + }, WaitMedium, RetryMedium).Should(BeTrue())e2e/common.go (1)
54-66: Remove duplicate scheme registration for configv1.configv1.Install is called twice.
Apply:
utilruntime.Must(configv1.Install(scheme.Scheme)) -utilruntime.Must(configv1.Install(scheme.Scheme))e2e/azure.go (4)
49-55: Avoid re-running InitCommonVariables in test scope.The suite already initializes in e2e/e2e_test.go. Drop this call to prevent redundant work.
Apply:
- InitCommonVariables()
106-114: Drop the nil-check on AcceleratedNetworking.Field is a bool in MAPI; a nil check is meaningless. Keep the semantic checks which matter.
Apply:
- Expect(mapiProviderSpec.AcceleratedNetworking).ToNot(BeNil())
128-136: Use canonical Azure resource ID casing.Prefer "resourceGroups" to match common Azure ID format.
Apply:
- providerID = fmt.Sprintf("azure:///subscriptions/%s/resourcegroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", subscriptionID, mapiProviderSpec.ResourceGroup, mi) + providerID = fmt.Sprintf("azure:///subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", subscriptionID, mapiProviderSpec.ResourceGroup, mi)
38-42: Remove unused constant.clusterSecretName isn’t used.
Apply:
- clusterSecretName = "capz-manager-cluster-credential"e2e/mapimachines.go (2)
171-182: Clarify: you validate the CAPI Machine, not the infra machine.The comment says “CAPI Infra Machine” but you fetch a CAPI Machine; adjust to avoid confusion.
-// verifyCapiMachinePaused verifies that the mirror CAPI Infra Machine has Paused condition True and MAPI Machine has a CAPI Infra Machine mirror. +// verifyCapiMachinePaused verifies that the mirror CAPI Machine has Paused condition True.
34-48: Deduplicate condition/timeout constants across files.Synchronized/Paused and default waits are re-declared in multiple e2e files. Move them to a shared e2e/constants.go to prevent drift.
I can extract these into a common constants file and update call sites.
e2e/baremetal.go (2)
54-60: AfterEach platform Skip is redundant.BeforeAll already skips for non-baremetal. Keeping this adds noise to logs.
Consider returning early if resources are nil instead of Skip().
125-152: Handle AlreadyExists and ensure idempotent cleanup.You tolerate AlreadyExists, but DeleteObjects in AfterEach may not remove a pre-existing template created outside the test. Add a unique suffix (GenerateName) or namespace/name scoping for this suite.
e2e/machineset_migration.go (3)
52-53: Polish skip message.Minor grammar fix.
Apply:
- Skip(fmt.Sprintf("Skipping tests on %s, this only support on aws", platform)) + Skip(fmt.Sprintf("Skipping tests on %s, this is only supported on AWS", platform))
199-200: Typo: “Sepc” → “Spec”.Apply:
- ).Should(Equal("m5.large"), "MAPI MSet Sepc should be updated to reflect existing CAPI mirror") + ).Should(Equal("m5.large"), "MAPI MSet Spec should be updated to reflect existing CAPI mirror")
39-46: Consolidate condition constants across files.Synchronized/MAPI Paused/CAPI Paused are declared here and in other tests. Centralize to avoid drift.
I can add e2e/constants.go and update references in this and mapimachines.go.
Dockerfile.rhel (1)
4-7: Consider error handling for the gzip operationThe build step creates the test extension binary and compresses it, but doesn't verify the gzip operation succeeded. Consider adding error checking or using
&&to ensure all operations complete successfully.-RUN make build && \ - mkdir -p /tmp/build && \ - cp /go/src/github.com/openshift/cluster-capi-operator/bin/cluster-api-tests-ext /tmp/build/cluster-api-tests-ext && \ - gzip /tmp/build/cluster-api-tests-ext +RUN make build && \ + mkdir -p /tmp/build && \ + cp /go/src/github.com/openshift/cluster-capi-operator/bin/cluster-api-tests-ext /tmp/build/cluster-api-tests-ext && \ + gzip /tmp/build/cluster-api-tests-ext && \ + test -f /tmp/build/cluster-api-tests-ext.gze2e/machine_migration.go (1)
520-539: Consider consolidating nil checks in cleanup functionThe cleanup function has good defensive nil checks, but could be simplified by moving these checks into the respective Delete functions.
func cleanupMachineResources(ctx context.Context, cl client.Client, capiMachines []*clusterv1.Machine, mapiMachines []*machinev1beta1.Machine) { for _, m := range capiMachines { - if m == nil { - continue - } - - By(fmt.Sprintf("Deleting CAPI Machine %s", m.Name)) - capiframework.DeleteMachines(ctx, cl, capiframework.CAPINamespace, m) + if m != nil { + By(fmt.Sprintf("Deleting CAPI Machine %s", m.Name)) + capiframework.DeleteMachines(ctx, cl, capiframework.CAPINamespace, m) + } } for _, m := range mapiMachines { - if m == nil { - continue - } - - By(fmt.Sprintf("Deleting MAPI Machine %s", m.Name)) - Expect(mapiframework.DeleteMachines(ctx, cl, m)).To(Succeed(), "Should be able to delete all Machines") - mapiframework.WaitForMachinesDeleted(cl, m) + if m != nil { + By(fmt.Sprintf("Deleting MAPI Machine %s", m.Name)) + Expect(mapiframework.DeleteMachines(ctx, cl, m)).To(Succeed(), "Should be able to delete all Machines") + mapiframework.WaitForMachinesDeleted(cl, m) + } } }cmd/cluster-api-tests-ext/types.go (2)
59-61: Fix comment/field name mismatch.The comment says "IsNoOptionalCapabilities" but the field is "HasNoOptionalCapabilities".
-// IsNoOptionalCapabilities indicates the cluster has no optional capabilities enabled +// HasNoOptionalCapabilities indicates the cluster has no optional capabilities enabled
21-31: Consider adding explicit JSON tags for stability.Relying on case-insensitive matching is brittle. Adding tags (mirroring upstream) avoids surprises in mixed-case keys.
Example:
- ProjectID string - Region string - Zone string - NumNodes int + ProjectID string `json:"projectID,omitempty"` + Region string `json:"region,omitempty"` + Zone string `json:"zone,omitempty"` + NumNodes int `json:"numNodes,omitempty"`cmd/cluster-api-tests-ext/provider.go (2)
112-127: Remove duplicate KubeConfig assignment.
TestContext.KubeConfigis set twice; keep one to avoid confusion.testContext.KubectlPath = "kubectl" - testContext.KubeConfig = os.Getenv("KUBECONFIG") testContext.NodeOSDistro = "custom" testContext.MasterOSDistro = "custom"
161-163: Default JUnit directory when TEST_JUNIT_DIR is unset.Improves CI ergonomics by writing reports under ARTIFACT_DIR.
- testContext.ReportDir = os.Getenv("TEST_JUNIT_DIR") + if v := os.Getenv("TEST_JUNIT_DIR"); v != "" { + testContext.ReportDir = v + } else { + testContext.ReportDir = filepath.Join(os.Getenv("ARTIFACT_DIR"), "junit") + }e2e/aws.go (2)
159-168: Template name collisions across runs.Using a fixed
awsMachineTemplateNamerisks AlreadyExists/stale-spec reuse. Consider a unique name (e.g.,GenerateName: "aws-machine-template-") and reference the created name.Want me to post a minimal refactor that captures the created template name and wires it into the MachineSet?
173-200: Credential sourcing flexibility.Falling back to the default AWS provider chain (env/IRSA/EC2 metadata) would reduce coupling to a specific Secret name.
If useful, I can provide a fallback that tries the default chain when the Secret is absent.
e2e/powervs.go (7)
42-45: Avoid “Conformance” label confusion; prefer extension-specific labels.Using Label("Conformance") risks accidental inclusion in broader “openshift/conformance” contexts. Suggest switching to a capi-specific marker (e.g., Label("CAPIConformance")) and updating suite qualifiers accordingly.
Would you like a follow-up patch to add a CAPI-specific label and adjust cmd/cluster-api-tests-ext/main.go qualifiers?
49-53: Platform gating duplication; consider relying on label-based selection.You already implement platform gating in the extension (main.go) via [platform:...] labels. The extra Skip here is redundant and adds noise when run under the extension.
If you still want a safety net for out-of-band runs, keep the BeforeAll guard but make AfterEach a no-op (see next comment).
38-40: Avoid hardcoding API version for IBMPowerVSMachineTemplate.Use the provider’s GroupVersion to prevent drift.
Apply this diff:
-const ( - powerVSMachineTemplateName = "powervs-machine-template" - powerVSMachineTemplateVersion = "infrastructure.cluster.x-k8s.io/v1beta2" -) +const powerVSMachineTemplateName = "powervs-machine-template" +var powerVSMachineTemplateAPIVersion = ibmpowervsv1.GroupVersion.String()corev1.ObjectReference{ Kind: "IBMPowerVSMachineTemplate", - APIVersion: powerVSMachineTemplateVersion, + APIVersion: powerVSMachineTemplateAPIVersion, Name: powerVSMachineTemplateName, },Also applies to: 75-77
69-81: MachineSet name collision risk; prefer unique names per run.Hardcoding "ibmpowervs-machineset" risks clashes across runs/namespaces. Consider adding a random or timestamp suffix.
I can propose a helper-based name generator if desired.
135-138: Drop redundant fmt.Println on error.The Expect line already surfaces the error; printing it duplicates output.
Apply this diff:
- if err := cl.Create(ctx, ibmPowerVSMachineTemplate); err != nil && !apierrors.IsAlreadyExists(err) { - fmt.Println(err) - Expect(err).ToNot(HaveOccurred()) - } + if err := cl.Create(ctx, ibmPowerVSMachineTemplate); err != nil && !apierrors.IsAlreadyExists(err) { + Expect(err).ToNot(HaveOccurred()) + }
143-172: Replace panics with Gomega assertions for clearer failures.Panics abruptly abort the suite and obscure context. Use Expect for nil checks and Fail for unknown type.
Apply this diff:
func getNetworkResourceReference(networkResource mapiv1.PowerVSResource) ibmpowervsv1.IBMPowerVSResourceReference { switch networkResource.Type { case mapiv1.PowerVSResourceTypeID: - if networkResource.ID == nil { - panic("networkResource reference is specified as ID but it is nil") - } + Expect(networkResource.ID).ToNot(BeNil(), "networkResource reference is specified as ID but it is nil") return ibmpowervsv1.IBMPowerVSResourceReference{ ID: networkResource.ID, } case mapiv1.PowerVSResourceTypeName: - if networkResource.Name == nil { - panic("networkResource reference is specified as Name but it is nil") - } + Expect(networkResource.Name).ToNot(BeNil(), "networkResource reference is specified as Name but it is nil") return ibmpowervsv1.IBMPowerVSResourceReference{ Name: networkResource.Name, } case mapiv1.PowerVSResourceTypeRegEx: - if networkResource.RegEx == nil { - panic("networkResource reference is specified as RegEx but it is nil") - } + Expect(networkResource.RegEx).ToNot(BeNil(), "networkResource reference is specified as RegEx but it is nil") return ibmpowervsv1.IBMPowerVSResourceReference{ RegEx: networkResource.RegEx, } default: - panic("networkResource reference is not specified") + Fail("networkResource reference is not specified") } }
86-98: ProviderSpec source selection is naive (first MachineSet).Picking Items[0] can select an arbitrary template. Consider filtering by cluster label/role or matching the target namespace to reduce flakiness.
I can draft a filter using label selectors (e.g., machine.openshift.io/cluster-api-cluster).
cmd/cluster-api-tests-ext/main.go (2)
52-57: Provider input via env only; consider a flag for clarity and CI integration.Add --provider (or --test-provider) flag with env fallback to improve UX and debuggability.
I can wire a cobra flag and default to TEST_PROVIDER if unspecified.
59-74: Make platform label extraction more robust and compile the regex once.Handle hyphens/nums and avoid recompiling per test. Also extract the captured platform directly.
Apply this diff:
-// Let's scan for tests with a platform label and create the rule for them such as [platform:vsphere] -foundPlatforms := make(map[string]string) - -for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { - re := regexp.MustCompile(`\[platform:[a-z]*]`) - - match := re.FindStringSubmatch(test) - for _, platformDef := range match { - if _, ok := foundPlatforms[platformDef]; !ok { - platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1] - foundPlatforms[platformDef] = platform - specs.Select(extensiontests.NameContains(platformDef)). - Include(extensiontests.PlatformEquals(platform)) - } - } -} +// Scan for [platform:<name>] labels and gate by platform +foundPlatforms := make(map[string]string) +re := regexp.MustCompile(`\[platform:([A-Za-z0-9_-]+)\]`) +for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { + match := re.FindStringSubmatch(test) + if len(match) < 2 { + continue + } + platformDef := match[0] + platform := match[1] + if _, ok := foundPlatforms[platformDef]; ok { + continue + } + foundPlatforms[platformDef] = platform + specs.Select(extensiontests.NameContains(platformDef)). + Include(extensiontests.PlatformEquals(platform)) +}e2e/framework/machine.go (4)
31-33: Typo in doc comment.“Optionaly” → “Optionally”.
Apply this diff:
-// Optionaly, labels may be used to constrain listed machinesets. +// Optionally, labels may be used to constrain listed machinesets.
79-80: Doc comment grammar and type name.Use proper article/casing: “Gets an AWSMachine by its name.”
Apply this diff:
-// GetAWSMachine get a awsmachine by its name. +// GetAWSMachine gets an AWSMachine by its name.
106-107: Docstring out of date with signature.Function no longer returns an error; update the comment.
Apply this diff:
-// DeleteMachines deletes the specified machines and returns an error on failure. +// DeleteMachines deletes the specified machines and waits until they are gone.
78-90: Consider removing error returns from assert-style getters.Both getters assert via Eventually(...Should(Succeed())) and always return nil error. Either return errors instead of asserting, or drop the error return.
I can refactor to return errors for better composability if preferred.
Also applies to: 92-104
e2e/gcp.go (2)
142-156: Avoid name collisions for GCPMachineTemplateConstant name risks AlreadyExists and stale spec reuse across runs. Either generate a unique name and pass it to NewMachineSetParams, or proactively delete any pre-existing object before create and wait for NotFound.
Example pre-clean before create:
gcpMachineTemplate := &gcpv1.GCPMachineTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: gcpMachineTemplateName, Namespace: framework.CAPINamespace, }, Spec: gcpv1.GCPMachineTemplateSpec{ Template: gcpv1.GCPMachineTemplateResource{ Spec: gcpMachineSpec, }, }, } +// Best-effort pre-clean to avoid cross-run collisions +_ = cl.Delete(ctx, &gcpv1.GCPMachineTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: gcpMachineTemplateName, + Namespace: framework.CAPINamespace, + }, +}) +// Optionally poll until NotFound here if flakes observed. + if err := cl.Create(ctx, gcpMachineTemplate); err != nil && !apierrors.IsAlreadyExists(err) { Expect(err).ToNot(HaveOccurred()) }
87-94: Prefer deterministic MAPI MachineSet selectionPicking Items[0] is fragile. Filter by clusterName and role (e.g., worker) or select by label to ensure a stable source for the provider spec.
e2e/framework/machineset.go (1)
78-90: Optional: add standard CAPI labels for easier introspectionAugment selector/template labels with cluster.x-k8s.io/cluster-name. Not required for correctness but helps tooling/debug.
Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ - "machine.openshift.io/cluster-api-cluster": params.clusterName, + "machine.openshift.io/cluster-api-cluster": params.clusterName, + "cluster.x-k8s.io/cluster-name": params.clusterName, machineSetOpenshiftLabelKey: params.msName, }, }, ... Labels: map[string]string{ - "machine.openshift.io/cluster-api-cluster": params.clusterName, + "machine.openshift.io/cluster-api-cluster": params.clusterName, + "cluster.x-k8s.io/cluster-name": params.clusterName, machineSetOpenshiftLabelKey: params.msName, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (107)
.golangci.yml(1 hunks)Dockerfile.rhel(1 hunks)Makefile(2 hunks)cmd/cluster-api-tests-ext/main.go(1 hunks)cmd/cluster-api-tests-ext/provider.go(1 hunks)cmd/cluster-api-tests-ext/types.go(1 hunks)e2e/aws.go(1 hunks)e2e/aws_test.go(0 hunks)e2e/azure.go(1 hunks)e2e/azure_test.go(0 hunks)e2e/baremetal.go(1 hunks)e2e/baremetal_test.go(0 hunks)e2e/common.go(1 hunks)e2e/e2e_test.go(1 hunks)e2e/framework/cluster.go(3 hunks)e2e/framework/framework.go(3 hunks)e2e/framework/machine.go(5 hunks)e2e/framework/machineset.go(7 hunks)e2e/framework/machinetemplate.go(4 hunks)e2e/framework/node.go(3 hunks)e2e/framework/util.go(4 hunks)e2e/gcp.go(1 hunks)e2e/gcp_test.go(0 hunks)e2e/go.mod(0 hunks)e2e/machine_migration.go(1 hunks)e2e/machine_migration_test.go(0 hunks)e2e/machineset_migration.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/mapimachines.go(1 hunks)e2e/mapimachines_test.go(0 hunks)e2e/powervs.go(1 hunks)e2e/powervs_test.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txt(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.md(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.yml(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/tracing.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txt(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go(0 hunks)e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.go(0 hunks)
⛔ Files not processed due to max files limit (22)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/CHANGELOG.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/README.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/assets.json
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/autorest.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/availabilitysets_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/build.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservationgroups_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/capacityreservations_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/ci.yml
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/client_factory.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceoperatingsystems_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroleinstances_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudserviceroles_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservices_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/cloudservicesupdatedomain_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleries_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimages_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5/communitygalleryimageversions_client.go
💤 Files with no reviewable changes (84)
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txt
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.md
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.go
- e2e/aws_test.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.go
- e2e/powervs_test.go
- e2e/machine_migration_test.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.go
- e2e/gcp_test.go
- e2e/go.mod
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.go
- e2e/baremetal_test.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.go
- e2e/azure_test.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.yml
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.go
- e2e/mapimachines_test.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.go
- e2e/machineset_migration_test.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/tracing.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.go
- e2e/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.go
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 27-27: Target "build" should be declared PHONY.
(phonydeclared)
🪛 YAMLlint (1.37.1)
.golangci.yml
[error] 142-142: trailing spaces
(trailing-spaces)
🔇 Additional comments (16)
e2e/framework/machinetemplate.go (1)
68-93: Good improvement: reject empty prefix early.
Returning an error for empty prefixes prevents accidental mass operations.e2e/framework/cluster.go (1)
30-76: Rename + error wrapping LGTM.API rename to CreateCoreCluster and wrapping Get error improves clarity.
e2e/e2e_test.go (1)
24-31: Suite bootstrap simplification LGTM.Centralizing init via InitCommonVariables() is clean.
e2e/azure.go (1)
44-47: Verify suite labels before merging.This spec is labeled Conformance and Serial (and Disruptive). Given earlier reviewer cautions, confirm it won’t be surfaced in openshift/conformance suites unintentionally.
Would you like me to adjust labels or gate it behind the extension-only suite?
e2e/mapimachines.go (1)
50-50: Confirm Conformance labeling.This suite is Serial and Disruptive; keeping Label("Conformance") may promote it into openshift/conformance unexpectedly.
Would you prefer moving these to a non-conformance suite (e.g., capio/conformance/serial only) per prior reviewer guidance?
e2e/machineset_migration.go (1)
48-48: Confirm Conformance labeling.These are Serial and Disruptive; retaining Label("Conformance") may unintentionally put them in origin’s conformance suites.
Recommend keeping them out of openshift/conformance per earlier reviewer guidance unless they meet the criteria.
e2e/framework/node.go (1)
26-40: Context-aware API update looks goodThe addition of context support to
GetNodeForMachinealigns well with the broader PR changes to make the e2e framework context-aware. The error wrapping improvement also provides better debugging context.Makefile (1)
42-44: cluster-api-tests-ext target implementation looks goodThe new target correctly builds the test extension binary from the cmd/cluster-api-tests-ext directory and follows the existing pattern of other build targets.
e2e/machine_migration.go (2)
40-40: Test suite labels align with caution about conformance inclusionThe test is labeled with "Conformance" and "Serial", but based on the PR discussion, there's caution about adding tests to openshift/conformance suites. These should only be included if they meet the strict criteria: fast, highly reliable, <5 minutes runtime, and >=99% pass rate.
Given the discussion in the PR comments about conformance criteria, please verify that these machine migration tests meet the requirements for inclusion in conformance suites. The tests involve creating/deleting machines which may not meet the "fast" and "highly reliable" criteria.
261-316: Strong implementation of machine creation helperThe
createCAPIMachinefunction properly handles:
- Deep copying of reference machine specs
- Clearing instance-specific fields
- Creating both Machine and AWSMachine resources
- Waiting for machine to reach Running state
cmd/cluster-api-tests-ext/types.go (1)
17-61: Struct copy looks good.Fields map cleanly to the upstream e2e.CloudConfig shape; json:"type" for ProviderName is correct.
cmd/cluster-api-tests-ext/provider.go (1)
125-126: Confirm kubectl vs oc.Ensure e2e helpers/scripts expect
kubectl; some OpenShift jobs assumeoc.Would you like me to scan the repo/jobs to confirm which binary is available in CI images?
e2e/aws.go (1)
45-45: Verify labeling for inclusion in suites.
Label("Conformance"), Label("Serial")may pull this into openshift conformance suites via the extension selectors. Given reviewer cautions, confirm this is intended.e2e/framework/machine.go (1)
33-33: Good change: ctx as the first parameter.Aligns with Go conventions and prior feedback.
e2e/gcp.go (1)
39-41: Confirm intent: Conformance + Serial + Disruptive labelsThese labels could land this in origin conformance suites unintentionally. Please confirm the suite wiring and whether this should be excluded from openshift/conformance. If not intended, drop Label("Conformance") (and/or use extension-only suite labels).
e2e/framework/machineset.go (1)
92-95: Bootstrap DataSecretName usage — validate assumptionsIn CAPI, DataSecretName is typically set by the bootstrap controller. Confirm this direct reference is valid in your operator flow and namespace context for the secret.
| if !cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...) { | ||
| GinkgoWriter.Print("Instances created by MAPI and CAPI are not equal\n" + cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the instance comparison assertive (fail on mismatch).
Currently it only logs the diff; the test will pass even if instances differ.
- if !cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...) {
- GinkgoWriter.Print("Instances created by MAPI and CAPI are not equal\n" + cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...))
- }
+ diff := cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...)
+ ExpectWithOffset(1, diff).To(BeEmpty(), "Instances created by MAPI and CAPI differ:\n%s", diff)📝 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.
| if !cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...) { | |
| GinkgoWriter.Print("Instances created by MAPI and CAPI are not equal\n" + cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...)) | |
| } | |
| } | |
| // Replace the non-assertive comparison with an assertion that fails on mismatch | |
| - if !cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...) { | |
| - GinkgoWriter.Print("Instances created by MAPI and CAPI are not equal\n" + | |
| - cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...)) | |
| diff := cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...) | |
| ExpectWithOffset(1, diff).To(BeEmpty(), | |
| "Instances created by MAPI and CAPI differ:\n%s", diff) | |
| } |
🤖 Prompt for AI Agents
In e2e/aws.go around lines 308 to 311, the comparison between mapiEC2Instance
and capiEC2Instance only logs a diff on mismatch which lets the test continue;
change this to an assertion that fails the test when they differ by replacing
the log-only behavior with a Gomega Expect (or equivalent test failure) that
checks cmp.Equal(...) is true and includes the cmp.Diff output in the failure
message so the test fails immediately on mismatch and still reports the diff for
debugging.
| azureCredentialsSecret := corev1.Secret{} | ||
| azureCredentialsSecretKey := types.NamespacedName{Name: "capz-manager-bootstrap-credentials", Namespace: "openshift-cluster-api"} | ||
| err := cl.Get(context.Background(), azureCredentialsSecretKey, &azureCredentialsSecret) | ||
| Expect(err).To(BeNil(), "capz-manager-bootstrap-credentials secret should exist") | ||
|
|
||
| subscriptionID := azureCredentialsSecret.Data["azure_subscription_id"] | ||
| azureImageID := fmt.Sprintf("/subscriptions/%s%s", subscriptionID, mapiProviderSpec.Image.ResourceID) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use shared ctx and constants for secret lookup; cast subscription ID to string.
Align with framework context and constants; make subscription ID type explicit.
Apply:
- azureCredentialsSecret := corev1.Secret{}
- azureCredentialsSecretKey := types.NamespacedName{Name: "capz-manager-bootstrap-credentials", Namespace: "openshift-cluster-api"}
- err := cl.Get(context.Background(), azureCredentialsSecretKey, &azureCredentialsSecret)
- Expect(err).To(BeNil(), "capz-manager-bootstrap-credentials secret should exist")
-
- subscriptionID := azureCredentialsSecret.Data["azure_subscription_id"]
- azureImageID := fmt.Sprintf("/subscriptions/%s%s", subscriptionID, mapiProviderSpec.Image.ResourceID)
+ azureCredentialsSecret := corev1.Secret{}
+ azureCredentialsSecretKey := types.NamespacedName{
+ Name: capzManagerBootstrapCredentials,
+ Namespace: framework.CAPINamespace,
+ }
+ err := cl.Get(ctx, azureCredentialsSecretKey, &azureCredentialsSecret)
+ Expect(err).ToNot(HaveOccurred(), "capz-manager-bootstrap-credentials secret should exist")
+
+ subscriptionID := string(azureCredentialsSecret.Data["azure_subscription_id"])
+ imageID := mapiProviderSpec.Image.ResourceID
+ if !strings.HasPrefix(imageID, "/subscriptions/") {
+ imageID = fmt.Sprintf("/subscriptions/%s%s", subscriptionID, imageID)
+ }
+ azureImageID := imageID📝 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.
| azureCredentialsSecret := corev1.Secret{} | |
| azureCredentialsSecretKey := types.NamespacedName{Name: "capz-manager-bootstrap-credentials", Namespace: "openshift-cluster-api"} | |
| err := cl.Get(context.Background(), azureCredentialsSecretKey, &azureCredentialsSecret) | |
| Expect(err).To(BeNil(), "capz-manager-bootstrap-credentials secret should exist") | |
| subscriptionID := azureCredentialsSecret.Data["azure_subscription_id"] | |
| azureImageID := fmt.Sprintf("/subscriptions/%s%s", subscriptionID, mapiProviderSpec.Image.ResourceID) | |
| azureCredentialsSecret := corev1.Secret{} | |
| azureCredentialsSecretKey := types.NamespacedName{ | |
| Name: capzManagerBootstrapCredentials, | |
| Namespace: framework.CAPINamespace, | |
| } | |
| err := cl.Get(ctx, azureCredentialsSecretKey, &azureCredentialsSecret) | |
| Expect(err).ToNot(HaveOccurred(), "capz-manager-bootstrap-credentials secret should exist") | |
| subscriptionID := string(azureCredentialsSecret.Data["azure_subscription_id"]) | |
| imageID := mapiProviderSpec.Image.ResourceID | |
| if !strings.HasPrefix(imageID, "/subscriptions/") { | |
| imageID = fmt.Sprintf("/subscriptions/%s%s", subscriptionID, imageID) | |
| } | |
| azureImageID := imageID |
🤖 Prompt for AI Agents
In e2e/azure.go around lines 115 to 121, replace context.Background() with the
shared test context variable (ctx), use the existing constants for the secret
name and namespace instead of hard-coded strings (e.g., the capz bootstrap
credentials secret constant and the Cluster API namespace constant used
elsewhere in tests), and ensure subscriptionID is converted to a string before
formatting (e.g., string(azureCredentialsSecret.Data["azure_subscription_id"])
or use fmt.Sprintf("%s", ...) ) so azureImageID is constructed from a string
value.
| ) | ||
|
|
||
| const ( | ||
| machineAPINamespace = "openshift-machine-api" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use framework.MAPINamespace instead of a hard-coded MAPI namespace.
Avoid divergence with the rest of the suite and centralize the namespace source.
Apply:
- machineAPINamespace = "openshift-machine-api"
+ // use framework.MAPINamespace- By(fmt.Sprintf("Listing worker machines in namespace: %s", machineAPINamespace))
+ By(fmt.Sprintf("Listing worker machines in namespace: %s", framework.MAPINamespace))- Eventually(cl.List(ctx, machineList, client.InNamespace(machineAPINamespace), workerLabelSelector)).Should(Succeed(), "Failed to list worker machines")
+ Eventually(cl.List(ctx, machineList, client.InNamespace(framework.MAPINamespace), workerLabelSelector)).Should(Succeed(), "Failed to list worker machines")Also applies to: 102-102, 105-106
🤖 Prompt for AI Agents
In e2e/mapimachines.go around line 35 (and also at lines 102 and 105-106),
replace the hard-coded machineAPINamespace = "openshift-machine-api" with the
centralized framework.MAPINamespace constant; update all occurrences so the file
imports and references framework.MAPINamespace instead of the literal string,
ensuring consistency with the rest of the suite and removing the hard-coded
namespace.
| } | ||
| // Clear status and other instance-specific fields that should not be copied. | ||
| newMachine.Spec.ProviderID = nil | ||
| newMachine.ObjectMeta.Labels = nil |
There was a problem hiding this comment.
Do not drop all labels on the cloned Machine.
Resetting Labels to nil can prevent controllers from recognizing/owning the resource (cluster ID/role labels are required), leading to provisioning failures or flakes.
Apply:
- newMachine.ObjectMeta.Labels = nil
+ // Preserve labels from the template; drop/override only if strictly necessary.📝 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.
| newMachine.ObjectMeta.Labels = nil | |
| // Preserve labels from the template; drop/override only if strictly necessary. |
🤖 Prompt for AI Agents
In e2e/mapimachines.go around line 136, do not clear all labels on the cloned
Machine by setting newMachine.ObjectMeta.Labels = nil; instead preserve and
propagate the original Machine's labels (or at minimum ensure cluster ID/role
labels are copied) when creating the clone — replace the nil assignment with
logic that copies/merges original.ObjectMeta.Labels into
newMachine.ObjectMeta.Labels so controllers can still recognize/own the
resource.
| Eventually(komega.Object(newMachine), DefaultTimeout, DefaultInterval). | ||
| Should(HaveField("Status.Phase", HaveValue(Equal("Running"))), | ||
| fmt.Sprintf("Machine %s did not enter 'Running' phase within the timeout", newMachine.Name)) | ||
|
|
There was a problem hiding this comment.
*Fix Phase assertion to handle both string and string.
machine.Status.Phase may be a string or *string depending on the API; current HaveValue assumes a pointer and can fail.
Apply:
- Should(HaveField("Status.Phase", HaveValue(Equal("Running"))),
+ Should(HaveField("Status.Phase", SatisfyAny(Equal("Running"), PointTo(Equal("Running")))),📝 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.
| Eventually(komega.Object(newMachine), DefaultTimeout, DefaultInterval). | |
| Should(HaveField("Status.Phase", HaveValue(Equal("Running"))), | |
| fmt.Sprintf("Machine %s did not enter 'Running' phase within the timeout", newMachine.Name)) | |
| Eventually(komega.Object(newMachine), DefaultTimeout, DefaultInterval). | |
| Should(HaveField("Status.Phase", SatisfyAny(Equal("Running"), PointTo(Equal("Running")))), | |
| fmt.Sprintf("Machine %s did not enter 'Running' phase within the timeout", newMachine.Name)) |
🤖 Prompt for AI Agents
In e2e/mapimachines.go around lines 142 to 145, the Phase assertion assumes
machine.Status.Phase is a *string which can fail when it's a string; change the
matcher to accept either a string or a *string (for example use
SatisfyAny(Equal("Running"), PointTo(Equal("Running"))) in place of
HaveValue(Equal("Running"))) so the Eventually.Should call passes whether Phase
is a string or a pointer to string.
| AfterEach(func() { | ||
| if platform != configv1.PowerVSPlatformType { | ||
| // Because AfterEach always runs, even when tests are skipped, we have to | ||
| // explicitly skip it here for other platforms. | ||
| Skip("Skipping PowerVS E2E tests") | ||
| } | ||
| framework.DeleteMachineSets(ctx, cl, machineSet) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t call Skip in AfterEach; just no-op for other platforms.
Skip marks the spec as skipped at teardown time. A simple return is cleaner.
Apply this diff:
AfterEach(func() {
if platform != configv1.PowerVSPlatformType {
- // Because AfterEach always runs, even when tests are skipped, we have to
- // explicitly skip it here for other platforms.
- Skip("Skipping PowerVS E2E tests")
+ // AfterEach always runs; for other platforms just no-op.
+ return
}
framework.DeleteMachineSets(ctx, cl, machineSet)
framework.WaitForMachineSetsDeleted(ctx, cl, machineSet)
framework.DeleteObjects(cl, powerVSMachineTemplate)
})📝 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.
| AfterEach(func() { | |
| if platform != configv1.PowerVSPlatformType { | |
| // Because AfterEach always runs, even when tests are skipped, we have to | |
| // explicitly skip it here for other platforms. | |
| Skip("Skipping PowerVS E2E tests") | |
| } | |
| framework.DeleteMachineSets(ctx, cl, machineSet) | |
| AfterEach(func() { | |
| if platform != configv1.PowerVSPlatformType { | |
| // AfterEach always runs; for other platforms just no-op. | |
| return | |
| } | |
| framework.DeleteMachineSets(ctx, cl, machineSet) | |
| framework.WaitForMachineSetsDeleted(ctx, cl, machineSet) | |
| framework.DeleteObjects(cl, powerVSMachineTemplate) | |
| }) |
🤖 Prompt for AI Agents
In e2e/powervs.go around lines 55 to 61, AfterEach currently calls Skip() for
non-PowerVS platforms which marks the spec as skipped during teardown; instead,
change the logic so the function just returns for non-PowerVS platforms and only
runs framework.DeleteMachineSets(ctx, cl, machineSet) when platform ==
configv1.PowerVSPlatformType (i.e., remove the Skip call and guard the
DeleteMachineSets call behind the platform check).
|
|
||
| # Build binaries | ||
| build: operator migration manifests-gen | ||
| build: operator migration manifests-gen cluster-api-tests-ext |
There was a problem hiding this comment.
Declare 'build' target as PHONY
The build target should be declared as PHONY since it doesn't create a file named "build". This ensures it always runs when invoked, regardless of whether a file/directory named "build" exists.
Add the following declaration near other PHONY declarations:
+.PHONY: build
build: operator migration manifests-gen cluster-api-tests-ext📝 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.
| build: operator migration manifests-gen cluster-api-tests-ext | |
| .PHONY: build | |
| build: operator migration manifests-gen cluster-api-tests-ext |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 27-27: Target "build" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In Makefile around line 27, the 'build' target is not declared phony which can
cause make to skip it if a file named "build" exists; add a .PHONY declaration
for build alongside the other PHONY targets (e.g. add ".PHONY: build" near the
existing PHONY declarations) so the build recipe always runs regardless of
filesystem state.
|
close this, created a new pr #413 to move the tests-extension pkg to a separate module |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
10 participants
Per design of Openshift Test Extensions create a Cluster CAPI Operator OpenShift tests extension.
And to find the e2e test cases, I deleted the go.mod file in the e2e directory to make it part of the main module.
Get extension usage
Get extension definition info
List test cases
Run test suites
make e2e