Merge lp:~leonardr/launchpadlib/bug-714043 into lp:launchpadlib

Proposed by Leonard Richardson on 2011-02-07
Status: Rejected
Rejected by: Martin Pool on 2011-05-31
Proposed branch: lp:~leonardr/launchpadlib/bug-714043
Merge into: lp:launchpadlib
Diff against target: 169 lines (+59/-39)
3 files modified
src/launchpadlib/NEWS.txt (+13/-0)
src/launchpadlib/ (+1/-1)
src/launchpadlib/ (+45/-38)
To merge this branch: bzr merge lp:~leonardr/launchpadlib/bug-714043
Reviewer Review Type Date Requested Status
Martin Pool (community) 2011-02-07 Disapprove on 2011-03-22
Review via email:

Description of the change

This branch fixes bug 71403 with two little bits of code:

1. If you call login_with() without specifying a service_root, you get 'staging', but you also get a warning. This lets you avoid mistakes in your code, but makes it obvious what you need to do if you want your changes to persist.

2. If you call login_anonymously() without specifying a service_root, you get 'production'.

Martin, let me know if you like this solution.

To post a comment you must log in.
Martin Pool (mbp) wrote :

Hi Leonard, thanks for picking this up.

Can we think of any case in which the user would thank us for defaulting to talking to staging? I think the only case would be if they start up a client and make a lot of changes just assuming it has no effect. I don't know why someone would think that; fairly obviously the library is for talking to Launchpad and has the effect of manipulating objects in Launchpad.

If somebody is randomly poking things, they'll be doing so under their own account and they'll suffer the consequences.

If we default to staging then the first thing people are likely to do is to override it to talk to the real site. People want to write code that actually manipulates Launchpad. If they have bugs, those bugs are going to affect the real lp.

Consistent behaviour between anonymous and authenticated access is desirable, all else being equal.

So, I think it should just default to production.

We can look for precedents in other libraries. I've never encountered one that doesn't talk to the production site by default.

Leonard Richardson (leonardr) wrote :

I guess it depends on the development style. When I write a launchpadlib script, I run it against staging until it's debugged, and the last thing I do is switch it to production. This branch would kind of annoy me, but I'd work around it by explicitly passing in service_root='staging' to suppress the warning. If someone else's style is to switching to production immediately, under this branch they'll at least be reminded that they need to do that.

"If somebody is randomly poking things, they'll be doing so under their own account and they'll suffer the consequences." "If they have bugs, those bugs are going to affect the real lp." You're saying these things in a very matter-of-fact way, but it doesn't have to be this way. These are reasons for not defaulting to staging.

I'm not aware of any service-specific clients that can talk to staging servers *at all*, but I'm not up to date on these things. Most web service providers don't publish their staging servers.

I'm happy to put this up for a vote on launchpad-users.

Robert Collins (lifeless) wrote :

I think gathering some user input would be good; perhaps a -users
thread as you suggest.

Martin Pool (mbp) wrote :

On 9 February 2011 08:19, Robert Collins <email address hidden> wrote:
> I think gathering some user input would be good; perhaps a -users
> thread as you suggest.

+1, maybe also forward it to canonical-tech.

Martin Pool (mbp) wrote :

We did have a thread, in which there was a mix of people agreeing it was confusing or annoying, and people saying they think it is nice to have a "safe" mode by default.

Also, since this was first proposed, staging has been down a lot (whereas lpnet has generally been quite reliable), and it seems to me it is definitely moving towards being a non-production-quality internal-use system, and not something users ought to rely on.

(In the interim, Leonard has moved on to other projects and I'm not sure he'll chase this up, but I'm going to comment here for other people who might look at the bug or mp. I may pick this up myself eventually.)

So my overall vote, for what it's worth, is to reject this and to make lplib actually default to the real service. Clients must write code that is adequately safe when it's actually used.

review: Disapprove
Martin Pool (mbp) wrote :

For the sake of getting this off my queue and making a decision, I'm going to reject it. If someone else wants to pick it up, or even repropose this branch, please do. I hope we still fix this bug.

Unmerged revisions

117. By Leonard Richardson on 2011-02-07

Added NEWS.

116. By Leonard Richardson on 2011-02-07

Initial implementation.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/launchpadlib/NEWS.txt'
2--- src/launchpadlib/NEWS.txt 2011-01-18 21:57:09 +0000
3+++ src/launchpadlib/NEWS.txt 2011-02-07 20:53:52 +0000
4@@ -2,6 +2,19 @@
5 NEWS for launchpadlib
6 =====================
8+1.9.5 (2010-02-07)
11+- Fixed a bug that prevented the deprecated get_token_and_login code
12+ from working, and required that users of get_token_and_login get a
13+ new token on every usage.
15+- Added a warning to users when their code runs against staging and
16+ they might not realize it.
18+- By default, anonymous read-only access runs against the production
19+ server, since there's no chance of the client making mistakes.
21 1.9.4 (2010-01-18)
22 ==================
25=== modified file 'src/launchpadlib/'
26--- src/launchpadlib/ 2011-01-18 22:33:53 +0000
27+++ src/launchpadlib/ 2011-02-07 20:53:52 +0000
28@@ -14,4 +14,4 @@
29 # You should have received a copy of the GNU Lesser General Public License
30 # along with launchpadlib. If not, see <>.
32-__version__ = '1.9.4'
33+__version__ = '1.9.5'
35=== modified file 'src/launchpadlib/'
36--- src/launchpadlib/ 2011-01-18 21:57:09 +0000
37+++ src/launchpadlib/ 2011-02-07 20:53:52 +0000
38@@ -31,6 +31,7 @@
39 ScalarValue, # Re-import for client convenience
40 ServiceRoot,
41 )
42+from lazr.restfulclient.authorize.oauth import SystemWideConsumer
43 from lazr.restfulclient._browser import RestfulHttp
44 from launchpadlib.credentials import (
45 AccessToken,
46@@ -309,16 +310,18 @@
47 get_token_and_login() is removed, this code can be streamlined
48 and moved into its other call site, login_with().
49 """
51 if isinstance(consumer_name, Consumer):
52- # Create the credentials with no Consumer, then set its .consumer
53- # property directly.
54- credentials = Credentials(None)
55- credentials.consumer = consumer_name
56+ consumer = consumer_name
57 else:
58- # Have the Credentials constructor create the Consumer
59- # automatically.
60- credentials = Credentials(consumer_name)
61+ # Create a system-wide consumer. lazr.restfulclient won't
62+ # do this automatically, but launchpadlib's default is to
63+ # do a desktop-wide integration.
64+ consumer = SystemWideConsumer(consumer_name)
66+ # Create the credentials with no Consumer, then set its .consumer
67+ # property directly.
68+ credentials = Credentials(None)
69+ credentials.consumer = consumer
70 if authorization_engine is None:
71 authorization_engine = cls.authorization_engine_factory(
72 service_root, consumer_name, None, allow_access_levels)
73@@ -335,13 +338,30 @@
74 credential_store.credential_save_failed,
75 "credential_store")
77- credentials = authorization_engine(credentials, credential_store)
78+ # Try to get the credentials out of the credential store.
79+ cached_credentials = credential_store.load(
80+ authorization_engine.unique_consumer_id)
81+ if cached_credentials is None:
82+ # They're not there. Acquire new credentials using the
83+ # authorization engine.
84+ credentials = authorization_engine(credentials, credential_store)
85+ else:
86+ # We acquired credentials. But, the application name
87+ # wasn't stored along with the credentials, because in a
88+ # desktop integration scenario, a single set of
89+ # credentials may be shared by many applications. We need
90+ # to set the application name for this specific instance
91+ # of the credentials.
92+ credentials = cached_credentials
93+ credentials.consumer.application_name = (
94+ authorization_engine.application_name)
96 return cls(credentials, authorization_engine, credential_store,
97 service_root, cache, timeout, proxy_info, version)
99 @classmethod
100 def login_anonymously(
101- cls, consumer_name, service_root=uris.STAGING_SERVICE_ROOT,
102+ cls, consumer_name, service_root=uris.LPNET_SERVICE_ROOT,
103 launchpadlib_dir=None, timeout=None, proxy_info=None,
104 version=DEFAULT_VERSION):
105 """Get access to Launchpad without providing any credentials."""
106@@ -355,7 +375,7 @@
108 @classmethod
109 def login_with(cls, application_name=None,
110- service_root=uris.STAGING_SERVICE_ROOT,
111+ service_root=None,
112 launchpadlib_dir=None, timeout=None, proxy_info=None,
113 authorization_engine=None, allow_access_levels=None,
114 max_failed_attempts=None, credentials_file=None,
115@@ -457,6 +477,15 @@
116 :rtype: `Launchpad`
118 """
119+ if service_root is None:
120+ service_root = uris.STAGING_SERVICE_ROOT
121+ staging_warning = (
122+ "Running against the Launchpad staging server. Changes you "
123+ "make will be reverted, and the data you read may be "
124+ "slightly out of date. To run against production, pass "
125+ 'service_root="production" into login_with().')
126+ warnings.warn(staging_warning)
128 (service_root, launchpadlib_dir, cache_path,
129 service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
131@@ -513,33 +542,11 @@
132 "allow_access_levels", allow_access_levels,
133 authorization_engine.allow_access_levels)
135- credentials = credential_store.load(
136- authorization_engine.unique_consumer_id)
138- if credentials is None:
139- # Credentials were not found in the local store. Go
140- # through the authorization process.
141- launchpad = cls._authorize_token_and_login(
142- authorization_engine.consumer, service_root,
143- cache_path, timeout, proxy_info, authorization_engine,
144- allow_access_levels, credential_store,
145- credential_save_failed, version)
146- else:
147- # The application name wasn't stored locally, because in a
148- # desktop integration scenario, a single set of
149- # credentials may be shared by many applications. We need
150- # to set the application name for this specific instance
151- # of the credentials.
152- credentials.consumer.application_name = (
153- authorization_engine.application_name)
155- # Credentials were found in the local store. Create a
156- # Launchpad object.
157- launchpad = cls(
158- credentials, authorization_engine, credential_store,
159- service_root=service_root, cache=cache_path, timeout=timeout,
160- proxy_info=proxy_info, version=version)
161- return launchpad
162+ return cls._authorize_token_and_login(
163+ authorization_engine.consumer, service_root,
164+ cache_path, timeout, proxy_info, authorization_engine,
165+ allow_access_levels, credential_store,
166+ credential_save_failed, version)
168 @classmethod
169 def _warn_of_deprecated_login_method(cls, name):


People subscribed via source and target branches