Closed
Bug 1028030
Opened 10 years ago
Closed 10 years ago
Add placeholders for email and password in TPS config file
Categories
(Testing Graveyard :: TPS, defect)
Testing Graveyard
TPS
Tracking
(firefox31 fixed, firefox32 fixed, firefox33 fixed)
RESOLVED
FIXED
mozilla33
People
(Reporter: cosmin-malutan, Assigned: cosmin-malutan)
Details
Attachments
(1 file, 5 obsolete files)
4.03 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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".
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Here is the patch.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Attachment #8443304 -
Flags: review?(hskupin)
Comment 2•10 years ago
|
||
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•10 years ago
|
||
(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 4•10 years ago
|
||
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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8443304 -
Attachment is obsolete: true
Comment 6•10 years ago
|
||
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•10 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•10 years ago
|
||
Here is the patch. I missed.
Attachment #8443417 -
Attachment is obsolete: true
Attachment #8445073 -
Flags: review?(hskupin)
Comment 9•10 years ago
|
||
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•10 years ago
|
||
I changed the nits. Thanks.
Attachment #8445073 -
Attachment is obsolete: true
Attachment #8445810 -
Flags: review?(hskupin)
Comment 11•10 years ago
|
||
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•10 years ago
|
||
Sorry for that.
Attachment #8445810 -
Attachment is obsolete: true
Attachment #8445814 -
Flags: review?(hskupin)
Updated•10 years ago
|
Attachment #8445814 -
Flags: review?(hskupin) → review+
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b1550f5c23f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 14•10 years ago
|
||
The patch applies cleanly on aurora and beta.
Comment 15•10 years ago
|
||
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•10 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.
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f91a98e164ea https://hg.mozilla.org/releases/mozilla-beta/rev/80a9e64d75f5
Updated•6 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•