Code review comment for lp:~axwalk/juju-core/lp1121916-juju-status-filters
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
35a32c5
(Get the code!)
https:/
File cmd/juju/status.go (right):
https:/
cmd/juju/
On 2013/08/05 14:23:02, rog wrote:
> "scope" seems like a slightly odd name here.
> "patterns" perhaps?
Done. I was following pyjuju, but I agree that it sounds slightly odd.
https:/
cmd/juju/
runtime state of various system entities."
On 2013/08/05 14:23:02, rog wrote:
> I think we should add some documentation on the pattern-matching
behaviour here.
Done.
https:/
cmd/juju/
subordinateTo string) (bool, error) {
On 2013/08/05 14:23:02, rog wrote:
> I'm not sure we need the subordinateTo argument here.
> A unit already knows its own principal unit; we
> could either use u.DeployerTag to find it out,
> or implement a trivial Unit.PrincipalName
> function in state.
Good point, I've added Unit.PrincipalName.
https:/
cmd/juju/
On 2013/08/05 14:23:02, rog wrote:
> we usually use a colon only for a following error.
> perhaps:
> return false, fmt.Errorf("pattern syntax error in %q", pattern)
> ?
Done.
https:/
cmd/juju/
*unitMatcher) {
On 2013/08/05 14:23:02, rog wrote:
> It'd be nice to see a comment explaining the purpose of this function
here.
> Rather than have the lazy initialisation, it might be
> nicer to have something like this:
> // newUnitMatcher returns a unitMatcher
> // that matches units with the specified patterns.
> func newUnitMatcher(
> // matchesAll reports whether the unit matcher
> // matches any unit.
> func (um *unitMatcher) matchesAny() bool
> Then in fetchAllService
> um.matchesAny rather than using the somewhat
> oblique um!=nil test (assuming I have the
> intent correct)
> Alternatively continue to use the ==nil test
> and only call newUnitMatcher if len(patterns)>0.
Done.
https:/
cmd/juju/
On 2013/08/05 14:23:02, rog wrote:
> I think we should try to avoid calling for subordinate units, because
we have
> all the information in the principal units.
> Since we are guaranteed to have a principal unit if the unit matcher
has matched
> its subordinate, we could just do:
> if unit.IsPrincipal() {
> mid, err := unit.AssignedMa
> // The only error we can get from AssignedMachineId
> // on a principal unit is if there is no machine id
> // assigned to the unit.
> if err == nil {
> machineIds.Add(mid)
> }
> }
> You might want to move some or all the logic in this um!=nil
> block into a separate function (see also my suggestion in
fetchMachines
> below)
Done, created a a new function fetchUnitMachin
The !=nil check is replaced with matchesAny, but remains here (in Run).
Passing the matcher to fetchUnitMachineIds seemed a bit awkward.
https:/
cmd/juju/
On 2013/08/05 14:23:02, rog wrote:
> I don't think we should be throwing away the error here.
> It might represent a genuine network error.
Done.
https:/
cmd/juju/
On 2013/08/05 14:23:02, rog wrote:
> Any particular reason we need to call SortedValues here rather than
Values?
> In fact, I'm not really sure we need to do this
> here - can't we do this in the loop where machineIds
> is first populated?
Nope, a holdover. Moved to fetchUnitMachin
https:/
cmd/juju/
On 2013/08/05 14:23:02, rog wrote:
> I'm not sure I see this. Each principal unit knows its
> own subordinates, and each subordinate unit knows its
> own principal unit (see my comment in unitMatcher.match)
> Given this, I think we can filter the
> units before they get put into the map,
> and lose the new code below.
Done. Much cleaner, thanks!
https:/
cmd/juju/
ok {
On 2013/08/05 14:23:02, rog wrote:
> I'm not sure this needs to change. The old code works
> just fine if context.
Done.
https:/
File testing/
https:/
testing/
On 2013/08/05 14:23:02, rog wrote:
> is this necessary?
Apparently not. I've removed the file, and updated status_test.go to
expect revno=0. Thanks.
https:/