After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 670306 - should do some kickstart sanity check before starting the installer
should do some kickstart sanity check before starting the installer
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: installer
unspecified
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-02-17 17:29 UTC by Marc-Andre Lureau
Modified: 2016-03-31 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wizard: Ensure needed info is provided at 'setup' (6.50 KB, patch)
2012-02-18 14:39 UTC, Zeeshan Ali
none Details | Review
wizard: Ensure needed info is provided at 'setup' (2.89 KB, patch)
2012-02-18 15:06 UTC, Zeeshan Ali
needs-work Details | Review
express,fedora: Use a default password (2.51 KB, patch)
2012-02-18 15:07 UTC, Zeeshan Ali
reviewed Details | Review
express,fedora: Use a default password (1.77 KB, patch)
2012-02-23 13:52 UTC, Zeeshan Ali
none Details | Review
wizard: Ensure needed info is provided at 'setup' (4.25 KB, patch)
2012-03-05 20:08 UTC, Zeeshan Ali
committed Details | Review
express,fedora: Use a default password (3.32 KB, patch)
2012-03-11 17:55 UTC, Zeeshan Ali
none Details | Review
express,fedora: Require password for Fedora (3.45 KB, patch)
2012-03-21 01:13 UTC, Zeeshan Ali
none Details | Review
express,fedora: Require password for Fedora (3.42 KB, patch)
2012-03-26 18:00 UTC, Zeeshan Ali
committed Details | Review
Mark a user-visible string for translation (1.10 KB, patch)
2012-03-26 18:03 UTC, Zeeshan Ali
committed Details | Review

Description Marc-Andre Lureau 2012-02-17 17:29:04 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.
Comment 1 Zeeshan Ali 2012-02-18 00:18:39 UTC
(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..
Comment 2 Zeeshan Ali 2012-02-18 00:19:26 UTC
(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
Comment 3 Zeeshan Ali 2012-02-18 00:32:23 UTC
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.
Comment 4 Zeeshan Ali 2012-02-18 14:39:20 UTC
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?
Comment 5 Zeeshan Ali 2012-02-18 15:06:52 UTC
Created attachment 207926 [details] [review]
wizard: Ensure needed info is provided at 'setup'
Comment 6 Zeeshan Ali 2012-02-18 15:07:48 UTC
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. ;-)
Comment 7 Marc-Andre Lureau 2012-02-20 15:16:34 UTC
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?
Comment 8 Zeeshan Ali 2012-02-20 15:48:31 UTC
(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..
Comment 9 Zeeshan Ali 2012-02-23 13:52:41 UTC
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. ;-)
Comment 10 Marc-Andre Lureau 2012-03-05 18:15:49 UTC
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.
Comment 11 Zeeshan Ali 2012-03-05 20:08:48 UTC
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.
Comment 12 Marc-Andre Lureau 2012-03-05 20:58:05 UTC
Review of attachment 209025 [details] [review]:

ack
Comment 13 Zeeshan Ali 2012-03-05 21:13:27 UTC
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'
Comment 14 Zeeshan Ali 2012-03-11 17:55:06 UTC
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.
Comment 15 Zeeshan Ali 2012-03-21 01:13:45 UTC
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.
Comment 16 Marc-Andre Lureau 2012-03-21 01:17:23 UTC
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
Comment 17 Zeeshan Ali 2012-03-21 01:24:04 UTC
(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). :(
Comment 18 Zeeshan Ali 2012-03-21 02:45:01 UTC
(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?
Comment 19 Christophe Fergeau 2012-03-21 09:14:08 UTC
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.
Comment 20 Christophe Fergeau 2012-03-21 09:14:36 UTC
(In reply to comment #19)
>  you are breaking string.

I meant "string freeze" obviously.
Comment 21 Zeeshan Ali 2012-03-26 18:00:07 UTC
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.
Comment 22 Zeeshan Ali 2012-03-26 18:03:23 UTC
Created attachment 210647 [details] [review]
Mark a user-visible string for translation
Comment 23 Marc-Andre Lureau 2012-03-26 18:31:40 UTC
Review of attachment 210646 [details] [review]:

ack
Comment 24 Marc-Andre Lureau 2012-03-26 18:31:43 UTC
Review of attachment 210647 [details] [review]:

ack
Comment 25 Zeeshan Ali 2012-03-26 19:17:45 UTC
Patches committed to 3.5.x branch.