Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(370)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 32890043: -

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by hazmat
Modified:
12 years, 1 month ago
Reviewers:
mp+196718, rog
Visibility:
Public.
Fixes two issues that broke manual provider usage for me, needlessly requiring of dns names instead of ips, and typo that broke non-state server api connections. https://code.launchpad.net/~hazmat/juju-core/manual-provider-fixes/+merge/196718 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 6

Patch Set 2 : - #

Created: 12 years, 1 month ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -14 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M environs/manual/provisioner.go View 1 chunk +1 line, -1 line 0 comments Download
M provider/null/instance.go View 1 2 chunks +1 line, -13 lines 0 comments Download
Total messages: 5
|
hazmat
Please take a look.
12 years, 1 month ago (2013年11月26日 14:21:52 UTC) #1
Please take a look.
Sign in to reply to this message.
rog
LGTM but we should probably have a test in environs/manual. https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go File environs/manual/provisioner.go (right): https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go#newcode185 ...
12 years, 1 month ago (2013年11月26日 14:29:13 UTC) #2
LGTM but we should probably have a test in environs/manual.
https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):
https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:185: Addrs: configParameters.APIAddrs,
this seems like it should have an associated test.
https://codereview.appspot.com/32890043/diff/1/provider/null/instance.go
File provider/null/instance.go (right):
https://codereview.appspot.com/32890043/diff/1/provider/null/instance.go#newc...
provider/null/instance.go:40: // do a reverse lookup.
this comment is no longer appropriate.
https://codereview.appspot.com/32890043/diff/1/provider/null/instance.go#newc...
provider/null/instance.go:42: return host, nil
s/host/inst.host/
Sign in to reply to this message.
hazmat
thanks for the review. https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go File environs/manual/provisioner.go (right): https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go#newcode185 environs/manual/provisioner.go:185: Addrs: configParameters.APIAddrs, On 2013年11月26日 14:29:13, ...
12 years, 1 month ago (2013年11月26日 14:52:05 UTC) #3
thanks for the review.
https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):
https://codereview.appspot.com/32890043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:185: Addrs: configParameters.APIAddrs,
On 2013年11月26日 14:29:13, rog wrote:
> this seems like it should have an associated test.
i took a look at this but it doesn't slot into the existing tests very well.
i'll ask axw to address in a followup, but for now i'd like to address the
broken provider for what is a clear logical typo.
https://codereview.appspot.com/32890043/diff/1/provider/null/instance.go
File provider/null/instance.go (right):
https://codereview.appspot.com/32890043/diff/1/provider/null/instance.go#newc...
provider/null/instance.go:40: // do a reverse lookup.
On 2013年11月26日 14:29:13, rog wrote:
> this comment is no longer appropriate.
done.
https://codereview.appspot.com/32890043/diff/1/provider/null/instance.go#newc...
provider/null/instance.go:42: return host, nil
On 2013年11月26日 14:29:13, rog wrote:
> s/host/inst.host/
done.
Sign in to reply to this message.
hazmat
Please take a look.
12 years, 1 month ago (2013年11月26日 14:52:40 UTC) #4
Please take a look.
Sign in to reply to this message.
rog
On 2013年11月26日 14:52:40, hazmat wrote: > Please take a look. still LGTM, thanks.
12 years, 1 month ago (2013年11月26日 14:54:18 UTC) #5
On 2013年11月26日 14:52:40, hazmat wrote:
> Please take a look.
still LGTM, thanks.
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /