Closed Bug 1063532 Opened 5 years ago Closed 5 years ago

Make Proxxy a standalone class not a Mixin

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: massimo, Assigned: massimo)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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 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+
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 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+
Attachment #8491723 - Flags: checked-in+
In prod with reconfig on 2014-09-22 08:20 PT
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.