card complete or bug fix so update to reflect it #432
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughTwo E2E test files were modified to enable previously pending tests. One test is reactivated by changing its status from pending to active. The other test implements concrete logic to verify rejection of MAPI MachineSet creation when a CAPI MachineSet with the same name exists. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes These are straightforward test modifications involving simple spec status changes and basic test assertion logic. No complex logic, interdependencies, or architectural changes are present. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration 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 (2)
🧰 Additional context used🧬 Code graph analysis (1)e2e/machineset_migration_mapi_authoritative_test.go (1)
🔇 Additional comments (2)
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 |
|
Scheduling tests matching the |
|
@huali9 @sunzhaohua2 @chrischdi , the image we are using here is 4.21 of private tests , which missed the revert(branch cut) . As far as this PR is concerned this is good. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@huali9: 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. |
| machineSetParams.Name = existingCAPIMSAuthorityMAPIName | ||
| machineSetParams.Labels[mapiframework.MachineSetKey] = existingCAPIMSAuthorityMAPIName | ||
| machineSetParams.MachinesetAuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI | ||
| machineSetParams.MachineAuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI |
There was a problem hiding this comment.
No, one for MachineSet (spec.authoritativeAPI) and the other for Machine (spec.template.spec.authoritativeAPI)
| By("Attempting to create a MAPI MachineSet with the same name as existing CAPI MachineSet") | ||
| machineSetParams := mapiframework.BuildMachineSetParams(ctx, cl, 0) | ||
| machineSetParams.Name = existingCAPIMSAuthorityMAPIName | ||
| machineSetParams.Labels[mapiframework.MachineSetKey] = existingCAPIMSAuthorityMAPIName |
There was a problem hiding this comment.
question/non-blocking nit: why are we setting this? IIUC we don't read any labels to determine VAP enforcement
There was a problem hiding this comment.
Oh, I copied this from the existing implementation for consistency. I assumed we had reviewed that pattern before. If it's not needed for VAP enforcement, happy to remove it.
|
Do we have sufficient e2e coverage for the other part of the VAP? so creating a MAPI MachineSet with AuthoritativeAPI ClusterAPI (this should be allowed) We may not need to add one explicitly, we may be covering this somewhere else already - I'm just not sure :) |
Thank you for the asking! We already have this part "creating a MAPI MachineSet with AuthoritativeAPI ClusterAPI (this should be allowed)" This test focuses on MAPI MachineSet with AuthoritativeAPI MachineAPI, I don't think we have explicit coverage for this scenario. The current VAP e2e tests are limited to Machines (added by Milind). Not sure if we wanted to extend the VAP test coverage to MachineSets as well. @miyadav @sunzhaohua2 Do you have any thoughts on this? |
Once pr #440 merged, we can create a similar file for machineset VAP test. This pr is mostly dup with #415 @huali9 can you add |
@sunzhaohua2 Sorry, I didn't notice #415 earlier. Since this PR is mostly a duplicate of #415, I think it makes sense for me to close this one. Feel free to proceed with #415 instead! Thank you! /close |
|
@huali9: Closed this PR. 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 kubernetes-sigs/prow repository. |
7 participants
https://issues.redhat.com/browse/OCPCLOUD-3188 and https://issues.redhat.com/browse/OCPBUGS-55337 closed, so update the code to reflect it.
tested locally passed. @sunzhaohua2 @miyadav @damdo PTAL, thanks!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.