issue 4087 - Use Mozilla's product-details data to determine current product versions

globals/get_browser_versions.py

 import re
 import os
 import sys
 import json
 import urllib
 import errno
 import logging
 import time
 from xml.dom import minidom
 
 from jinja2 import contextfunction
 
 BROWSERS = {}
+BASE_URL = 'https://product-details.mozilla.org/1.0'
+FIREFOX_URL = BASE_URL + '/firefox_versions.json'
+THUNDERBIRD_URL = BASE_URL + '/thunderbird_versions.json'
+SEAMONKEY_URL = 'http://www.seamonkey-project.org/seamonkey_versions.json'
 
 CHROME_UPDATE_XML = '''\
 <?xml version="1.0" encoding="UTF-8"?>
 <request protocol="3.0" ismachine="0">
   <os platform="win" version="99" arch="x64"/>
   <app appid="{4DC8B4CA-1BDA-483E-B5FA-D3C12E15B62D}">
     <updatecheck/>
   </app>
   <app appid="{4DC8B4CA-1BDA-483E-B5FA-D3C12E15B62D}" ap="x64-beta-multi-chrome">
     <updatecheck/>
   </app>
   <app appid="{4DC8B4CA-1BDA-483E-B5FA-D3C12E15B62D}" ap="x64-dev-multi-chrome">
     <updatecheck/>
   </app>
 </request>'''
 
 cache = {}
 
 
-def get_json_versions(product):
-    urls = {

[Vasily Kuznetsov, 2016/08/22 17:51:55]

This dict seems somewhat redundant here. Wouldn't it be better to move these
urls into constants at the top level of the file and then use them directly in
`get_xxx_versions` functions? Kind of like this:

def get_json_versions(url):
    ...

FIREFOX_URL = '....'
def get_firefox_versions():
    versions = get_json_versions(FIREFOX_URL)

Then `get_json_versions` becomes more generic and the hardcoded URLs end up in
constants instead of the dict. I know that in other functions the URLs are
hardcoded directly into the calls, but here we'd be using FIREFOX_URL twice and,
well, having the URLs in the constants seems cleaner anyway.

[Jon Sonesen, 2016/08/29 13:15:39]

So rather than a constant dict just have the function call using the constant
url as the product? I am happy to do this :D

-            'Firefox':
-            'https://product-details.mozilla.org/1.0/firefox_versions.json',
-            'Thunderbird':
-            'https://product-details.mozilla.org/1.0/thunderbird_versions.json',
-            'Seamonkey':
-            'http://www.seamonkey-project.org/seamonkey_versions.json'
-    }
-    response = urllib.urlopen(urls[product])
+def get_json_versions(product_url):
+    response = urllib.urlopen(product_url)
     try:
         doc = json.load(response)
     except json.ValueError:
-        print 'URL: %s not returning json object'.format(urls[product])
+        print 'URL: %s not returning json object'.format(product_url)
     finally:
         response.close()
 
     for key, value in doc.iteritems():
         if value:
-            doc[key] = re.search(r'^(\d+)(?:\.\d+)?', value).group(0)

[Vasily Kuznetsov, 2016/08/22 17:51:55]

Isn't this a bit fragile? What if the regex doesn't match?

[Jon Sonesen, 2016/08/29 13:15:39]

I agree that it is fragile...but also I thought we wanted this to fail 'loudly'
which this would be doing. Also, this is how the code was originally, also, 

+            match = re.search(r'^(\d+)(?:\.\d+)?', value)
+            if match:
+                doc[key] = match.group(0)
     return doc
 
 
 def get_firefox_versions():
-    versions = get_json_versions('Firefox')
+    versions = get_json_versions(FIREFOX_URL)
     return {
             'current': versions['LATEST_FIREFOX_VERSION'],
             'unreleased': [
                 versions['LATEST_FIREFOX_DEVEL_VERSION'],

[Vasily Kuznetsov, 2016/08/22 17:51:55]

It would be nice to give more friendly error messages in case the key is not
there, but all other browser version getters don't really take care of this.
Probably makes sense to follow suit here for consistency.

[Jon Sonesen, 2016/08/29 13:15:39]

I thought we decided not to so the build 'fails loudly' basically meaning we
need to go investigate why there is not the expected key in case they change
something.

                 versions['FIREFOX_AURORA'],
                 versions['FIREFOX_NIGHTLY'],
             ]
     }
 
 
 def get_thunderbird_versions():
-    versions = get_json_versions('Thunderbird')
-    firefox_versions = get_json_versions('Firefox')
-    return {
-        'current': versions['LATEST_THUNDERBIRD_VERSION'],
+    tbird_versions = get_json_versions(THUNDERBIRD_URL)
+    firefox_versions = get_json_versions(FIREFOX_URL)
+    return {
+        'current': tbird_versions['LATEST_THUNDERBIRD_VERSION'],
         'unreleased': [
-            versions['LATEST_THUNDERBIRD_DEVEL_VERSION'],
-            versions['LATEST_THUNDERBIRD_ALPHA'],

[Vasily Kuznetsov, 2016/08/22 17:51:55]

I think this should be LATEST_THUNDERBIRD_ALPHA_VERSION

[Jon Sonesen, 2016/08/29 13:15:39]

you're right....dang haha

+            tbird_versions['LATEST_THUNDERBIRD_DEVEL_VERSION'],
+            tbird_versions['LATEST_THUNDERBIRD_ALPHA_VERSION'],
             firefox_versions['FIREFOX_NIGHTLY'],
         ]
     }
 
 BROWSERS['firefox'] = lambda: get_firefox_versions()
 BROWSERS['thunderbird'] = lambda: get_thunderbird_versions()
 
 
 def get_seamonkey_versions():
-    seamonkey_versions = get_json_versions('Seamonkey')
+    seamonkey_versions = get_json_versions(SEAMONKEY_URL)
     versions = {
         'current': seamonkey_versions['LATEST_SEAMONKEY_VERSION'],
         'unreleased': [
             seamonkey_versions['LATEST_SEAMONKEY_MILESTONE_VERSION'],
             seamonkey_versions['LATEST_SEAMONKEY_TESTING_VERSION'],
             seamonkey_versions['LATEST_SEAMONKEY_DEVEL_VERSION'],
         ]
     }
 
     return versions
@@ skipping 149 matching lines @@
         file.seek(0)
         json.dump(persistent_cache, file)
         file.truncate()
 
     if not versions['previous']:
         logging.warning("Couldn't determine previous browser version, "
                         'please set %s.previous in %s', browser, filename)
 
     cache[browser] = versions
     return versions