Merge into trunk : scheme_in_env : Code : Ubuntu Single Sign On Client

Status:	Merged
Approved by:	dobey on 2013-04-19
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/__init__.py (+4/-2) ubuntu_sso/account.py (+2/-2) ubuntu_sso/qt/sso_wizard_page.py (+4/-4) ubuntu_sso/utils/webclient/timestamp.py (+2/-2)
To merge this branch:	bzr merge lp:~elopio/ubuntu-sso-client/scheme_in_env
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
dobey (community)		2013-04-17	Approve on 2013-04-19
Review via email: mp+159513@code.launchpad.net

Commit message

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

lp:~elopio/ubuntu-sso-client/scheme_in_env updated on 2013-04-19

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.

Revision history for this message

dobey (dobey) wrote on 2013-04-19:

#

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 on 2013-04-19:

#

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 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...

<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 (jfunk@canonical-cloaked-8326895B.nyc.res.rr.com) 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 useful discussions to have about this right now :)
<elopio> I don't see it as overly complex, because it adds test coverage, and makes use of the pep8 recommendations.
<elopio> at the price of breaking an api only published on a PPA for less than a month.
<dobey> i think you're confused about what api you're breaking
<elopio> and for free to you, as I did the complex change.
<elopio> dobey: um, yes, that sounds likely. Can you explain to me what API you are talking about?
<dobey> SERVICE_URL has been there for years
<elopio> ah, yes, you are right on that.
<elopio> sorry, I need coffee.
<dobey> and function calls are an unnecessary performance hit
<mandel> elopio, in theory, if it is public to use when importing from a python module, it is an API
<dobey> even if you wanted to be more purist about pep8/constants here, global variable would be the better way to go, rather than functions
<dobey> mandel: in actuality, if it's in python, and it's installed to /usr/lib/python*, it's an API :)
<elopio> ok, in my opinion, you introduced a bug by turning the constants into variables here: https://code.launchpad.net/~dobey/ubuntu-sso-client/testable-env/+merge/156080
<elopio> you would argue that python has no constants, and that it's not worth it breaking the API.
<dobey> elopio: you're saying i introduced a bug by implementing the feature you requested, which was to fix a bug that you could no longer use the previous "constant" that was being overwritten by an environment variable, due to the rebranding changes?
<elopio> dobey: yes, and without tests :) I'm not blaming you, I appreciated it a lot and it was really useful.
<dobey> i would argue that having environment variables and such to change core functionality, regardless of implementation details, is a pain in the ass, and a security flaw
<dobey> especially when the result is to replace https:// URLs with http:// ones
<elopio> I'm just pointing it out, and trying to fix what I consider broken because of the lack of tests and the wrong use of capitals. I don't agree with your reasons for disapproval, but I can live with this feature being untested.
<dobey> elopio: please stop conflating my disapproval due to complexity as a requirement that there be no tests
<elopio> dobey: the security should be covered by the servers, not by this client that can be easily hacked. So no security risk introduced. It's just easier to hack now.
<dobey> elopio: yeah, you go argue that one to the security team :)
<dobey> "the client doesn't need to be secure because the server should cover that"
<dobey> anyway
<dobey> you don't need to convert everything to functions to be able to test it
<dobey> and testing os.environ.get() is not a useful test
<elopio> I can go and argue that, with the security team, yes :)
<elopio> and I keep disagreeing with that. I'm not testing os.environ.get, that's actually patched.
<elopio> and I think that when something is takes a variable and does something with it to return a new value, like SERVICE_URL does now, it should be a function.
<elopio> however, it seems I'm not able to convince you of that.
<dobey> there's no need to write a function that wraps a single function call
<dobey> like i said, if you want to go that route, they should be global vars, not functions
<dobey> making it a function that calls a function induces a tiny performance hit (and they can add up when that function gets references many times)
<elopio> dobey: a global var would work for service_url, not for sso_auth_host as it's scope shouldn't go outside the module.
<mandel> elopio, in this case I agree with dobey, adding that extra wrap does not really bring anything to the table and it adds the performance cost
<mandel> elopio, also, it makes the code more error prone...
<mandel> elopio, this is going to sound a little crazy, but what about simply chaging the value of the vars that dobey added for the tests per testcase
<mandel> elopio, and assert things like you are doing right now (and no functions)
<mandel> elopio, use addCleanup to set the variable back to its original value once the test has been finished
<mandel> better to use addCleanup than tearDown because it will be executed always
<dobey> if you want to refactor things, i think we can discuss that as a seaparate thing, and not include it as part of the change to use a full base url instead of only host:port for the env var
<elopio> sorry, I'm back. mandel: global vars that don't go outside the module instead of functions sound just fine.
<elopio> dobey: and I think that's the only solution, as I need the feature and I'm not able to convince.
<elopio> I'll roll-back to the simple solution, and propose a different branch.
<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 (jfunk@canonical-cloaked-8326895B.nyc.res.rr.com) 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 useful discussions to have about this right now :)
<elopio> I don't see it as overly complex, because it adds test coverage, and makes use of the pep8 recommendations.
<elopio> at the price of breaking an api only published on a PPA for less than a month.
<dobey> i think you're confused about what api you're breaking
<elopio> and for free to you, as I did the complex change.
<elopio> dobey: um, yes, that sounds likely. Can you explain to me what API you are talking about?
<dobey> SERVICE_URL has been there for years
<elopio> ah, yes, you are right on that.
<elopio> sorry, I need coffee.
<dobey> and function calls are an unnecessary performance hit
<mandel> elopio, in theory, if it is public to use when importing from a python module, it is an API
<dobey> even if you wanted to be more purist about pep8/constants here, global variable would be the better way to go, rather than functions
<dobey> mandel: in actuality, if it's in python, and it's installed to /usr/lib/python*, it's an API :)
<elopio> ok, in my opinion, you introduced a bug by turning the constants into variables here: https://code.launchpad.net/~dobey/ubuntu-sso-client/testable-env/+merge/156080
<elopio> you would argue that python has no constants, and that it's not worth it breaking the API.
<dobey> elopio: you're saying i introduced a bug by implementing the feature you requested, which was to fix a bug that you could no longer use the previous "constant" that was being overwritten by an environment variable, due to the rebranding changes?
<elopio> dobey: yes, and without tests :) I'm not blaming you, I appreciated it a lot and it was really useful.
<dobey> i would argue that having environment variables and such to change core functionality, regardless of implementation details, is a pain in the ass, and a security flaw
<dobey> especially when the result is to replace https:// URLs with http:// ones
<elopio> I'm just pointing it out, and trying to fix what I consider broken because of the lack of tests and the wrong use of capitals. I don't agree with your reasons for disapproval, but I can live with this feature being untested.
<dobey> elopio: please stop conflating my disapproval due to complexity as a requirement that there be no tests
<elopio> dobey: the security should be covered by the servers, not by this client that can be easily hacked. So no security risk introduced. It's just easier to hack now.
<dobey> elopio: yeah, you go argue that one to the security team :)
<dobey> "the client doesn't need to be secure because the server should cover that"
<dobey> anyway
<dobey> you don't need to convert everything to functions to be able to test it
<dobey> and testing os.environ.get() is not a useful test
<elopio> I can go and argue that, with the security team, yes :)
<elopio> and I keep disagreeing with that. I'm not testing os.environ.get, that's actually patched.
<elopio> and I think that when something is takes a variable and does something with it to return a new value, like SERVICE_URL does now, it should be a function.
<elopio> however, it seems I'm not able to convince you of that.
<dobey> there's no need to write a function that wraps a single function call
<dobey> like i said, if you want to go that route, they should be global vars, not functions
<dobey> making it a function that calls a function induces a tiny performance hit (and they can add up when that function gets references many times)
<elopio> dobey: a global var would work for service_url, not for sso_auth_host as it's scope shouldn't go outside the module.
<mandel> elopio, in this case I agree with dobey, adding that extra wrap does not really bring anything to the table and it adds the performance cost
<mandel> elopio, also, it makes the code more error prone...
<mandel> elopio, this is going to sound a little crazy, but what about simply chaging the value of the vars that dobey added for the tests per testcase
<mandel> elopio, and assert things like you are doing right now (and no functions)
<mandel> elopio, use addCleanup to set the variable back to its original value once the test has been finished
<mandel> better to use addCleanup than tearDown because it will be executed always
<dobey> if you want to refactor things, i think we can discuss that as a seaparate thing, and not include it as part of the change to use a full base url instead of only host:port for the env var
<elopio> sorry, I'm back. mandel: global vars that don't go outside the module instead of functions sound just fine.
<elopio> dobey: and I think that's the only solution, as I need the feature and I'm not able to convince.
<elopio> I'll roll-back to the simple solution, and propose a different branch.

lp:~elopio/ubuntu-sso-client/scheme_in_env updated on 2013-04-19

1036. By Leo Arias on 2013-04-19: Revert to rev 1029.

Revision history for this message

dobey (dobey) wrote on 2013-04-19:

#

With all the refactoring gone, this looks OK now.

review: Approve

1	=== modified file 'ubuntu_sso/__init__.py'
2	--- ubuntu_sso/__init__.py 2013-03-28 21:50:02 +0000
3	+++ ubuntu_sso/__init__.py 2013-04-19 15:34:27 +0000
4	@@ -31,8 +31,10 @@
5
6
7	# Host URL changing environment variables
8	-SSO_AUTH_HOST = os.environ.get('SSO_AUTH_HOST', 'login.ubuntu.com')
9	-SSO_UONE_HOST = os.environ.get('SSO_UONE_HOST', 'one.ubuntu.com')
10	+SSO_AUTH_BASE_URL = os.environ.get(
11	+ 'SSO_AUTH_BASE_URL', 'https://login.ubuntu.com')
12	+SSO_UONE_BASE_URL = os.environ.get(
13	+ 'SSO_UONE_BSAE_URL', 'https://one.ubuntu.com')
14
15	# DBus constants
16	DBUS_BUS_NAME = "com.ubuntu.sso"
17
18	=== modified file 'ubuntu_sso/account.py'
19	--- ubuntu_sso/account.py 2013-03-28 21:43:02 +0000
20	+++ ubuntu_sso/account.py 2013-04-19 15:34:27 +0000
21	@@ -39,7 +39,7 @@
22
23	from twisted.internet import defer
24
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 @@
31
32
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'
38
39
40	=== modified file 'ubuntu_sso/qt/sso_wizard_page.py'
41	--- ubuntu_sso/qt/sso_wizard_page.py 2013-03-28 21:50:02 +0000
42	+++ ubuntu_sso/qt/sso_wizard_page.py 2013-04-19 15:34:27 +0000
43	@@ -47,7 +47,7 @@
44	)
45	from twisted.internet import defer
46
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 (
51	ERROR_STYLE,
52	@@ -72,10 +72,10 @@
53
54
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
60	BASE_PING_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')
65
66
67	=== modified file 'ubuntu_sso/utils/webclient/timestamp.py'
68	--- ubuntu_sso/utils/webclient/timestamp.py 2013-03-28 21:50:02 +0000
69	+++ ubuntu_sso/utils/webclient/timestamp.py 2013-04-19 15:34:27 +0000
70	@@ -32,7 +32,7 @@
71
72	from twisted.internet import defer
73
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
77
78	logger = setup_logging("ubuntu_sso.utils.webclient.timestamp")
79	@@ -44,7 +44,7 @@
80
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
85
86	def __init__(self, webclient_class):
87	"""Initialize this instance."""

Ubuntu Single Sign On Client

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

Commit message

Description of the change

Preview Diff

Subscribers