Add placeholders for email and password in TPS config file

RESOLVED FIXED in mozilla33

Status

RESOLVED FIXED
4 years ago
3 months ago

People

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

Tracking

unspecified
mozilla33
Bug Flags:
in-testsuite -

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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".
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
(Assignee)

Comment 1

4 years ago
Created attachment 8443304 [details] [diff] [review]
patch v1.0

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+
(Assignee)

Comment 3

4 years ago
Created attachment 8443372 [details] [diff] [review]
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.
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-
(Assignee)

Comment 5

4 years ago
Created attachment 8443417 [details] [diff] [review]
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.
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-
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
Created attachment 8445073 [details] [diff] [review]
patch v3.0

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-
(Assignee)

Comment 10

4 years ago
Created attachment 8445810 [details] [diff] [review]
patch v3.1

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+
(Assignee)

Comment 12

4 years ago
Created attachment 8445814 [details] [diff] [review]
patch v3.2

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
Last Resolved: 4 years ago
status-firefox33: affected → fixed
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Comment 16

4 years ago
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.

Updated

3 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.