cockpit-machines
cockpit-machines copied to clipboard
cockpit-project
adds VM (domain) title and description to UI
The libvirt domain XML has the title and description element which is not represented in the UI. This commit adds, if available, the Title to the VM List in the form "<TITLE> (<NAME>)" and adds Title and description to the VM detail page including the option to modify it.
- [ ] Blocked on new design mockups for the overview from @garrett
Hey @Britz . Thanks a lot for this PR.
Before we dive deeper into code review, do you mind explaining why did you decide to implement this specific feature? I'm asking, since I have not seen an issue or request for it before, so I don't really know what use case this solves, and what kind of additional information would a user input into the title/description which cannot be part of the VM's name.
In cockpit-machines, we are quite careful about adding new features and expanding the UI, because we don't want the UI to become overcrowded with a lot of information, inputs, modals, etc.. So I'm just trying to find out what kind of use case this functionality solves, and how many users would use it..., so we can find the best place for this functionality in our UI :)
Did you decide to implement it based on some issue or some request from somebody? Or did you implement it because you personally set title/description for your own VMs? Can you then perhaps explain and give is examples what kind of information do you input into it?
Lastly, virt-manager also supports setting title/description. But a lot of virt-manager users use it for quasi-categories. Since virt-manager doesn't support VM categories, users input the category into the title/description, so they can categorize it that way. That's not ideal from virt-manager, and if this is the aim here too, I would like to avoid going exactly the same way. (btw we have a request for adding categories too)
Btw, here are the screenshots I took when I built this PR locally, so other people who just wanna talk about the design don't have to build it:
(seems like this PR breaks our grid in overview tab, that needs to be fixed)
ng)
@KKoukiou: According to the test driven development, It would be great if you could this, I will try to help then. I've see that there are tests but no documentation on what and how to test and how to setup tests, so I just skipped it for now. I had not time to reverse engineer that by myself.
@skobyda: Thanks for adding the screenshots. The rename dialog actually did not change, I will add additional screenshots to pinpoint the changes.
Your question about why is quite simple. I simply implemented it, be cause we need that feature to better describe the purpose of the VM. Of cause you can code a lot of information into naming patterns, but i don't like extensively long domain names, especially when you need to type them regularly on shell, e.g., using virsh. We started with naming patterns, but since the setup gained a lot of complexity, this was just not feasible anymore and we switch to a way cleaner naming using the hostnames and domain names.
To our setup/project: We are currently developing a small-scale on premises trusted research environment to do research on medical data in Germany (so strict laws like GDPR apply) and we use plain libvirt on our servers with an increasing amount of machines. We also tried oVirt, proxmox and even OpenStack and they do not fit our current requirements, so we need to use pure libvirt for now. Cockpit machine is a nice way to have a quick overview over the machines but it is far away from being a productive replacement of the underlaying console tools, thus we need both. If you are interested in our infrastructure, you can have a look at https://semla.dfki.de.
I committed my changes and tried to fix all already mentioned issues, except the missing tests. I also did not really understand how to edit the PO files, since they seem to be generated.
Since I am a german, I only added the German translations of the additional text snippets. Should I add these the other languages without translation, such that other can easily spot and contribute to missing translations? How is the process there?
@KKoukiou, according to the tests, I also need some help to understand the output. For me it is unclear how to work with the failed tests in the pull request. Looking at the logs, which are hard to read, it seems that the errors has nothing to do with my changes.
Here are some screenshot of the changes including explanations.
The VM List
If the title is set, it is shown instead of the name and the vm-name is shown in parenthesis (see intra and ipa-intra entries). Since VM names cannot contain whitespaces and there is one between the title and name, this is unambiguous.
VM details Page
Title, name and description are set and shown both in the headline and in the overview.
The elements in the overview are for clarification and to be able to edit the values, since the VM is running, the edit buttons are not active. An alternative would be to add "edit Title" and "edit Description" entries to the VM actions menu.
If the VM is shut down, the edit buttons are active and can be clicked.
The change title dialog: For some unkown reasons, the specified translations do not work here. Empty values are allowed to delete the title, perhaps it is better to add a dedicated delete button in addition. In a first version I just put additional fields to the rename dialog, but since renaming also fails if there are no changes on the name, that did not worked out as expected
The change description dialog: Basically the same as the title dialog, empty values are also allowed.
If the title is not set, the name element is not shown in the overwiew page, since it is indistinguishably shown in the header and it can be edited via the VM actions menu. Empty title and empty description entries are shown, since otherwise there would be no possibility to set them.
@Britz I had a discussion yesterday with our designer (@garrett) and he believes adding these few extra options in the overview makes the UI rather cramped. He is working on a new design which will fit this in nicely, and then after I can pick up your commits to incorporate in the new design, add tests etc.
Thanks for the contributions and sorry for the waiting time on this. I hope this is not super urgent.
@KKoukiou That's great to here. It's in deed not the best design and really pragmatic. My first approach was to only put it in the header and add three field inside the rename dialogue, but that actually also fails, if the name of the vm does not change and thus it would always create errors if only the title or the description would be changed, which is not nice. Since it is easy to install from Git, we can also use the version in my branch until the feature is there.
When you have final idea of how to incorporate this information, perhaps I can help to implement. I also has other required changes on my todo list, but I had not yet time to implement or to check if there are already issues. This includes, e.g., the name of the created vnet interface on the host in the network interface overview table, which is a very important information.
My first approach was to only put it in the header and add three field inside the rename dialogue,
I think that's a reasonable approach since it leads to fewer changes to the page. I think it's also discoverable: if people care about the names of their machines and want to improve them, they will see the additional options like title and description, and might change those instead of the id.
I wouldn't use "Title" in the UI, but rather something like "Display name" or "Long name". Or "Full name" like we do with accounts.
but that actually also fails, if the name of the vm does not change and thus it would always create errors if only the title or the description would be changed, which is not nice.
I am pretty sure we can make this work somehow.
I have done my own version of this (mostly to get more familiar with the code in general): #1644. Let's continue there, ok?