Wizard: fix root not removable when duplicate #3614

avitova · 2025-09-04T13:56:00Z

We allow adding more root mountpoints via import feature, which should not be possible. This fix allows users to remove extra root partition, but keeps it in the wizard so that users know about it.

avitova · 2025-09-04T14:01:44Z

Better safe than sorry. 🎉 The bug is here:

Screencast.From.2025-09-04.16-00-54.mp4

codecov · 2025-09-04T14:12:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.53%. Comparing base (92ef8ee) to head (2503f5d).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3614      +/-   ##
==========================================
- Coverage   82.58%   82.53%   -0.06%     
==========================================
  Files         204      204              
  Lines       24674    24694      +20     
  Branches     2550     2559       +9     
==========================================
+ Hits        20378    20382       +4     
- Misses       4269     4285      +16     
  Partials       27       27

Files with missing lines	Coverage Δ
...rd/steps/FileSystem/components/FileSystemTable.tsx	`74.76% <100.00%> (+0.67%)`	⬆️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92ef8ee...2503f5d. Read the comment docs.

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

avitova · 2025-09-04T16:50:06Z

/retest

kingsleyzissou

Nice!

avitova · 2025-09-05T09:34:32Z

/retest

ksiekl · 2025-09-05T11:48:12Z

Great! I just wonder why in your bug video there wasn't an exclamation mark next to the FS configuration step in nav bar 🤔 Was is just a slow load? Maybe that's worth investigating if our validation is not set correctly for that step, but that's a different PR kinda thing. Nice work!

avitova · 2025-09-05T13:14:34Z

Yeah, the validation happens for some reason only on click of the next button. 😄

mgold1234 · 2025-09-07T10:29:57Z

@avitova very nice, is it possible to add unit test to check this change?

avitova · 2025-09-08T10:38:12Z

Good point, @mgold1234 . However, I decided to do Playwright tests, as they are more easy to write and read. What do you think about this way of importing blueprints that we define in a file, @tkoscieln ?
I found some more unexpected behaviour in our error handling thanks to the Playwright test. Let's get #3623 in first. Thanks!

We allow adding more root mountpoints via import feature, which should not be possible. This fix allows users to remove extra root partition, but keeps it in the wizard so that users know about it.

tkoscieln

@avitova this way of importing blueprints LGTM. Added some additional comments.

tkoscieln · 2025-09-08T12:18:59Z

playwright/fixtures/importFileContents.ts

I would maybe create a subdirectory fixtures/data to separate these files from the fixture functions. But this is a nitpick.

tkoscieln · 2025-09-08T12:23:27Z

playwright/Import/Import.spec.ts

+  const frame = await ibFrame(page);
+
+  await test.step('Import BP', async (step) => {
+    step.skip(!isHosted(), 'Importing is not available in the plugin');


We should propably skip the entire test if the importing is not avaiable in Cockpit plugin at all. You can use test.skip for that.

This is also what is failing the cockpit tests in Schutzbot. It expects the wizard to be open from the Import helper function, but it is not.

tkoscieln · 2025-09-08T12:29:53Z

playwright/helpers/wizardHelpers.ts

@@ -143,3 +146,17 @@ export const importBlueprint = async (
    await page.getByRole('button', { name: 'Review and Finish' }).click();
  }
 };
+
+export const importDynamicFixture = async (


We should propably change the name of this helper function to something else since it just creates a file 🤔 Also please add docstring 🥺

tkoscieln · 2025-09-08T12:54:49Z

playwright/Import/Import.spec.ts

I just realized this test is not running in the CI at all. Since I've introduced the Boot tests, the playwright action for the other tests that should run on PRs need to have the directory specified for the playwright command here. So since you placed the test in a new directory, it needs to be added there. I should propably document this, my bad 😬

tkoscieln · 2025-09-08T13:06:24Z

playwright/Import/Import.spec.ts

+    await registerLater(frame);
+  });
+
+  await test.step('Select the Locale step', async () => {


Could you divide this one big step into several smaller steps that encapsulate every wizard step that is being checked?

This commits adds a constant with blueprint in order for us to test different cases of errors that might ocurr out of a blueprint.

avitova · 2025-09-08T13:42:29Z

Thank you, Tomas! I tried to address your comments.

avitova requested review from regexowl, mgold1234 and ksiekl September 4, 2025 14:14

avitova force-pushed the edge-case-import branch 2 times, most recently from fb08d6a to 958abb6 Compare September 4, 2025 15:21

avitova requested a review from kingsleyzissou September 4, 2025 16:49

avitova force-pushed the edge-case-import branch from 958abb6 to cec74a0 Compare September 5, 2025 07:50

avitova requested a review from a team as a code owner September 5, 2025 07:50

kingsleyzissou previously approved these changes Sep 5, 2025

View reviewed changes

kingsleyzissou enabled auto-merge September 5, 2025 08:48

avitova force-pushed the edge-case-import branch from cec74a0 to a73a0a2 Compare September 5, 2025 09:52

ksiekl previously approved these changes Sep 5, 2025

View reviewed changes

avitova dismissed stale reviews from ksiekl and kingsleyzissou via 6cb2b2e September 8, 2025 10:33

avitova force-pushed the edge-case-import branch 3 times, most recently from 8974b71 to 3387288 Compare September 8, 2025 10:37

Wizard: duplicated root not removable (HMS-9280)

b64e6ef

We allow adding more root mountpoints via import feature, which should not be possible. This fix allows users to remove extra root partition, but keeps it in the wizard so that users know about it.

avitova force-pushed the edge-case-import branch from 3387288 to 0494874 Compare September 8, 2025 12:14

tkoscieln requested changes Sep 8, 2025

View reviewed changes

Playwright: add helper function to use external blueprints for import

ef2a221

This commits adds a constant with blueprint in order for us to test different cases of errors that might ocurr out of a blueprint.

avitova force-pushed the edge-case-import branch from 0494874 to e1f4186 Compare September 8, 2025 13:39

Playwright: add Import tests using predefined blueprint jsons

2503f5d

avitova force-pushed the edge-case-import branch from e1f4186 to 2503f5d Compare September 8, 2025 15:00

Wizard: fix root not removable when duplicate #3614

Are you sure you want to change the base?

Wizard: fix root not removable when duplicate #3614

Uh oh!

Conversation

avitova commented Sep 4, 2025

Uh oh!

avitova commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

avitova commented Sep 4, 2025

Uh oh!

kingsleyzissou left a comment

Choose a reason for hiding this comment

Uh oh!

avitova commented Sep 5, 2025

Uh oh!

ksiekl commented Sep 5, 2025

Uh oh!

avitova commented Sep 5, 2025

Uh oh!

mgold1234 commented Sep 7, 2025

Uh oh!

avitova commented Sep 8, 2025

Uh oh!

tkoscieln left a comment

Choose a reason for hiding this comment

Uh oh!

tkoscieln Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tkoscieln Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tkoscieln Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tkoscieln Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tkoscieln Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tkoscieln Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

avitova commented Sep 8, 2025

Uh oh!

Uh oh!

avitova commented Sep 4, 2025 •

edited

Loading

codecov bot commented Sep 4, 2025 •

edited

Loading