Code review comment for lp:~frankban/juju-quickstart/env-manage-remove-environment

On 12/20/2013 05:24 AM, Francesco Banconi wrote:
> On 2013/12/19 19:11:13, gary.poster wrote:
>> LGTM.
>
>> The warning says "This action cannot be undone" but really it won't
> take effect
>> if the person exits early (^X), right? Because we only will save at
> the end?
>> If I'm right, maybe the following would work?
>
> In the current implementation, you pass a save_callable to the views,
> which is
> called when required, e.g. when the user removes an environment.
> The save_callable accepts an env_db and it's not yet implemented:
> it will likely be something like functools.partial(envs.save, env_file).
> We can decide to change this, but I think this strategy gives us two
> things:
> 1) the UX is less surprising for the user: when he performs an action,
> that
> action is done, and not just scheduled;

+1. I misunderstood the plan.

> 2) it is possible to use the quickstart interactive session to manage
> environments without starting Juju: the user starts quickstart,
> creates/edits/removes environments and then exits (^X).
>
>
>> """
>> Remove the private-cloud-errors-environment
>
>> This change will be saved once you choose an environment to deploy.
> The action
>> cannot be undone.
>> """
>
>> I deleted my default. What is supposed to happen then? You don't
> have a
>> default anymore? (That's what happens now.)
>
> Yes, in that case there is no longer a default in the environments.yaml
> file, and
> I have to check if juju-core is ok with this. If not, we need to change
> this behavior. It's also important to define the concept of "default"
> in the quickstart context: for now in Urwid it is just the YAML default,
> but
> in the real world it can be overwritten by juju switch (which, in turn,
> looks for that value in the JUJU_ENV env var and in the current_env
> file,
> the latter being an implementation detail). When you run quickstart
> without specifying -e, the default environment from juju switch is
> started.
> In the interactive session, the default environment is instead the one
> in the
> YAML file, and that's because the interactive session is intended to be
> a CRUD for envs.yaml. We might want to either unify those two concepts
> or
> keep them separate and show both in the UX.

Ah, yeah, that's a bit confusing (in Juju itself).

>
>
>> QA good with those notes.
>
>> Thanks!
>
> Thank you!
>
>
>
> https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.py#newcode39
>> quickstart/tests/cli/helpers.py:39: def emit(self, widget):
>> Don't you have this in another base class? If so, maybe this ought to
> be a
>> standalone function? it does not require "self."
>
> We had that, but now it is abstracted in this Mixin, used by
> both test cases. That said, you are right, this does not
> require self, so it can be just a function. The same for
> get_button_caption and inspect_dialog. Fixed.
>
>
>
> https://codereview.appspot.com/44310043/
>