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

NEW
Unassigned

Status

Release Engineering
Mozharness
3 years ago
3 years ago

People

(Reporter: armenzg, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [easier-mozharness][good next bug])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8509841 [details] [diff] [review]
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!
Attachment #8509841 - Flags: review?(jlund)
Attachment #8509841 - Flags: review?(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.
(Reporter)

Comment 2

3 years ago
Created attachment 8510505 [details] [diff] [review]
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.
Attachment #8509841 - Attachment is obsolete: true
Attachment #8509841 - Flags: review?(jlund)
Attachment #8509841 - Flags: review?(catlee)
Attachment #8510505 - Flags: feedback?(jlund)
Attachment #8510505 - Flags: feedback?(catlee)
(Reporter)

Updated

3 years ago
Depends on: 1088208

Comment 3

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

Updated

3 years ago
Attachment #8510505 - Flags: feedback?(jlund) → feedback+
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 :)
Attachment #8510505 - Flags: feedback?(catlee)
(Reporter)

Updated

3 years ago
Assignee: nobody → armenzg
Whiteboard: [easier-mozharness]
(Reporter)

Comment 5

3 years ago
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,
(Reporter)

Updated

3 years ago
Blocks: 1101183
(Reporter)

Updated

3 years ago
Assignee: armenzg → nobody
Mentor: armenzg@mozilla.com
(Reporter)

Updated

3 years ago
Summary: Move download logic in ScriptMixin to its own mixin → Move download logic in ScriptMixin to mozharness/lib/python/download.py
(Reporter)

Updated

3 years ago
Whiteboard: [easier-mozharness] → [easier-mozharness][good next bug]
(Reporter)

Comment 6

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

3 years ago
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.
Attachment #8554595 - Flags: feedback?(armenzg)
(Reporter)

Comment 8

3 years ago
(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.
(Reporter)

Updated

3 years ago
No longer blocks: 1086672
(Reporter)

Updated

3 years ago
Attachment #8554595 - Flags: feedback?(armenzg)
You need to log in before you can comment on or make changes to this bug.