Merge lp:~elopio/ubuntu-sso-client/scheme_in_env into lp:ubuntu-sso-client

[Leo Arias]

Changed the environment variables for sso auth and uone from hosts to urls.

[lp:~elopio/ubuntu-sso-client/scheme_in_env]

1034. By Leo Arias on 2013-04-19

The variable constants are no longer needed.

1035. By Leo Arias on 2013-04-19

Removed unused method.

[dobey (dobey)]

This is an overly complex change in order to accomplish the desired outcome. I see no need to do all this extra work by moving things to functions rather than constants (which may be imported/used by other applications and libraries as well). There's no need to break API to support the environment variable including the URL scheme as well as the host and port.

review: Disapprove

[Leo Arias (elopio)]

<dobey> elopio: why did you make it so complicated?
<mandel> elopio, stupid comment, but 2011-2013 is good enough, right dobey?
dobey> mandel: eh?
<mandel> dobey, branch from elopio line 132 in the diff
<dobey> elopio: individual years is useful if some years were skipped. like "2008, 2012-2013" if nothing was changed in 2009 or 2010
<dobey> elopio: or if multiple contributors have different copyright in different years
<dobey> elopio: but since all copyright goes to canonical for most all of our projects, it's not necessary
<dobey> mandel: about gnu.org, or the branch review? :)
<elopio> dobey: it's just a small detail I got the custom to follow.
<mandel> dobey, gnu.org
<elopio> http://www.gnu.org/licenses/gpl-howto.html. It says that there must be an explicit satement about the usage of year ranges.
<elopio> but It shouldn't be a big deal.
<elopio> if you prefer, I can roll back to year ranges.
<dobey> elopio: well, i voted disapprove on the branch, because i think it's a whole lot of unnecessary change for what you want to accomplish
<elopio> dobey: I disagree, as the original approach treats as constants things that can vary depending on the environment.
<elopio> so, they are not constants.
<elopio> and it's missing the tests for the feature to override urls with env vars.
<dobey> python doesn't have constants
<dobey> missing tests is one thing. breaking API and turning everything into functions is unnecessary, even to test the things (though i don't think we should be writing tests to check that python's __dict__.get() method works correctly
<elopio> dobey: if you follow pep8, a variable all in capitals means a constant.
--> jfunk (<email address hidden>) has joined #u1-client
<elopio> dobey: I'm not testing pythons dict, I'm testing the environment variables.
<elopio> and I consider that my branch just changes the behavior introduced less than a month ago.
<dobey> it says "constants are usually" …
<elopio> dobey: and it's the only thing it recommends to use all in capitals.
<elopio> and for global variables it says, "Let's hope that these variables are meant for use inside one module only."
<dobey> regardless. there are no such things as constants in python. in normal usage the values of these constants will not change.
<elopio> dobey: I agree with the first part. Not with the second. A constant that sometimes changes, I think it's not a constant. And as variables, they should be lower case, and it's discouraged to import them in a different module. Thus, a function is the way to go, I think.
<dobey> that's irrelevant. it's still python, and in python there are no such things as constants
<dobey> and it's irrelevant to the complexity of change
<dobey> your change is overly complex for the desired change, and breaks existing API
<elopio> dobey: I can just roll back to rev 1029 of my branch to turn your disapproval, but I enjoy the discussion. I hope you don't see me as a pain in the ass for this, though I know the chances are high :D
<elopio> dobey: I agree it's more complex that just changing the strinsg.
<dobey> arguing about definition of constant in relation to python, and purity of following pep8 aren't usef...

[lp:~elopio/ubuntu-sso-client/scheme_in_env]

1036. By Leo Arias on 2013-04-19

Revert to rev 1029.

[dobey (dobey)]

With all the refactoring gone, this looks OK now.

review: Approve

=== modified file 'ubuntu_sso/__init__.py'
--- ubuntu_sso/__init__.py	2013-03-28 21:50:02 +0000
+++ ubuntu_sso/__init__.py	2013-04-19 15:34:27 +0000
@@ -31,8 +31,10 @@
 
 
 # Host URL changing environment variables
-SSO_AUTH_HOST = os.environ.get('SSO_AUTH_HOST', 'login.ubuntu.com')
-SSO_UONE_HOST = os.environ.get('SSO_UONE_HOST', 'one.ubuntu.com')
+SSO_AUTH_BASE_URL = os.environ.get(
+    'SSO_AUTH_BASE_URL', 'https://login.ubuntu.com')
+SSO_UONE_BASE_URL = os.environ.get(
+    'SSO_UONE_BSAE_URL', 'https://one.ubuntu.com')
 
 # DBus constants
 DBUS_BUS_NAME = "com.ubuntu.sso"
 
=== modified file 'ubuntu_sso/account.py'
--- ubuntu_sso/account.py	2013-03-28 21:43:02 +0000
+++ ubuntu_sso/account.py	2013-04-19 15:34:27 +0000
@@ -39,7 +39,7 @@
 
 from twisted.internet import defer
 
-from ubuntu_sso import SSO_AUTH_HOST
+from ubuntu_sso import SSO_AUTH_BASE_URL
 from ubuntu_sso.logger import setup_logging
 from ubuntu_sso.utils import compat, webclient
 from ubuntu_sso.utils.webclient import restful
@@ -47,7 +47,7 @@
 
 
 logger = setup_logging("ubuntu_sso.account")
-SERVICE_URL = "https://%s/api/1.0/" % SSO_AUTH_HOST
+SERVICE_URL = "%s/api/1.0/" % SSO_AUTH_BASE_URL
 SSO_STATUS_OK = 'ok'
 SSO_STATUS_ERROR = 'error'
 
 
=== modified file 'ubuntu_sso/qt/sso_wizard_page.py'
--- ubuntu_sso/qt/sso_wizard_page.py	2013-03-28 21:50:02 +0000
+++ ubuntu_sso/qt/sso_wizard_page.py	2013-04-19 15:34:27 +0000
@@ -47,7 +47,7 @@
 )
 from twisted.internet import defer
 
-from ubuntu_sso import constants, main, SSO_UONE_HOST
+from ubuntu_sso import constants, main, SSO_UONE_BASE_URL
 from ubuntu_sso.logger import setup_gui_logging, log_call
 from ubuntu_sso.qt import (
     ERROR_STYLE,
@@ -72,10 +72,10 @@
 
 
 APP_NAME = u"Ubuntu One"
-TC_URL = u"https://%s/terms/" % SSO_UONE_HOST
-POLICY_URL = u"https://%s/privacy/" % SSO_UONE_HOST
+TC_URL = u"%s/terms/" % SSO_UONE_BASE_URL
+POLICY_URL = u"%s/privacy/" % SSO_UONE_BASE_URL
 BASE_PING_URL = \
-    u"https://%s/oauth/sso-finished-so-get-tokens/{email}" % SSO_UONE_HOST
+    u"%s/oauth/sso-finished-so-get-tokens/{email}" % SSO_UONE_BASE_URL
 # the result of platform_data is given by urlencode, encoded with ascii
 PING_URL = BASE_PING_URL + u"?" + platform_data().decode('ascii')
 
 
=== modified file 'ubuntu_sso/utils/webclient/timestamp.py'
--- ubuntu_sso/utils/webclient/timestamp.py	2013-03-28 21:50:02 +0000
+++ ubuntu_sso/utils/webclient/timestamp.py	2013-04-19 15:34:27 +0000
@@ -32,7 +32,7 @@
 
 from twisted.internet import defer
 
-from ubuntu_sso import SSO_UONE_HOST
+from ubuntu_sso import SSO_UONE_BASE_URL
 from ubuntu_sso.logger import setup_logging
 
 logger = setup_logging("ubuntu_sso.utils.webclient.timestamp")
@@ -44,7 +44,7 @@
 
     CHECKING_INTERVAL = 60 * 60  # in seconds
     ERROR_INTERVAL = 30  # in seconds
-    SERVER_IRI = u"http://%s/api/time" % SSO_UONE_HOST
+    SERVER_IRI = u"%s/api/time" % SSO_UONE_BASE_URL
 
     def __init__(self, webclient_class):
         """Initialize this instance."""