Repository: conjure-up/conjure-up

Issue:
make model explicit when running juju-cli commands or API calls
utilize `juju -m [controller:]model` when executing juju cli commands. Currently we switch to the model we want and then run juju. This has the effect of killing any other existing conjure-up that may be running in seperate model.

As for the API we always switch to the known current model and login to the api, we need to login to the API without switching models.

Discussion:

Patch:
diff --git a/conjureup/app.py b/conjureup/app.py
index 926cd3c..7bd28d1 100644
--- a/conjureup/app.py
+++ b/conjureup/app.py
@@ -243,13 +243,6 @@ def main():
             utils.warning("Could not find spell {}".format(opts.spell))
             sys.exit(1)
 
-        if not os.path.exists(os.path.join(opts.spell,
-                                           "metadata.yaml")):
-            utils.warning("'{}' does not appear to be a spell. "
-                          "{}/metadata.yaml was not found.".format(
-                              opts.spell, opts.spell))
-            sys.exit(1)
-
         spell_name = os.path.basename(os.path.abspath(spell))
         utils.set_chosen_spell(spell_name,
                                path.join(opts.cache_dir, spell_name))
diff --git a/conjureup/controllers/clouds/gui.py b/conjureup/controllers/clouds/gui.py
index f36ff2d..9969194 100644
--- a/conjureup/controllers/clouds/gui.py
+++ b/conjureup/controllers/clouds/gui.py
@@ -16,9 +16,7 @@ class CloudsController:
         app.ui.show_exception_message(exc)
 
     def __add_model(self):
-        juju.switch_controller(app.current_controller)
-        juju.add_model(app.current_model)
-        juju.switch_model(app.current_model)
+        juju.add_model(app.current_model, app.current_controller)
 
     def finish(self, cloud):
         """ Load the Model controller passing along the selected cloud.
diff --git a/conjureup/controllers/clouds/tui.py b/conjureup/controllers/clouds/tui.py
index edc2bc4..08cf4de 100644
--- a/conjureup/controllers/clouds/tui.py
+++ b/conjureup/controllers/clouds/tui.py
@@ -22,12 +22,10 @@ class CloudsController:
             return controllers.use('newcloud').render(app.argv.cloud)
 
         app.current_controller = existing_controller
-        juju.switch_controller(app.current_controller)
         app.current_model = petname.Name()
         utils.info("Creating new juju model named '{}', "
                    "please wait.".format(app.current_model))
-        juju.add_model(app.current_model)
-        juju.switch_model(app.current_model)
+        juju.add_model(app.current_model, app.current_controller)
 
         return controllers.use('deploy').render()
 
diff --git a/conjureup/controllers/controllerpicker/gui.py b/conjureup/controllers/controllerpicker/gui.py
index 5834d02..74085ec 100644
--- a/conjureup/controllers/controllerpicker/gui.py
+++ b/conjureup/controllers/controllerpicker/gui.py
@@ -15,9 +15,7 @@ class ControllerPicker:
         app.ui.show_exception_message(exc)
 
     def __add_model(self):
-        juju.switch_controller(app.current_controller)
-        juju.add_model(app.current_model)
-        juju.switch_model(app.current_model)
+        juju.add_model(app.current_model, app.current_controller)
 
     def finish(self, controller):
         utils.pollinate(app.session_id, 'CS')
diff --git a/conjureup/controllers/deploy/gui.py b/conjureup/controllers/deploy/gui.py
index 62399c4..f8c66a2 100644
--- a/conjureup/controllers/deploy/gui.py
+++ b/conjureup/controllers/deploy/gui.py
@@ -27,7 +27,7 @@ class DeployController:
         """ runs pre deploy script if exists
         """
         app.env['JUJU_PROVIDERTYPE'] = model_info(
-            juju.get_current_model())['provider-type']
+            app.current_model)['provider-type']
 
         pre_deploy_sh = os.path.join(app.config['spell-dir'],
                                      'steps/00_pre-deploy')
diff --git a/conjureup/controllers/deploy/tui.py b/conjureup/controllers/deploy/tui.py
index 9103fe9..0bfebb1 100644
--- a/conjureup/controllers/deploy/tui.py
+++ b/conjureup/controllers/deploy/tui.py
@@ -26,7 +26,8 @@ class DeployController:
         """ runs pre deploy script if exists
         """
         # Set provider type for post-bootstrap
-        app.env['JUJU_PROVIDERTYPE'] = model_info('default')['provider-type']
+        app.env['JUJU_PROVIDERTYPE'] = model_info(
+            app.current_model)['provider-type']
 
         pre_deploy_sh = os.path.join(app.config['spell-dir'],
                                      'steps/00_pre-deploy')
diff --git a/conjureup/controllers/newcloud/gui.py b/conjureup/controllers/newcloud/gui.py
index 6f1d6b4..ef95e90 100644
--- a/conjureup/controllers/newcloud/gui.py
+++ b/conjureup/controllers/newcloud/gui.py
@@ -39,7 +39,6 @@ class NewCloudController:
         utils.pollinate(app.session_id, 'J004')
         EventLoop.remove_alarms()
         app.ui.set_footer('Bootstrap complete...')
-        juju.switch_controller(app.current_controller)
         self.__post_bootstrap_exec()
 
     def __do_bootstrap(self, cloud=None, credential=None):
@@ -66,7 +65,7 @@ class NewCloudController:
     def __post_bootstrap_exec(self):
         """ Executes post-bootstrap.sh if exists
         """
-        info = model_info(juju.get_current_model())
+        info = model_info(app.current_model)
         # Set our provider type environment var so that it is
         # exposed in future processing tasks
         app.env['JUJU_PROVIDERTYPE'] = info['provider-type']
@@ -106,9 +105,6 @@ class NewCloudController:
                 'bootstrap processing phase: {}.'.format(result)))
         utils.pollinate(app.session_id, 'J002')
         app.ui.set_footer('')
-        app.log.debug("Switching to controller: {}".format(
-            app.current_controller))
-        juju.switch_controller(app.current_controller)
         controllers.use('deploy').render()
 
     def finish(self, credentials=None, back=False):
diff --git a/conjureup/hooklib/writer.py b/conjureup/hooklib/writer.py
index f3e5445..db8e935 100644
--- a/conjureup/hooklib/writer.py
+++ b/conjureup/hooklib/writer.py
@@ -9,7 +9,7 @@ CACHEDIR = os.getenv('CONJURE_UP_CACHEDIR',
 SPELL_NAME = os.getenv('CONJURE_UP_SPELL', '_unspecified_spell')
 LOGFILE = os.path.join(CACHEDIR, '{spell}.log'.format(spell=SPELL_NAME))
 
-log = setup_logging("conjure-up/{}".format(SPELL_NAME), LOGFILE, True)
+log = setup_logging(SPELL_NAME, LOGFILE, True)
 
 
 def success(msg):
diff --git a/conjureup/juju.py b/conjureup/juju.py
index 7fcebba..b28369c 100644
--- a/conjureup/juju.py
+++ b/conjureup/juju.py
@@ -29,7 +29,7 @@ this.USER_TAG = None
 def requires_login(f):
     def _decorator(*args, **kwargs):
         if not this.IS_AUTHENTICATED:
-            login()
+            login(force=True)
         return f(*args, **kwargs)
     return wraps(f)(_decorator)
 
@@ -92,15 +92,15 @@ def login(force=False):
     if this.IS_AUTHENTICATED is True and not force:
         return
 
-    if not get_current_controller():
+    if app.current_controller is None:
         raise Exception("Unable to determine current controller")
 
-    if not get_current_model():
+    if app.current_model is None:
         raise Exception("Tried to login with no current model set.")
 
-    env = get_controller(get_current_controller())
-    account = get_account(get_current_controller())
-    uuid = get_model(get_current_model())['model-uuid']
+    env = get_controller(app.current_controller)
+    account = get_account(app.current_controller)
+    uuid = get_model(app.current_controller, app.current_model)['model-uuid']
     server = env['api-endpoints'][0]
     this.USER_TAG = "user-{}".format(account['user'].split("@")[0])
     url = os.path.join('wss://', server, 'model', uuid, 'api')
@@ -279,52 +279,6 @@ def get_cloud(name):
     raise LookupError("Unable to locate cloud: {}".format(name))
 
 
-def _do_switch(target):
-    try:
-        app.log.debug('calling juju switch {}'.format(target))
-        run('juju-2.0 switch {}'.format(target),
-            shell=True, check=True, stdout=DEVNULL, stderr=DEVNULL)
-    except CalledProcessError as e:
-        raise LookupError("Unable to switch: {}".format(e))
-
-
-def switch_model(model):
-    """Switch
-
-    Arguments:
-    model: Model to select
-
-    Returns: Raises exception if model is not a model in the current
-    controller, or if we otherwise failed to switch models.
-    """
-
-    if model not in [m['name'] for m in get_models()['models']]:
-        raise Exception("model '{}' not found in controller '{}'.".format(
-            model, get_current_controller()))
-    _do_switch(model)
-
-
-def switch_controller(controller):
-    """ switch controllers
-
-    Arguments:
-    controller: controller to switch to
-
-    Returns None.
-    Raises exception if failed to switch.
-    """
-    assert controller is not None
-
-    cinfo = get_controllers()
-    prev_controller = cinfo.get('current-controller', None)
-    if prev_controller == controller:
-        return
-    if controller not in cinfo.get('controllers', {}).keys():
-        raise Exception("Could not find controller '{}'".format(controller))
-    _do_switch(controller)
-    login(True)
-
-
 def deploy(bundle):
     """ Juju deploy bundle
 
@@ -559,35 +513,17 @@ def get_accounts():
     raise Exception("Unable to find accounts")
 
 
-def model_by_owner(user):
-    """ List model associated with user
-
-    Arguments:
-    user: username to query
-
-    Returns:
-    Dictionary containing model information for user
-    """
-    models = get_models()
-    for m in models:
-        if m['owner'] == user:
-            return m
-    raise LookupError(
-        "Unable to find user: {}".format(
-            user
-        ))
-
-
-def get_model(name):
+def get_model(controller, name):
     """ List information for model
 
     Arguments:
     name: model name
+    controller: name of controller to work in
 
     Returns:
     Dictionary of model information
     """
-    models = get_models()['models']
+    models = get_models(controller)['models']
     for m in models:
         if m['name'] == name:
             return m
@@ -595,23 +531,29 @@ def get_model(name):
         "Unable to find model: {}".format(name))
 
 
-def add_model(name):
+def add_model(name, controller):
     """ Adds a model to current controller
+
+    Arguments:
+    controller: controller to add model in
     """
-    sh = run('juju-2.0 add-model {}'.format(name),
+    sh = run('juju-2.0 add-model {} -c {}'.format(name, controller),
              shell=True, stdout=DEVNULL, stderr=PIPE)
     if sh.returncode > 0:
         raise Exception(
             "Unable to create model: {}".format(sh.stderr.decode('utf8')))
 
 
-def get_models():
+def get_models(controller):
     """ List available models
 
+    Arguments:
+    controller: existing controller to get models for
+
     Returns:
     List of known models
     """
-    sh = run('juju-2.0 list-models --format yaml',
+    sh = run('juju-2.0 list-models --format yaml -c {}'.format(controller),
              shell=True, stdout=PIPE, stderr=PIPE)
     if sh.returncode > 0:
         raise LookupError(
diff --git a/conjureup/log.py b/conjureup/log.py
index 974c02f..5384e92 100644
--- a/conjureup/log.py
+++ b/conjureup/log.py
@@ -4,25 +4,6 @@ import stat
 from logging.handlers import SysLogHandler, TimedRotatingFileHandler
 
 
-class _log:
-
-    def __init__(self, app, logger):
-        self.app = app
-        self.logger = logger
-
-    def debug(self, msg):
-        self.logger.debug("{}: {}".format(self.app, msg))
-
-    def error(self, msg):
-        self.logger.error("{}: {}".format(self.app, msg))
-
-    def info(self, msg):
-        self.logger.info("{}: {}".format(self.app, msg))
-
-    def exception(self, msg):
-        self.logger.exception("{}: {}".format(self.app, msg))
-
-
 def setup_logging(app, logfile, debug=False):
     cmdslog = TimedRotatingFileHandler(logfile,
                                        when='D',
@@ -49,4 +30,4 @@ def setup_logging(app, logfile, debug=False):
             syslog_h.set_name(app)
             logger.addHandler(syslog_h)
 
-    return _log(app, logger)
+    return logger
diff --git a/conjureup/ui/views/applicationconfigure.py b/conjureup/ui/views/applicationconfigure.py
index 3f3f3d1..9fa5b7c 100644
--- a/conjureup/ui/views/applicationconfigure.py
+++ b/conjureup/ui/views/applicationconfigure.py
@@ -3,15 +3,13 @@
 """
 
 import logging
-from functools import partial
 
-from urwid import Columns, Filler, Frame, Pile, Text, WidgetWrap
+from urwid import Filler, Pile, Text, WidgetWrap
 
 from conjureup import utils
 from conjureup.ui.widgets.option_widget import OptionWidget
-from ubuntui.ev import EventLoop
-from ubuntui.utils import Color, Padding
-from ubuntui.widgets.buttons import PlainButton, menu_btn
+from ubuntui.utils import Padding
+from ubuntui.widgets.buttons import PlainButton
 from ubuntui.widgets.hr import HR
 
 log = logging.getLogger('conjure')
@@ -25,77 +23,12 @@ class ApplicationConfigureView(WidgetWrap):
         self.options_copy = self.application.options.copy()
         self.metadata_controller = metadata_controller
         self.widgets = self.build_widgets()
-        self.description_w = Text("")
-        self.showing_all = False
-        self.buttons_selected = False
-        self.frame = Frame(body=self.build_widgets(),
-                           footer=self.build_footer())
-        super().__init__(self.frame)
-
-        self.metadata_controller.get_readme(
-            self.application.csid.as_seriesname(),
-            partial(self._handle_readme_load))
-
-    def _handle_readme_load(self, readme_f):
-        EventLoop.loop.event_loop._loop.call_soon_threadsafe(
-            partial(self._update_readme_on_main_thread,
-                    readme_f.result()))
-
-    def _update_readme_on_main_thread(self, readme):
-        rt = self._trim_readme(readme)
-        self.description_w.set_text(rt)
-
-    def _trim_readme(self, readme):
-        rls = readme.splitlines()
-        rls = [l for l in rls if not l.startswith("#")]
-        nrls = []
-        for i in range(len(rls)):
-            if i + 1 == len(rls):
-                break
-            if len(rls[i]) > 0:
-                if rls[i][0] in ['-', '#', '=']:
-                    continue
-            if len(rls[i + 1]) > 0:
-                if rls[i + 1][0] in ['-', '=']:
-                    continue
-            nrls.append(rls[i])
-
-        if len(nrls) == 0:
-            return
-
-        if nrls[0] == '':
-            nrls = nrls[1:]
-        # split after two paragraphs:
-        if '' in nrls:
-            firstparidx = nrls.index('')
-        else:
-            firstparidx = 1
-        try:
-            splitidx = nrls.index('', firstparidx + 1)
-        except:
-            splitidx = firstparidx
-        nrls = nrls[:splitidx]
-        return "\n".join(nrls)
+        super().__init__(self.widgets)
+        self.pile.focus_position = 1
 
     def selectable(self):
         return True
 
-    def keypress(self, size, key):
-        # handle keypress first, then get new focus widget
-        rv = super().keypress(size, key)
-        if key in ['tab', 'shift tab']:
-            self._swap_focus()
-        return rv
-
-    def _swap_focus(self):
-        if not self.buttons_selected:
-            self.buttons_selected = True
-            self.frame.focus_position = 'footer'
-            self.buttons.focus_position = 3
-        else:
-            self.buttons_selected = False
-            self.frame.focus_position = 'body'
-
     def build_widgets(self):
         ws = [Text("Configure {}".format(
             self.application.service_name))]
@@ -105,47 +38,14 @@ class ApplicationConfigureView(WidgetWrap):
                                    current_value=self.application.num_units,
                                    value_changed_callback=self.handle_scale)
         ws.append(num_unit_ow)
-        ws += self.get_whitelisted_option_widgets()
-        self.toggle_show_all_button_index = len(ws) + 1
-        self.toggle_show_all_button = PlainButton(
-            "Show Advanced Configuration",
-            self.do_toggle_show_all_config)
-        ws += [HR(),
-               Columns([('weight', 1, Text(" ")),
-                        (36, Color.button_secondary(
-                            self.toggle_show_all_button))])]
+        ws += self.get_option_widgets()
+        ws += [HR(), PlainButton("Cancel", self.do_cancel),
+               PlainButton("Accept Changes", self.do_commit)]
         self.pile = Pile(ws)
         return Padding.center_90(Filler(self.pile, valign="top"))
 
-    def build_footer(self):
-        cancel = menu_btn(on_press=self.do_cancel,
-                          label="\n  BACK\n")
-        confirm = menu_btn(on_press=self.do_commit,
-                           label="\n APPLY CHANGES\n")
-        self.buttons = Columns([
-            ('fixed', 2, Text("")),
-            ('fixed', 13, Color.menu_button(
-                cancel,
-                focus_map='button_primary focus')),
-            Text(""),
-            ('fixed', 20, Color.menu_button(
-                confirm,
-                focus_map='button_primary focus')),
-            ('fixed', 2, Text(""))
-        ])
-
-        footer = Pile([
-            HR(top=0),
-            Padding.center_90(self.description_w),
-            Padding.line_break(""),
-            Color.frame_footer(Pile([
-                Padding.line_break(""),
-                self.buttons]))
-        ])
-
-        return footer
-
-    def get_whitelisted_option_widgets(self):
+    def get_option_widgets(self):
+        ws = []
         service_id = self.application.csid.as_str_without_rev()
         options = self.metadata_controller.get_options(service_id)
 
@@ -153,21 +53,7 @@ class ApplicationConfigureView(WidgetWrap):
             self.application.service_name)
         hidden = [n for n in options.keys() if n not in svc_opts_whitelist]
         log.info("Hiding options not in the whitelist: {}".format(hidden))
-
-        return self._get_option_widgets(svc_opts_whitelist, options)
-
-    def get_non_whitelisted_option_widgets(self):
-        service_id = self.application.csid.as_str_without_rev()
-        options = self.metadata_controller.get_options(service_id)
-
-        svc_opts_whitelist = utils.get_options_whitelist(
-            self.application.service_name)
-        hidden = [n for n in options.keys() if n not in svc_opts_whitelist]
-        return self._get_option_widgets(hidden, options)
-
-    def _get_option_widgets(self, opnames, options):
-        ws = []
-        for opname in opnames:
+        for opname in svc_opts_whitelist:
             opdict = options[opname]
             cv = self.application.options.get(opname, None)
             ow = OptionWidget(opname,
@@ -179,24 +65,6 @@ class ApplicationConfigureView(WidgetWrap):
             ws.append(ow)
         return ws
 
-    def do_toggle_show_all_config(self, sender):
-        if not self.showing_all:
-            new_ows = self.get_non_whitelisted_option_widgets()
-            header = Text("Advanced Configuration Options")
-            opts = self.pile.options()
-            self.pile.contents.append((header, opts))
-            for ow in new_ows:
-                self.pile.contents.append((ow, opts))
-            self.toggle_show_all_button.set_label(
-                "Hide Advanced Configuration")
-            self.showing_all = True
-        else:
-            i = self.toggle_show_all_button_index
-            self.pile.contents = self.pile.contents[:i + 1]
-            self.toggle_show_all_button.set_label(
-                "Show Advanced Configuration")
-            self.showing_all = False
-
     def handle_edit(self, opname, value):
         self.options_copy[opname] = value
 


Test patch:
diff --git a/test/test_controllers_clouds_gui.py b/test/test_controllers_clouds_gui.py
index 9f60952..0b96672 100644
--- a/test/test_controllers_clouds_gui.py
+++ b/test/test_controllers_clouds_gui.py
@@ -78,6 +78,10 @@ class CloudsGUIFinishTestCase(unittest.TestCase):
             'conjureup.controllers.clouds.gui.juju.get_controller_in_cloud')
         self.mock_gcc = self.gcc_patcher.start()
 
+        self.petname_patcher = patch(
+            'conjureup.controllers.clouds.gui.petname')
+        self.mock_petname = self.petname_patcher.start()
+
     def tearDown(self):
         self.controllers_patcher.stop()
         self.utils_patcher.stop()
@@ -85,13 +89,15 @@ class CloudsGUIFinishTestCase(unittest.TestCase):
         self.app_patcher.stop()
         self.juju_patcher.stop()
         self.gcc_patcher.stop()
+        self.mock_petname.stop()
 
     def test_finish_w_controller(self):
         "clouds.finish with an existing controller"
         self.mock_gcc.return_value = 'testcontroller'
+        self.mock_petname.Name.return_value = 'moo'
         self.controller.finish('testcloud')
-        self.mock_juju.assert_has_calls([
-            call.switch_controller('testcontroller')])
+        self.mock_juju.add_model.assert_called_once_with('moo',
+                                                         'testcontroller')
 
     def test_finish_no_controller(self):
         "clouds.finish without existing controller"
diff --git a/test/test_controllers_clouds_tui.py b/test/test_controllers_clouds_tui.py
index 30d5ca9..42e1299 100644
--- a/test/test_controllers_clouds_tui.py
+++ b/test/test_controllers_clouds_tui.py
@@ -6,7 +6,7 @@
 
 
 import unittest
-from unittest.mock import MagicMock, call, patch
+from unittest.mock import ANY, MagicMock, call, patch
 
 from conjureup.controllers.clouds.tui import CloudsController
 
@@ -94,7 +94,7 @@ class CloudsTUIFinishTestCase(unittest.TestCase):
         self.mock_gcc.return_value = 'testcontroller'
         self.controller.finish()
         self.mock_juju.assert_has_calls([
-            call.switch_controller('testcontroller')])
+            call.add_model(ANY, 'testcontroller')])
 
     def test_finish_no_controller(self):
         "clouds.finish without existing controller"