OCPCLOUD-2641,OCPCLOUD-3188: add e2e tests to verify ValidatingAdmissionPolicy #415
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sunzhaohua2: This pull request references OCPCLOUD-2641 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. This pull request references OCPCLOUD-3188 which is a valid 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. |
|
[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 |
WalkthroughThis pull request adds helper functions for testing failure scenarios in machine and machineset migration, alongside corresponding end-to-end tests that verify creation fails when a resource with the same name already exists. A previously pending test for non-authoritative MAPI MachineSet provider spec validation is also enabled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@sunzhaohua2: This pull request references OCPCLOUD-2641 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. This pull request references OCPCLOUD-3188 which is a valid 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
e2e/machineset_migration_capi_authoritative_test.go (1)
61-63: Enabling providerSpec mirror check looks correctActivating this spec and asserting
InstanceTypeon the MAPI providerSpec matches the authoritative CAPI MachineSet is consistent with the setup in this context and should give good signal on mirror correctness. If you want stronger coverage later, you could extendverifyMAPIMachineSetProviderSpeccalls here to check a couple more key fields, but not required for this PR.e2e/machineset_migration_helpers.go (1)
85-99: Failure helper is consistent with the success path; consider tightening error assertionsThe helper mirrors
createMAPIMachineSetWithAuthoritativeAPInicely and gives a clear place to assert admission failures and expected messages. Two optional tweaks you might consider:
- Assert on the error more directly, e.g.
Expect(err).To(MatchError(ContainSubstring(expectedErrorMessage))), to couple the “must fail” and “message” expectations into a single check.- If you care about the failure class (e.g. validation vs conflict), you could also assert
apierrors.IsInvalid(err)/IsForbidden(err)in addition to the substring to guard against unrelated failures.Not blocking; current form is perfectly serviceable for these e2e tests.
e2e/machineset_migration_mapi_authoritative_test.go (1)
58-60: Name‑collision failure test is wired correctlyThe scenario—pre‑create CAPI MachineSet, then assert MAPI MachineSet creation with the same name fails with the expected message via the helper—matches the intended admission behavior and keeps cleanup on the CAPI side only, which is appropriate for an expected failure.
If you end up adding more of these, you might factor the shared error substring (
"a Cluster API MachineSet with the same name already exists") into a package‑level const to avoid brittle duplication, but this is fine as‑is.e2e/machine_migration_helpers.go (1)
131-168: MAPI machine failure helper matches the existing patternsThis helper lines up well with
createMAPIMachineWithAuthority: it selects a worker as a template, scrubs instance‑specific fields, applies the requested authority, and asserts that admission rejects the create with the expected message. That should give you a good place to centralize all “must fail” machine cases.If you want to tighten it later, you could:
- Use
Expect(cl.Create(...)).To(MatchError(ContainSubstring(expectedErrorMessage)))to express the failure and message expectation in one go.- Optionally also assert on the error class (
apierrors.IsInvalid/IsForbidden) to distinguish admission rejections from transport issues.Not required for this PR.
e2e/machine_migration_mapi_authoritative_test.go (1)
32-50: Collision test between CAPI and MAPI Machines is well‑structuredPre‑creating the CAPI Machine, then asserting that creating a MAPI Machine with the same name fails via
createMAPIMachineExpectFailurecleanly exercises the VAP/validation behavior you care about. Cleanup only tracks the CAPI side here, which matches the “no MAPI created” expectation.If you ever want extra defensiveness against unexpected admission behavior changes, you could extend the helper or this context to best‑effort clean up a successfully created MAPI Machine, but that’s a nice‑to‑have rather than a blocker.
📜 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/machine_migration_helpers.go(1 hunks)e2e/machine_migration_mapi_authoritative_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)
🧰 Additional context used
🧬 Code graph analysis (3)
e2e/machine_migration_helpers.go (2)
e2e/framework/machine.go (1)
GetMachines(18-44)pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)
e2e/machineset_migration_helpers.go (1)
e2e/framework/machineset.go (1)
CreateMachineSet(49-94)
e2e/machine_migration_mapi_authoritative_test.go (1)
pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)
|
@sunzhaohua2: 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. |
| } | ||
|
|
||
| // createMAPIMachineExpectFailure attempts to create a MAPI Machine with specified authoritativeAPI and expects the creation to fail. | ||
| func createMAPIMachineExpectFailure(ctx context.Context, cl client.Client, machineName string, authority mapiv1beta1.MachineAuthority, expectedErrorMessage string) { |
There was a problem hiding this comment.
Nit .. Can rename the function as createSameNameMachineBlockedByVAPAuthMapi it would convey exactly what the function is doing without checking its details ?
| } | ||
|
|
||
| // createMAPIMachineSetExpectFailure attempts to create a MAPI MachineSet with specified authoritativeAPI and expects the creation to fail. | ||
| func createMAPIMachineSetExpectFailure(ctx context.Context, cl client.Client, replicas int, machineSetName string, machineSetAuthority mapiv1beta1.MachineAuthority, machineAuthority mapiv1beta1.MachineAuthority, expectedErrorMessage string) { |
There was a problem hiding this comment.
same comment as above for function name
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.