Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create vm dialog refactor - add unattended installation option #12716

Open
wants to merge 2 commits into
base: master
from

Conversation

@KKoukiou
Copy link
Contributor

KKoukiou commented Sep 5, 2019

@KKoukiou KKoukiou added the no-test label Sep 5, 2019
@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 5, 2019

What's a "valid" password? I have no way of knowing what is considered a good versus bad password here.

Screenshot_2019-09-05 Virtual Machines - Rain

@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 5, 2019

It's still having problems downloading images...

Screenshot_2019-09-05 Virtual Machines - Rain(2)

@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 5, 2019

Unattended is enabled for things that don't have support for it, such as Rawhide. (Although... shouldn't Rawhide have support for it too?)

Screenshot_2019-09-05 Virtual Machines - Rain(3)

@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 5, 2019

Why aren't spaces supported in the name? Is that an arbitrary restriction?

If they aren't actually supported, then we should mention it and prevent creation until the space is removed. (Or automatically filter out spaces or convert them to dashes or underscores.)

Screenshot_2019-09-05 Virtual Machines - Rain(4)

@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 5, 2019

It's possible to attempt to create a VM without mandatory fields filled in... (The create button should be disabled until requirements are fulfilled.)
Screenshot_2019-09-05 Virtual Machines - Rain(5)

After clicking create, the button is disabled and errors are shown, however...
Screenshot_2019-09-05 Virtual Machines - Rain(6)

@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 5, 2019

  • Please change "Installation Source Type" to "Installation Type". It's too long and too similar to "Installation Source" otherwise. 😉
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch 2 times, most recently from d3436ba to bf55bc8 Sep 9, 2019
@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 9, 2019

Why can't I click the button? (Yes, I know it's because there's a space in the name. We should let there be a space in the name, if possible. If we cannot, then we should at least indicate why a VM cannot be created.)

Screenshot_2019-09-09 Virtual Machines - Rain(1)

Creating a CentOS image still doesn't work...

Screenshot_2019-09-09 Virtual Machines - Rain(2)

@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from bf55bc8 to 7d20106 Sep 9, 2019
@garrett

This comment has been minimized.

Copy link
Contributor

garrett commented Sep 9, 2019

Here's some odd password related error with Centos 7:

Screenshot_2019-09-09 Virtual Machines - Rain(3)

I think I didn't check the password box. Or perhaps I did and set a password? I don't remember for this one. 😁

@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch 5 times, most recently from 58d07cf to 5691a6f Sep 9, 2019
@KKoukiou KKoukiou removed the no-test label Sep 11, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch 2 times, most recently from 384f61d to 53b2db1 Sep 11, 2019
@KKoukiou

This comment has been minimized.

Copy link
Contributor Author

KKoukiou commented Sep 12, 2019

Here's some odd password related error with Centos 7:

Screenshot_2019-09-09 Virtual Machines - Rain(3)

I think I didn't check the password box. Or perhaps I did and set a password? I don't remember for this one. grin

That was a bug and was fixed upstream with https://gitlab.com/libosinfo/osinfo-db/merge_requests/33
Thanks for finding this!
Note: there is already BZ for backport it in rhel-8-1. https://bugzilla.redhat.com/show_bug.cgi?id=1750788

@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from 53b2db1 to 4858448 Sep 12, 2019
KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Sep 12, 2019
@KKoukiou KKoukiou changed the title WIP: Create vm dialog refactor - add unattended installation option Create vm dialog refactor - add unattended installation option Sep 12, 2019
KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Sep 12, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from 4858448 to a773ef6 Sep 12, 2019
KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Sep 12, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from a773ef6 to 919b5f1 Sep 12, 2019
@KKoukiou KKoukiou requested review from allisonkarlitskaya and removed request for allisonkarlitskaya Sep 13, 2019
@KKoukiou KKoukiou requested a review from skobyda Sep 27, 2019
@marusak marusak self-requested a review Sep 30, 2019
Copy link
Contributor

skobyda left a comment

Bunch of stuff. Feel free to ignore any comment which you do not find important.

test/verify/machineslib.py Outdated Show resolved Hide resolved
pkg/machines/components/memorySelectRow.jsx Outdated Show resolved Hide resolved
pkg/machines/components/create-vm-dialog/Password.jsx Outdated Show resolved Hide resolved
pkg/machines/components/create-vm-dialog/Password.jsx Outdated Show resolved Hide resolved
pkg/machines/components/create-vm-dialog/Password.jsx Outdated Show resolved Hide resolved
disabled={unattendedDisabled}
onChange={e => onValueChanged('unattendedInstallation', e.target.checked)} />
{_("Run unattended installation")}
<FieldLevelHelp content={_("Perform an unattended install using libosinfo's install script support")} close />

This comment has been minimized.

Copy link
@skobyda

skobyda Sep 30, 2019

Contributor

To be honest I found how you have to click on info icon to get the content quite confusing. Especially since we have OverlayTrigger which gets enabled by hovering over this whole row.
So when I hovered over the info icon, I got this helpblock. And I thought that's the content of the info icon and did not realize that the helpblock you can see in the screenshot comes from OverLayTrigger for the whole row. I realized I have to click on the icon only now when I'm reviewing it.
Screenshot from 2019-09-30 17-27-24

Perhaps we can use PF4 Tooltip which you do not have to click on, or do not use 2 helpblock which are kinda "overlaying each other" in the same field?
WDYT @garrett ?
Screenshot from 2019-09-30 17-35-52

This comment has been minimized.

Copy link
@garrett

garrett Sep 30, 2019

Contributor

Yeah, I agree that this is confusing.

Both of these popups are "smells" in my opinion. (As in "the leftovers in the fridge smell bad; they should be thrown out" kind of smell.)

What if we drop the tooltips and call the feature "Automatic installation" and only show it when it applies? That should let us drop both tooltips. Right?

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

Showing the option only when available means that it will be always hidden before a user selects an OS. Then when a users selects the OS we will just insert a new field in the modal.
Automatic Installation vs Unattended installation? I also don't see why the first s batter.
Unattended is commonly used as a term when the user doesn't need to do something interactively.

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

I removed the right info help icon.
I don't agree with the 'Automatic installation' label name though, since 'Unattended' is a general term for referring to actions that don't require user interaction.

</FormGroup>
<label htmlFor='user-password' className='control-label'>
{_("User Password")}
<FieldLevelHelp content={_("The username is your current host username")} close />

This comment has been minimized.

Copy link
@skobyda

skobyda Sep 30, 2019

Contributor

Does it make sense to have this infoblock which informs user why they cannot set username and what the username will be next to password field? If I would be user wondering why I can't set the username, or what the username will be, I wouldn't look for the answer in the password's helpblock :D. Perhaps let's not use tooltip, but instead put the information in a simple sentence above User Password row? @garrett WDTY?
Screenshot from 2019-09-30 17-43-13

This comment has been minimized.

Copy link
@garrett

garrett Sep 30, 2019

Contributor

Yeah, this part is tricky, especially since there are two password fields.

Usually, when there are two password fields, the second is a confirmation. That's not the case here... but I think a lot of people will treat it like that pattern. (Just like the checkbox on our log in page is not for remembering the password, but for having admin rights. That throws some people off too. 😒)

We could change it to something like:

"Password for $USER" where $USER is the username.

If they don't want to have their own user account on the system, do they just leave the password blank? If so, we could change the popup text to make it a bit more clear. It then could be something like, "Your current account will be set up on the virtual machine. Leave the password blank if you do not wish to have a user account created."

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

Not all distribution supports that though - for example debian will not allow you to now set a user password and only proceed with root.

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

Fixed.

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

I put the text bellow the password line, do you really prefer to make it a pop up?

pkg/machines/getOSList.py Show resolved Hide resolved
pkg/machines/getOSList.py Show resolved Hide resolved
pkg/machines/getOSList.py Outdated Show resolved Hide resolved
pkg/machines/components/create-vm-dialog/Password.css Outdated Show resolved Hide resolved
Copy link
Member

martinpitt left a comment

Thanks! Seems between @garrett and @skobyda the UI/workflow parts are already well covered in review. I looked at the non-UI aspects.

pkg/machines/getOSList.py Show resolved Hide resolved
test/verify/machineslib.py Outdated Show resolved Hide resolved
@@ -182,6 +183,14 @@ function validateParams(vmParams) {
}
}

if (vmParams.unattendedInstallation && !vmParams.rootPassword) {
validationFailed.rootPassword = _("Please insert a valid root password");

This comment has been minimized.

Copy link
@martinpitt

martinpitt Oct 1, 2019

Member

"insert" sounds a bit strange here IMHO -- maybe "set"?

Also, we shouldn't require both a root and a user password (sudo!), one ought to be enough? E. g. Ubuntu doesn't use root passwords at all by default in its installers and all released media -- you can set it of course, but it's highly discouraged.

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

You are right. I adjusted the code accordingly. Please re-review this part.

pkg/machines/scripts/create_machine.sh Show resolved Hide resolved
test/verify/machineslib.py Outdated Show resolved Hide resolved
@@ -1878,6 +1894,12 @@ def createAndVerifyVirtInstallArgs(self):
wait(lambda: self.machine.execute(virt_install_cmd), delay=3)
virt_install_cmd_out = self.machine.execute(virt_install_cmd)
self.assertTrue("--install os={}".format(self.os_short_id in virt_install_cmd_out))
if self.is_unattended:
self.assertTrue("profile={0}".format('desktop' if self.profile == 'Workstation' else 'jeos') in virt_install_cmd_out)

This comment has been minimized.

Copy link
@martinpitt

martinpitt Oct 1, 2019

Member

Please use assertIn(needle, haystack), that provides much more useful output on failures.

This comment has been minimized.

Copy link
@KKoukiou

KKoukiou Oct 1, 2019

Author Contributor

Fixed.

KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Oct 1, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from 49667d6 to 8fa05e8 Oct 1, 2019
@marusak

This comment has been minimized.

Copy link
Member

marusak commented Oct 1, 2019

It would be great if you could split this into a few PRs. It is huge and it will take a long time to land it as a whole. For example:

Separate PR:
machines: Disallow whitespaces in VMs names when creating them
(ideally with allowing whitespaces but making sure that disk name does not contain them)

Separete PR:
machines: Change "Installation Source Type" to "Installation Type" in…
machines: Move Name field to the top of Create New VM dialog

Separate PR:
Garrett's changes (at least I think they are in general valid, if they are connected to a specific commit then please squash into it)

@KKoukiou KKoukiou added the no-test label Oct 1, 2019
KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Oct 1, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from 8fa05e8 to 89db23a Oct 1, 2019
KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Oct 1, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from 89db23a to 720d539 Oct 1, 2019
@KKoukiou KKoukiou removed the no-test label Oct 1, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from 720d539 to b40e2ca Oct 1, 2019
KKoukiou added a commit to KKoukiou/cockpit that referenced this pull request Oct 2, 2019
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from b40e2ca to df6cfb5 Oct 2, 2019
@KKoukiou KKoukiou added the blocked label Oct 10, 2019
@martinpitt martinpitt added needs-rebase and removed blocked labels Oct 10, 2019
@martinpitt

This comment has been minimized.

Copy link
Member

martinpitt commented Oct 10, 2019

#12917 landed, so unblocking and needs-rebase instead.

fidencio and others added 2 commits Sep 2, 2019
This, again, is a *hint* about whether the OS in question supports
unattended installations or not. Mind, though that the medias should be
specifically checked for whether they support it or not as some distros
are providing medias not capable to be unattended installed.

The reason medias are treated differently is that, for some distros,
some of the medias are not capable to deal with unattended
installations. Fedora is one of the main examples here, as its "live"
medias do not have anaconda running during the boot time, which means
there's nothing to read & interpret the kickstart file passed to the
kernel command line.

The logic to check whether a media supports or not unattended
installation is:
- Check whether the OsinfoOs supports unattended installation;
  - In case OsinfoOs does *not* support, an OsinfoMedia won't support
    either;
  - In case OsinfoOs supports unattended installation:
    - Check whether the OsinfoMedia supports the unattended
      installation;
      - In case the media has the "installer-script" set to false, the
        media does *not* support unattended installation;
      - Otherwise, it does;
        - In case it does, check which are the profiles supported;
    - In case there's no info about unattended scripts for a specific
      media, the ones supported by the OsinfoOs should be used.

This whole logic is a little bit tricky but, unfortunately, that's how
libosinfo evolved when having to deal with distros changes with
regarding to live / installable medias.
installation

Closes #12716
@KKoukiou KKoukiou force-pushed the KKoukiou:create-vm-dialog-refactor branch from df6cfb5 to bf8ee14 Oct 14, 2019
@KKoukiou KKoukiou removed the needs-rebase label Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.