628655 - Support daily builds in download script

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With bug 528064 we now have the capability to easily download builds from our staging server and mirrors. But it only implements the download for release and release candidate builds. We have to implement the NightlyScraper class to also support nightly builds.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Heather, would you be ok with when we take parts of your script for out downloader script? It's only missing the nightly build pieces.

https://github.com/harthur/mozregression/blob/master/mozregression/runnightly.py

Comment 2

[Heather Arthur [:harth]]

(In reply to comment #1)
> Heather, would you be ok with when we take parts of your script for out
> downloader script? It's only missing the nightly build pieces.
> 
> https://github.com/harthur/mozregression/blob/master/mozregression/runnightly.py

That's fine, yeah

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the new channels we have to support daily builds for Nightly and Aurora. Both will be covered by my upcoming patch.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Please note that we do not support the old structure on the FTP server yet. It will only work with the structure we have right now. As long as we do not run regression tests with Mozmill there will also be no need for it.

Comment 5

[Jeff Hammel]

Comment on attachment 542293 [details] [diff] [review]
Patch v1

Nit:
+    parser.add_option('--build', '-b',
+                     dest='build',
+                     default=1,
indentation of dest, etc should be one additional space

You should also do `type="int"` if you expect an int. Otherwise you'll get a string

+        self.list_regex = 'nightly/%(YEAR)s/%(MONTH)s/' % {
+                              'YEAR': self.date.year,
+                              'MONTH': str(self.date.month).zfill(2) }

I find this type of filtering very brittle. I would expect some tests, a docstring, or otherwise documentation on why things are done this way, preferably citing a URL

+        regex = r'%(DATE)s-(\d+-)+mozilla-%(BRANCH)s%(L10N)s$' % {
+                    'DATE': self.date.strftime('%Y-%m-%d'),
+                    'BRANCH': self.branch,
+                    'L10N': '' if self.locale == 'en-US' else '-l10n'}

Likewise

+    @property
+    def binary_regex(self):

Is there any reason to have this a property vs a function?

I really find the filtering stuff more complex and it would be nice to have this live as a utility.  I'll grant a review, fixing the type="int" issue, but it is with a grain of salt

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #5)
> +    @property
> +    def binary_regex(self):
> 
> Is there any reason to have this a property vs a function?

Why should that not be a property? From all what I can see it speaks for a getter. Also all other elements on the classes are properties.

> I really find the filtering stuff more complex and it would be nice to have
> this live as a utility.  I'll grant a review, fixing the type="int" issue,
> but it is with a grain of salt

The parser is already a separate class, which we can put into another module in the future. But all the regular expressions are closely tied to the scraper class and can't be moved out.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Any of the review comments have been addressed and documentation added. I hope it's more understandable now.

Also I have found an issue with branch handling. We only supported branches with a 'mozilla-' prefix. Now we support all branches like 'private-browsing', 'ux', and others.

Comment 8

[Jeff Hammel]

Comment on attachment 542293 [details] [diff] [review]
Patch v1

+    def __init__(self, branch='mozilla-central', date=date.today(),
So, this will set the default date argument for the function to when the module is first loaded.  In other words, it will be wrong after 24-N hours.  Probably better to set to None and then do 

if date is None:
   date = datetime.date.today()

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v3

With updated review comments. Thanks for the clarification, that were news to me.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v3.1

I should not upload broken patches. Here the qrefreshed version of the last patch.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v3.2

Not sure what hg is doing. Even with a hg qrefresh the latest changes weren't included. Seems to be fixed now.

Comment 12

[Jeff Hammel]

Comment on attachment 542783 [details] [diff] [review]
Patch v3.2

+from datetime import date

...

+    def __init__(self, branch='mozilla-central', date=None,
+                 build=1, *args, **kwargs):
         MozillaScraper.__init__(self, *args, **kwargs)
-        raise NotImplementedError("Download of nightly builds hasn't been implemented yet.")
+        self.branch = branch
+        self.build = build
+        self.date = date if date else datetime.date.today()

This won't work, as you don't import datetime abict (just from datetime import date).  I recommend changing the import 

r+ if you fix that

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v4

Thanks for catching that Jeff! What an awkward mistake. Here the updated patch and taking over r+.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/78fcedc41024

Thanks everyone. Support for tinderbox or hourly builds can be added later on another bug.