Skip to content

Comments

Resolves: MTV-3380 | Allow setting converter pod label, node selectors and affinities#2260

Open
avivtur wants to merge 2 commits intokubev2v:mainfrom
avivtur:feature/MTV-3380-importer-pod-scheduling
Open

Resolves: MTV-3380 | Allow setting converter pod label, node selectors and affinities#2260
avivtur wants to merge 2 commits intokubev2v:mainfrom
avivtur:feature/MTV-3380-importer-pod-scheduling

Conversation

@avivtur
Copy link
Member

@avivtur avivtur commented Feb 12, 2026

📝 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://

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 14.87%. Comparing base (13484d0) to head (eda550d).
⚠️ Report is 986 commits behind head on main.

Files with missing lines Patch % Lines
...rNodeSelector/ConvertorNodeSelectorDetailsItem.tsx 0.00% 24 Missing and 1 partial ⚠️
...nts/ConvertorLabels/ConvertorLabelsDetailsItem.tsx 0.00% 22 Missing and 1 partial ⚠️
...ConvertorAffinity/ConvertorAffinityDetailsItem.tsx 0.00% 21 Missing and 1 partial ⚠️
.../components/SettingsSection/utils/patchPlanSpec.ts 0.00% 7 Missing ⚠️
...rgetNodeSelector/TargetNodeSelectorDetailsItem.tsx 0.00% 5 Missing ⚠️
...nents/TargetAffinity/TargetAffinityDetailsItem.tsx 0.00% 4 Missing ⚠️
...omponents/TargetLabels/TargetLabelsDetailsItem.tsx 0.00% 4 Missing ⚠️
...ils/components/SettingsSection/SettingsSection.tsx 0.00% 3 Missing ⚠️
...mponents/AffinityModal/utils/affinityToRowsData.ts 0.00% 1 Missing ⚠️
...tyViewDetailsItemContent/utils/getAffinityRules.ts 0.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good! I recommend breaking tests into test.step( blocks.

@avivtur
Copy link
Member Author

avivtur commented Feb 12, 2026

/retest

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost identical to TargetAffinityModal.ts, consider creating a shared base class.

Copy link
Collaborator

@Pedro-S-Abreu Pedro-S-Abreu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost identical to TargetLabelsModal.ts

Copy link
Collaborator

@Pedro-S-Abreu Pedro-S-Abreu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost identical to TargetNodeSelectorModal.ts

Copy link
Collaborator

@Pedro-S-Abreu Pedro-S-Abreu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider reusing or generalizing the existing Target Node methods instead of creating identical Convertor copies

@avivtur avivtur force-pushed the feature/MTV-3380-importer-pod-scheduling branch 6 times, most recently from 9454977 to 46c2b71 Compare February 18, 2026 13:12
testId: string,
modal: { waitForModalToOpen: () => Promise<void> },
): Promise<void> {
await this.page.getByTestId(testId).locator('button').click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

planName is also redundant, changed

@avivtur avivtur force-pushed the feature/MTV-3380-importer-pod-scheduling branch 2 times, most recently from 1d2eea3 to ad54fcf Compare February 19, 2026 13:13
@avivtur
Copy link
Member Author

avivtur commented Feb 19, 2026

/retest

…s and affinities

Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
@avivtur avivtur force-pushed the feature/MTV-3380-importer-pod-scheduling branch from 0d90717 to eda550d Compare February 19, 2026 19:31
@sonarqubecloud
Copy link

Copy link
Contributor

@jpuzz0 jpuzz0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 optional nit comment. LGTM

content={
<AffinityViewDetailsItemContent
affinity={plan?.spec?.targetAffinity as K8sIoApiCoreV1Affinity}
/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants