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

Issue 12235043: juju status: add service/unit filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 9 months ago by axw
Modified:
8 years, 8 months ago
Reviewers:
gz, axw1, mp+178004, fwereade, rog
Visibility:
Public.

Description

juju status: add service/unit filters Any positional arguments after juju status are now interpreted as filters for either service or unit. If the filter includes a '/', then it is considered a unit filter, else a service filter. Service filters are transformed to unit filters by appending '/*'. A unit passes a filter if one of the following conditions is true: - Its name matches the pattern. - The unit is a principal, and one of its subordinates matches the pattern. - The unit is a subordinate, and its principal matches the pattern. If all units of a service are filtered out, then the service is filtered out; if all units of a machine are filtered out, the machine is filtered out (except to fill out the container hierarchy). Unlike in pyjuju, this implementation allows only the '*' wildcard in patterns. '?' and character ranges are not permitted. Partially fixes bug 1121916 https://code.launchpad.net/~axwalk/juju-core/lp1121916-juju-status-filters/+merge/178004 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : juju status: add service/unit filters #

Total comments: 2

Patch Set 3 : juju status: add service/unit filters #

Total comments: 23

Patch Set 4 : juju status: add service/unit filters #

Total comments: 8

Patch Set 5 : juju status: add service/unit filters #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+631 lines, -18 lines) Patch
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/cmd_test.go View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M cmd/juju/status.go View 1 2 3 4 10 chunks +178 lines, -14 lines 1 comment Download
M cmd/juju/status_test.go View 1 2 3 4 8 chunks +393 lines, -3 lines 0 comments Download
M state/unit.go View 1 2 3 4 1 chunk +6 lines, -0 lines 1 comment Download
M state/unit_test.go View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download
A testing/repo/series/monitoring/metadata.yaml View 1 2 1 chunk +16 lines, -0 lines 0 comments Download
M testing/repo/series/wordpress/metadata.yaml View 1 2 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 18
axw
Please take a look.
8 years, 9 months ago (2013-08-01 06:24:33 UTC) #1
gz
Rietveld has screwed the diff, you'll need to run `lbox propose -cr -v` again which ...
8 years, 9 months ago (2013-08-01 15:39:18 UTC) #2
axw1
On 2013/08/01 15:39:18, gz wrote: > Rietveld has screwed the diff, you'll need to run ...
8 years, 9 months ago (2013-08-02 02:31:15 UTC) #3
axw
Please take a look.
8 years, 9 months ago (2013-08-02 02:34:16 UTC) #4
fwereade
Thanks, this is a great start, but we need to think about subordinates in a ...
8 years, 9 months ago (2013-08-02 16:25:03 UTC) #5
axw
Please take a look.
8 years, 9 months ago (2013-08-05 10:18:34 UTC) #6
axw1
https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go#newcode113 cmd/juju/status.go:113: mid, err := unit.AssignedMachineId() On 2013/08/02 16:25:03, fwereade wrote: ...
8 years, 9 months ago (2013-08-05 10:19:11 UTC) #7
rog
I like the way this is going - I think the matching semantics are nice ...
8 years, 9 months ago (2013-08-05 14:23:01 UTC) #8
axw1
https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode29 cmd/juju/status.go:29: scope []string On 2013/08/05 14:23:02, rog wrote: > "scope" ...
8 years, 9 months ago (2013-08-06 04:06:09 UTC) #9
axw
Please take a look.
8 years, 9 months ago (2013-08-06 04:08:20 UTC) #10
rog
LGTM with the second fix below, and one other passing thought. https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go File cmd/juju/status.go (right): ...
8 years, 9 months ago (2013-08-07 17:02:30 UTC) #11
fwereade
LGTM, thanks -- sorry for the delay, it's a busy week. https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go File cmd/juju/status.go (right): ...
8 years, 9 months ago (2013-08-07 17:06:53 UTC) #12
axw1
On 2013/08/07 17:02:30, rog wrote: > LGTM with the second fix below, and one other ...
8 years, 9 months ago (2013-08-08 02:28:58 UTC) #13
axw1
On 2013/08/07 17:06:53, fwereade wrote: > LGTM, thanks -- sorry for the delay, it's a ...
8 years, 9 months ago (2013-08-08 02:46:27 UTC) #14
axw
Please take a look.
8 years, 9 months ago (2013-08-08 03:06:08 UTC) #15
fwereade
On 2013/08/08 02:46:27, axw1 wrote: > On 2013/08/07 17:06:53, fwereade wrote: > https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#newcode170 > > ...
8 years, 9 months ago (2013-08-08 07:40:59 UTC) #16
rog
still LGTM with two final thoughts. https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go#newcode137 cmd/juju/status.go:137: const valid = ...
8 years, 9 months ago (2013-08-08 07:52:42 UTC) #17
axw1
8 years, 9 months ago (2013-08-08 08:13:14 UTC) #18
On 2013/08/08 07:52:42, rog wrote:
> still LGTM with two final thoughts.
> 
> https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go
> File cmd/juju/status.go (right):
> 
>
https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go#newcode137
> cmd/juju/status.go:137: const valid = "abcdefghijklmnopqrstuvwxyz0123456789-*"
> or:
> 
> var validPattern = regexp.MustCompile("^[a-z0-9-*]$")
> 
> and use if !validPattern.MatchString(s) instead of validatePattern.
> 
> which would make it easier to add extra stuff if, for instance,
> we started to allow unicode characters in service names.

Fair enough, done.

> https://codereview.appspot.com/12235043/diff/42001/state/unit.go
> File state/unit.go (right):
> 
> https://codereview.appspot.com/12235043/diff/42001/state/unit.go#newcode96
> state/unit.go:96: var _ AgentEntity = (*Unit)(nil)
> This has moved into interface.go now.

Thanks, merged.
Sign in to reply to this message.

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