Closed
Bug 1063532
Opened 10 years ago
Closed 10 years ago
Make Proxxy a standalone class not a Mixin
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Release Engineering
Applications: MozharnessCore
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: massimo, Assigned: massimo)
References
Details
Attachments
(1 file, 1 obsolete file)
14.44 KB,
patch
|
jlund
:
review+
jlund
:
feedback+
massimo
:
checked-in+
|
Details | Diff | Splinter Review |
We have just backed out https://bugzilla.mozilla.org/attachment.cgi?id=8481269. virtualenv was working fine with proxxy but the changes made create errors in the some test scripts as: Traceback (most recent call last): File "scripts/android_panda.py", line 35, in <module> class PandaTest(TestingMixin, MercurialScript, BlobUploadMixin, MozpoolMixin, BuildbotMixin, SUTDeviceMozdeviceMixin, MozbaseMixin): TypeError: Error when calling the metaclass bases Cannot create a consistent method resolution order (MRO) for bases SUTDeviceMozdeviceMixin, LogMixin, object, BuildbotMixin, MozbaseMixin, VirtualenvMixin, ScriptMixin The problem here is the order of inheritance defined by the Panda class (and probably other classes too). As suggested by Jordan, here https://bugzilla.mozilla.org/show_bug.cgi?id=1017759#c89, we should make this class a standalone class.
Assignee | ||
Comment 1•10 years ago
|
||
Hey Jordan, Work in progress. I have run a some tests locally and on my slave and it *seems* to work fine so I've landed it on ash-mozharness too. I am looking for unexpected behaviors because this patch modifies TestingMixin and many things rely on this module. While tests are running, can you give me a feedback on this patch? Thanks!
Attachment #8488648 -
Flags: feedback?(jlund)
Comment 2•10 years ago
|
||
Comment on attachment 8488648 [details] [diff] [review] Bug 1063532 - Make Proxxy a standalone class not a Mixin.patch Review of attachment 8488648 [details] [diff] [review]: ----------------------------------------------------------------- as always, loving the pep fixes :) I think this looks good and should achieve what we want here ::: mozharness/mozilla/proxxy.py @@ +37,5 @@ > "regions": [".use1.", ".usw2."], > } > > + def __init__(self, config, log_obj): > + self.config = config hmm, I am not sure what the best practice is here. should we pass the whole runtime config (like you are here) or should proxxy objs have their own configs? As in should we just pass: self.config.get('proxxy', {}) instead of self.config? ::: mozharness/mozilla/tooltool.py @@ +21,4 @@ > cmd = tooltool > # get the tooltools servers from configuration > default_urls = self.config['tooltool_servers'] > + proxxy = Proxxy(self.conf, self.log_obj) s/self.conf/self.config/ ?
Attachment #8488648 -
Flags: feedback?(jlund) → feedback+
Assignee | ||
Comment 3•10 years ago
|
||
Thanks for the feedback, Jordan. here's another patch (tested on ash): changes: * fixed typo (self.conf) * pep8/docstings * Proxxy takes a full config but it stores only the 'proxxy' element * removed query_proxxy_config() method from Proxxy This patch might "just work" and be ready for production (depending on your review/feedback of course!), another more complete approach to this problem is having a similar mechanism as we do with enable_mock(). It would be nice to have proxxy in ScriptMixin so that a call to download_file() uses the standard download mechanism or download_proxxied_file depending on the "enable_proxxy" value.
Attachment #8488648 -
Attachment is obsolete: true
Attachment #8491723 -
Flags: feedback?(jlund)
Comment 4•10 years ago
|
||
Comment on attachment 8491723 [details] [diff] [review] [mozharness] Bug 1063532 - Make Proxxy a standalone class not a Mixin.patch Review of attachment 8491723 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, I think we are good to land on default and test everything bar gaia-try branch. GaiaTest changes, I think, can only be testing in prod ::: mozharness/mozilla/testing/testbase.py @@ +101,5 @@ > + def download_proxied_file(self, url, file_name=None, parent_dir=None, > + create_parent_dir=True, error_level=FATAL, > + exit_code=3): > + self._query_proxxy() > + proxxy = self.proxxy might as well combine the above two lines no? or do you like making it explicit that this sets a self attr? if so, that's cool with me
Attachment #8491723 -
Flags: review+
Attachment #8491723 -
Flags: feedback?(jlund)
Attachment #8491723 -
Flags: feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8491723 -
Flags: checked-in+
Comment 5•10 years ago
|
||
In prod with reconfig on 2014-09-22 08:20 PT
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•