network: Add sessionStorage event informing about NetworkManager Chec…#22830
Conversation
|
Anaconda would need to get informed also about the change confirmation dialog. |
rvykydal
force-pushed
the
add-session-storage-network-checkpoint-event
branch
from
9885d07 to
aa90f41
Compare
|
Adding tests |
pkg/networkmanager/interfaces.js
Outdated
| }) | ||
| .catch(function () { | ||
| // Clear checkpoint status on failure | ||
| window.sessionStorage.setItem("cockpit_has_checkpoint", "false"); |
There was a problem hiding this comment.
Added tests (integration)
jelly
left a comment
jelly
left a comment
There was a problem hiding this comment.
I am wondering if there isn't a NetworkManager API to subscribe to checkpoints? And wouldn't that be enough to satisfy Anaconda's needs?
jelly
left a comment
jelly
left a comment
There was a problem hiding this comment.
Alternatively we do have a page communication mechanism in pkg/lib/notifications.ts why won't we use that over sessionStorage.
pkg/networkmanager/interfaces.js
Outdated
| }) | ||
| .catch(function () { | ||
| // Clear checkpoint status on failure | ||
| window.sessionStorage.setItem("cockpit_has_checkpoint", "false"); |
There was a problem hiding this comment.
I would move this after the rollback or destroy below.
|
Looks good to me! Maybe we should generalize this to "cockpit:busy" which could be used for dialogs and checkpoints and whatever else might come up? |
The notifications work by sending messages to the Shell, which is not used with Anaconda (I think). Anaconda could implement the same protocol, but I think that's a distraction. "ad-hoc" session storage is good enough, I'd say. |
|
|
||
| testlib.wait(checkpoint_cleared) | ||
|
|
||
| def testCheckpointSessionStorageEventRollback(self): |
There was a problem hiding this comment.
This test needs to be excluded on Testing Farm, see test/browser/run-test.sh.
|
@rvykydal, do you want me to finish this? |
…kpoint Resolves: COCKPIT-1417
rvykydal
force-pushed
the
add-session-storage-network-checkpoint-event
branch
from
2ab8c24 to
377d6fd
Compare
I would wait with this a bit until we hit another instance of this, but yes, we may end up with it. I'd also need to look deeper whether we really need individual events or not. |
rvykydal
changed the title
At this stage of integration with Cockpit network module (POC/MVP before design review) I'd prefer to synchronize via Cockpit. In the current WebUI we are using NM API only via super minimal Anaconda backend network module (connectivity status) and don't use any NM API on our own yet. |
Related: COCKPIT-1417
mvollmer
force-pushed
the
add-session-storage-network-checkpoint-event
branch
from
377d6fd to
3f56d2a
Compare
|
I have force pushed a linting fix. |
| @@ -1701,6 +1709,9 @@ export function with_checkpoint(model, modify, options) { | |||
| manager.checkpoint_rollback(cp); | |||
| else | |||
| manager.checkpoint_destroy(cp); | |||
There was a problem hiding this comment.
This added line is not executed by any test. Details
5 participants
…kpoint
This is a PR with basic implementation of https://issues.redhat.com/browse/COCKPIT-1417 that should allow me to work on Anaconda part using the copr build.
Resolves: COCKPIT-1417