-
Notifications
You must be signed in to change notification settings - Fork 21
Validate storage show warnings #789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Validate storage show warnings #789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adamkankovsky - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -280,13 +301,24 @@ export const applyStorage = async ({ devices, luks, onFail, onSuccess, partition | |||
await setBootloaderDrive({ drive: "" }); | |||
} | |||
|
|||
const tasks = await partitioningConfigureWithTask({ partitioning }); | |||
const configureTasks = await partitioningConfigureWithTask({ partitioning }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider flattening the async flow by combining storage task calls into a single async/await block.
Consider flattening the async flow by combining the storage task calls into a single async/await block. For example, rather than nesting callbacks in both the onConfigureTaskSuccess
and the partitioningValidate
helper, inline the operations sequentially. This makes the flow of operations easier to follow. For instance:
const configureTasks = await partitioningConfigureWithTask({ partitioning });
try {
await applyPartitioning({ partitioning });
const tasks = await new StorageClient().client.call(
partitioning,
INTERFACE_NAME_PARTITIONING,
"ValidateWithTask",
[]
);
const taskProxy = new StorageClient().client.proxy(
"org.fedoraproject.Anaconda.Task",
tasks[0]
);
const result = await taskProxy.GetResult();
onSuccess(result.v);
} catch (error) {
onFail(error);
}
In this refactoring, the sequence is linear without multiple layers of callbacks. This keeps all behavior intact while reducing indirection and simplifying error handling.
2781f69
to
7aa2603
Compare
7aa2603
to
56b46f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamkankovsky aside of the design with the many warnings that I leave on you and @garrett to resollve, I see a small issue here, with a specific warning..
The one that says, 'It is recommended to create a new filesystem ...'
This warning is not applicable when we exit the cockpit-storage view, as we just created a new filesystem possible.
I suggest we identify this warnings, and hide them. Unfortunately doing a string-match-pattern would not suffice in this case because backend warnings can be also translated.
I believe we need to somehow add this logic to the backend, to give information to the front-end about the warning category or something.
@adamkankovsky: Critical question that would inform design decisions: Are the recommendations actually recommendations... or requirements? That is, can you successfully ignore them and have a fully working system after installation, or will you hit problems during or after installation if you ignore them? Also: Is the answer the same for all of the "warnings" in the screenshot above, or are some different from the others? I have some ideas (with a few different paths) on how to address the UX/UI issues, but I need to know the answers to the above questions to proceed. Thanks! |
const onConfigureTaskSuccess = async () => { | ||
try { | ||
await applyPartitioning({ partitioning }); | ||
await partitioningValidate({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KKoukiou @adamkankovsky May I ask why we validate partitioning after applying it? In the Gtk backend we do it before: https://github.com/rhinstaller/anaconda/blob/1d9120e716aff354c7895ebde205f4ba6f9f347a/pyanaconda/ui/lib/storage.py#L307
Also we should handle also validation errors (not only warnings) before applying the partitioning?
followup for @KKoukiou branch work: https://github.com/KKoukiou/anaconda-webui/tree/validate-storage-show-warnings
