NO-JIRA: port OTP CAPI tests to OTE framework - #627
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@stefanonardo: This pull request explicitly references no jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ordered OpenShift end-to-end suites covering Cluster API Deployment configuration, IPAM CRDs, worker Secret recreation, and webhook rejection of invalid updates and deletions. ChangesCluster API e2e validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant KubernetesAPI
participant Webhook
E2ETest->>KubernetesAPI: Submit invalid infrastructure reference
KubernetesAPI->>Webhook: Validate patch
Webhook-->>KubernetesAPI: Reject invalid kind
KubernetesAPI-->>E2ETest: Return validation error
E2ETest->>KubernetesAPI: Submit dry-run deletion
KubernetesAPI->>Webhook: Validate deletion
Webhook-->>KubernetesAPI: Reject deletion
KubernetesAPI-->>E2ETest: Return denial error
🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
74e1114 to
5df1330
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/otp_capi_machines.go`:
- Around line 25-29: Require the named managed resources directly: in
e2e/otp_capi_machines.go lines 25-29, replace the ClusterList lookup and
first-item selection with a fetch of the Cluster named clusterName; in
e2e/otp_capi.go lines 123-125, fail the test when the matching infrastructure
resource is absent instead of skipping it.
In `@e2e/otp_capi.go`:
- Around line 23-26: The user-facing test output in e2e/otp_capi.go lines 23-26
must say “Cluster API” instead of “CAPI,” including the deployment-listing step
and namespace assertion. In e2e/otp_capi_machines.go lines 19-27, update the
skip, test, step, and assertion text to use “Cluster API” consistently; no
direct changes are required elsewhere.
- Around line 127-130: Update the deletion checks to use dry-run operations via
client.DryRunAll: in e2e/otp_capi.go, pass the option to the infrastructure
resource Delete call; in e2e/otp_capi_machines.go, pass it to the Cluster Delete
call. Preserve the existing denied-error assertions at both sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6551d145-75c2-420a-8dd9-964ce5e7400d
📒 Files selected for processing (2)
e2e/otp_capi.goe2e/otp_capi_machines.go
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| var _ = Describe("[OTP][Jira:OCPCLOUD][OCPFeatureGate:ClusterAPIInstall][sig-cluster-lifecycle] Cluster_Infrastructure CAPI", Ordered, func() { |
There was a problem hiding this comment.
do we keep [OTP] tag if it is no longer in OTP? What about [Jira:OCPCLOUD] tag?
There was a problem hiding this comment.
it's required by the OTE integration guide for tracking purposes
| @@ -0,0 +1,147 @@ | |||
| package e2e | |||
There was a problem hiding this comment.
do we want otp_ filename prefix?
There was a problem hiding this comment.
not really, I renamed the tests
|
/test e2e-aws-capi-techpreview |
2118b06 to
5453621
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/webhook_validation.go (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive parameter name.
Rename
ptoplatformto make the mapping logic self-explanatory. As per coding guidelines, use descriptive names for functions, variables, and identifiers.Suggested refactor
-func platformToInfraClusterKind(p configv1.PlatformType) string { - switch p { +func platformToInfraClusterKind(platform configv1.PlatformType) string { + switch platform {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/webhook_validation.go` around lines 84 - 85, Rename the parameter `p` to `platform` in `platformToInfraClusterKind` and update all references within the function’s switch or mapping logic accordingly, without changing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/operator.go`:
- Around line 50-64: Update the test around “should have FallbackToLogsOnError
as terminationMessagePolicy” to list Deployments in framework.CAPINamespace
instead of Pods, assert the Deployment list is non-empty, and inspect each
deployment’s deploy.Spec.Template.Spec.Containers for
TerminationMessageFallbackToLogsOnError.
- Around line 79-89: Update the secret deletion and re-sync verification around
secretKey to capture the original Secret UID before calling cl.Delete. In the
Eventually assertion, fetch the recreated Secret into a variable and require its
UID to differ from the saved UID, ensuring the check confirms recreation rather
than observing the deleting object.
---
Nitpick comments:
In `@e2e/webhook_validation.go`:
- Around line 84-85: Rename the parameter `p` to `platform` in
`platformToInfraClusterKind` and update all references within the function’s
switch or mapping logic accordingly, without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0aa85948-a4f0-47a9-89f0-f21f24c261ec
📒 Files selected for processing (2)
e2e/operator.goe2e/webhook_validation.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/go.mod`:
- Line 23: Update the module requirements in e2e/go.mod for
github.com/metal3-io/baremetal-operator/apis to v0.8.1 and
go.mongodb.org/mongo-driver to v1.17.7, preserving all other dependency versions
and declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 391ef080-064d-45de-bcaf-77a417e57e87
⛔ Files ignored due to path filters (1)
e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
e2e/go.mode2e/operator.goe2e/webhook_validation.go
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/operator.go
- e2e/webhook_validation.go
5453621 to
e1b4b82
Compare
|
/test e2e-aws-ovn-techpreview |
e1b4b82 to
6bee369
Compare
|
/test e2e-aws-ovn-techpreview |
|
/refresh tide |
openshift-ci
Bot
added
the
needs-rebase
6bee369 to
0b306f2
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
5b8e32a to
588eb2f
Compare
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| var _ = Describe("[OTP][Jira:OCPCLOUD][OCPFeatureGate:ClusterAPIMachineManagement][sig-cluster-lifecycle] Cluster_Infrastructure CAPI", Ordered, func() { |
There was a problem hiding this comment.
Do we need Ordered? Also for the other Describe block.
| github.com/metal3-io/cluster-api-provider-metal3/api v1.11.2 | ||
| github.com/onsi/ginkgo/v2 v2.28.1 | ||
| github.com/onsi/gomega v1.39.1 | ||
| github.com/openshift-eng/openshift-tests-extension v0.0.0-20260612102633-2fd5b2fa4221 |
There was a problem hiding this comment.
Not major but.
The openshift-tests-extension submodule was intentionally kept as a separate Go module to isolate its dependencies from the main module.
We could switch
- oteg.Informing()
+ Label("Lifecycle:informing")to eliminate the dependency since Ginkgo is already a main module dependency.
There was a problem hiding this comment.
The other repos are not following this practise, so we might keep it as it is.
There was a problem hiding this comment.
I believe the OTE migration skill did this, I'll replace it with Label
There was a problem hiding this comment.
maybe it was coming from here: https://github.com/openshift/cluster-capi-operator/blob/main/openshift-tests-extension/README.md?plain=1#L90
If your moving forward with it, can you also update the README?
| ) | ||
|
|
||
| var _ = Describe("[OTP][Jira:OCPCLOUD][OCPFeatureGate:ClusterAPIMachineManagement][sig-cluster-lifecycle] Cluster_Infrastructure CAPI", Ordered, func() { | ||
|
|
There was a problem hiding this comment.
When tests are invoked via make e2e (bypassing OTE tag filtering), they'll run regardless of gate state and fail on a non-TechPreview cluster. Needs a BeforeEach FG guard.
BeforeEach(func() {
if !framework.IsFeatureGateEnabled(ctx, cl, features.FeatureGateClusterAPIMachineManagement) {
Skip("Feature gate ClusterAPIMachineManagement is not enabled.")
}
})| @@ -0,0 +1,96 @@ | |||
| package e2e | |||
There was a problem hiding this comment.
Both files are missing the Apache 2.0 license header that every other file in this package carries
| var _ = Describe("[OTP][Jira:OCPCLOUD][OCPFeatureGate:ClusterAPIMachineManagement][sig-cluster-lifecycle] Cluster API Webhook Validation", Ordered, func() { | ||
|
|
||
| It("should deny deletion of infrastructure cluster resources", Label("Disruptive"), oteg.Informing(), func() { | ||
| switch platform { |
There was a problem hiding this comment.
This is a nice addon for skiping when using make e2e and it also skips the test at runtime when running with openshift-tests.
However, #613 introduces Label("platform:<name>") with OTE suite-level wiring that filters at scheduling time.
Adding the label avoids OTE spinning up test infrastructure for these tests on unsupported platforms.
This might be a follow up, since the convention is only introduced at #613
59f23b2 to
3d9bc10
Compare
There was a problem hiding this comment.
PR #606 introduces tests for the foundational deployment layer - highly deterministic, fast checks.
This PR ports deeper operational/functional behavior tests that happen to share the same gate.
That said, most tests here are also checking static state (annotations exist, CRDs exist, pod spec fields) - equally deterministic.
The secret re-sync test is the exception: it deletes a secret and waits for the controller to recreate it, making it a reconciliation/timing test and inherently more fragile. Also worth noting: the secret sync controller activates with the platform-specific gate (e.g. ClusterAPIMachineManagementAWS), so this test may be gated on the wrong feature gate.
I don't see direct overlaps between the two PRs, but #606 established a convention where tests for a specific feature gate live in a single file (e.g. cluster_api_machine_management.go, cluster_api_machine_management_aws.go). These PR breaks the pattern.
| func platformToInfraClusterKind(p configv1.PlatformType) string { | ||
| switch p { | ||
| case configv1.AWSPlatformType: | ||
| return "AWSCluster" | ||
| case configv1.GCPPlatformType: | ||
| return "GCPCluster" | ||
| case configv1.VSpherePlatformType: | ||
| return "VSphereCluster" | ||
| case configv1.AzurePlatformType: | ||
| return "AzureCluster" | ||
| default: | ||
| return "" | ||
| } | ||
| } |
There was a problem hiding this comment.
The codebase already has pkg/util.GetCAPITypesForInfrastructure() which maps platform to infra cluster kind. See webhook_test.go for usage.
| Eventually(func() bool { | ||
| recreated := &corev1.Secret{} | ||
| if err := cl.Get(ctx, secretKey, recreated); err != nil { | ||
| return false | ||
| } | ||
| return recreated.UID != originalUID | ||
| }, framework.WaitMedium, framework.RetryMedium).Should(BeTrue(), | ||
| "worker-user-data secret should be re-created from %s", framework.MAPINamespace) | ||
| }) |
There was a problem hiding this comment.
I see your not using komega here, and other places.
# example with komega
recreated := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: secretKey.Name, Namespace: secretKey.Namespace}}
Eventually(komega.Object(recreated)).WithTimeout(framework.WaitMedium).WithPolling(framework.RetryMedium).Should(
HaveField("UID", Not(Equal(originalUID))),
)This is the most impactful one - it fixes the swallowed-error problem and the missing-komega problem in a single change.
The other calls would also benefit from retry on transient API errors. The simple Gets are lower priority but still worth aligning for consistency.
3d9bc10 to
93a0cf8
Compare
Port 6 tests from openshift-tests-private clusterinfrastructure/capi to OTE using controller-runtime patterns. Tests include [OTP], [Jira:OCPCLOUD], [OCPFeatureGate:ClusterAPIInstall] tags and start as Informing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
93a0cf8 to
f3b8ac4
Compare
|
@stefanonardo: all tests passed! 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. |
|
Since #606 has been merged we can already (but I am postponing to apply in one go):
Lets just wait for #613
|
|
I was starting to implement the above comment but stumbled with the 2 concerns.
The following ported tests have pass rates bellow it:
Maybe we shouldn't port them.
|
5 participants
Summary
clusterinfrastructure/capito the existing OTE framework[OTP],[Jira:OCPCLOUD],[OCPFeatureGate:ClusterAPIMachineManagement],[sig-cluster-lifecycle]and start asInformingFiles
e2e/operator.go— operator configuration checks (deployment annotations, CRD installation, terminationMessagePolicy, secret sync)e2e/webhook_validation.go— webhook enforcement (infra cluster deletion denied, invalid cluster kind patch rejected, cluster deletion denied)Tests ported
🤖 Generated with Claude Code
Summary by CodeRabbit