Code review comment for lp:~evarlast/juju-quickstart/which-juju

Revision history for this message
Francesco Banconi (frankban) wrote :

This branch is nice Jay, thank you.
However, this change makes me a little bit nervous, for two reasons.

1) In older versions of Juju, we require to bootstrap using sudo on local envs. Due to the fact we support those versions, that logic is indeed in quickstart. This means that, under certain circumstances, we can end up calling "sudo $JUJU". I am not convinced that the use cases for this change justify the security issues that can be be raised.

2) I like the fact that Quickstart only supports stable/packaged versions of juju (/usr/bin/juju). This means it's relatively easy to dupe possible bugs being filed.

As mentioned, I am not sure about introducing this feature, but if we decide to follow this path, I'd like quickstart to at least do the following:

- check that the given path exists and it's executable;
- expose a big WARNING to the user, informing she's using a customized juju binary, and requiring a user input in order to proceed.

I'd also like to s/$JUJU/$JUJU_CMD or similar.

What do you think?

« Back to merge proposal