1028030 - Add placeholders for email and password in TPS config file

Status: RESOLVED FIXED

(Testing Graveyard :: TPS, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

As part of TPS-CI implementation we would need to edit the config file to insert the email address and password. For that we would need some placeholders, similar with what we have for "testdir".

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

Here is the patch.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8443304 [details] [diff] [review]
patch v1.0

Review of attachment 8443304 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I wonder if we should enhance the environment creation script to accept those values already as options. That way no further processing would be necessary afterward, and from Coversheet we only have to call the env script.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Looks good, but I wonder if we should enhance the environment creation
> script to accept those values already as options. That way no further
> processing would be necessary afterward, and from Coversheet we only have to
> call the env script.
Sounds like a good approach for me, I updated the patch for handling that, I'm not treating sync account credentials to don't over complicate the patch.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8443372 [details] [diff] [review]
patch v2.0

Review of attachment 8443372 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/tps/create_venv.py
@@ +46,4 @@
>  
>  def main():
>      parser = optparse.OptionParser('Usage: %prog [options] path_to_venv')
> +    parser.add_option('--password',

We need that and the username for the old sync authentication too. Not sure why you left this out. As we talked on IRC we want to do that, also with our CI.

I would leave --password and --username for the fxaccount authentication, and add --sync-*** for the old sync related data (and note that this might be removed soon).

@@ +62,5 @@
> +                      type='string',
> +                      dest='username',
> +                      metavar='FX_ACCOUNT_USERNAME',
> +                      default=None,
> +                      help='The Firefox Account to use.')

s/to use/username/

@@ +104,5 @@
>      config_in_path = os.path.join(here, 'config', 'config.json.in')
>      replacements = {'__TESTDIR__': testdir, '__EXTENSIONDIR__': extdir}
> +    if options.username and options.password:
> +        replacements.update({'__FX_ACCOUNT_USERNAME__': options.username,
> +                             '__FX_ACCOUNT_PASSWORD__': options.password})

If those have not been specified lets print out a warning, so the user knows it has to be updated afterward.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

Here is the updated patch. I added the potions for sync account, and I print a message if the credentials are not updated.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8443417 [details] [diff] [review]
patch v2.1

Review of attachment 8443417 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/tps/create_venv.py
@@ +56,5 @@
> +                      type='string',
> +                      dest='passphrase',
> +                      metavar='SYNC_ACCOUNT_PASSPHRASE',
> +                      default=None,
> +                      help='The old Firefox Sync Account passphrase.')

Why didn't this option get the sync prefix added? This is not Firefox Account related at all. You can see this already in the meta variable.

@@ +80,5 @@
> +                      type='string',
> +                      dest='username',
> +                      metavar='FX_ACCOUNT_USERNAME',
> +                      default=None,
> +                      help='The Firefox Account to use.')

One of my last review comments has not been fixed. Same applies for sync too.

@@ +124,5 @@
>      replacements = {'__TESTDIR__': testdir, '__EXTENSIONDIR__': extdir}
> +    if options.username and options.password:
> +        fx_account = {'__FX_ACCOUNT_USERNAME__': options.username,
> +                      '__FX_ACCOUNT_PASSWORD__': options.password}
> +        replacements.update(fx_account)

Nit: I don't see why we need this extra temporary variable.

@@ +132,5 @@
> +    if options.sync_username and options.sync_password and options.passphrase:
> +        sync_account = {'__SYNC_ACCOUNT_USERNAME__': options.sync_username,
> +                        '__SYNC_ACCOUNT_PASSWORD__': options.sync_password,
> +                        '__SYNC_ACCOUNT_PASSPHRASE__': options.passphrase}
> +        replacements.update(sync_account)

Same as above.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> > +    if options.sync_username and options.sync_password and options.passphrase:
> > +        sync_account = {'__SYNC_ACCOUNT_USERNAME__': options.sync_username,
> > +                        '__SYNC_ACCOUNT_PASSWORD__': options.sync_password,
> > +                        '__SYNC_ACCOUNT_PASSPHRASE__': options.passphrase}
> > +        replacements.update(sync_account)
> 
> Same as above.

I made the changes, the reason I added a variable here was to save some space in order to have 80 chars per line and don't have to split it after ":" which also fails on pep8.
I removed that and I start with the whole object on the new line.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

Here is the patch. I missed.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8445073 [details] [diff] [review]
patch v3.0

Review of attachment 8445073 [details] [diff] [review]:
-----------------------------------------------------------------

Still some nits. I hope that we will have it next time.

::: testing/tps/create_venv.py
@@ +51,5 @@
> +                      dest='password',
> +                      metavar='FX_ACCOUNT_PASSWORD',
> +                      default=None,
> +                      help='The Firefox Account password.')
> +    parser.add_option('--sync-passphrase',

This is not sorted alphabetical. So move it next to the old sync account related entry.

@@ +74,5 @@
> +                      type='string',
> +                      dest='sync_username',
> +                      metavar='SYNC_ACCOUNT_USERNAME',
> +                      default=None,
> +                      help='The old Firefox Sync Account username.')

'account' here. It's not part of a name.

@@ +123,5 @@
>      config_in_path = os.path.join(here, 'config', 'config.json.in')
>      replacements = {'__TESTDIR__': testdir, '__EXTENSIONDIR__': extdir}
> +    if options.username and options.password:
> +        replacements.update({'__FX_ACCOUNT_USERNAME__': options.username,
> +                             '__FX_ACCOUNT_PASSWORD__': options.password})

Why don't you use the same indentation as below?

@@ +125,5 @@
> +    if options.username and options.password:
> +        replacements.update({'__FX_ACCOUNT_USERNAME__': options.username,
> +                             '__FX_ACCOUNT_PASSWORD__': options.password})
> +    else:
> +        print "Firefox account credentials were not updated in config file."

I would say it more pro-active: 'Firefox Account credentials not specified. Please update the config file manually.'

@@ +133,5 @@
> +            '__SYNC_ACCOUNT_USERNAME__': options.sync_username,
> +            '__SYNC_ACCOUNT_PASSWORD__': options.sync_password,
> +            '__SYNC_ACCOUNT_PASSPHRASE__': options.sync_passphrase})
> +    else:
> +        print "Old sync account credentials were not updated in config file."

Same here.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

I changed the nits.
Thanks.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8445810 [details] [diff] [review]
patch v3.1

Review of attachment 8445810 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nit as mentioned below fixed.

::: testing/tps/create_venv.py
@@ +63,5 @@
> +                      dest='sync_password',
> +                      metavar='SYNC_ACCOUNT_PASSWORD',
> +                      default=None,
> +                      help='The old Firefox Sync account password.')
> +    parser.add_option('--sync-passphrase',

password comes still after passphrase for me in the alphabet.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.2

Sorry for that.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/mozilla-central/rev/0b1550f5c23f

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on aurora and beta.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The patch has to apply cleanly given that we keep tps in sync across branches. What you have to do here is to verify that it is working with the packaged tests of the next nightly build. If that passes we can backport the patch.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

I ran a complete testrun with packaged tests from below and it worked great.
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64/1403703608/firefox-33.0a1.en-US.linux-x86_64.tests.zip
So this can be backported.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f91a98e164ea
https://hg.mozilla.org/releases/mozilla-beta/rev/80a9e64d75f5