Resolves: MTV-3380 | Allow setting converter pod label, node selectors and affinities#2260
Resolves: MTV-3380 | Allow setting converter pod label, node selectors and affinities#2260avivtur wants to merge 2 commits intokubev2v:mainfrom
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2260 +/- ##
===========================================
- Coverage 36.81% 14.87% -21.95%
===========================================
Files 158 1136 +978
Lines 2548 20253 +17705
Branches 599 3988 +3389
===========================================
+ Hits 938 3012 +2074
- Misses 1428 16578 +15150
- Partials 182 663 +481 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Tests look good! I recommend breaking tests into test.step( blocks.
|
/retest |
There was a problem hiding this comment.
This is almost identical to TargetAffinityModal.ts, consider creating a shared base class.
There was a problem hiding this comment.
almost identical to TargetLabelsModal.ts
There was a problem hiding this comment.
almost identical to TargetNodeSelectorModal.ts
There was a problem hiding this comment.
Consider reusing or generalizing the existing Target Node methods instead of creating identical Convertor copies
9454977 to
46c2b71
Compare
src/plans/details/tabs/Details/components/SettingsSection/utils/patchPlanSpec.ts
Outdated
Show resolved
Hide resolved
...onents/SettingsSection/components/ConvertorNodeSelector/ConvertorNodeSelectorDetailsItem.tsx
Show resolved
Hide resolved
...onents/SettingsSection/components/ConvertorNodeSelector/ConvertorNodeSelectorDetailsItem.tsx
Outdated
Show resolved
Hide resolved
| testId: string, | ||
| modal: { waitForModalToOpen: () => Promise<void> }, | ||
| ): Promise<void> { | ||
| await this.page.getByTestId(testId).locator('button').click(); |
There was a problem hiding this comment.
Nice refactor consolidating these into a shared helper. .locator('button').click() grabs the first button inside the test-id container — if a detail item ever has multiple buttons (e.g., help popover + edit), this could click the wrong one. Is there a more specific selector for the edit button?
There was a problem hiding this comment.
this should be in a different PR, @Pedro-S-Abreu can we open a ticket for this improvment?
| const { name: planName, namespace } = testPlan.metadata; | ||
| await planDetailsPage.navigate(planName, namespace); | ||
| await planDetailsPage.verifyPlanTitle(planName); | ||
| return { planDetailsPage, planName, namespace, testData: testPlan.testData }; |
There was a problem hiding this comment.
Nit: namespace and testData are returned here but never destructured in any of the test calls (lines 27, 73, 132). Could simplify the return to just { planDetailsPage, planName }.
There was a problem hiding this comment.
planName is also redundant, changed
1d2eea3 to
ad54fcf
Compare
|
/retest |
…s and affinities Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
0d90717 to
eda550d
Compare
|
jpuzz0
left a comment
There was a problem hiding this comment.
2 optional nit comment. LGTM
| content={ | ||
| <AffinityViewDetailsItemContent | ||
| affinity={plan?.spec?.targetAffinity as K8sIoApiCoreV1Affinity} | ||
| /> |
There was a problem hiding this comment.
nit: I think you can store plan?.spec?.targetAffinity as K8sIoApiCoreV1Affinity as a variable so you don't have to cast twice
| @@ -0,0 +1,63 @@ | |||
| import type { FC } from 'react'; | |||
| import { isPlanEditable } from 'src/plans/details/components/PlanStatus/utils/utils'; | |||
There was a problem hiding this comment.
Nit: partially addressed from previous review — @components/* and @utils/* aliases are now used, but isPlanEditable still imports via 'src/plans/...' path. Consistent with the sibling target components though, so more of a future cleanup note.



📝 Links
https://issues.redhat.com/browse/MTV-3380
📝 Description
Adding settings under plan details for converterLabels, converterNodeSelectors and converterAffinities
🎥 Demo
Screen.Recording.2026-02-12.at.12.26.34.mov
📝 CC://