vm: Support for VM descriptions #1644

mvollmer · 2024-05-20T14:04:25Z

Based on work by @Britz, thanks a lot!

This is also a proving ground for robust and convenient virt-xml invocations. I plan to renovate the rest of domain.js and other files accordingly.

The description is a <p> so that it doesn't get too shouty when people put a lot of text there.

"Edit description" action:

Machines: Support for VM descriptions

Cockpit can now show and change the descriptions of virtual machines.

martinpitt · 2024-05-22T06:02:35Z

This looks a bit sad, retrying for comparison.

martinpitt

Thanks! Depending on how much time you can/want to throw at this, I can be convinced to give a +1. But I'm not super happy about this just yet.

src/components/vm/vmRenameDialog.jsx

src/libvirtApi/domain.js

test/check-machines-lifecycle

src/components/vm/vmRenameDialog.jsx

martinpitt · 2024-05-22T15:42:03Z

Yay for the new pixel test!

martinpitt · 2024-05-22T15:44:54Z

Very cool, thanks! Looks good, just needs the new pixel test mopped up.

martinpitt

Ah, I was about to mop up the pixel update, but you already did it -- I didn't see the force-push here. Force-pushed a rebase, and re-running the tests to check the flakes.

LGTM, assuming green. Thanks!

martinpitt · 2024-05-28T07:01:58Z

This still needs a proper release note, but otherwise looks good to land. Thanks!

garrett

I thought this was already discussed before and we specifically decided to not do it this way. (Note: it was different people working on cockpit-machines at that point in time.)

A VM is not a person with a first, middle, and last names. It does not have a "Full name".

If we want to get around the limitation for the name that serves as a type of identifier (ID) for the VM, then we should have "Name" be a display name and generate the machine's unique and limited (in characters) name based on the display name. I believe that's the intent for this, to work around an implementation detail where the name itself is restricted to various limits (mainly, which characters it can use).

It does not makes sense to have yet another field to do the exact same thing. We already have name for a way to identify the machine and description to explain what it does. Having both will be confusing and also take up extra space everywhere where there's a name, for no good reason.

So, instead of having the split here's how we might handle it instead:

Completely move the short ID name to the background, have it based on the display name, and always show the display name (treating the non-display name as an implementation detail). We could always set both the display name and ID name or only set the display if it doesn't fit within the ID's restrictions.

When displaying VMs, we'd do something like "display name || ID name" where it would default to showing the display name (if it exists) and falling back to the ID name if a display name is not shown. And we could even do something where we transform the display name to the ID name and if they match, only show display name, else show "Display name (ID name)". I think we're already doing this (if it already exists) without a way to set it, right?

martinpitt · 2024-05-28T08:58:25Z

Completely move the short ID name to the background

I'm not a fan of this TBH. The name which we currently display is the one you'd see in virsh list, use on the CLI, etc. I find the whole concept of adding yet another name a bit dubious TBH -- it's rather obscure, feels like "too much human-y fiddling" and redundant. libvirt calls this "title", which sounds even worse than "full name" to me at least.

I was screptical about this already, and @garrett 's comments pushed me over the line. How about we entirely drop this title/full name again, and only retain the optional description? That seems marginally more useful to me, and isn't so much "in your face" and redundant.

design still being discussed

mvollmer · 2024-05-28T11:28:29Z

only retain the optional description?

Makes sense to me.

mvollmer · 2024-05-28T11:28:59Z

@garrett's design also makes a lot of sense to me. :-)

mvollmer · 2024-05-28T12:00:39Z

only retain the optional description?

Makes sense to me.

But does it then make sense to use the Rename dialog to set the description?

martinpitt · 2024-05-29T05:49:00Z

Good point -- dang, looks like this is back to square one wrt. design then? 😢

mvollmer · 2024-06-20T10:05:21Z

We should probably read the description from that, and not from the "online", "running" XML.

Done. (It's called the "inactive" definition.)

mvollmer · 2024-07-16T10:10:54Z

This failure is a new flake... let's see.

mvollmer · 2024-07-16T11:06:13Z

This failure is a new flake... let's see.

One apparently can't ever use set_val with React components. Fixed by using set_input_text. It can in fact create multiple lines, but one has to use "\r" instead of "\n".

martinpitt · 2024-07-16T11:28:58Z

@vollmer: Please don't use \r and \n in set_input_text() or b.input_text() any more, this isn't portable across browsers. Use b.key("Enter") or similar.

mvollmer · 2024-07-16T11:34:24Z

@vollmer: Please don't use \r and \n in set_input_text() or b.input_text() any more, this isn't portable across browsers. Use b.key("Enter") or similar.

Hmm, can we make it so that set_input_text("\n") does the right thing? I'll make a PR.

mvollmer · 2024-07-16T12:15:30Z

@vollmer: Please don't use \r and \n in set_input_text() or b.input_text() any more, this isn't portable across browsers. Use b.key("Enter") or similar.

Hmm, can we make it so that set_input_text("\n") does the right thing? I'll make a PR.

The test now dioes this:

        b.set_input_text("#edit-description-dialog-description", desc1, value_check=False, blur=False)
        b.key("Enter")
        b.set_input_text("#edit-description-dialog-description", desc2, value_check=False, append=True)
        b.wait_val("#edit-description-dialog-description", desc1 + "\n" + desc2)

It would be really nice to replace that with

        b.set_input_text("#edit-description-dialog-description", desc1 + "\n" + desc2)

Not only because it is less to type, but also because this is the natural first thing to try, and it is not at all obvious what to do once you figure out that it doesn't work.

@Britz

Based on work by @Britz, thanks a lot!

mvollmer · 2024-09-05T11:40:58Z

Whoops, dropped the ball on this, sorry. Let's get this back on track.

based on the discussion above, this PR leaves "title" and "id" alone. It is only about the description now.

The description of a VM is shown on the details page, and there is an action in the menu to edit it. The description of this PR is up-to-date with the code.

packit-as-a-service · 2024-09-05T11:41:36Z

We were not able to find or create Copr project packit/cockpit-project-cockpit-machines-1644 specified in the config with the following error:

Packit received HTTP 500 Internal Server Error from Copr Service. Check the Copr status page: https://copr.fedorainfracloud.org/status/stats/, or ask for help in Fedora Build System matrix channel: https://matrix.to/#/#buildsys:fedoraproject.org.

Unless the HTTP status code above is >= 500, please check your configuration for:

typos in owner and project name (groups need to be prefixed with @)
whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
whether the project itself exists (Packit creates projects only in its own namespace)
whether Packit is allowed to build in your Copr project
whether your Copr project/group is not private

cockpituous · 2024-09-05T12:03:23Z

src/components/vm/vmEditDescriptionDialog.jsx

+        } catch (exc) {
+            dialogErrorSet({
+                dialogError: cockpit.format(_("Failed to set description of VM $0"), vm.name),
+                dialogErrorDetail: exc.message


These 4 added lines are not executed by any test.

cockpituous · 2024-09-05T12:03:23Z

src/components/vm/vmEditDescriptionDialog.jsx

+            <Form onSubmit={e => {
+                e.preventDefault();
+                onSubmit();


These 3 added lines are not executed by any test.

cockpituous · 2024-09-05T12:03:23Z

src/components/vm/vmEditDescriptionDialog.jsx

+                onSubmit();
+            }}
+            isHorizontal>
+                {!isObjectEmpty(error) && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />}


This added line is not executed by any test.

jelly

One small comment/question, otherwise looks good (!)

jelly · 2024-09-05T12:23:48Z

src/libvirtApi/domain.js

@@ -888,6 +888,33 @@ export function domainSendNMI({
    return call(connectionName, objPath, 'org.libvirt.Domain', 'InjectNMI', [0], { timeout, type: 'u' });
 }

+function shlex_quote(str) {
+    // yay, command line apis...
+    return "'" + str.replaceAll("'", "'\"'\"'") + "'";


What about https://docs.python.org/3/library/shlex.html#shlex.quote ?

Yeah, what about it? :-) This is supposed to be a JavaScript implementation of it. Is something missing?

Mistaken this for a Python file, don't ask :)

jelly · 2024-09-05T12:30:38Z

test/check-machines-lifecycle

+
+        # Non-ascii chars, unbalanced quotes, backslash, multiple lines
+        desc1 = '"Döscrü\\ptiän \'\'\' كرة القدم'
+        desc2 = 'Second <b>line</b>'


I hope this does not get rendered as such :)

It gets rendered as text, not as HTML.

Addressed by not showing/setting the title at all

jelly

Cool!

mvollmer mentioned this pull request May 20, 2024

adds VM (domain) title and description to UI #1001

Closed

1 task

mvollmer requested a review from garrett May 20, 2024 14:06

mvollmer force-pushed the vm-titles-and-description branch 7 times, most recently from a2a1286 to d9dc7b9 Compare May 22, 2024 05:29

mvollmer marked this pull request as ready for review May 22, 2024 05:29

mvollmer requested a review from martinpitt May 22, 2024 05:29

martinpitt requested changes May 22, 2024

View reviewed changes

src/components/vm/vmRenameDialog.jsx Outdated Show resolved Hide resolved

src/libvirtApi/domain.js Outdated Show resolved Hide resolved

test/check-machines-lifecycle Outdated Show resolved Hide resolved

src/components/vm/vmRenameDialog.jsx Outdated Show resolved Hide resolved

mvollmer force-pushed the vm-titles-and-description branch 3 times, most recently from 3d21e50 to feb47bf Compare May 22, 2024 14:10

mvollmer force-pushed the vm-titles-and-description branch from feb47bf to 99dee05 Compare May 23, 2024 07:28

martinpitt force-pushed the vm-titles-and-description branch from 99dee05 to 1310449 Compare May 28, 2024 02:25

martinpitt previously approved these changes May 28, 2024

View reviewed changes

martinpitt added the release-note label May 28, 2024

garrett previously requested changes May 28, 2024

View reviewed changes

mvollmer force-pushed the vm-titles-and-description branch from 4dfcf5c to 10072f9 Compare June 20, 2024 10:04

mvollmer marked this pull request as ready for review June 20, 2024 10:04

mvollmer force-pushed the vm-titles-and-description branch from 10072f9 to cb75374 Compare June 20, 2024 10:12

mvollmer force-pushed the vm-titles-and-description branch from cb75374 to beaf9ae Compare July 16, 2024 09:32

mvollmer force-pushed the vm-titles-and-description branch from beaf9ae to 10af9f1 Compare July 16, 2024 10:57

mvollmer force-pushed the vm-titles-and-description branch from 10af9f1 to dd4339b Compare July 16, 2024 11:22

mvollmer force-pushed the vm-titles-and-description branch from dd4339b to b74ad42 Compare July 16, 2024 12:06

mvollmer force-pushed the vm-titles-and-description branch from b74ad42 to 24d90a3 Compare July 19, 2024 12:35

vm: Support for VM descriptions

cd80563

Based on work by @Britz, thanks a lot!

mvollmer requested a review from garrett September 5, 2024 11:41

mvollmer force-pushed the vm-titles-and-description branch from 24d90a3 to cd80563 Compare September 5, 2024 11:41

mvollmer requested a review from jelly September 5, 2024 11:44

cockpituous reviewed Sep 5, 2024

View reviewed changes

jelly reviewed Sep 5, 2024

View reviewed changes

mvollmer requested review from jelly and cockpituous and removed request for cockpituous and garrett September 6, 2024 07:17

jelly approved these changes Sep 6, 2024

View reviewed changes

mvollmer merged commit afd6141 into cockpit-project:main Sep 6, 2024
27 checks passed

martinpitt removed the release-note label Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: Support for VM descriptions #1644

vm: Support for VM descriptions #1644

mvollmer commented May 20, 2024 •

edited by martinpitt

Loading

martinpitt commented May 22, 2024

martinpitt left a comment

martinpitt commented May 22, 2024

martinpitt commented May 22, 2024

martinpitt left a comment

martinpitt commented May 28, 2024

garrett left a comment

martinpitt commented May 28, 2024

mvollmer commented May 28, 2024

mvollmer commented May 28, 2024

mvollmer commented May 28, 2024

martinpitt commented May 29, 2024

mvollmer commented Jun 20, 2024

mvollmer commented Jul 16, 2024

mvollmer commented Jul 16, 2024

martinpitt commented Jul 16, 2024

mvollmer commented Jul 16, 2024

mvollmer commented Jul 16, 2024

mvollmer commented Sep 5, 2024

packit-as-a-service bot commented Sep 5, 2024

cockpituous Sep 5, 2024

cockpituous Sep 5, 2024

cockpituous Sep 5, 2024

jelly left a comment

jelly Sep 5, 2024

mvollmer Sep 6, 2024 •

edited

Loading

jelly Sep 6, 2024

jelly Sep 5, 2024

mvollmer Sep 6, 2024

jelly left a comment

vm: Support for VM descriptions #1644

vm: Support for VM descriptions #1644

Conversation

mvollmer commented May 20, 2024 • edited by martinpitt Loading

Machines: Support for VM descriptions

martinpitt commented May 22, 2024

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented May 22, 2024

martinpitt commented May 22, 2024

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented May 28, 2024

garrett left a comment

Choose a reason for hiding this comment

martinpitt commented May 28, 2024

mvollmer commented May 28, 2024

mvollmer commented May 28, 2024

mvollmer commented May 28, 2024

martinpitt commented May 29, 2024

mvollmer commented Jun 20, 2024

mvollmer commented Jul 16, 2024

mvollmer commented Jul 16, 2024

martinpitt commented Jul 16, 2024

mvollmer commented Jul 16, 2024

mvollmer commented Jul 16, 2024

mvollmer commented Sep 5, 2024

packit-as-a-service bot commented Sep 5, 2024

cockpituous Sep 5, 2024

Choose a reason for hiding this comment

cockpituous Sep 5, 2024

Choose a reason for hiding this comment

cockpituous Sep 5, 2024

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

jelly Sep 5, 2024

Choose a reason for hiding this comment

mvollmer Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

jelly Sep 6, 2024

Choose a reason for hiding this comment

jelly Sep 5, 2024

Choose a reason for hiding this comment

mvollmer Sep 6, 2024

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

mvollmer commented May 20, 2024 •

edited by martinpitt

Loading

mvollmer Sep 6, 2024 •

edited

Loading