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

On 2014/08/21 17:33:59, jrwren wrote:

> I agree with all of the issues you have raised. I feel that addressing
them now
> is out of scope of the original intent of the change, which was for
debugging
> purposes for an expert experienced user, not the user who needs
dependencies for
> juju resolved.

I am not sure we want to achieve this goal while introducing a subtle
bug.
I'd propose to simplify this, by not allowing quickstart to be run at
all if sudo is
required and a customized command is provided. I guess the main goal is
to
allow people to run quickstart on a newer compiled version of Juju, and
so
this should not be a problem.

A possible solution is like the following (code not tested).

1) Make get_juju_command return a customized flag, assuming that flag is
true
    only when JUJU is set and it differs from the default command, e.g.:

def get_juju_command(platform):
     """Return the path to the Juju command on the given platform.

     Also return a flag indicating whether the user requested to
customize
     the Juju command by providing a JUJU environment variable.

     If the platform does not have a novel location, the default will be
     returned.
     """
     platform_command = settings.JUJU_CMD_PATHS.get(
         platform,
         settings.JUJU_CMD_PATHS['default'])
     customized_command = os.getenv('JUJU', '').strip()
     if customized_command and (customized_command != platform_command):
         logging.warn('a customized Juju is being used: ' +
customized_command)
         return customized_command, True
     return platform_command, False

Note that in the warning we don't want to repeat "Warning:" and we don't
want
capitalization.

2) Add an app.juju_requires_sudo like the following:

def juju_requires_sudo(env_type, juju_version, customized):
     """Report whether the given Juju version requires "sudo".

     The env_type argument is the current environment type.

     Raise a ProgramExit if a customized version of Juju is being used
and Juju
     itself requires "sudo".
     """
     # If the Juju version is less than 1.17.2 then use sudo for local
envs.
     if env_type == 'local' and juju_version < (1, 17, 2):
         if customized:
             raise ProgramExit(b'cannot use a customized Juju for
reasons...')
         return True
     return False

3) At this point, the logic in manage.run is very simple, as it is
supposed
    to be:

     juju_command, customized = platform_support.get_juju_command(
         options.platform)

     logging.debug('ensuring Juju and dependencies are installed')
     juju_version = app.ensure_dependencies(
         options.distro_only, options.platform, juju_command)

     requires_sudo = app.juju_requires_sudo(
         options.env_type, juju_version, customized)

     logging.debug('ensuring SSH keys are available')
     app.ensure_ssh_keys()

     print('bootstrapping the {} environment (type: {})'.format(
         options.env_name, options.env_type))
     if options.env_type == 'local':
         # If this is a local environment, notify the user that "sudo"
will be
         # required to bootstrap the application, even in newer Juju
versions
         # where "sudo" is invoked by juju-core itself.
         print('sudo privileges will be required to bootstrap the
environment')

     already_bootstrapped, bootstrap_node_series = app.bootstrap(
         options.env_name, juju_command,
         requires_sudo=requires_sudo, debug=options.debug,
         upload_tools=options.upload_tools,
         upload_series=options.upload_series,
         constraints=options.constraints)
     ...

Also note that my current compiled Juju version is
"1.21-alpha1-trusty-amd64".
Yeah, unfortunately it's not a semantic version: we don't have a patch
number.
This means the logic in utils.get_juju_version must be slightly changed
to
support customized Juju.

I know this is not the original plan, and it's not as easy as it seemed
originally, and I am sorry about that. But I think if we want this
functionality in quickstart we must deal with this stuff and do that
properly.
On the other hand, I am not surprised by the fact that introducing
support for
non-stable versions of Juju introduces some complications.
Thanks a lot for your patience.

https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.py#newcode124
> > quickstart/platform_support.py:124: print("Warning: a customized
juju is being
> > used.")
> > Please use logging.warn, e.g.:
> > logging.warn('a customized juju is being used: ' + juju_command)
> >

> Will this get printed to the user?

Yes it will.

> I followed the existing warning in ensure_ssh_keys print('Warning: no
SSH keys
> were found in ~/.ssh

That should not be a warning technically, it asks for user interaction
and
should be considered a normal step in the quickstart execution.
If you search for "logging.warn" you'll find more examples.

https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.py#newcode126
> > quickstart/platform_support.py:126: else:
> > Unnecessary else block.
> >

> I'm still waiting for that team coding standard document. :)

Heh, this is more a personal preference, not strictly a coding style I
guess.
The less indentation levels the better.

https://codereview.appspot.com/132770043/diff/1/quickstart/tests/test_platform_support.py#newcode170
> > quickstart/tests/test_platform_support.py:170: class
> > TestGetJujuCommand(unittest.TestCase):
> > We should add a test here for the common case when the env var is
not set at
> > all.

> I am surprised it wasn't already there.

Me too. Perhaps that's due to the fact the function used to be really
straightforward. That's no longer the case.

Thanks a lot Jay, feel free to ping me for any questions.

https://codereview.appspot.com/132770043/