-
Notifications
You must be signed in to change notification settings - Fork 68
feat: allow software conflicts resolution #2348
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
Conversation
Replaces "Solve it" with "Review and fix" to better reflect the action required from users. The previous wording suggested an automatic fix, while in reality users must choose one of several proposed solutions. Also updates the link's appearance in the warning message by removing the icon and using the `isInline` prop instead. This aligns it with other links in the storage interface and ensures the design system underlines it for better visibility.
{model::Conflict} was not reintroduced when solving a conflict in merge
commit f22ae09
|
@jreidinger I think UI internals are now in a better shape. A few unit tests were added, as well as some aesthetic changes. Please, feel free to review these changes and make as much suggestions as needed. Thanks! |
|
@jreidinger @imobachgs Sorry guys, but help needed with Rust part here. I think I messed it up while merging master, although the needed change looked pretty straightforward to me. See f22ae09 and 3d039d9 |
That was leaked in the conflicts mutation hook becuase copy&paste
Missing in commit dfa0628
lslezak
left a comment
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.
Minor issues, but probably sending the complete data in the event is not a good idea.
| return client.onEvent((event) => { | ||
| if (event.type === "ConflictsChanged") { | ||
| const { conflicts } = event; | ||
| queryClient.setQueryData([conflictsQuery().queryKey], conflicts); |
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.
I'm not sure if it is a good idea to send the complete list of the conflicts as a part of the event via the web socket. There might be dozens of conflicts and sending them to all clients sounds too much to me. Not all clients might be interested in the details, like the agama monitor client.
I'd prefer no details in the event and let the client to fetch the data again if it needs them. With Tanstack query it is enough to invalidate the cache, it will re-fetch automatically.
Something like:
queryClient.invalidateQueries({ queryKey: [conflictsQuery().queryKey] })See https://tanstack.com/query/v5/docs/framework/react/guides/query-invalidation
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.
Make sense. I didn't know why we were doing an optimistic update here and thought that there were a reason. But if no body opposes I'll go for the invalidation as you suggest.
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.
I guess that apart of changing the frontend for just invalidating the query some changes are needed in the backend for not broadcasting all the conflicts
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.
Agama example:
agama/web/src/queries/status.ts
Lines 82 to 88 in 9116f0e
| if (type === "IssuesChanged") { | |
| queryClient.invalidateQueries({ queryKey: ["status"] }); | |
| } | |
| if (event.type === "ProductChanged") { | |
| queryClient.invalidateQueries({ queryKey: selectedProductQuery().queryKey }); | |
| } |
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.
Good point. However, how big (in size) the list of conflicts can be? I would consider changing it only if it is a real problem. In general, I dislike those events where you need to do another fetch to get the real data, although I understand that in this case it could make sense.
However, I think that a proper solution would be to allow the subscribers to choose the "topics" they care about, so they only receive a lot of useless information. But we are not there yet.
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.
Yes, the backend needs an update as well.
Co-authored-by: Ladislav Slezák <[email protected]>
As suggested in code review Co-authored-by: Ladislav Slezák <[email protected]>
imobachgs
left a comment
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.
I have reviewed the Rust part.
| return client.onEvent((event) => { | ||
| if (event.type === "ConflictsChanged") { | ||
| const { conflicts } = event; | ||
| queryClient.setQueryData([conflictsQuery().queryKey], conflicts); |
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.
Good point. However, how big (in size) the list of conflicts can be? I would consider changing it only if it is a real problem. In general, I dislike those events where you need to do another fetch to get the real data, although I understand that in this case it could make sense.
However, I think that a proper solution would be to allow the subscribers to choose the "topics" they care about, so they only receive a lot of useless information. But we are not there yet.
Co-authored-by: Imobach González Sosa <[email protected]>
Prepare to release Agama 15: * #2258 * #2270 * #2277 * #2279 * #2283 * #2284 * #2285 * #2286 * #2287 * #2288 * #2291 * #2292 * #2293 * #2295 * #2297 * #2299 * #2300 * #2301 * #2302 * #2303 * #2305 * #2306 * #2307 * #2308 * #2309 * #2313 * #2314 * #2315 * #2317 * #2318 * #2319 * #2320 * #2321 * #2322 * #2323 * #2324 * #2325 * #2328 * #2329 * #2330 * #2331 * #2335 * #2336 * #2337 * #2338 * #2339 * #2340 * #2342 * #2345 * #2346 * #2348 * #2349 * #2350 * #2351 * #2352 * #2353 * #2354 * #2355 * #2357 * #2358 * #2359 * #2360 * #2361 * #2362 * #2363 * #2364 * #2365 * #2366 * #2368 * #2369 * #2370 * #2371 * #2372 * #2374 * #2377 * #2378 * #2379 * #2380 * #2381 * #2382 * #2384 * #2385 * #2386 * #2388 * #2389 * #2390 * #2391 * #2392 * #2394 * #2397 * #2398 * #2401 * #2403
https://build.opensuse.org/request/show/1280489 by user IGonzalezSosa + anag_factory - Version 15 - Proper handling of WebSocket secure connections (gh#agama-project/agama#2391): - "agama monitor" does not use "insecure" by default. - Do not encrypt the connection when using ws: URLs. - Cache progress reporting to avoid blocking the clients (gh#agama-project/agama#2389). - Update schema of the storage model (gh#agama-project/agama#2346). - Provide software conflicts HTTP API (gh#agama-project/agama#2348) - Cache issues to avoid blocking the clients (gh#agama-project/agama#2379). - Cache the software configuration and products in the web server, the software backend is blocked during package installation (bsc#1241208) - Add support for bridge connections (gh#agama-project/agama#2258). - Do not crash when network events do not contain "addresses", "nameserver
https://build.opensuse.org/request/show/1280475 by user IGonzalezSosa + anag_factory - Version 15 - Handle reused MD RAIDs that are already included at the storage configuration (gh/agama-project/agama#2346). - Allow to resolve in web UI software conflicts (gh#agama-project/agama#2348) - Do not crash when navigating to a Wi-Fi network (bsc#1243415) - Keep margin between sidebar and main content in all breakpoints (gh/agama-project/agama#2370). - Rework the installer l10n settings (gh#agama-project/agama#2359): - Improve discoverability of language and keyboard layout settings. - Add contextual messages to help users differentiate between installer and product localization settings. - Add the ability to reuse installer settings for the product to install. - In local connections, keyboard layout change is now available directly from modal dialog
Problem
There is missing conflict resolution in Agama web UI.
Solution
Implement it with having it in backend, dbus, http API and also in web UI.
Testing
Screenshots
Click to show/hide a few screenshots
Bear in mind that these screenshots might be outdated because changes requested by reviewers after uploading them. They will be replaced only if these changes are highly noticeable
Warning with link at Software page
Software conflicts resolution page when there is only one conflict
Software conflicts resolution page when there are multiple conflicts
Software conflicts resolution page when there are no conflicts