workspace-runner

Code review comment for lp:~abentley/workspace-runner/known-hosts

known-hosts
Merge into trunk

Revision history for this message

Aaron Bentley (abentley) wrote on 2015-07-13:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2015-07-13 09:46 AM, Martin Packman wrote:
> Review: Approve
>
> Looks fine.
>
> The next step however, if we want to add more ssh configuration,
> should be just having a dedicated ssh config dir for the
> SSHConnection object, rather than giving everything as command line
> options. That gives use true isolation from the user's ssh config
> and we can be precise about what elements we want to preserve.

I'm not sure I agree. It's nice that the overrides appear in log
messages, and it would be a pain deciding what to preserve and what to
override.

>> + options = list(chain(*[['-o', o] for o in
>> config_options]))
>
> Heh, and you complained about me using map()? :)
>
> You can just do this:
>
> options = ["-o" + o for f in config_options]
>
> The ssh command line parser is fine with that.

In this specific case, where ['-ofoo'] is equivalent to ['-o', 'foo'],
sure. In general, list(chain(*my_lists)) is the least obscure way I
can find of generating a single large list containing all the elements
of a list of lists.

>> if self.private_key is not None: options.extend(['-i',
>> self.private_key]) return options @@ -190,8 +204,11 @@ except
>> Exception as e: logging.exception(e) sys.exit(1) +
>> finally: + primitives.destroy() finally: -
>> primitives.destroy() + connection.known_hosts = None
>
> Unsetting the known_hosts attribute is a little unpleasant. Would
> prefer if SSHConnection just had responsibility for the file.

Maybe in the abstract, but it's a public attribute, so there's nothing
actually wrong with setting/unsetting it. I didn't think it was worth
writing another context manager (e.g. SSHConnection.known_hosts) and
having another layer of indirection just to manage the known hosts file.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVo8U+AAoJEK84cMOcf+9h1x0IALQpJ8dc9iuTA9fMgMs8aSux
qHxNKc5BHmFzNPrX+eXIJnES6+TCwO+vcjlenNAVEmD/ovw3rpEh6PDOdJCvksqf
JDEc273NtXA+985wOCIae71iF+Qi4ogucAoKZTzxOwND3RPzUqkZ5Fe/NAxB/t3c
uJfacxjTvM6JL8KEQUsz7UBUugo7pRQED6BVqxzE72xRwNSBWxjPMUkyljrWQzxC
3R2PgPbC4LuuNqdFj6PZaTQ1V0f+ZeUMKhlNl/qXbsZaumme13rZRhMVJVa5UlXm
5yKZq8tOf812BlG9ACA0S/8ahKniHg9u9piIteFhc4ppyr3G2Z2LzpNA7xjXRUs=
=TbsU
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2015-07-13 09:46 AM, Martin Packman wrote:
> Review: Approve
> 
> Looks fine.
> 
> The next step however, if we want to add more ssh configuration,
> should be just having a dedicated ssh config dir for the
> SSHConnection object, rather than giving everything as command line
> options. That gives use true isolation from the user's ssh config
> and we can be precise about what elements we want to preserve.

I'm not sure I agree.  It's nice that the overrides appear in log
messages, and it would be a pain deciding what to preserve and what to
override.

>> +        options = list(chain(*[['-o', o] for o in
>> config_options]))
> 
> Heh, and you complained about me using map()? :)
> 
> You can just do this:
> 
> options = ["-o" + o for f in config_options]
> 
> The ssh command line parser is fine with that.

In this specific case, where ['-ofoo'] is equivalent to ['-o', 'foo'],
sure.  In general, list(chain(*my_lists)) is the least obscure way I
can find of generating a single large list containing all the elements
of a list of lists.

>> if self.private_key is not None: options.extend(['-i',
>> self.private_key]) return options @@ -190,8 +204,11 @@ except
>> Exception as e: logging.exception(e) sys.exit(1) +
>> finally: +            primitives.destroy() finally: -
>> primitives.destroy() +        connection.known_hosts = None
> 
> Unsetting the known_hosts attribute is a little unpleasant. Would
> prefer if SSHConnection just had responsibility for the file.

Maybe in the abstract, but it's a public attribute, so there's nothing
actually wrong with setting/unsetting it.  I didn't think it was worth
writing another context manager (e.g. SSHConnection.known_hosts) and
having another layer of indirection just to manage the known hosts file.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

« Back to merge proposal