search

cockpit-project / cockpit-machines

Cockpit UI for virtual machines
GNU Lesser General Public License v2.1
289 stars 74 forks source link

vm: Support for VM descriptions #1644

Closed mvollmer closed 1 month ago

mvollmer commented 4 months ago

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:

image

Machines: Support for VM descriptions

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

screenshot-page

screenshot

martinpitt commented 4 months ago

This looks a bit sad, retrying for comparison.

martinpitt commented 4 months ago

Yay for the new pixel test!

martinpitt commented 4 months ago

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

martinpitt commented 4 months ago

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

martinpitt commented 4 months ago

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.

mvollmer commented 4 months ago

only retain the optional description?

Makes sense to me.

mvollmer commented 4 months ago

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

mvollmer commented 4 months ago

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 commented 4 months ago

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

martinpitt commented 4 months ago

That said, while "Rename" is not very discoverable, it saves the need for yet another kebab menu entry and dialog. Perhaps changing it to "Rename/Describe"?

garrett commented 4 months ago

That said, while "Rename" is not very discoverable, it saves the need for yet another kebab menu entry and dialog. Perhaps changing it to "Rename/Describe"?

To have both in one string, we'd probably want something like "Edit name and description" instead.

garrett commented 4 months ago

To handle descriptions in the table, we can either have a column that shows up when there is a description in any VM (and not there if there is none), like this:

Board

Or we can make it optional but in the name field, as some subtext, like this:

Table _ Table (grid)

As horizontal space is usually a premium and it doesn't make sense to be able to sort on the description (the table should be sortable, but isn't — however, this is a different topic), it probably doesn't make sense to have it in its own column. Adding it below is probably the best option here.

What do you think?

martinpitt commented 4 months ago

Agreed about not showing it in a separate column -- the PR also doesn't do that. It's currently shown in the VM details page (see screenshot in description). TBH I'd leave it out from the overview list -- it can be arbitrarily long (it's multi-line), and is a fringe case. I could imagine an (i) icon with a popup in the description though, that doesn't break the layout.

garrett commented 4 months ago

Sure, keeping it the description in the details page only can make sense... but then we shouldn't have an action to change it on the list. That's an either/or thing. We already have a way to edit it on the details page — where you can see it.

So I guess that means there's nothing left of this PR we want to go in, and just want to keep the status quo? (Unless I'm missing something.)

mvollmer commented 3 months ago

virt-xml --metadata --update doesn't work and thus we can't update the description of a running VM. But that seems unmotivated, it's just some text without connection to anything involved in actually running the VM.

But virt-xml can change the "offline", "stale", "persistent", whateverthisiscalled XML definition. We should probably read the description from that, and not from the "online", "running" XML.

mvollmer commented 3 months ago

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

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

mvollmer commented 2 months ago

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

mvollmer commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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.

mvollmer commented 1 month ago

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[bot] commented 1 month ago

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:

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