The default bug view has changed. See this FAQ.

Make Proxxy a standalone class not a Mixin

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: massimo, Assigned: massimo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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

3 years ago
Created attachment 8488648 [details] [diff] [review]
Bug 1063532 - Make Proxxy a standalone class not a Mixin.patch

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

3 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

3 years ago
Created attachment 8491723 [details] [diff] [review]
[mozharness] Bug 1063532 - Make Proxxy a standalone class not a Mixin.patch

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

3 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

3 years ago
Attachment #8491723 - Flags: checked-in+
In prod with reconfig on 2014-09-22 08:20 PT
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.