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

Issue 101810046: Allow charms without a peer listed.

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by jimmiebtlr
Modified:
9 years, 11 months ago
Reviewers:
mp+221156, fwereade, hduran
Visibility:
Public.

Description

Allow charms without a peer listed. https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charms/+merge/221156 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1

Patch Set 2 : Allow charms without a peer listed. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -17 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M charm/meta.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M charm/meta_test.go View 1 5 chunks +15 lines, -6 lines 1 comment Download
M schema/schema.go View 4 chunks +20 lines, -0 lines 0 comments Download
M schema/schema_test.go View 4 chunks +9 lines, -9 lines 0 comments Download
A testing/repo/quantal/empty-fields/metadata.yaml View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 5
jimmiebtlr
Please take a look.
9 years, 11 months ago (2014-05-28 05:33:29 UTC) #1
fwereade
LGTM apart from the serialisation change. What prompted that? https://codereview.appspot.com/101810046/diff/1/charm/meta.go File charm/meta.go (right): https://codereview.appspot.com/101810046/diff/1/charm/meta.go#newcode101 charm/meta.go:101: ...
9 years, 11 months ago (2014-05-28 09:55:54 UTC) #2
jimmiebtlr
Please take a look.
9 years, 11 months ago (2014-05-29 01:55:10 UTC) #3
fwereade
LGTM, I'm approving this despite the unnecessary lines in the test. A followup that just ...
9 years, 11 months ago (2014-06-02 09:11:54 UTC) #4
hduran
9 years, 11 months ago (2014-06-02 14:23:21 UTC) #5
On 2014/06/02 09:11:54, fwereade wrote:
> LGTM, I'm approving this despite the unnecessary lines in the test. A followup
> that just dropped those would be very much appreciated.
> 
> https://codereview.appspot.com/101810046/diff/20001/charm/meta_test.go
> File charm/meta_test.go (right):
> 
>
https://codereview.appspot.com/101810046/diff/20001/charm/meta_test.go#newcod...
> charm/meta_test.go:407: empty_input.Requires = nil
> Aren't these nil already?

LGTM Altough for the records I would appreciate a more detailed description for
the
merge proposal.
Sign in to reply to this message.

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