Issue 29323474: Issue 2896 - Automate uploading of Firefox development builds to AMO (Closed)
|
Created:
Aug. 12, 2015, 11:45 a.m. by Wladimir Palant Modified:
Aug. 18, 2015, 9:04 a.m. Visibility:
Public. |
DescriptionI didn`t clean up the imports to keep the changeset small but I did remove the setupStderr() call.
Patch Set 1 #
Total comments: 12
Patch Set 2 : Cleaned up imports #MessagesTotal messages: 6
https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:394: def load_url(url, data=None): Any reason, why you don'T simply use urllib3.request, when relying on urllib3 anyway? https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:409: class CSRFParser(HTMLParser.HTMLParser): Don't we already use minidom somewhere else? That would be simpler to use than implementing a parser. https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:463: time.sleep(2) I suppose there is no way to get notified when it's finished? :( Can/Should we at least check whether the validation is still pending and retry in case it takes more than 2 seconds? https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:468: if upload_response['validation'].get('errors', 0): Nit: Omit the default value? None evaluates to False as well.
https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:394: def load_url(url, data=None): On 2015/08/12 13:29:26, Sebastian Noack wrote: > Any reason, why you don'T simply use urllib3.request, when relying on urllib3 > anyway? Yes, urllib3 doesn't support cookie jars. https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:409: class CSRFParser(HTMLParser.HTMLParser): On 2015/08/12 13:29:26, Sebastian Noack wrote: > Don't we already use minidom somewhere else? That would be simpler to use than > implementing a parser. Does minidom parse HTML? Not that I know. https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:463: time.sleep(2) On 2015/08/12 13:29:26, Sebastian Noack wrote: > I suppose there is no way to get notified when it's finished? :( No, it's exactly how the web interface does it - by polling. > Can/Should we at least check whether the validation is still pending and retry > in case it takes more than 2 seconds? That's exactly what we do - it's a while loop. The good news is, there is a server-side timeout, so this won't be running forever. https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:468: if upload_response['validation'].get('errors', 0): On 2015/08/12 13:29:26, Sebastian Noack wrote: > Nit: Omit the default value? None evaluates to False as well. Yes, I considered it but the default value indicates that we expect a number here.
LGTM as far as I'm concerned, with a nit. https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:28: import sys, os, os.path, subprocess, ConfigParser, json, hashlib Nit: Might be a good opportunity to clean the commits up (in regards to PEP-8), but I wouldn't insist. https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:409: class CSRFParser(HTMLParser.HTMLParser): On 2015/08/12 13:34:10, Wladimir Palant wrote: > On 2015/08/12 13:29:26, Sebastian Noack wrote: > > Don't we already use minidom somewhere else? That would be simpler to use than > > implementing a parser. > > Does minidom parse HTML? Not that I know. I've used BeautifulSoup for this kind of stuff. But as far as I'm concerned, we can leave it as it is, we're just trying to get the CSRF token after all.
https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:28: import sys, os, os.path, subprocess, ConfigParser, json, hashlib On 2015/08/13 12:52:27, Felix Dahlke wrote: > Nit: Might be a good opportunity to clean the commits up (in regards to PEP-8), > but I wouldn't insist. As I said, I intentionally didn't clean up the imports - otherwise it would obscure the actual changes. I did it in a follow-up commit now.
Message was sent while issue was closed.
LGTM https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29323474/diff/29323475/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:409: class CSRFParser(HTMLParser.HTMLParser): On 2015/08/13 12:52:27, Felix Dahlke wrote: > On 2015/08/12 13:34:10, Wladimir Palant wrote: > > On 2015/08/12 13:29:26, Sebastian Noack wrote: > > > Don't we already use minidom somewhere else? That would be simpler to use > than > > > implementing a parser. > > > > Does minidom parse HTML? Not that I know. > > I've used BeautifulSoup for this kind of stuff. But as far as I'm concerned, we > can leave it as it is, we're just trying to get the CSRF token after all. Alright, minidom is only for XML, though it also works with some HTML input. But what you actually would need is lxml which has proper (and quite error-tolerant) support of HTML. Note that BeautifulSoup is just an obscure wrapper around lxml (and some other libraries). So if we add a dependency here, I'd rather use lxml directly. However, I'm also fine with with sticking to HTMLParser, which comes with the Python corelib, rather than adding a new dependency. 
|
