Issue 29374637: Issue 4549 - Implement the Windows Store API to upload development builds (Closed)
|
Created:
Feb. 6, 2017, 11:39 a.m. by Oleksandr Modified:
Sept. 7, 2017, 11:08 p.m. Visibility:
Public. |
DescriptionIssue 4549 - Implement the Windows Store API to upload development builds
Patch Set 1 : Issue 4549 - Implement the Windows Store API to upload development builds #Patch Set 2 : Add .sitescripts.example changes #
Total comments: 42
Patch Set 3 : Move to underscore notation. Simplify the networking code. Address comments. #
Total comments: 22
Patch Set 4 : Make sure response is always read. Address the nits. #Patch Set 5 : Switch to urllib2 #
Total comments: 18
Patch Set 6 : Update buildtools. Remove submission update logic. Always close requests. #
Total comments: 4
Patch Set 7 : Address nits #Patch Set 8 : Rebase #
MessagesTotal messages: 20
Hi Ollie! Pretty impressive! Getting this to work must not have been a walk in the park. I have some mostly style-guide-related suggestions below. One thing that is more substantial: `uploadToWindowsStore()` method is quite long and has some code repetition that we probably could avoid. See the comments there for some ideas that I had. Cheers, Vasily P.S. It would be nice to have a test for this but I see that it would be pretty meaningless since we'd just have to mock all this package uploading API. Such test probably makes no sense but let me know if you have better ideas. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:242: import buildtools.packagerEdge as packagerEdge 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:244: metadata = packagerEdge.packager.readMetadata(self.tempdir, 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`? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: def uploadToWindowsStore(self): 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:542: 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:546: '%s\n%s' % (msg, fp.read()), hdrs, fp) 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:546: '%s\n%s' % (msg, fp.read()), hdrs, fp) 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:568: # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-fo... 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:573: apiServer = 'manage.devcenter.microsoft.com' 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:579: # https://msdn.microsoft.com/en-us/windows/uwp/monetize/get-an-add-on 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:594: deleteSubmissionResponse.read() 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:711: elif self.config.type == 'edge' and self.config.clientID and self.config.clientSecret and self.config.refreshToken and self.config.tenantID: What happens if we don't have `config.clientID` or `config.clientSecret` or others? Is it right to just silently do nothing?
Thanks for having a go, Ollie! BTW, did you run "tox", which is the runner for our tests and linter (i.e. flake8)? Some flake8 errors are disabled for this file (due to legacy code), but some other issues might still have been caught. On 2017/02/07 12:38:16, Vasily Kuznetsov wrote: > P.S. It would be nice to have a test for this but I see that it would be pretty > meaningless since we'd just have to mock all this package uploading API. Such > test probably makes no sense but let me know if you have better ideas. Well, we did pretty much the same for the CrowdIn integration in the CMS. So mocking an external API, in order to automatically test our code against it, might exactly be what we need here. But that might be non-trivial, and we don't have any tests for the Mozilla Add-Ons and Chrome Webstore upload either yet. So also considering that this change is somewhat urgent, I wouldn't insist on automated tests before landing this. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: def uploadToWindowsStore(self): On 2017/02/07 12:38:16, Vasily Kuznetsov wrote: > 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? 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:546: '%s\n%s' % (msg, fp.read()), hdrs, fp) On 2017/02/07 12:38:16, Vasily Kuznetsov wrote: > 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? 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? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:553: response = json.load(opener.open( 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://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:554: 'https://login.microsoftonline.com/{0}/oauth2/token'.format( 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:565: auth_token = response['token_type'] + ' ' + response['access_token'] 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) https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:568: # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-fo... On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > 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? 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:571: 'User-Agent': 'Python'} Is it even necessary to set a User-Agent here? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:573: apiServer = 'manage.devcenter.microsoft.com' On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > 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. 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:574: ingestionConnection = httplib.HTTPSConnection(apiServer) "ingestion" seems like a weird term to use here, is this from Microsoft's terminology? Otherwise, I might prefer going with just "connection". https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:594: deleteSubmissionResponse.read() Also what about closing the response? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:646: submission = json.loads( Couldn't you simply use json.load(ingestionConnection.getresponse()) here? https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:652: 'statusDetails': submission['statusDetails']}) The indentation here seems off. The keys should be aligned. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:653: ingestionConnection.close() It would be better to use the with-statement (or try/finally) to make sure the connection is closed (see the related comment above). https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:711: elif self.config.type == 'edge' and self.config.clientID and self.config.clientSecret and self.config.refreshToken and self.config.tenantID: On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > What happens if we don't have `config.clientID` or `config.clientSecret` or > others? Is it right to just silently do nothing? 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.
> Well, we did pretty much the same for the CrowdIn integration in the CMS. So > mocking an external API, in order to automatically test our code against it, > might exactly be what we need here. But that might be non-trivial, and we don't > have any tests for the Mozilla Add-Ons and Chrome Webstore upload either yet. So > also considering that this change is somewhat urgent, I wouldn't insist on > automated tests before landing this. Sounds good. Let's land it without the automated tests but create another issue to add the tests (same as we did with CMS). John or I can work on that second issue, we have some experience in web API mocking by now. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:568: # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-fo... On 2017/02/07 13:54:42, Sebastian Noack wrote: > On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > > 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? > > 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. 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:711: elif self.config.type == 'edge' and self.config.clientID and self.config.clientSecret and self.config.refreshToken and self.config.tenantID: On 2017/02/07 13:54:42, Sebastian Noack wrote: > On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > > What happens if we don't have `config.clientID` or `config.clientSecret` or > > others? Is it right to just silently do nothing? > > 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. Acknowledged.
On 2017/02/07 13:54:43, Sebastian Noack wrote: > BTW, did you run "tox", which is the runner for our tests and linter (i.e. > flake8)? Some flake8 errors are disabled for this file (due to legacy code), but > some other issues might still have been caught. Even better. Python extenssion for VS Code (https://marketplace.visualstudio.com/items?itemName=donjayamanne.python) automatically picks up our flake8 rules and highlights errors on the fly :) https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:242: import buildtools.packagerEdge as packagerEdge On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > 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. Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:244: metadata = packagerEdge.packager.readMetadata(self.tempdir, On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > 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`? Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: def uploadToWindowsStore(self): On 2017/02/07 13:54:43, Sebastian Noack wrote: > On 2017/02/07 12:38:16, Vasily Kuznetsov wrote: > > 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? > > 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. Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:542: On 2017/02/07 12:38:16, Vasily Kuznetsov wrote: > 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? Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:546: '%s\n%s' % (msg, fp.read()), hdrs, fp) On 2017/02/07 13:54:42, Sebastian Noack wrote: > On 2017/02/07 12:38:16, Vasily Kuznetsov wrote: > > 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? > > 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? I have moved all the networking code to just httplib, since there was no reason to use urllib2. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:565: auth_token = response['token_type'] + ' ' + response['access_token'] On 2017/02/07 13:54:42, Sebastian Noack wrote: > 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) Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:568: # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-fo... On 2017/02/07 15:05:26, Vasily Kuznetsov wrote: > On 2017/02/07 13:54:42, Sebastian Noack wrote: > > On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > > > 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? > > > > 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. > > 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. Acknowledged. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:571: 'User-Agent': 'Python'} On 2017/02/07 13:54:42, Sebastian Noack wrote: > Is it even necessary to set a User-Agent here? It is not. Removed. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:573: apiServer = 'manage.devcenter.microsoft.com' On 2017/02/07 13:54:43, Sebastian Noack wrote: > On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > > 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. > > 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. Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:574: ingestionConnection = httplib.HTTPSConnection(apiServer) On 2017/02/07 13:54:42, Sebastian Noack wrote: > "ingestion" seems like a weird term to use here, is this from Microsoft's > terminology? Otherwise, I might prefer going with just "connection". Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:579: # https://msdn.microsoft.com/en-us/windows/uwp/monetize/get-an-add-on On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > 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? Fixed, and also same for URL below. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:594: deleteSubmissionResponse.read() On 2017/02/07 12:38:15, Vasily Kuznetsov wrote: > 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? 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. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:594: deleteSubmissionResponse.read() On 2017/02/07 13:54:42, Sebastian Noack wrote: > Also what about closing the response? I don't think we have to close the response. We will close the connection now, however. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:652: 'statusDetails': submission['statusDetails']}) On 2017/02/07 13:54:43, Sebastian Noack wrote: > The indentation here seems off. The keys should be aligned. Done. https://codereview.adblockplus.org/29374637/diff/29374656/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:653: ingestionConnection.close() On 2017/02/07 13:54:42, Sebastian Noack wrote: > It would be better to use the with-statement (or try/finally) to make sure the > connection is closed (see the related comment above). Done.
https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: method, url, body=None, headers={}): Using mutable types as default arguments is rather dangerous and confusing, since they are initialized on function definition, not on function call. def f(k, d={}) d[k] = True return d f('foo') => {'foo': True} f('bar') => {'foo': True, 'bar': True} f('foo') is f('bar') => True I cannot even tell whether that is a problem in practice here, without looking at the code of HTTPSConnection.request(). But it's generally wise to just avoid mutable types in default arguments. Here, it would even reduce the code: def get_response(self, connection, *args): connection.request(*args) ... https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): The parentheses here are redundant, flake8-abp would have complained about that. So by now I'm pretty sure that our flake8 configuration isn't in effect in your environment. Note that tox.ini lists some flake8 extensions (including ours flake8-abp) as dependency. If you run tox, it will setup a virtualenv where it installs all dependencies, and then runs flake8 (and our tests). So even if VS Code has flake8 integration, outside of tox, you'd have to install flake8 extensions manually/globally. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): Shouldn't we rather check for non-2xx? In Python this can be done pretty easily: if 200 <= response.status < 300: ... https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() It seems wherever get_repsonse() is called, and the return-value/response isn't ignored, JSON is expected. So how about parsing the JSON here, rather than duplicating json.loads() 4 times? https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() I looked at the code of httplib, and indeed responses need to be closed. That might seem counter-intuitive, as the connection is reused, and the response merely represents a chunk of data retrieved over that connection. But each response has its own file descriptor, created with socket.makefile(), which reads data from the underlying connection. You might have figured that it also works if you don't close the response. That is because file descriptors are automatically closed when the script terminates, and this script opens less than the maximum number of file descriptors on your operating system. Even before the script terminates, some file descriptors may be closed while the garbage collector calls the __del__ method of unreferenced file objects. However, this can quickly become a problem if this script will have to deal with more file descriptors in the future, or if we should ever use this code in the context of a long-running service which usually does not terminate, or if we use a different Python implementation that does not support __del__, or if we run it in an environment with a lower maximum number of file descriptors. So always make sure, to close any resources as soon as possible. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:570: token_response) Nit: If you just call the variable "response", you don't have to wrap this line. And since there is no other response in this context, there shouldn't be any ambiguity.
https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): On 2017/02/09 12:51:57, Sebastian Noack wrote: > Shouldn't we rather check for non-2xx? In Python this can be done pretty easily: > > if 200 <= response.status < 300: > ... Your condition seems to check for 2xx instead of non-2xx :) Another thing is: Httplib doesn't seem to handle redirects and things like this. Could this create a problem for us, if some API calls would try redirecting us or is this not a real possibility? https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/09 12:51:57, Sebastian Noack wrote: > I looked at the code of httplib, and indeed responses need to be closed. That > might seem counter-intuitive, as the connection is reused, and the response > merely represents a chunk of data retrieved over that connection. But each > response has its own file descriptor, created with socket.makefile(), which > reads data from the underlying connection. > > You might have figured that it also works if you don't close the response. That > is because file descriptors are automatically closed when the script terminates, > and this script opens less than the maximum number of file descriptors on your > operating system. Even before the script terminates, some file descriptors may > be closed while the garbage collector calls the __del__ method of unreferenced > file objects. However, this can quickly become a problem if this script will > have to deal with more file descriptors in the future, or if we should ever use > this code in the context of a long-running service which usually does not > terminate, or if we use a different Python implementation that does not support > __del__, or if we run it in an environment with a lower maximum number of file > descriptors. So always make sure, to close any resources as soon as possible. You're right, it does have a close() method indeed (that wasn't very obvious from the docs: https://docs.python.org/2/library/httplib.html#httpresponse-objects). However, looking at the source code as well, it seems that `response.read()` would close the response by itself so there's no need to close it. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:587: file_upload_con.request('PUT', '{}?{}{}'.format( Maybe combining the url fragments on a separate line would make this construction a bit more compact and readable?
https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > Shouldn't we rather check for non-2xx? In Python this can be done pretty > easily: > > > > if 200 <= response.status < 300: > > ... > > Your condition seems to check for 2xx instead of non-2xx :) > > Another thing is: Httplib doesn't seem to handle redirects and things like this. > Could this create a problem for us, if some API calls would try redirecting us > or is this not a real possibility? Would it be a problem if a new connection is used for each request? If not, perhaps we should consider just using urllib2, it resolves redirects automatically, and already raises an exception for non-2xx responses. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > I looked at the code of httplib, and indeed responses need to be closed. That > > might seem counter-intuitive, as the connection is reused, and the response > > merely represents a chunk of data retrieved over that connection. But each > > response has its own file descriptor, created with socket.makefile(), which > > reads data from the underlying connection. > > > > You might have figured that it also works if you don't close the response. > That > > is because file descriptors are automatically closed when the script > terminates, > > and this script opens less than the maximum number of file descriptors on your > > operating system. Even before the script terminates, some file descriptors may > > be closed while the garbage collector calls the __del__ method of unreferenced > > file objects. However, this can quickly become a problem if this script will > > have to deal with more file descriptors in the future, or if we should ever > use > > this code in the context of a long-running service which usually does not > > terminate, or if we use a different Python implementation that does not > support > > __del__, or if we run it in an environment with a lower maximum number of file > > descriptors. So always make sure, to close any resources as soon as possible. > > You're right, it does have a close() method indeed (that wasn't very obvious > from the docs: > https://docs.python.org/2/library/httplib.html#httpresponse-objects). However, > looking at the source code as well, it seems that `response.read()` would close > the response by itself so there's no need to close it. The code is rather complex with a lot of special cases. But it seems the underlying file descriptor is at least closed automatically when the complete response has been read. So in the current patch, we still leak file descriptors, because we not always read from the response. This should no longer happen when we parse the returned json no matter what, as I suggested in another comment. But I still feel that it's cleaner and more robust if we close it explicitly.
https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/09 20:50:23, Sebastian Noack wrote: > On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > > I looked at the code of httplib, and indeed responses need to be closed. > That > > > might seem counter-intuitive, as the connection is reused, and the response > > > merely represents a chunk of data retrieved over that connection. But each > > > response has its own file descriptor, created with socket.makefile(), which > > > reads data from the underlying connection. > > > > > > You might have figured that it also works if you don't close the response. > > That > > > is because file descriptors are automatically closed when the script > > terminates, > > > and this script opens less than the maximum number of file descriptors on > your > > > operating system. Even before the script terminates, some file descriptors > may > > > be closed while the garbage collector calls the __del__ method of > unreferenced > > > file objects. However, this can quickly become a problem if this script will > > > have to deal with more file descriptors in the future, or if we should ever > > use > > > this code in the context of a long-running service which usually does not > > > terminate, or if we use a different Python implementation that does not > > support > > > __del__, or if we run it in an environment with a lower maximum number of > file > > > descriptors. So always make sure, to close any resources as soon as > possible. > > > > You're right, it does have a close() method indeed (that wasn't very obvious > > from the docs: > > https://docs.python.org/2/library/httplib.html#httpresponse-objects). However, > > looking at the source code as well, it seems that `response.read()` would > close > > the response by itself so there's no need to close it. > > The code is rather complex with a lot of special cases. But it seems the > underlying file descriptor is at least closed automatically when the complete > response has been read. So in the current patch, we still leak file descriptors, > because we not always read from the response. This should no longer happen when > we parse the returned json no matter what, as I suggested in another comment. > But I still feel that it's cleaner and more robust if we close it explicitly. Yeah, I was assuming always parsing, I also think it's a good idea. Also I think I will agree with you about closing the response after reading just to be on the safe side and not have to understand all that code in httplib. What bothered me before about closing it is that `HTTPResponse.close` is not even mentioned in the documentation so technically it could be removed. Having looked at how things are in Python 3, I don't think this is a real issue. There the responses inherit some generic stream class from `io` module and therefore would have a `close` method.
https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: method, url, body=None, headers={}): On 2017/02/09 12:51:57, Sebastian Noack wrote: > Using mutable types as default arguments is rather dangerous and confusing, > since they are initialized on function definition, not on function call. > > def f(k, d={}) > d[k] = True > return d > > f('foo') => {'foo': True} > f('bar') => {'foo': True, 'bar': True} > f('foo') is f('bar') => True > > I cannot even tell whether that is a problem in practice here, without looking > at the code of HTTPSConnection.request(). But it's generally wise to just avoid > mutable types in default arguments. Here, it would even reduce the code: > > def get_response(self, connection, *args): > connection.request(*args) > ... Done. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): On 2017/02/09 20:50:23, Sebastian Noack wrote: > On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > > Shouldn't we rather check for non-2xx? In Python this can be done pretty > > easily: > > > > > > if 200 <= response.status < 300: > > > ... Original thinking is that we do not handle HTTP 3xx responses (redirects) so raise exception as if it was and error. For HTTP 1xx responses aren't really defined by HTTP standard so we just assume go along if they do happen. > > Your condition seems to check for 2xx instead of non-2xx :) > > > > Another thing is: Httplib doesn't seem to handle redirects and things like > this. > > Could this create a problem for us, if some API calls would try redirecting us > > or is this not a real possibility? Currently it doesn't create any problems. I, in fact considered not handling redirects as a good thing. If there's ever an API change I think we should know about it, rather than blindly redirecting. > > Would it be a problem if a new connection is used for each request? If not, > perhaps we should consider just using urllib2, it resolves redirects > automatically, and already raises an exception for non-2xx responses. There are no problems to create a new connection for each call AFAIK. Using one connection just feels more robust. I don't feel strong about this, but wouldn't it be better to move to Requests (or urllib3) not urllib2 then? Those support Keep-Alive at least. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/09 20:50:23, Sebastian Noack wrote: > On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > > I looked at the code of httplib, and indeed responses need to be closed. > That > > > might seem counter-intuitive, as the connection is reused, and the response > > > merely represents a chunk of data retrieved over that connection. But each > > > response has its own file descriptor, created with socket.makefile(), which > > > reads data from the underlying connection. > > > > > > You might have figured that it also works if you don't close the response. > > That > > > is because file descriptors are automatically closed when the script > > terminates, > > > and this script opens less than the maximum number of file descriptors on > your > > > operating system. Even before the script terminates, some file descriptors > may > > > be closed while the garbage collector calls the __del__ method of > unreferenced > > > file objects. However, this can quickly become a problem if this script will > > > have to deal with more file descriptors in the future, or if we should ever > > use > > > this code in the context of a long-running service which usually does not > > > terminate, or if we use a different Python implementation that does not > > support > > > __del__, or if we run it in an environment with a lower maximum number of > file > > > descriptors. So always make sure, to close any resources as soon as > possible. > > > > You're right, it does have a close() method indeed (that wasn't very obvious > > from the docs: > > https://docs.python.org/2/library/httplib.html#httpresponse-objects). However, > > looking at the source code as well, it seems that `response.read()` would > close > > the response by itself so there's no need to close it. > > The code is rather complex with a lot of special cases. But it seems the > underlying file descriptor is at least closed automatically when the complete > response has been read. So in the current patch, we still leak file descriptors, > because we not always read from the response. This should no longer happen when > we parse the returned json no matter what, as I suggested in another comment. > But I still feel that it's cleaner and more robust if we close it explicitly. It is not possible to reuse the connection object until the response is read. In the worst case scenario we may forget to read exactly one response, which implies it is not closed by a read operation. But even then it will be closed when the connection is closed. So I still don't see a reason for manual response closes. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/09 12:51:57, Sebastian Noack wrote: > It seems wherever get_repsonse() is called, and the return-value/response isn't > ignored, JSON is expected. So how about parsing the JSON here, rather than > duplicating json.loads() 4 times? There is no json response for deleting a submission and for updating it. That is why I opted for not including json.loads here. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:570: token_response) On 2017/02/09 12:51:57, Sebastian Noack wrote: > Nit: If you just call the variable "response", you don't have to wrap this line. > And since there is no other response in this context, there shouldn't be any > ambiguity. Done. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:587: file_upload_con.request('PUT', '{}?{}{}'.format( On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > Maybe combining the url fragments on a separate line would make this > construction a bit more compact and readable? Done.
Hi Ollie and Sebastian, It looks like everything is pretty much settled except for the httplib vs. urllib2 question and json.loads inside or outside get_response. I don't feel very strong about either of these but in both cases I seem to agree with Sebastian. Cheers, Vasily P.S. I'm on vacation, so I might not be very responsive. I will try to look at the reviews but if there's no word from me for too long and it looks good to Sebastian, feel free to land it. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): On 2017/02/13 02:57:36, Oleksandr wrote: > On 2017/02/09 20:50:23, Sebastian Noack wrote: > > > > Would it be a problem if a new connection is used for each request? If not, > > perhaps we should consider just using urllib2, it resolves redirects > > automatically, and already raises an exception for non-2xx responses. > > There are no problems to create a new connection for each call AFAIK. Using one > connection just feels more robust. I don't feel strong about this, but wouldn't > it be better to move to Requests (or urllib3) not urllib2 then? Those support > Keep-Alive at least. I think I would prefer to just use urllib2 too because it's in standard library and the code would be simpler. It might be a bit slower because of no keepalive, but if it works, I don't think the loss of performance would be material for this script.
https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:544: if (response.status >= 300): On 2017/02/13 02:57:36, Oleksandr wrote: > On 2017/02/09 20:50:23, Sebastian Noack wrote: > > On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > > > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > > > Shouldn't we rather check for non-2xx? In Python this can be done pretty > > > easily: > > > > > > > > if 200 <= response.status < 300: > > > > ... > > Original thinking is that we do not handle HTTP 3xx responses (redirects) so > raise exception as if it was and error. For HTTP 1xx responses aren't really > defined by HTTP standard so we just assume go along if they do happen. Well, the HTTP status codes 100, 101 and 102 are defined in the standard, and none of them is expected here. So raising an exception in that case (like urllib2 would do) makes sense. > > > Your condition seems to check for 2xx instead of non-2xx :) > > > > > > Another thing is: Httplib doesn't seem to handle redirects and things like > > this. > > > Could this create a problem for us, if some API calls would try redirecting > us > > > or is this not a real possibility? > > Currently it doesn't create any problems. I, in fact considered not handling > redirects as a good thing. If there's ever an API change I think we should know > about it, rather than blindly redirecting. Well, a redirect doesn't necessarily indicate a permanent relocation of the API. In fact there is a dedicated status code (307) specifically for temporary redirects. One example is where you get redirected to a random server so that the load is distributed between several servers. If they don't already do something like that, it might be unlikely that they will do at some point. But if you don't handle redirects you are implementing an incomplete HTTP client. > > Would it be a problem if a new connection is used for each request? If not, > > perhaps we should consider just using urllib2, it resolves redirects > > automatically, and already raises an exception for non-2xx responses. > > There are no problems to create a new connection for each call AFAIK. Using one > connection just feels more robust. I don't feel strong about this, but wouldn't > it be better to move to Requests (or urllib3) not urllib2 then? Those support > Keep-Alive at least. Well robust, seems to be the wrong word, given that status codes and redirects aren't handled automatically. Not to mention, what happens if the server closes the connection, before we are done? The problem with Requests is a largely leaky abstraction. Most notably connections are leaked in an LRU cache, until either the process terminates or the capacity of the connection pool is exceeded. urllib3 gives you control over the connection pooling at least, but it has other issues that Requests, which uses urllib3 under the hood, attempts to work around. That said, these libraries are kind of a mess, and generally I'd like to avoid third-party libraries. While reusing the connection would be nice, it doesn't seem to justify extra complexity or even third-party libraries. Also note that the Chrome Webstore upload, which is quite similar, is using urllib2 as well, and I'm quite happy with that code. So consistency might be another argument for urllib2 here. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/13 02:57:36, Oleksandr wrote: > On 2017/02/09 20:50:23, Sebastian Noack wrote: > > On 2017/02/09 16:37:58, Vasily Kuznetsov wrote: > > > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > > > I looked at the code of httplib, and indeed responses need to be closed. > > That > > > > might seem counter-intuitive, as the connection is reused, and the > response > > > > merely represents a chunk of data retrieved over that connection. But each > > > > response has its own file descriptor, created with socket.makefile(), > which > > > > reads data from the underlying connection. > > > > > > > > You might have figured that it also works if you don't close the response. > > > That > > > > is because file descriptors are automatically closed when the script > > > terminates, > > > > and this script opens less than the maximum number of file descriptors on > > your > > > > operating system. Even before the script terminates, some file descriptors > > may > > > > be closed while the garbage collector calls the __del__ method of > > unreferenced > > > > file objects. However, this can quickly become a problem if this script > will > > > > have to deal with more file descriptors in the future, or if we should > ever > > > use > > > > this code in the context of a long-running service which usually does not > > > > terminate, or if we use a different Python implementation that does not > > > support > > > > __del__, or if we run it in an environment with a lower maximum number of > > file > > > > descriptors. So always make sure, to close any resources as soon as > > possible. > > > > > > You're right, it does have a close() method indeed (that wasn't very obvious > > > from the docs: > > > https://docs.python.org/2/library/httplib.html#httpresponse-objects). > However, > > > looking at the source code as well, it seems that `response.read()` would > > close > > > the response by itself so there's no need to close it. > > > > The code is rather complex with a lot of special cases. But it seems the > > underlying file descriptor is at least closed automatically when the complete > > response has been read. So in the current patch, we still leak file > descriptors, > > because we not always read from the response. This should no longer happen > when > > we parse the returned json no matter what, as I suggested in another comment. > > But I still feel that it's cleaner and more robust if we close it explicitly. > > It is not possible to reuse the connection object until the response is read. In > the worst case scenario we may forget to read exactly one response, which > implies it is not closed by a read operation. But even then it will be closed > when the connection is closed. So I still don't see a reason for manual response > closes. Good point. So it seems, in fact, there isn't any scenario where, the response's file descriptor isn't automatically closed (if the connection is closed). Also considering that the response's close() method is undocumented in Python 2, not explicitly closing it seems reasonable, if we'd stick with httplib. https://codereview.adblockplus.org/29374637/diff/29374887/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:547: return response.read().decode() On 2017/02/13 02:57:36, Oleksandr wrote: > On 2017/02/09 12:51:57, Sebastian Noack wrote: > > It seems wherever get_repsonse() is called, and the return-value/response > isn't > > ignored, JSON is expected. So how about parsing the JSON here, rather than > > duplicating json.loads() 4 times? > > There is no json response for deleting a submission and for updating it. That is > why I opted for not including json.loads here. I see. So if json.load() would fail for delete/update actions, I guess, it makes sense to just return the plain response text, rather than magically handling that special case, here.
Patch Set: Switch to urllib2
https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (left): https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:28: from datetime import datetime Thanks for getting rid of the unused import here. But please also remove the putty-ignore for F401 in tox.ini. So that flake8/tox can make sure that we don't regress. For reference, F401 is the flake8 error for unused imports which has been whitelisted for this file, when we introduced flake8, but didn't want to cleanup all of the existing code in one go. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:478: class HTTPErrorBodyHandler(urllib2.HTTPDefaultErrorHandler): I'd put this class on the top-level rather than nesting it inside the NigthlyBuild class. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: Nit: we don't add blank lines at the top of any block. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:557: response = json.load(opener.open(request)) The response has to be closed, and this time for real, as with urllib2 each request/response uses a separate connection. with contextlib.closing(opener.open(request)) as response: data = json.load(response) https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:583: opener.open(request) Please don't forget closing the response. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:596: api_server = 'https://manage.devcenter.microsoft.com' Since you only use this variable once, perhaps inline it below? https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:606: app_obj = json.load(opener.open(request)) Again, please make sure to close the response. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:622: submission = json.load(opener.open(request)) Again, please make sure to close the response. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:630: submission['applicationPackages'].append( Perhaps cache submission['applicationPackages'] into avariable above. Then you don't need to wrap this line, and avoid some small repetition as well. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:639: added_submission['minimumDirectXVersion'] = old_directx_version Why do we modify the added_submission dictionary anyway, it seems it isn't used for anything after this line? https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:654: submission = json.load(opener.open(request)) Again, please make sure to close the response.
Patch set 6. Also updated buildtools to include #4865. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (left): https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:28: from datetime import datetime On 2017/02/15 11:20:04, Sebastian Noack wrote: > Thanks for getting rid of the unused import here. But please also remove the > putty-ignore for F401 in tox.ini. So that flake8/tox can make sure that we don't > regress. For reference, F401 is the flake8 error for unused imports which has > been whitelisted for this file, when we introduced flake8, but didn't want to > cleanup all of the existing code in one go. Done. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:478: class HTTPErrorBodyHandler(urllib2.HTTPDefaultErrorHandler): On 2017/02/15 11:20:04, Sebastian Noack wrote: > I'd put this class on the top-level rather than nesting it inside the > NigthlyBuild class. Done. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:541: On 2017/02/15 11:20:04, Sebastian Noack wrote: > Nit: we don't add blank lines at the top of any block. Done. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:557: response = json.load(opener.open(request)) On 2017/02/15 11:20:04, Sebastian Noack wrote: > The response has to be closed, and this time for real, as with urllib2 each > request/response uses a separate connection. > > with contextlib.closing(opener.open(request)) as response: > data = json.load(response) Done. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:622: submission = json.load(opener.open(request)) On 2017/02/15 11:20:04, Sebastian Noack wrote: > Again, please make sure to close the response. Done. https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:639: added_submission['minimumDirectXVersion'] = old_directx_version On 2017/02/15 11:20:05, Sebastian Noack wrote: > Why do we modify the added_submission dictionary anyway, it seems it isn't used > for anything after this line? Interesting. Initially it was required to modify the submission if we were going to upload a new file. Here I accidentally missed the submission of the updated submission and it still works as expected. I am removing the whole update submission logic then. Thanks for noticing! https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:654: submission = json.load(opener.open(request)) On 2017/02/15 11:20:04, Sebastian Noack wrote: > Again, please make sure to close the response. Done.
https://codereview.adblockplus.org/29374637/diff/29375818/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29374637/diff/29375818/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:542: auth_token = '' Defining auth_token here seems redundant. https://codereview.adblockplus.org/29374637/diff/29375818/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:564: Nit: We don't add a blank line at the beginning of a block. Please also remove other occurrences of that practice below as well. https://codereview.adblockplus.org/29374637/diff/29375818/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:586: auth_token = self.get_windows_store_access_token() This variable seems redundant, just inline it below? https://codereview.adblockplus.org/29374637/diff/29375818/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:626: new_submission_path = '{}/{}'.format( Nit: We generally prefer aligning arguments over hanging indentation. new_submission_path = '{}/{}'.format(submissions_path, submission_id)
Looks basically good to me. However, I cannot cleanly apply the patch on top of tip. Mind rebasing?
Rebased
LGTM and merged: https://hg.adblockplus.org/sitescripts/rev/9f97177512c7 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
