[patch] slave: Make URL parsing more robust

[anatoly techtonik <techtonik@…>]

Currently, if you've mistaken build and supply invalid http://bitten.edgewall.org/build URL, bitten-slave will timeout instead of failing.

>>> url = 'http://bitten.edgewall.org/build'
>>> url[:-7]
'http://bitten.edgewall.or'
>>> url.rsplit('/', 1)[0]
'http://bitten.edgewall.org'
>>>

The patch fixes this behaviour:

bitten/slave.py

@@ -306 +306 @@
             try:
                 try:
                     if self.username and not self.auth_map.get(url):
-                        login_url = '%s/login?referer=%s' % (url[:-7],
-                                                        urllib.quote_plus(url))
+                        login_url = '%s/login?referer=%s' % (
+                                 url.rsplit('/', 1)[0], urllib.quote_plus(url))
                         # First request to url, authentication needed
                         if self.form_auth:
                             log.debug('Performing http form authentication')

[hodgestar]

Patch to automatically add '/builds' if it is missing from URLs.

[hodgestar]

I've added a patch that checks non-local URLs for whether they end in '/builds' and adds '/builds' if it isn't found. This allows one to use either '​http://project' or '​http://project/builds' when calling bitten-slave. If people are happy with the patch I'll update the documentation before committing. At the moment passing '​http://project/' results in '​http://projects//builds' which works for me but is perhaps less clean that without the second double slash. I'm +-0 on adding the extra code to ensure we end up with just one slash.

It was pointed out by osimons that #587 is a duplicate of this.

[hodgestar]

P.S. My patch does ensure that Bitten never contacts the wrong domain regardless of how bad the URL is. The final URL used is available via 'bitten-slave -v' as always.

[osimons]

Just use url.rstrip('/') + '/builds' to remove double slashes. Also, seems a bit overkill to del urls - we don't really go deleting all the other locals after we've done with them either?

Anyway, the idea is good i think, and would allow just pointing slave to a project. Of course, the typo that triggered the ticket will still be wrong (now /build/builds) but wrong urls from user input can happen anytime and anywhere and is beyond our control.

[hodgestar]

Modified patch include osimons' suggestions committed as r995 and backported to 0.6.x in r996.

[anatoly techtonik <techtonik@…>]

While this code wins +1 for doing the right thing and +1 for brevity, it still loses -3 on clarity. If I was the reviewer, I'd add obligatory comment. =)

slave.py

@@ -227 +227 @@
         if self.local:
             self.urls = urls
         else:
+            # add /builds suffix if missing
             self.urls = [
                 not url.endswith('/builds') and url.rstrip('/') + '/builds'
                 or url for url in urls]

[anatoly techtonik <techtonik@…>]

Anyway, thank for fruitful collaboration (even though the change minor). =)