Implement base models for environments.

Implement base models for environments.

This is a preparation branch for the
environments management functionality.

The envs module includes a docstring describing
how those new functions will be used.

Tests: `make check`.
No QA required.

https://code.launchpad.net/~frankban/juju-quickstart/env-manage-base-models/+merge/198265

(do not edit description out of merge proposal)

[frankban, 2013-12-09 14:08:06]

Please take a look.

[bac, 2013-12-09 15:20:02]

LGTM with some suggestions.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:69: a new, non preexisting environment.
i think 'non preexisting' is redundant and should be removed.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:207: os.fsync(temp_file.fileno())
The flush and fscync are not handled by the close()?

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:245: # Why not using just env_data.copy()? Because
this way internal mutable
Picky, but "Why not just use env_data.copy()?" reads better.

https://codereview.appspot.com/39380043/diff/1/quickstart/tests/test_utils.py
File quickstart/tests/test_utils.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/tests/test_utils.py...
quickstart/tests/test_utils.py:256: '# in date 14/02/27 07:42:47.\n\n'
s/in date/at/

I think using the format  "2014-02-27 07:42:47 UTC" would be better.

https://codereview.appspot.com/39380043/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/utils.py#newcode151
quickstart/utils.py:151: 
perhaps  now.isoformat(sep=' ')?

[frankban, 2013-12-09 15:54:13]

Please take a look.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:69: a new, non preexisting environment.
On 2013/12/09 15:20:02, bac wrote:
> i think 'non preexisting' is redundant and should be removed.

Done.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:207: os.fsync(temp_file.fileno())
On 2013/12/09 15:20:02, bac wrote:
> The flush and fscync are not handled by the close()?

Good question. According to
http://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-python it
does not seem so.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:245: # Why not using just env_data.copy()? Because
this way internal mutable
On 2013/12/09 15:20:02, bac wrote:
> Picky, but "Why not just use env_data.copy()?" reads better.

Done.

https://codereview.appspot.com/39380043/diff/1/quickstart/tests/test_utils.py
File quickstart/tests/test_utils.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/tests/test_utils.py...
quickstart/tests/test_utils.py:256: '# in date 14/02/27 07:42:47.\n\n'
On 2013/12/09 15:20:02, bac wrote:
> s/in date/at/
> 
> I think using the format  "2014-02-27 07:42:47 UTC" would be better.

Done.

https://codereview.appspot.com/39380043/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/utils.py#newcode151
quickstart/utils.py:151: 
On 2013/12/09 15:20:02, bac wrote:
> perhaps  now.isoformat(sep=' ')?

Done.

[frankban, 2013-12-09 18:25:19]

LGTM thanks for the changes.

[frankban, 2013-12-09 18:25:46]

On 2013/12/09 18:25:19, frankban wrote:
> LGTM thanks for the changes.

oops, wrong MP.

[rharding, 2013-12-09 19:40:53]

LGTM on the code. 

I'm not 100% sold on the api itself. It seems like the envs could and should
hide away the raw config data in env_dict. Then expose an api for others to use
easily on top of that. 

At the very least, I think removing the redundant 'env' in some of the method
names would help a bit.

https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
quickstart/models/envs.py:34: envs_dict = load('~/.juju/environments.yaml')
would it make things clearer that this is the un-touched data by naming it
raw_envs_dict?

https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
quickstart/models/envs.py:66: 
This all reads a little verbose and needing to get all the parts right. 

What about having the env model be a class that tracks the raw envs_dict
internally?

env = Environment.load('~/.juju/environments.yaml')
env.raw == envs_dict
print env.name

errors = validate(env)
if errors:
    env.normalize()
    if env.is_dirty():
        env.save()

https://codereview.appspot.com/39380043/diff/20001/quickstart/tests/models/te...
File quickstart/tests/models/test_envs.py (right):

https://codereview.appspot.com/39380043/diff/20001/quickstart/tests/models/te...
quickstart/tests/models/test_envs.py:258: envs.get_env_data(self.envs_dict,
'no-such')
if the code is usually used as envs.XXX can we remove the env from the function
names to make the api read easier?

load/save and such do not have it so they read nicer.

[frankban, 2013-12-10 09:52:53]

On 2013/12/09 19:40:53, rharding wrote:
> LGTM on the code. 
> 
> I'm not 100% sold on the api itself. It seems like the envs could and should
> hide away the raw config data in env_dict. Then expose an api for others to
use
> easily on top of that. 

Hi Rick,

thanks for the review. Your suggestions are very interesting,
and I'd like to know what you think about the following
before proceeding.

This API is the result of an effort I made after a couple of
great pre-implementation calls with Gary. This implementation
is not object oriented on purpose. The idea behind this is
to expose unsurprising data-structures and reuse the basic
objects everybody is used to work with (i.e. Python dicts).
The envs_dict is exactly that, it's just the dict 
representation of a YAML data structure already knew by
everyone. So instead of having classes and methods we have
basic objects and functions to manipulate those objects.

The goal is to achieve more simplicity, but simplicity is
not a "simple" concept to deal with, and I'd like to 
understand from you in what ways this approach seems
confusing, and why a more object oriented one, like the
one you are suggesting, is better.

> At the very least, I think removing the redundant 'env' in some of the method
> names would help a bit.

I am not sure about that: envs.get_data and envs.set_data
seem ambiguous to me: we need to make it clear we are
getting/setting data on a single environment. That said,
I am always open to suggestions re: naming things.
Ideas?


>
https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
> quickstart/models/envs.py:34: envs_dict = load('~/.juju/environments.yaml')
> would it make things clearer that this is the un-touched data by naming it
> raw_envs_dict?

As mentioned above, this is intended to be the data structure 
we work with, and not just something encapsulated in an object.

>
https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
> quickstart/models/envs.py:66: 
> This all reads a little verbose and needing to get all the parts right. 
> 
> What about having the env model be a class that tracks the raw envs_dict
> internally?
> 
> env = Environment.load('~/.juju/environments.yaml')
> env.raw == envs_dict
> print env.name
> 
> errors = validate(env)
> if errors:
>     env.normalize()
>     if env.is_dirty():
>         env.save()

In this case we would also need an object (e.g. a MutableSequence)
representing all the envs and another one representing
a single env. However, as mentioned above, I'll suspend this
branch because I consider this discussion important.
What do a class give us, why is it preferable in this
context?

>
https://codereview.appspot.com/39380043/diff/20001/quickstart/tests/models/te...
> quickstart/tests/models/test_envs.py:258: envs.get_env_data(self.envs_dict,
> 'no-such')
> if the code is usually used as envs.XXX can we remove the env from the
function
> names to make the api read easier?
> 
> load/save and such do not have it so they read nicer.

See above, get_data seems not explicit enough to me.
Perhaps get_by_name and set_by_name? <shrug>

[gary.poster, 2013-12-10 14:02:42]

couple of ideas.  take or leave :-)

https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
quickstart/models/envs.py:34: envs_dict = load('~/.juju/environments.yaml')
On 2013/12/09 19:40:53, rharding wrote:
> would it make things clearer that this is the un-touched data by naming it
> raw_envs_dict?

all_environments?  environments_db?  env_db?

https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
quickstart/models/envs.py:43: errors = validate(envs_meta, env_data)
Maybe

env_db = load('...')
env_type_db = get_env_type_db()
env_data = get_env_data(env_db, 'myenv')
env_metadata = get_env_metadata(env_type_db, env_data)
errors = validate(env_metadata, env_data)

[rharding, 2013-12-10 14:06:10]

On 2013/12/10 14:02:42, gary.poster wrote:
> couple of ideas.  take or leave :-)
> 
> https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py
> File quickstart/models/envs.py (right):
> 
>
https://codereview.appspot.com/39380043/diff/20001/quickstart/models/envs.py#...
> quickstart/models/envs.py:34: envs_dict = load('~/.juju/environments.yaml')
> On 2013/12/09 19:40:53, rharding wrote:
> > would it make things clearer that this is the un-touched data by naming it
> > raw_envs_dict?
> 
> all_environments?  environments_db?  env_db?

Wow, I completely missed it contained "all" of the environments from the yaml
file. Because it was named so close with env_data I thought the env_dict was the
section of the yaml file that matched to the env_data in question (one section
of the yaml). So the all_environments would help set that mindset I think.

[frankban, 2013-12-10 14:39:01]

Please take a look.

[frankban, 2013-12-10 15:48:47]

*** Submitted:

Implement base models for environments.

This is a preparation branch for the
environments management functionality.

The envs module includes a docstring describing
how those new functions will be used.

Tests: `make check`.
No QA required.

R=bac, rharding, gary.poster
CC=
https://codereview.appspot.com/39380043