Closed Bug 1028030 Opened 7 years ago Closed 7 years ago

Add placeholders for email and password in TPS config file

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set
normal

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

Details

Attachments

(1 file, 5 obsolete files)

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".
Attached patch patch v1.0 (obsolete) — Splinter Review
Here is the patch.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Attachment #8443304 - Flags: review?(hskupin)
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.
Attachment #8443304 - Flags: review?(hskupin) → review+
Attached patch patch v2.0 (obsolete) — Splinter Review
(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.
Attachment #8443372 - Flags: review?(hskupin)
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.
Attachment #8443372 - Flags: review?(hskupin) → review-
Attached patch patch v2.1 (obsolete) — Splinter Review
Here is the updated patch. I added the potions for sync account, and I print a message if the credentials are not updated.
Attachment #8443372 - Attachment is obsolete: true
Attachment #8443417 - Flags: review?(hskupin)
Attachment #8443304 - Attachment is obsolete: true
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.
Attachment #8443417 - Flags: review?(hskupin) → review-
(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.
Attached patch patch v3.0 (obsolete) — Splinter Review
Here is the patch. I missed.
Attachment #8443417 - Attachment is obsolete: true
Attachment #8445073 - Flags: review?(hskupin)
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.
Attachment #8445073 - Flags: review?(hskupin) → review-
Attached patch patch v3.1 (obsolete) — Splinter Review
I changed the nits.
Thanks.
Attachment #8445073 - Attachment is obsolete: true
Attachment #8445810 - Flags: review?(hskupin)
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.
Attachment #8445810 - Flags: review?(hskupin) → review+
Attached patch patch v3.2Splinter Review
Sorry for that.
Attachment #8445810 - Attachment is obsolete: true
Attachment #8445814 - Flags: review?(hskupin)
Attachment #8445814 - Flags: review?(hskupin) → review+
https://hg.mozilla.org/mozilla-central/rev/0b1550f5c23f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
The patch applies cleanly on aurora and beta.
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.
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.
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.