pyjuju

Merge lp://staging/~jimbaker/pyjuju/expose-status into lp://staging/pyjuju

expose-status
Merge into trunk

Proposed by Jim Baker on 2011-06-23

Status:	Merged
Approved by:	Gustavo Niemeyer on 2011-06-28
Approved revision:	264
Merged at revision:	272
Proposed branch:	lp://staging/~jimbaker/pyjuju/expose-status
Merge into:	lp://staging/pyjuju
Diff against target:	222 lines (+77/-26) 2 files modified ensemble/control/status.py (+11/-2) ensemble/control/tests/test_status.py (+66/-24)
To merge this branch:	bzr merge lp://staging/~jimbaker/pyjuju/expose-status
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Gustavo Niemeyer		Approve on 2011-06-28
Kapil Thangavelu (community)	2011-06-23	Approve on 2011-06-24
Review via email: mp+65718@code.staging.launchpad.net

Description of the change

Small branch to add expose and open ports info to status output for YAML and JSON. (Does not address dot output, since this is necessarily a subset anyway.)

The one difference with the bug report is that exposed is written as "true" instead of "yes" for YAML. Probably this could be managed with appropriate settings to the YAML dump method, however, I found this to be poorly documented. In any event, "true" seems just reasonable a way to write this out from a user's perspective (reads similarly well).

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2011-06-24:

It doesn't appear there are any tests for non-exposed services or units without open ports.

review: Needs Fixing

Revision history for this message

Jim Baker (jimbaker) wrote on 2011-06-24:

On Fri, Jun 24, 2011 at 7:09 AM, Kapil Thangavelu <
<email address hidden>> wrote:

> Review: Needs Fixing
> It doesn't appear there are any tests for non-exposed services or units
> without open ports.
>

In the test setup, not all services were exposed (only varnish is), and not
every unit opened ports (only wordpress and varnish do). Currently the
status doesn't output information if a service is not exposed (eg "exposed":
false) or if a service unit doesn't have ports open ("open-ports": []). This
was intentional to minimize the output, but certainly could be changed if it
makes more sense.

Consequently, the absence is now being tested explicitly in these tests
because of the use of exact comparison in nearly all assertions. In the two
exceptions, I used assertGreaterEqual in a couple assertions because in this
situation the keys were being extracted for all services, but of course in
the test setup, it's only in varnish that is exposed and would have the
"exposed" key. This assertion should be made exact by removing the loop.

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2011-06-24:

Thanks for the clarification, this looks pretty good.

review: Approve

lp://staging/~jimbaker/pyjuju/expose-status updated on 2011-06-28

263. By Jim Baker on 2011-06-28: Tests to verify that open-ports is not set in status unless service is exposed
264. By Jim Baker on 2011-06-28: Do not set open-ports unless service is exposed

Revision history for this message

Jim Baker (jimbaker) wrote on 2011-06-28:

Per the recent commits, status will output the open-ports key for a unit only if the service is exposed.

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2011-06-28:

This looks good. One minor, and +1.

[1]

21 + if open_ports:
22 + u["open-ports"] = ["{port}/{proto}".format(**port_info)
23 + for port_info in open_ports]

If the service _is_ exposed, it'd be more useful to do something like:

u["open-ports"] = None

This would make more clear for the user that even though the service is
exposed, there are no open ports, and that's why he's not able to access
whatever he's trying to.

review: Approve

Revision history for this message

Jim Baker (jimbaker) wrote on 2011-06-29:

> This looks good. One minor, and +1.
>
> [1]
>
> 21 + if open_ports:
> 22 + u["open-ports"] =
> ["{port}/{proto}".format(**port_info)
> 23 + for port_info in open_ports]
>
> If the service _is_ exposed, it'd be more useful to do something like:
>
> u["open-ports"] = None
>
> This would make more clear for the user that even though the service is
> exposed, there are no open ports, and that's why he's not able to access
> whatever he's trying to.

Probably makes even more sense to say u["open-ports"] = []. An empty list seems more evocative that there are no open ports, but there could be. This also corresponds to the semantics of how it's managed in ZK, where we default it to an empty list (or technically, tuple).