GNOME Bugzilla – Bug 670306
should do some kickstart sanity check before starting the installer
Last modified: 2016-03-31 14:02:29 UTC
I got the following error from the installer: line 2 of kickstart file: Kickstart command keyboard requires one argument Would be nice if the wizard could check that it has everything needed before starting the VM/installer.
(In reply to comment #0) > I got the following error from the installer: > > line 2 of kickstart file: > Kickstart command keyboard requires one argument > > > Would be nice if the wizard could check that it has everything needed before > starting the VM/installer. More like we should not put empty commands when some values are missing. The same happens when you don't specify a password i think. I'll have a look..
(In reply to comment #1) > More like we should not put empty commands when some values are missing. The > same happens when you don't specify a password i think. I'll have a look.. empty -> partial
Looking at the code: http://git.gnome.org/browse/gnome-boxes/tree/src/unattended-installer.vala#n87 , the only reason this will happen is that your environment is screwed? Also, I can cure the error message you get but you will still be asked to specify the keyboard layout by Anaconda so its not any big improvement i'd say. Some of the things (like password i mentioned) still need fixing though so I'll provide a patch for that at least.
Created attachment 207925 [details] [review] wizard: Ensure needed info is provided at 'setup' Since Fedora locks the user accounts (root and user) if password is not provided, we now require password for Fedora. This has the unfortunate side-affect of mandatory password for its SPICE connection as well. An alternative approach could be to have some default password but what would that be and how do we inform the user about it?
Created attachment 207926 [details] [review] wizard: Ensure needed info is provided at 'setup'
Created attachment 207927 [details] [review] express,fedora: Use a default password Since Fedora requires us to set a password for root and user accounts, we set a default password for these accounts in the guest if user doesn't specify one. The default password is 'password' and it is hoped that this is the simplest and most widely known default password. ;-)
Review of attachment 207927 [details] [review]: ::: src/unattended-installer.vala @@ +10,2 @@ private abstract class Boxes.UnattendedInstaller: InstallerMedia { + private const string DEFAULT_PASSWORD = "password"; Probably not a good idea to use a default password. If a password is needed, the user should be asked to give one instead, or confirm that the default "password" is ok @@ +36,2 @@ protected string disk_path; + protected bool password_needed; I might be missing something. where is password_needed used?
(In reply to comment #7) > Review of attachment 207927 [details] [review]: > > ::: src/unattended-installer.vala > @@ +10,2 @@ > private abstract class Boxes.UnattendedInstaller: InstallerMedia { > + private const string DEFAULT_PASSWORD = "password"; > > Probably not a good idea to use a default password. If a password is needed, > the user should be asked to give one instead, Thats what I went for first. As I mentioned on IRC the issue with that is we then enforce the password for (spice) display as well and we really need to let user be able to connect without a password if he/she wishes to. > or confirm that the default > "password" is ok That would require some UI design and we can add that later, perhaps for now we can just display a notification using Notificationbar? > @@ +36,2 @@ > protected string disk_path; > + protected bool password_needed; > > I might be missing something. where is password_needed used? Leftover from the other approach I was taking..
Created attachment 208267 [details] [review] express,fedora: Use a default password Since Fedora requires us to set a password for root and user accounts, we set a default password for these accounts in the guest if user doesn't specify one. The default password is 'password' and it is hoped that this is the simplest and most widely known default password. ;-)
Review of attachment 207926 [details] [review]: ::: src/wizard.vala @@ +68,2 @@ case WizardPage.REVIEW: + if (!review ()) for consistency, we should app.notificationbar.display_error() here with an exception, like we do for WizardPage.PREPARATION above.
Created attachment 209025 [details] [review] wizard: Ensure needed info is provided at 'setup' I'd prefer the other way around to void further complicating the property setter.
Review of attachment 209025 [details] [review]: ack
Comment on attachment 209025 [details] [review] wizard: Ensure needed info is provided at 'setup' Attachment 209025 [details] pushed as 6a3c5a5 - wizard: Ensure needed info is provided at 'setup'
Created attachment 209439 [details] [review] express,fedora: Use a default password This version shows the password to user at the 'Review' page if default password was chosen.
Created attachment 210214 [details] [review] express,fedora: Require password for Fedora Since Fedora requires us to set a password for root and user accounts, we now require user to provide a password. To not complicate users' life more than needed in this case, we free them from display (SPICE) authentication.
Review of attachment 210214 [details] [review]: looks good, didn't test it thought, sorry :( small comment: ::: src/unattended-installer.vala @@ +157,3 @@ + + if (password_mandatory && password == "") + // FIXME: Mark for translation after string freeze but this is going to be shown to the user? this is UI/string change still
(In reply to comment #16) > Review of attachment 210214 [details] [review]: > > looks good, didn't test it thought, sorry :( I did, with Fedora and XP. :) > small comment: > > ::: src/unattended-installer.vala > @@ +157,3 @@ > + > + if (password_mandatory && password == "") > + // FIXME: Mark for translation after string freeze > > but this is going to be shown to the user? this is UI/string change still Ah yes! (though not string but UI change). :(
(In reply to comment #17) > (In reply to comment #16) > > @@ +157,3 @@ > > + > > + if (password_mandatory && password == "") > > + // FIXME: Mark for translation after string freeze > > > > but this is going to be shown to the user? this is UI/string change still > > Ah yes! (though not string but UI change). :( On second thought, I don't think this is counted as a UI change as there is no new UI component, only one more error message shown through an existing UI. So if you don't object I'll go ahead and push it to 3.5.x branch?
If you add a new string to the UI that will be shown to the user, you are breaking string. Not marking it for translation to work around the string freeze is very ugly if the string is user-visible.
(In reply to comment #19) > you are breaking string. I meant "string freeze" obviously.
Created attachment 210646 [details] [review] express,fedora: Require password for Fedora This one makes the error message generic (as it should be) and marks it for translation. I'll obviously need permission before I merge this into 3.4 (master) branch but we can already merge this to 3.5.x.
Created attachment 210647 [details] [review] Mark a user-visible string for translation
Review of attachment 210646 [details] [review]: ack
Review of attachment 210647 [details] [review]: ack
Patches committed to 3.5.x branch.