Wizard: make users optional for image mode builds (HMS-11164) - #4739
Wizard: make users optional for image mode builds (HMS-11164)#4739lucasgarfield wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The hardcoded
DISK_IMAGE_TYPESarray inNoUsersAlertcould drift from the list of supported disk image types; consider centralizing these types or deriving them from existing configuration/state to avoid future mismatches. - The
hasUsercheck inNoUsersAlertonly looks atuser.name.trim() !== ''; if there are placeholder or partially configured users, you may want a more robust condition (e.g., ensuring the user is actually usable for login) to avoid misleading warnings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded `DISK_IMAGE_TYPES` array in `NoUsersAlert` could drift from the list of supported disk image types; consider centralizing these types or deriving them from existing configuration/state to avoid future mismatches.
- The `hasUser` check in `NoUsersAlert` only looks at `user.name.trim() !== ''`; if there are placeholder or partially configured users, you may want a more robust condition (e.g., ensuring the user is actually usable for login) to avoid misleading warnings.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/Review/components/tests/NoUsersAlert.test.tsx" line_range="33-40" />
<code_context>
+});
+
+describe('NoUsersAlert', () => {
+ test('warns when a disk image has no users', async () => {
+ renderWithRedux(<NoUsersAlert />, imageModeOverrides());
+
+ expect(await screen.findByText(/no users added/i)).toBeInTheDocument();
+ expect(screen.getByText(/cloud-init/i)).toBeInTheDocument();
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider covering all supported disk image types and mixed-type outputs
The current test only exercises the `guest-image` type, but `NoUsersAlert` treats `guest-image`, `aws`, and `ami` as disk images. To better align tests with the implementation, please:
1. Add coverage for `aws` and `ami` (e.g., via a parameterized test) to confirm they also show the warning when no users are configured.
2. Add a case where `imageTypes` mixes disk and non-disk types (e.g. `['guest-image', 'bootable-container-iso']`) to confirm that any disk image still triggers the alert.
This will keep the tests resilient if `DISK_IMAGE_TYPES` changes in the future.
```suggestion
describe('NoUsersAlert', () => {
test.each([
['guest-image'],
['aws'],
['ami'],
])('warns when a %s disk image has no users', async (imageType) => {
renderWithRedux(
<NoUsersAlert />,
imageModeOverrides({
output: {
...initialState.output,
imageTypes: [imageType],
},
})
);
expect(await screen.findByText(/no users added/i)).toBeInTheDocument();
expect(screen.getByText(/cloud-init/i)).toBeInTheDocument();
});
test('warns when any disk image is present alongside non-disk images', async () => {
renderWithRedux(
<NoUsersAlert />,
imageModeOverrides({
output: {
...initialState.output,
imageTypes: ['guest-image', 'bootable-container-iso'],
},
})
);
expect(await screen.findByText(/no users added/i)).toBeInTheDocument();
expect(screen.getByText(/cloud-init/i)).toBeInTheDocument();
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| describe('NoUsersAlert', () => { | ||
| test('warns when a disk image has no users', async () => { | ||
| renderWithRedux(<NoUsersAlert />, imageModeOverrides()); | ||
|
|
||
| expect(await screen.findByText(/no users added/i)).toBeInTheDocument(); | ||
| expect(screen.getByText(/cloud-init/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Consider covering all supported disk image types and mixed-type outputs
The current test only exercises the guest-image type, but NoUsersAlert treats guest-image, aws, and ami as disk images. To better align tests with the implementation, please:
- Add coverage for
awsandami(e.g., via a parameterized test) to confirm they also show the warning when no users are configured. - Add a case where
imageTypesmixes disk and non-disk types (e.g.['guest-image', 'bootable-container-iso']) to confirm that any disk image still triggers the alert.
This will keep the tests resilient if DISK_IMAGE_TYPES changes in the future.
| describe('NoUsersAlert', () => { | |
| test('warns when a disk image has no users', async () => { | |
| renderWithRedux(<NoUsersAlert />, imageModeOverrides()); | |
| expect(await screen.findByText(/no users added/i)).toBeInTheDocument(); | |
| expect(screen.getByText(/cloud-init/i)).toBeInTheDocument(); | |
| }); | |
| describe('NoUsersAlert', () => { | |
| test.each([ | |
| ['guest-image'], | |
| ['aws'], | |
| ['ami'], | |
| ])('warns when a %s disk image has no users', async (imageType) => { | |
| renderWithRedux( | |
| <NoUsersAlert />, | |
| imageModeOverrides({ | |
| output: { | |
| ...initialState.output, | |
| imageTypes: [imageType], | |
| }, | |
| }) | |
| ); | |
| expect(await screen.findByText(/no users added/i)).toBeInTheDocument(); | |
| expect(screen.getByText(/cloud-init/i)).toBeInTheDocument(); | |
| }); | |
| test('warns when any disk image is present alongside non-disk images', async () => { | |
| renderWithRedux( | |
| <NoUsersAlert />, | |
| imageModeOverrides({ | |
| output: { | |
| ...initialState.output, | |
| imageTypes: ['guest-image', 'bootable-container-iso'], | |
| }, | |
| }) | |
| ); | |
| expect(await screen.findByText(/no users added/i)).toBeInTheDocument(); | |
| expect(screen.getByText(/cloud-init/i)).toBeInTheDocument(); | |
| }); | |
30fadd6 to
dd6d0ae
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## lucas/cockpit/5-container-installer #4739 +/- ##
=======================================================================
+ Coverage 78.39% 79.09% +0.69%
=======================================================================
Files 264 265 +1
Lines 7016 7021 +5
Branches 2583 2546 -37
=======================================================================
+ Hits 5500 5553 +53
+ Misses 1437 1372 -65
- Partials 79 96 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
dd6d0ae to
c162763
Compare
c162763 to
89e254c
Compare
f3b2efa to
dd6d0ae
Compare
dd6d0ae to
2ace333
Compare
2ace333 to
068c089
Compare
The KVM image ships cloud-init, so a user can be configured at launch instead of at build time. Drop the on-prem image mode requirement to add a user before continuing, and remove the matching note from the Users step. Since forgetting a user is now an easy mistake, warn on the review step when a disk image (qcow2 or ami, not the installer iso) has no users, pointing at cloud-init as the alternative. Also note on the Users step that on-prem passwords are stored in plain text on the host, with an openssl passwd -6 hint for storing a hash instead. Resolves #4713
068c089 to
78776f1
Compare
|
Replaced by #4756 |
Users are no longer required for an on-prem build, since the KVM image ships cloud-init — the review step warns instead when a disk image has none configured.
Stack created with GitHub Stacks CLI • Give Feedback 💬