pyjuju

juju/control/__init__.py (+2/-0)
juju/control/debug_hooks.py (+1/-1)
juju/control/scp.py (+89/-0)
juju/control/ssh.py (+7/-31)
juju/control/tests/test_scp.py (+159/-0)
juju/control/tests/test_utils.py (+69/-1)
juju/control/utils.py (+40/-0)
misc/bash_completion.d/juju (+2/-2)

To merge this branch:

bzr merge lp://staging/~jimbaker/pyjuju/scp-command

Medium

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Kapil Thangavelu (community)		Approve on 2011-11-21
William Reade (community)	2011-10-25	Approve on 2011-10-30
Review via email: mp+80343@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-10-25.

Description of the change

Implements juju scp subcommand, which is just a frontend to scp that expands unit names and machine IDs to their corresponding hosts. All options not parsed by juju subcommands are passed through to the underlying scp command.

Unlike juju ssh, juju scp does not verify agent liveness. It seems to me that this is not necessarily desirable, especially for debugging, but if so, it's certainly easy to add this verification.

Previously, it was considered that a relative path for a unit name like mysql/0 should get prepended with /var/lib/juju/units/mysql-0, but this doesn't really work due to file permissions. So instead relative paths will relative to the scp default (/home/ubuntu).

Revision history for this message

William Reade (fwereade) wrote on 2011-10-25: Posted in a previous version of this proposal

All looks sensible to me.

[0]

This rather looks as though it was intended to be stacked on ssh-passthrough.

[1]

I didn't realise "juju ssh" demands a live agent; if anything should be changed, I think it's this. I rather imagine that the majority of uses of ssh/scp will be in response to something surprising happening, and if one of the surprising things is a dead machine agent it will be somewhat irritating to have "juju ssh" inoperable, where plain old "ssh" would still work fine..

review: Approve

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2011-10-25: Posted in a previous version of this proposal

Excerpts from William Reade's message of Tue Oct 25 14:27:33 UTC 2011:
> Review: Approve
>
> All looks sensible to me.
>
> [0]
>
> This rather looks as though it was intended to be stacked on ssh-passthrough.
>
> [1]
>
> I didn't realise "juju ssh" demands a live agent; if anything should be changed, I think it's this. I rather imagine that the majority of uses of ssh/scp will be in response to something surprising happening, and if one of the surprising things is a dead machine agent it will be somewhat irritating to have "juju ssh" inoperable, where plain old "ssh" would still work fine..

It depends on a live 'machine' agent because else we have a potentially large
window while we wait for the machine to come up and cloud-init to finish
(imports ssh keys) and for ssh to be operational.

-k

Revision history for this message

Jim Baker (jimbaker) wrote on 2011-10-25:

I have updated the proposal to have the prereq branch of ssh-passthrough

Revision history for this message

William Reade (fwereade) wrote on 2011-10-30:

[2] sorry, just noticed:

get_ip_address_for_unit and get_ip_address_for_machine appear to be duplicated in control.ssh and control.scp; we shoudl probably fix that :).

Other than that, still approve; sorry I missed this.

review: Approve

Revision history for this message

Jim Baker (jimbaker) wrote on 2011-10-30:

> [2] sorry, just noticed:
>
> get_ip_address_for_unit and get_ip_address_for_machine appear to be duplicated
> in control.ssh and control.scp; we shoudl probably fix that :).

Good catch, this can be simply refactored by consolidating these functions into juju.control.utils.

>
> Other than that, still approve; sorry I missed this.

Good to know! Thanks for the second look.

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2011-11-21:

LGTM, +1. Some minors

[0] as noted by william there is significant duplication between
ssh/scp commands that should be remedied prior to merge.

[1] please clean up the default help output, ie. the output on juju -h
should be one line.

[2]
"""XXX"""

NSFW ? ;-)

[3] actually this applies to both the scp and and ssh branches, but
i'd drop the 'passthrough' in the help output, it causes it to wrap on
the cli. we should probably METAVAR environment to ENV to try and get
this under 80.

[4] The detailed help should note the path prefix when using a unit for an address, else the implemented behavior is likely to be confusing.

review: Approve

lp://staging/~jimbaker/pyjuju/scp-command updated on 2011-12-02

418. By Jim Baker on 2011-11-30: Merged trunk
419. By Jim Baker on 2011-12-02: Addressed review points
420. By Jim Baker on 2011-12-02: PEP8/PyFlakes
421. By Jim Baker on 2011-12-02: Updated tab completion
422. By Jim Baker on 2011-12-02: Slim down help output
423. By Jim Baker on 2011-12-02: PEP8

Revision history for this message

Jim Baker (jimbaker) wrote on 2011-12-02:

> LGTM, +1. Some minors
>
> [0] as noted by william there is significant duplication between
> ssh/scp commands that should be remedied prior to merge.

Refactored, with some additional unit tests on these functions (they had not been tested before)

>
> [1] please clean up the default help output, ie. the output on juju -h
> should be one line.

Minimized accordingly

>
> [2]
> """XXX"""
>
> NSFW ? ;-)

Hah, just my attempt to ensure I got everything before pushing. Failed this time!

>
> [3] actually this applies to both the scp and and ssh branches, but
> i'd drop the 'passthrough' in the help output, it causes it to wrap on
> the cli. we should probably METAVAR environment to ENV to try and get
> this under 80.

Removed, and added the metavar

>
> [4] The detailed help should note the path prefix when using a unit for an
> address, else the implemented behavior is likely to be confusing.

No longer relevant, since this behavior as we discussed on irc doesn't make sense given the permissions in that particular path