Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(669)

Issue 88410043: cmd/juju/endpoint: return cached api-endpoints

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by jameinel
Modified:
10 years ago
Reviewers:
mp+216058, axw
Visibility:
Public.

Description

cmd/juju/endpoint: return cached api-endpoints We already do all the work of caching the information, just return it rather than looking things up in the provider again. The main win here is that "juju api-endpoints" no longer needs provider credentials. There are 2 other bits changing which I'm happy to revert if we feel we don't want to change it at this time. 1. Our code for caching the first time *only* caches the value that we found by looking up the APIInfo in the provider. However, since we just connected to the API we can just use that information. I kept the old code for backwards compatibility with servers that didn't return HostPorts. Though that might not be 1.18, in which case we *could* rip out all that logic. 2. 'juju api-endpoints' used to always lookup the information from the provider. Now it defaults to just spitting out what was cached, though you can supply --refresh to make it connect first. Time wise: 3.396s juju-1.18.1 api-endpoints 1.363s juju-proposed api-endpoints --refresh 0.140s juju-proposed api-endpoints I went ahead and left it with no refresh by default, because we honestly don't think these things are going to be changing much. But I could live with always connecting, and it would simplify the code slightly. (Takes out approximately 1 if() clause and the commandline parameter.) https://code.launchpad.net/~jameinel/juju-core/api-endpoints-from-cache-1268470/+merge/216058 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -49 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/endpoint.go View 2 chunks +9 lines, -16 lines 0 comments Download
M juju/api.go View 7 chunks +62 lines, -22 lines 1 comment Download
M juju/apiconn_test.go View 3 chunks +106 lines, -11 lines 0 comments Download
M juju/export_test.go View 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 2
jameinel
Please take a look.
10 years ago (2014-04-16 10:33:22 UTC) #1
axw
10 years ago (2014-04-17 06:30:30 UTC) #2
LGTM

https://codereview.appspot.com/88410043/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/88410043/diff/1/juju/api.go#newcode394
juju/api.go:394: func APIEndpointFor(envName string, refresh bool)
(configstore.APIEndpoint, error) {
The API endpoint is always "for" something, I don't think the function name
really needs it.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b