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

Proposed by Leo Arias
Status: Merged
Approved by: dobey
Approved revision: 1036
Merged at revision: 1029
Proposed branch: lp:~elopio/ubuntu-sso-client/scheme_in_env
Merge into: lp:ubuntu-sso-client
Diff against target: 87 lines (+12/-10)
4 files modified
ubuntu_sso/ (+4/-2)
ubuntu_sso/ (+2/-2)
ubuntu_sso/qt/ (+4/-4)
ubuntu_sso/utils/webclient/ (+2/-2)
To merge this branch: bzr merge lp:~elopio/ubuntu-sso-client/scheme_in_env
Reviewer Review Type Date Requested Status
dobey (community) Approve
Review via email:

Commit message

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

To post a comment you must log in.
1034. By Leo Arias

The variable constants are no longer needed.

1035. By Leo Arias

Removed unused method.

Revision history for this message
dobey (dobey) wrote :

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
Revision history for this message
Leo Arias (elopio) wrote :
Download full text (15.2 KiB)

<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, or the branch review? :)
<elopio> dobey: it's just a small detail I got the custom to follow.
<mandel> dobey,
<elopio> 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...

1036. By Leo Arias

Revert to rev 1029.

Revision history for this message
dobey (dobey) wrote :

With all the refactoring gone, this looks OK now.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntu_sso/'
2--- ubuntu_sso/ 2013-03-28 21:50:02 +0000
3+++ ubuntu_sso/ 2013-04-19 15:34:27 +0000
4@@ -31,8 +31,10 @@
7 # Host URL changing environment variables
8-SSO_AUTH_HOST = os.environ.get('SSO_AUTH_HOST', '')
9-SSO_UONE_HOST = os.environ.get('SSO_UONE_HOST', '')
10+SSO_AUTH_BASE_URL = os.environ.get(
11+ 'SSO_AUTH_BASE_URL', '')
12+SSO_UONE_BASE_URL = os.environ.get(
13+ 'SSO_UONE_BSAE_URL', '')
15 # DBus constants
16 DBUS_BUS_NAME = "com.ubuntu.sso"
18=== modified file 'ubuntu_sso/'
19--- ubuntu_sso/ 2013-03-28 21:43:02 +0000
20+++ ubuntu_sso/ 2013-04-19 15:34:27 +0000
21@@ -39,7 +39,7 @@
23 from twisted.internet import defer
25-from ubuntu_sso import SSO_AUTH_HOST
26+from ubuntu_sso import SSO_AUTH_BASE_URL
27 from ubuntu_sso.logger import setup_logging
28 from ubuntu_sso.utils import compat, webclient
29 from ubuntu_sso.utils.webclient import restful
30@@ -47,7 +47,7 @@
33 logger = setup_logging("ubuntu_sso.account")
34-SERVICE_URL = "https://%s/api/1.0/" % SSO_AUTH_HOST
35+SERVICE_URL = "%s/api/1.0/" % SSO_AUTH_BASE_URL
36 SSO_STATUS_OK = 'ok'
37 SSO_STATUS_ERROR = 'error'
40=== modified file 'ubuntu_sso/qt/'
41--- ubuntu_sso/qt/ 2013-03-28 21:50:02 +0000
42+++ ubuntu_sso/qt/ 2013-04-19 15:34:27 +0000
43@@ -47,7 +47,7 @@
44 )
45 from twisted.internet import defer
47-from ubuntu_sso import constants, main, SSO_UONE_HOST
48+from ubuntu_sso import constants, main, SSO_UONE_BASE_URL
49 from ubuntu_sso.logger import setup_gui_logging, log_call
50 from ubuntu_sso.qt import (
52@@ -72,10 +72,10 @@
55 APP_NAME = u"Ubuntu One"
56-TC_URL = u"https://%s/terms/" % SSO_UONE_HOST
57-POLICY_URL = u"https://%s/privacy/" % SSO_UONE_HOST
58+TC_URL = u"%s/terms/" % SSO_UONE_BASE_URL
59+POLICY_URL = u"%s/privacy/" % SSO_UONE_BASE_URL
61- u"https://%s/oauth/sso-finished-so-get-tokens/{email}" % SSO_UONE_HOST
62+ u"%s/oauth/sso-finished-so-get-tokens/{email}" % SSO_UONE_BASE_URL
63 # the result of platform_data is given by urlencode, encoded with ascii
64 PING_URL = BASE_PING_URL + u"?" + platform_data().decode('ascii')
67=== modified file 'ubuntu_sso/utils/webclient/'
68--- ubuntu_sso/utils/webclient/ 2013-03-28 21:50:02 +0000
69+++ ubuntu_sso/utils/webclient/ 2013-04-19 15:34:27 +0000
70@@ -32,7 +32,7 @@
72 from twisted.internet import defer
74-from ubuntu_sso import SSO_UONE_HOST
75+from ubuntu_sso import SSO_UONE_BASE_URL
76 from ubuntu_sso.logger import setup_logging
78 logger = setup_logging("ubuntu_sso.utils.webclient.timestamp")
79@@ -44,7 +44,7 @@
81 CHECKING_INTERVAL = 60 * 60 # in seconds
82 ERROR_INTERVAL = 30 # in seconds
83- SERVER_IRI = u"http://%s/api/time" % SSO_UONE_HOST
84+ SERVER_IRI = u"%s/api/time" % SSO_UONE_BASE_URL
86 def __init__(self, webclient_class):
87 """Initialize this instance."""


People subscribed via source and target branches