Issue 4549 - Implement the Windows Store API to upload development builds

sitescripts/extensions/bin/createNightlies.py

Index: sitescripts/extensions/bin/createNightlies.py
===================================================================
--- a/sitescripts/extensions/bin/createNightlies.py
+++ b/sitescripts/extensions/bin/createNightlies.py
@@ -42,6 +42,9 @@
 from urllib import urlencode
 import urllib2
 import urlparse
+import httplib
+import zipfile
+
 from xml.dom.minidom import parse as parseXml
 
 from sitescripts.extensions.utils import (
@@ -232,6 +235,21 @@
         self.basename = metadata.get('general', 'basename')
         self.updatedFromGallery = False
 
+    def readEdgeMetadata(self):
+        """
+          Read Edge-specific metadata from metadata file.
+        """
+        import buildtools.packagerEdge as packagerEdge

[Vasily Kuznetsov, 2017/02/07 12:38:15]

You don't really need the `import ... as ...` syntax if you're not renaming
`packagerEdge` to something. Could be just `from buildtools import
packagerEdge`, which avoids the repetition.

[Oleksandr, 2017/02/09 00:57:14]

Done.

+        # Now read metadata file
+        metadata = packagerEdge.packager.readMetadata(self.tempdir,

[Vasily Kuznetsov, 2017/02/07 12:38:15]

This `packagerEdge.packager` is actually `buildtools.packager` module (it's
imported into `packagerEdge` on line 12
https://hg.adblockplus.org/buildtools/file/tip/packagerEdge.py#l12). Also it
seems like `packagerEdge` itself is not used in this function. Is there some
specific reason you're importing `packagerEdge` instead of just directly
importing `buildtools.packager`?

[Oleksandr, 2017/02/09 00:57:15]

Done.

+                                                      self.config.type)
+        self.version = packagerEdge.packager.getBuildVersion(self.tempdir,
+                                                             metadata, False,
+                                                             self.buildNum)
+        self.basename = metadata.get('general', 'basename')
+
+        self.compat = []
+
     def writeUpdateManifest(self):
         """
           Writes update manifest for the current build
@@ -338,7 +356,7 @@
 
             command = [os.path.join(self.tempdir, 'build.py'),
                        '-t', self.config.type, 'build', '-b', self.buildNum]
-            if self.config.type not in {'gecko', 'gecko-webext'}:
+            if self.config.type not in {'gecko', 'gecko-webext', 'edge'}:
                 command.extend(['-k', self.config.keyFile])
             command.append(self.path)
             subprocess.check_call(command, env=env)
@@ -520,6 +538,120 @@
         if any(status not in ('OK', 'ITEM_PENDING_REVIEW') for status in response['status']):
             raise Exception({'status': response['status'], 'statusDetail': response['statusDetail']})
 
+    def uploadToWindowsStore(self):

[Vasily Kuznetsov, 2017/02/07 12:38:16]

This method is quite long. It seems like it basically does three things: getting
the access token, app submission API manipulations and zip file uploading.
Perhaps we could factor the access token part and the uploading part into
separate methods. They seem easy to separate from the main API code that
revolves around `ingestionConnection`. What do you think?

[Sebastian Noack, 2017/02/07 13:54:43]

Also new methods (and variables) should use underscores for compliance with our
coding practices and PEP-8, even if surrounding legacy code is still using
camelcase. Otherwise, it will just become more difficult to get rid of
inappropriately camelcased names in the long-term.

[Oleksandr, 2017/02/09 00:57:15]

Done.

+

[Vasily Kuznetsov, 2017/02/07 12:38:16]

Also the ingestionConnection-related code seems to be doing a lot of the same
thing: sending a request, getting a json response and decoding it (sometimes
ignoring it). Perhaps we could reduce code duplication and make it more compact
by creating a method (or a local function) that takes the parameters of
`ingestConnection.request()` and returns a decoded response?

[Oleksandr, 2017/02/09 00:57:15]

Done.

+        class HTTPErrorBodyHandler(urllib2.HTTPDefaultErrorHandler):
+            def http_error_default(self, req, fp, code, msg, hdrs):
+                raise urllib2.HTTPError(req.get_full_url(), code,
+                                        '%s\n%s' % (msg, fp.read()), hdrs, fp)

[Vasily Kuznetsov, 2017/02/07 12:38:16]

You're reading the content of `fp` and at the same time returning the file
descriptor in the exception. Would not that produce confusion if whatever is
catching this error would try to read from the file descriptor?

[Vasily Kuznetsov, 2017/02/07 12:38:16]

Our style guide now recommends using `'{} bla {}'.format(foo, bar)` instead of
`'% bla %' % (foo, bar). Even though there's existing code using % around here,
it would be better to use `.format` for all new code.

[Sebastian Noack, 2017/02/07 13:54:42]

The thing is HttpError objects are both exceptions and file-like objects. So
there has to be an underlying file-like object (I suppose) that is closed when
HTTPError.close() is called. But why do we need custom error handling here in
the first place?

[Oleksandr, 2017/02/09 00:57:15]

I have moved all the networking code to just httplib, since there was no reason
to use urllib2.

+
+        opener = urllib2.build_opener(HTTPErrorBodyHandler)
+
+        # use refresh token to obtain a valid access token
+        # https://docs.microsoft.com/en-us/azure/active-directory/active-directory-protocols-oauth-code#refreshing-the-access-tokens
+
+        response = json.load(opener.open(

[Sebastian Noack, 2017/02/07 13:54:42]

The response object isn't closed here. The canonical way to make sure file-like
objects are closed properly, in modern Python code, is using the with-statement.
However, in Python 2, urllib2.Response doesn't implement __enter__/__exit__ (the
magic methods that are called by the with statement) yet. So you'd have to wrap
the call with contextlib.closing() (or use try/finally).

  with contextlib.closing(opener.open(...)) as response:
    data = json.load(response)

+            'https://login.microsoftonline.com/{0}/oauth2/token'.format(

[Sebastian Noack, 2017/02/07 13:54:43]

Thanks for using the format() method here, but the explicit index is redundant
here, '..{}..'.format(...) would be sufficient, unless you want to reuse a
particular argument more than once.

+                self.config.tenantID),
+            urlencode([
+                ('refresh_token', self.config.refreshToken),
+                ('client_id', self.config.clientID),
+                ('client_secret', self.config.clientSecret),
+                ('grant_type', 'refresh_token'),
+                ('resource', 'https://graph.windows.net')
+            ])
+        ))
+
+        auth_token = response['token_type'] + ' ' + response['access_token']

[Sebastian Noack, 2017/02/07 13:54:42]

As agreed on in our coding practices, we use the format() method, rather than
the + operator, when concatenating more than two strings, which in particular
comes handy here where you use values form a dictionary:
  
  auth_token = '{[token_type]} {[access_token]}'.format(response)

[Oleksandr, 2017/02/09 00:57:16]

Done.

+
+        # Clone the previous submission for the new one. Largely based on code
+        # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-for-the-windows-store-submission-api#create-an-app-submission

[Vasily Kuznetsov, 2017/02/07 12:38:15]

This is a very long line. I wonder if we could use a url shortener on this url?
Also has disadvantages since the url becomes kind of opaque, but we'd have a
nice neat line like.

# Based on code from https://goo.gl/9eymX7

What do you think?

[Sebastian Noack, 2017/02/07 13:54:42]

I'd rather not obfuscate URLs in the source code (comments). Not to mention,
relying on third-party tools, so that if whatever URL shortener we'd use, if it
should be suspended at some point, we end up with broken links. On the other
hand, this will become a problem, once we enforce the max line length with
flake8, here. I guess, unless somebody has a better idea, let's leave this long
line alone for now.

[Vasily Kuznetsov, 2017/02/07 15:05:26]

Yeah, good point. I also don't see an ideal solution here, and this is not the
only place where this arises. Perhaps we should just relax the line length rule
for these cases of long urls in comments somehow.

[Oleksandr, 2017/02/09 00:57:15]

Acknowledged.

+        headers = {'Authorization': auth_token,
+                   'Content-type': 'application/json',
+                   'User-Agent': 'Python'}

[Sebastian Noack, 2017/02/07 13:54:42]

Is it even necessary to set a User-Agent here?

[Oleksandr, 2017/02/09 00:57:15]

It is not. Removed.

+
+        apiServer = 'manage.devcenter.microsoft.com'

[Vasily Kuznetsov, 2017/02/07 12:38:15]

PEP8 (and therefore our style guide) recommends lowercase variable names with
words separated by underscores and we started following that for all new code.
The names of methods of `NightlyBuild` can probably stay as they are since
that's consistent with all existing methods but for the names of the variables
inside of the method it would be better to follow the style guide.

[Sebastian Noack, 2017/02/07 13:54:43]

As I said above, I think we should also avoid camelcase for new methods. The
consistency argument isn't very consistent while we still insist on underscores
for variable names. But in the end I don't care much about consistency here, but
rather prefer more code being compliant with PEP-8. In particular, if we are not
going to implement elaborate tests for this, changing it later will be a hassle.

[Oleksandr, 2017/02/09 00:57:15]

Done.

+        ingestionConnection = httplib.HTTPSConnection(apiServer)

[Sebastian Noack, 2017/02/07 13:54:42]

"ingestion" seems like a weird term to use here, is this from Microsoft's
terminology? Otherwise, I might prefer going with just "connection".

[Oleksandr, 2017/02/09 00:57:15]

Done.

+
+        # Get application
+        appPath = '/v1.0/my/applications/%s' % self.config.devbuildGalleryID
+
+        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/get-an-add-on

[Vasily Kuznetsov, 2017/02/07 12:38:15]

Is this URL relevant to what we're doing here? The code around seems to be from
the link above and I couldn't see how this page is relevant. Or am I just
missing something?

[Oleksandr, 2017/02/09 00:57:16]

Fixed, and also same for URL below.

+        ingestionConnection.request('GET', appPath, '', headers)
+        appResponse = ingestionConnection.getresponse()
+
+        # Delete existing in-progress submission
+        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/get-an-add-on
+        appObj = json.loads(appResponse.read().decode())
+
+        submissionsPath = appPath + '/submissions'
+        if 'pendingApplicationSubmission' in appObj:
+            removeId = appObj['pendingApplicationSubmission']['id']
+            ingestionConnection.request('DELETE',
+                                        '%s/%s' % (submissionsPath, removeId),
+                                        '', headers)
+            deleteSubmissionResponse = ingestionConnection.getresponse()
+            deleteSubmissionResponse.read()

[Vasily Kuznetsov, 2017/02/07 12:38:15]

Should we do some error checking here? Or will we just get a non-2xx error code
(and hence an exception) in case of error?

[Sebastian Noack, 2017/02/07 13:54:42]

Also what about closing the response?

[Oleksandr, 2017/02/09 00:57:14]

If successful, the response would have an empty body. If not, there will be
non-2xx error code, which means we would raise an exception.

[Oleksandr, 2017/02/09 00:57:15]

I don't think we have to close the response. We will close the connection now,
however.

+
+        # Create submission
+        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/create-an-app-submission
+        ingestionConnection.request('POST', submissionsPath, '', headers)
+        createSubmissionResponse = ingestionConnection.getresponse()
+
+        submission = json.loads(
+                createSubmissionResponse.read().decode()
+            )
+
+        submissionId = submission['id']
+        fileUploadUrl = submission['fileUploadUrl']
+
+        # Update submission
+        oldSubmission = submission['applicationPackages'][0]
+        oldSubmission['fileStatus'] = 'PendingDelete'
+        submission['applicationPackages'].append(
+            {'fileStatus': 'PendingUpload'})
+        addedSubmission = submission['applicationPackages'][1]
+        addedSubmission['fileName'] = os.path.basename(self.path)
+        addedSubmission['minimumSystemRam'] = oldSubmission['minimumSystemRam']
+
+        oldDirectXVersion = oldSubmission['minimumDirectXVersion']
+        addedSubmission['minimumDirectXVersion'] = oldDirectXVersion
+
+        newSubmissionPath = '%s/%s' % (submissionsPath, submissionId)
+        ingestionConnection.request('PUT', newSubmissionPath,
+                                    json.dumps(submission), headers)
+        ingestionConnection.getresponse().read()
+
+        # Add .appx file to a .zip file
+        zipPath = os.path.splitext(self.path)[0] + '.zip'
+        with zipfile.ZipFile(zipPath, 'w', zipfile.ZIP_DEFLATED) as zf:
+            zf.write(self.path, os.path.basename(self.path))
+
+        # Upload that .zip file
+        request = urllib2.Request(fileUploadUrl.replace('+', '%2B'))
+        request.get_method = lambda: 'PUT'
+        request.add_header('x-ms-blob-type', 'BlockBlob')
+
+        with open(zipPath, 'rb') as file:
+            fileSize = os.fstat(file.fileno()).st_size - file.tell()
+            request.add_header('Content-Length', fileSize)
+            request.add_data(file)
+            opener.open(request)
+
+        # Commit submission
+        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/commit-an-app-submission
+        ingestionConnection.request('POST',
+                                    newSubmissionPath + '/commit',
+                                    '', headers)
+        submission = json.loads(

[Sebastian Noack, 2017/02/07 13:54:42]

Couldn't you simply use json.load(ingestionConnection.getresponse()) here?

+                ingestionConnection.getresponse().read().decode()
+            )
+
+        if submission['status'] != 'CommitStarted':
+            raise Exception({'status': submission['status'],
+                            'statusDetails': submission['statusDetails']})

[Sebastian Noack, 2017/02/07 13:54:43]

The indentation here seems off. The keys should be aligned.

[Oleksandr, 2017/02/09 00:57:14]

Done.

+        ingestionConnection.close()

[Sebastian Noack, 2017/02/07 13:54:42]

It would be better to use the with-statement (or try/finally) to make sure the
connection is closed (see the related comment above).

[Oleksandr, 2017/02/09 00:57:15]

Done.

+
     def run(self):
         """
           Run the nightly build process for one extension
@@ -543,6 +675,8 @@
                     self.readSafariMetadata()
                 elif self.config.type in {'gecko', 'gecko-webext'}:
                     self.readGeckoMetadata()
+                elif self.config.type == 'edge':
+                    self.readEdgeMetadata()
                 else:
                     raise Exception('Unknown build type {}' % self.config.type)
 
@@ -574,6 +708,9 @@
                 self.uploadToMozillaAddons()
             elif self.config.type == 'chrome' and self.config.clientID and self.config.clientSecret and self.config.refreshToken:
                 self.uploadToChromeWebStore()
+            elif self.config.type == 'edge' and self.config.clientID and self.config.clientSecret and self.config.refreshToken and self.config.tenantID:

[Vasily Kuznetsov, 2017/02/07 12:38:15]

What happens if we don't have `config.clientID` or `config.clientSecret` or
others? Is it right to just silently do nothing?

[Sebastian Noack, 2017/02/07 13:54:42]

At least this seems consistent with the equivalanet logic for Mozilla Add-Ons
and the Chrome Webstore, above. So unless we plan to change the behavior there,
I think the logic for the Windows Store is correct and consistent.

[Vasily Kuznetsov, 2017/02/07 15:05:26]

Acknowledged.

+                self.uploadToWindowsStore()
+
         finally:
             # clean up
             if self.tempdir: