Hide unimportant details from service details and show some important ones instead #15701

KKoukiou · 2021-04-15T16:54:20Z

So I was thinking how I can solve #14321
Went to service details and I realized that so much information there are not useful for most scenarios, or at least they were never useful for me till now.
So I though I should hide them by default, and display indead some more useful information, like the port a socket activated service is exposed at, for example.

Having the above mentioned property exposed we can move forward in the future with linking the service details of service sockets to the firewall page.

What do you think? @marusak ?

Services: Show memory usage and socket listeners

Socket units show the paths or ports on which they listen, and which service unit they activate:

Service units show their current memory usage and which socket unit activated them (if any). The static relationships like "Requires:" are now collapsed by default, to take less space and make it easier to see the journal output below.

garrett · 2021-04-15T18:29:05Z

Ooooo, I ❤️ this! You've got my approval for this PR when it's time for it.

Great work!

pkg/systemd/services/service-details.jsx

marusak · 2021-04-16T06:16:48Z

I like it a lot! Especially since this is the whole screen on my laptop screen:

And this is with this PR:

The space is used just soo much better. Also having Listen is very useful.

It shows some random 0 though, can you please take a look at that?

Unrelated but noticed that there is not bottom padding, I cannot scroll any further than:

It is something we should take a look at.

martinpitt

So cool, thanks @KKoukiou !

pkg/systemd/services/services.jsx

martinpitt

This works and looks really well 👏 Many thanks for this!

pkg/systemd/services/service-details.jsx

test/verify/check-system-services

martinpitt · 2021-04-20T17:26:37Z

/livetest

martinpitt

Thanks! There are still quite a number of issues from the previous review round, I marked the others as resolved. Many of the middle ones are unhelpfully hidden by github, you have to expand them 😿

pkg/systemd/services/service-details.jsx

KKoukiou · 2021-04-22T06:33:39Z

Still some test failures, marking as draft till these are resolved.

martinpitt · 2021-04-23T07:49:28Z

test-static-code fails:

test/verify/check-system-services:22: 'time' imported but unused
test/verify/check-system-services:810: redefinition of unused 'time' from line 22

KKoukiou · 2021-04-23T07:51:56Z

test-static-code fails:

test/verify/check-system-services:22: 'time' imported but unused
test/verify/check-system-services:810: redefinition of unused 'time' from line 22

Ups sorry, reposted.

martinpitt

This works really nicely now 👍 Also thanks for the nice dynamic memory test case!

A few remaining easy nitpicks.

pkg/systemd/services/services.jsx

pkg/systemd/services/service-details.jsx

test/verify/check-system-services

…for sockets MemoryCurrent does not emit an event when updated therefore just do polling.

Most of them are not of high interest and we display them as first information in the service details page. Let's add them behind an expandable section and let the user decide if they want to see them. Show the triggeredBy/triggers by default however since there are actually important for socket and timer activated services. systemctl status has the same approach.

We do have a lot of space in desktop mode which is unused, in smaller screens they collapse to vertical anyway.

Previously there was just too much information there, so we tried to categorize it. Now it's hidden by default so let's just divide the items.

Previously in console there was: Switch: Switch requires either a label or an aria-label to be specified

Also it's not used.

This custom CSS was removing the page's bottom padding, resulting in inability to scroll to the end.

In service details we used to block component refresh when we performed a unit lifecycle operation. This is still needed, but it can be done in a cleaner way with getDerivedStateFromProps.

…a list

martinpitt

Thanks! 👍 assuming the tests agree.

martinpitt · 2021-04-23T09:28:18Z

pkg/systemd/services/service-details.jsx

+                        "MemoryCurrent",
+                        result[0] && result[0].v > 0 ? result[0].v : null,
+                    );
+                }, ex => console.log(ex.message));


FWIW, the previous "Failed to.." string was fine, I meant to add ex.toString() as a second argument. But good enough.

martinpitt · 2021-04-23T13:17:21Z

@marusak: rhel-9-0 failed in an interesting way:

+ rpm -U --force build-results/cockpit-debuginfo-242.63.g64ad0d211-1.wip.el9.x86_64.rpm build-results/cockpit-system-242.63.g64ad0d211-1.wip.el9.noarch.rpm build-results/cockpit-debugsource-242.63.g64ad0d211-1.wip.el9.x86_64.rpm build-results/cockpit-ws-242.63.g64ad0d211-1.wip.el9.x86_64.rpm build-results/cockpit-tests-242.63.g64ad0d211-1.wip.el9.x86_64.rpm build-results/cockpit-bridge-242.63.g64ad0d211-1.wip.el9.x86_64.rpm build-results/cockpit-packagekit-242.63.g64ad0d211-1.wip.el9.noarch.rpm build-results/cockpit-storaged-242.63.g64ad0d211-1.wip.el9.noarch.rpm build-results/cockpit-pcp-242.63.g64ad0d211-1.wip.el9.x86_64.rpm build-results/cockpit-242.63.g64ad0d211-1.wip.el9.x86_64.rpm
error: Failed dependencies:
	(selinux-policy >= 34.1.2 if selinux-policy-targeted) is needed by cockpit-ws-242.63.g64ad0d211-1.wip.el9.x86_64

This is some weird collision with your PR? that just landed

martinpitt · 2021-04-23T13:20:36Z

Or perhaps some interaction between #15707 and the image refresh in cockpit-project/bots#1955 ? They landed in parallel

martinpitt · 2021-04-28T04:58:57Z

@garrett: I made new screenshots (the original one was way too wide) and added a release note. Can you please have a look?

garrett · 2021-04-28T07:24:15Z

@martinpitt Cool!

I do remember a screenshot showing a port; these don't though?

Also, since there's just white at the top, we might want to frame these in a dark grey outline.

garrett · 2021-04-28T07:31:38Z

Here's a cropped screenshot from the above thread that shows a port:

martinpitt · 2021-04-28T07:33:47Z

@garrett Right, I picked an Unix socket instead of a TCP one, but a TCP socket is fine as well. I just didn't want to use Cockpit's own service, as that felt a little too self-referential. But fine for me, of course :) Do you want to do the corresponding service one as well, or want me to re-do mine?

KKoukiou added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Apr 15, 2021

KKoukiou changed the title ~~WIP: Hide unimportant details from service details~~ WIP: Hide unimportant details from service details and show some important ones instead Apr 15, 2021

marusak reviewed Apr 16, 2021

View reviewed changes

pkg/systemd/services/service-details.jsx Outdated Show resolved Hide resolved

marusak reviewed Apr 16, 2021

View reviewed changes

pkg/systemd/services/service-details.jsx Show resolved Hide resolved

martinpitt requested changes Apr 16, 2021

View reviewed changes

pkg/systemd/services/services.jsx Outdated Show resolved Hide resolved

martinpitt added the release-note label Apr 16, 2021

KKoukiou changed the title ~~WIP: Hide unimportant details from service details and show some important ones instead~~ Hide unimportant details from service details and show some important ones instead Apr 16, 2021

KKoukiou force-pushed the service-details-note-important-details branch 2 times, most recently from 6ca9065 to daee0f0 Compare April 16, 2021 14:33

KKoukiou removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Apr 16, 2021

KKoukiou force-pushed the service-details-note-important-details branch from daee0f0 to a1ee5e1 Compare April 16, 2021 14:57

KKoukiou marked this pull request as ready for review April 17, 2021 10:05

KKoukiou requested a review from martinpitt April 17, 2021 10:05

martinpitt requested changes Apr 18, 2021

View reviewed changes

KKoukiou force-pushed the service-details-note-important-details branch 4 times, most recently from 313d0e6 to 9c15042 Compare April 20, 2021 07:19

KKoukiou requested a review from martinpitt April 20, 2021 08:10

martinpitt requested changes Apr 20, 2021

View reviewed changes

pkg/systemd/services/service-details.jsx Outdated Show resolved Hide resolved

pkg/systemd/services/service-details.jsx Outdated Show resolved Hide resolved

pkg/systemd/services/service-details.jsx Outdated Show resolved Hide resolved

KKoukiou force-pushed the service-details-note-important-details branch 2 times, most recently from 61ab94b to 7a300a1 Compare April 21, 2021 14:21

KKoukiou marked this pull request as draft April 22, 2021 06:33

KKoukiou force-pushed the service-details-note-important-details branch 3 times, most recently from ea1ed4b to b654695 Compare April 22, 2021 16:04

KKoukiou marked this pull request as ready for review April 23, 2021 06:33

KKoukiou requested a review from martinpitt April 23, 2021 06:33

KKoukiou force-pushed the service-details-note-important-details branch from b654695 to bdb08a5 Compare April 23, 2021 07:51

martinpitt requested changes Apr 23, 2021

View reviewed changes

KKoukiou added 11 commits April 23, 2021 10:24

services: Display current memory for all units and listen properties …

627928b

…for sockets MemoryCurrent does not emit an event when updated therefore just do polling.

services: keys can have spaces - stop doing the split join dance

ba7895e

services: Make DescriptionLists in service details horizontal

a47578b

We do have a lot of space in desktop mode which is unused, in smaller screens they collapse to vertical anyway.

services: Remove unused css

45da1a1

services: remove horizontal rules between service details

7fbc2d9

Previously there was just too much information there, so we tried to categorize it. Now it's hidden by default so let's just divide the items.

service: add aria-label to the Switch in details to avoid Pf4 warnings

072c276

Previously in console there was: Switch: Switch requires either a label or an aria-label to be specified

services: remove divider attribute since it's not standard

25091b7

Also it's not used.

services: let page padding follow the PF4 standard

f220d7f

This custom CSS was removing the page's bottom padding, resulting in inability to scroll to the end.

services: replace shouldComponentUpdate with getDerivedStateFromProps

233e31f

In service details we used to block component refresh when we performed a unit lifecycle operation. This is still needed, but it can be done in a cleaner way with getDerivedStateFromProps.

services: replace custom disabled class in service relationships comm…

ceca3b9

…a list

KKoukiou force-pushed the service-details-note-important-details branch from bdb08a5 to ceca3b9 Compare April 23, 2021 08:43

martinpitt approved these changes Apr 23, 2021

View reviewed changes

martinpitt merged commit 62254b0 into cockpit-project:master Apr 26, 2021

KKoukiou deleted the service-details-note-important-details branch April 26, 2021 05:39

martinpitt removed the release-note label Apr 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide unimportant details from service details and show some important ones instead #15701

Hide unimportant details from service details and show some important ones instead #15701

KKoukiou commented Apr 15, 2021 •

edited by martinpitt

Loading

garrett commented Apr 15, 2021

marusak commented Apr 16, 2021

martinpitt left a comment

martinpitt left a comment

martinpitt commented Apr 20, 2021

martinpitt left a comment

KKoukiou commented Apr 22, 2021

martinpitt commented Apr 23, 2021

KKoukiou commented Apr 23, 2021

martinpitt left a comment

martinpitt left a comment

martinpitt Apr 23, 2021

martinpitt commented Apr 23, 2021

martinpitt commented Apr 23, 2021

martinpitt commented Apr 28, 2021

garrett commented Apr 28, 2021 •

edited

Loading

garrett commented Apr 28, 2021

martinpitt commented Apr 28, 2021

Hide unimportant details from service details and show some important ones instead #15701

Hide unimportant details from service details and show some important ones instead #15701

Conversation

KKoukiou commented Apr 15, 2021 • edited by martinpitt Loading

Services: Show memory usage and socket listeners

garrett commented Apr 15, 2021

marusak commented Apr 16, 2021

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented Apr 20, 2021

martinpitt left a comment

Choose a reason for hiding this comment

KKoukiou commented Apr 22, 2021

martinpitt commented Apr 23, 2021

KKoukiou commented Apr 23, 2021

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Apr 23, 2021

Choose a reason for hiding this comment

martinpitt commented Apr 23, 2021

martinpitt commented Apr 23, 2021

martinpitt commented Apr 28, 2021

garrett commented Apr 28, 2021 • edited Loading

garrett commented Apr 28, 2021

martinpitt commented Apr 28, 2021

KKoukiou commented Apr 15, 2021 •

edited by martinpitt

Loading

garrett commented Apr 28, 2021 •

edited

Loading