1087664 - Move download logic in ScriptMixin to mozharness/lib/python/download.py

Status: RESOLVED INCOMPLETE

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Armen [:armenzg]]

Attachment: refactor.diff

I'm testing this on Ash.
The patch might change slightly to make sure that every jobs works, however, the main changes in this patch already work for me and shows my intent clearly.

If you could please review (or at least approval of direction) it before the final patch it would be great!

Comment 1

[Chris AtLee [:catlee]]

Comment on attachment 8509841 [details] [diff] [review]
refactor.diff

Review of attachment 8509841 [details] [diff] [review]:
-----------------------------------------------------------------

so, perhaps this is scope creep...

I'd really like to see you try to do this so that the main downloading functions are standalone functions rather than methods on a mixin class. You can make a mixin class for backwards compatibility if you want, but I really think mozharness needs to get away from mixins as much as possible.

Comment 2

[Armen [:armenzg]]

Attachment: wip - refactor.diff

I won't be able to work more on this bug for a bit, however, I would like your input for when I get back to it.

Comment 3

[Jordan Lund (:jlund)]

> I'd really like to see you try to do this so that the main downloading
> functions are standalone functions rather than methods on a mixin class. You
> can make a mixin class for backwards compatibility if you want, but I really
> think mozharness needs to get away from mixins as much as possible.

I skimmed over the patch. At a high level, it should work :)

It's nice to isolate the behavior into a class but I have to agree with catlee, Mozharness classes are becoming a little too 'mixed' up. I feel bad for python's MRO and for anyone trying to manipulate a Mixin without breaking some stray dependent script.

I don't want to block but if we can, with minimal effort, do something like mgerva did with Proxxy[1], I think that should be the way forward if we are taking the time to refactor Mozharness parts. Happy to discuss further.

[1] https://bugzil.la/1063532

Comment 4

[Chris AtLee [:catlee]]

Comment on attachment 8510505 [details] [diff] [review]
wip - refactor.diff

Review of attachment 8510505 [details] [diff] [review]:
-----------------------------------------------------------------

well, I'd still like these to be split out into non-mixins :)

Comment 5

[Armen [:armenzg]]

I found a work around for using the developer mode locally until this is fixed.

diff --git a/mozharness/mozilla/testing/testbase.py b/mozharness/mozilla/testing/testbase.py
--- a/mozharness/mozilla/testing/testbase.py
+++ b/mozharness/mozilla/testing/testbase.py
@@ -337,1 +337,1 @@ 2. running via buildbot and running the 
-        source = self.download_proxied_file(self.test_url, file_name=file_name,
+        source = self.download_file(self.test_url, file_name=file_name,
@@ -347,1 +347,1 @@ 2. running via buildbot and running the 
-        zipfile = self.download_proxied_file(url, parent_dir=dirs['abs_work_dir'],
+        zipfile = self.download_file(url, parent_dir=dirs['abs_work_dir'],
@@ -425,1 +425,1 @@ 2. running via buildbot and running the 
-        source = self.download_proxied_file(self.installer_url,
+        source = self.download_file(self.installer_url,
@@ -441,1 +441,1 @@ 2. running via buildbot and running the 
-        source = self.download_proxied_file(self.symbols_url,
+        source = self.download_file(self.symbols_url,

Comment 6

[Armen [:armenzg]]

Once there is a patch in here, one way to test it is by running a Firefox desktop unit test locally. [1]
You can see test jobs on treeherder.mozilla.org

[1] https://wiki.mozilla.org/ReleaseEngineering/Mozharness/How_to_run_tests_as_a_developer

Comment 7

[kartik gupta]

Attachment: patch1.diff

Armen, will this kind of script do? The content of the functions is the same as you had done earlier.

Comment 8

[Armen [:armenzg]]

(In reply to kartik gupta from comment #7)
> Created attachment 8554595 [details] [diff] [review]
> patch1.diff
> 
> Armen, will this kind of script do? The content of the functions is the same
> as you had done earlier.

We need the logic be pulled out of the DownloadMixin object. It needs to be functions that we can import and use.

This is what catlee and jlund were objecting to.

Comment 9

[Aki Sasaki (not active)]

Mozharness is likely in maintenance mode / future deprecation. Marking incomplete.