Implement the missing model bits.

Implement the missing model bits.

Thanks to Gary for his help in defining
this approach with me in several pre-imp
calls.

See the envs module docstring for a
description of the functions implemented
in this branch.

This branch also includes some 
fixes/improvements to the fields module,
e.g. the handling of default and unset values.

Tests: make check
No QA.

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

(do not edit description out of merge proposal)

[frankban, 2013-12-12 14:54:57]

Please take a look.

[matthew.scott, 2013-12-12 17:46:05]

Super work, as always.  LGTM, check okay.

[benji, 2013-12-13 17:15:59]

This branch looks good.  None of my comments are blockers, more like "things you
might like to consider", so this LGTM.

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

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#ne...
quickstart/models/fields.py:128: return super(StringField, self).normalize(value
or None)
If the given value is the empty string then None will be passed to .normalize
and he default will be returned.  That doesn't sound right.

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#ne...
quickstart/models/fields.py:164: return self.default
So it is not possible to have an empty string as a field value?  I've seen
systems with that policy before and it come back to bite them in the long run. 
However, it looks like we're already there so I'm not sure it is something we
should address right this minute.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
File quickstart/tests/models/test_envs.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:514: # The local environment metadata is
correctly returned.
It would be nice to expand "correctly returned" to describe a little more what
that means.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:526: # The ec2 environment metadata is
correctly returned.
Same comment for "correctly returned" as above.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:641: self.assertEqual(expected, obtained)
One of my personal preferences, for your consideration: I try to avoid variable
names in tests like "expected" and "obtained" and instead try to communicate
more about the values held.  For example, above I might call them "valid_pairs"
and "mapped_pairs".  That way the assertion says more about the truth the test
is trying to get at: self.assertEqual(mapped_pairs, valid_pairs)

Note that making the asserts read this way generally means the "obtained" value
is the first argument and the "expected" argument is second.  Python's test
infrastructure is fine with this, but we have to make sure any test-specific
assertion helpers are too.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:650: self.assertEqual(help, field.help)
I suspect you intended to pass field.name as the third argument to this assert
too.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:689: self.assertEqual({},
envs.validate(self.env_metadata, env_data))
It took me a while to understand this test.  Something like this might
have helped:

# To be valid, env_data must include the required values.
env_data = {'string-required': 'a string'}
validation_errors = envs.validate(self.env_metadata, env_data)
# No validation errors were found.
self.assertEqual(validation_errors, {})

Another thing was that the phrase "at least" primed me to expect the
test to be demonstrating invalid data instead of valid.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:741: 'string-default': '\t another one',
Changing the values by removing whitespace makes me nervous.  Maybe for the
kinds of values we deal with it is OK, but I've been bitten by this more than
once.  It seems better to me to let the occasional user screw up with whitespace
than to prevent another user from doing what they wan to do (like having a
multi-line config value).

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:776: # Since this value is the default one,
the field is excluded.
Given that defaults can change over time, I would be cautious about implicitly
removing default values that the user had explicitly provided.  If they provided
a value we should assume they want to avoid depending on the default.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:785: # reason the field is excluded.
This is very nicely commented.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_f...
File quickstart/tests/models/test_fields.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_f...
quickstart/tests/models/test_fields.py:318: # If the field is not required no
errors are raised.
Since you used a comma in the comment for the next test, you probably intended
to have one here too.

[frankban, 2013-12-16 12:12:04]

Please take a look.

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

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#ne...
quickstart/models/fields.py:128: return super(StringField, self).normalize(value
or None)
On 2013/12/13 17:15:59, benji wrote:
> If the given value is the empty string then None will be passed to .normalize
> and he default will be returned.  That doesn't sound right.

Changed the module so that default is never returned by normalize: see below.

https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#ne...
quickstart/models/fields.py:164: return self.default
On 2013/12/13 17:15:59, benji wrote:
> So it is not possible to have an empty string as a field value?  I've seen
> systems with that policy before and it come back to bite them in the long run.

> However, it looks like we're already there so I'm not sure it is something we
> should address right this minute.

An IntField is always normalized to a number or None (unset). An empty string is
accepted as an input value, but what we really want is a number or a "not set"
value.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
File quickstart/tests/models/test_envs.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:514: # The local environment metadata is
correctly returned.
On 2013/12/13 17:15:59, benji wrote:
> It would be nice to expand "correctly returned" to describe a little more what
> that means.

Done.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:526: # The ec2 environment metadata is
correctly returned.
On 2013/12/13 17:15:59, benji wrote:
> Same comment for "correctly returned" as above.

Done.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:641: self.assertEqual(expected, obtained)
On 2013/12/13 17:15:59, benji wrote:
> One of my personal preferences, for your consideration: I try to avoid
variable
> names in tests like "expected" and "obtained" and instead try to communicate
> more about the values held.  For example, above I might call them
"valid_pairs"
> and "mapped_pairs".  That way the assertion says more about the truth the test
> is trying to get at: self.assertEqual(mapped_pairs, valid_pairs)
> 
> Note that making the asserts read this way generally means the "obtained"
value
> is the first argument and the "expected" argument is second.  Python's test
> infrastructure is fine with this, but we have to make sure any test-specific
> assertion helpers are too.

Very interesting. +1 on more meaningful assertions.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:650: self.assertEqual(help, field.help)
On 2013/12/13 17:15:59, benji wrote:
> I suspect you intended to pass field.name as the third argument to this assert
> too.

Good catch, thanks.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:689: self.assertEqual({},
envs.validate(self.env_metadata, env_data))
On 2013/12/13 17:15:59, benji wrote:
> It took me a while to understand this test.  Something like this might
> have helped:
> 
> # To be valid, env_data must include the required values.
> env_data = {'string-required': 'a string'}
> validation_errors = envs.validate(self.env_metadata, env_data)
> # No validation errors were found.
> self.assertEqual(validation_errors, {})
> 
> Another thing was that the phrase "at least" primed me to expect the
> test to be demonstrating invalid data instead of valid. 

Done.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:741: 'string-default': '\t another one',
On 2013/12/13 17:15:59, benji wrote:
> Changing the values by removing whitespace makes me nervous.  Maybe for the
> kinds of values we deal with it is OK, but I've been bitten by this more than
> once.  It seems better to me to let the occasional user screw up with
whitespace
> than to prevent another user from doing what they wan to do (like having a
> multi-line config value).

Not sure about leaving leading/trailing spaces, but we should definitely allow
multi-lines values. We already do that, but I added a test to confirm the right
behavior.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
quickstart/tests/models/test_envs.py:776: # Since this value is the default one,
the field is excluded.
On 2013/12/13 17:15:59, benji wrote:
> Given that defaults can change over time, I would be cautious about implicitly
> removing default values that the user had explicitly provided.  If they
provided
> a value we should assume they want to avoid depending on the default.

This is really a great suggestion Benji, thank you! You are right, we want
explictly set values to be there when the environments.yaml is saved. I changed
the fields code so that the default value is used in the validation process but
not in the normalization one.

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_f...
File quickstart/tests/models/test_fields.py (right):

https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_f...
quickstart/tests/models/test_fields.py:318: # If the field is not required no
errors are raised.
On 2013/12/13 17:15:59, benji wrote:
> Since you used a comma in the comment for the next test, you probably intended
> to have one here too.

Done.

[benji, 2013-12-16 13:12:04]

On 2013/12/16 12:12:04, frankban wrote:
> Please take a look.

Thanks for the good explanations and changes.  Everything looks great.

> https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py
> File quickstart/models/fields.py (right):
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#ne...
> quickstart/models/fields.py:128: return super(StringField,
self).normalize(value
> or None)
> On 2013/12/13 17:15:59, benji wrote:
> > If the given value is the empty string then None will be passed to
.normalize
> > and he default will be returned.  That doesn't sound right.
> 
> Changed the module so that default is never returned by normalize: see below.
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/models/fields.py#ne...
> quickstart/models/fields.py:164: return self.default
> On 2013/12/13 17:15:59, benji wrote:
> > So it is not possible to have an empty string as a field value?  I've seen
> > systems with that policy before and it come back to bite them in the long
run.
> 
> > However, it looks like we're already there so I'm not sure it is something
we
> > should address right this minute.
> 
> An IntField is always normalized to a number or None (unset). An empty string
is
> accepted as an input value, but what we really want is a number or a "not set"
> value.

Ah, gotcha.

>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> File quickstart/tests/models/test_envs.py (right):
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:514: # The local environment metadata is
> correctly returned.
> On 2013/12/13 17:15:59, benji wrote:
> > It would be nice to expand "correctly returned" to describe a little more
what
> > that means.
> 
> Done.
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:526: # The ec2 environment metadata is
> correctly returned.
> On 2013/12/13 17:15:59, benji wrote:
> > Same comment for "correctly returned" as above.
> 
> Done.
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:641: self.assertEqual(expected, obtained)
> On 2013/12/13 17:15:59, benji wrote:
> > One of my personal preferences, for your consideration: I try to avoid
> variable
> > names in tests like "expected" and "obtained" and instead try to communicate
> > more about the values held.  For example, above I might call them
> "valid_pairs"
> > and "mapped_pairs".  That way the assertion says more about the truth the
test
> > is trying to get at: self.assertEqual(mapped_pairs, valid_pairs)
> > 
> > Note that making the asserts read this way generally means the "obtained"
> value
> > is the first argument and the "expected" argument is second.  Python's test
> > infrastructure is fine with this, but we have to make sure any test-specific
> > assertion helpers are too.
> 
> Very interesting. +1 on more meaningful assertions.
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:650: self.assertEqual(help, field.help)
> On 2013/12/13 17:15:59, benji wrote:
> > I suspect you intended to pass field.name as the third argument to this
assert
> > too.
> 
> Good catch, thanks.
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:689: self.assertEqual({},
> envs.validate(self.env_metadata, env_data))
> On 2013/12/13 17:15:59, benji wrote:
> > It took me a while to understand this test.  Something like this might
> > have helped:
> > 
> > # To be valid, env_data must include the required values.
> > env_data = {'string-required': 'a string'}
> > validation_errors = envs.validate(self.env_metadata, env_data)
> > # No validation errors were found.
> > self.assertEqual(validation_errors, {})
> > 
> > Another thing was that the phrase "at least" primed me to expect the
> > test to be demonstrating invalid data instead of valid. 
> 
> Done.
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:741: 'string-default': '\t another one',
> On 2013/12/13 17:15:59, benji wrote:
> > Changing the values by removing whitespace makes me nervous.  Maybe for the
> > kinds of values we deal with it is OK, but I've been bitten by this more
than
> > once.  It seems better to me to let the occasional user screw up with
> whitespace
> > than to prevent another user from doing what they wan to do (like having a
> > multi-line config value).
> 
> Not sure about leaving leading/trailing spaces, but we should definitely allow
> multi-lines values. We already do that, but I added a test to confirm the
right
> behavior.

Ah, I misunderstood the whitespace cleaning, thinking it was more aggressive
than it is.

>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_e...
> quickstart/tests/models/test_envs.py:776: # Since this value is the default
one,
> the field is excluded.
> On 2013/12/13 17:15:59, benji wrote:
> > Given that defaults can change over time, I would be cautious about
implicitly
> > removing default values that the user had explicitly provided.  If they
> provided
> > a value we should assume they want to avoid depending on the default.
> 
> This is really a great suggestion Benji, thank you! You are right, we want
> explictly set values to be there when the environments.yaml is saved. I
changed
> the fields code so that the default value is used in the validation process
but
> not in the normalization one.

Sounds good.

>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_f...
> File quickstart/tests/models/test_fields.py (right):
> 
>
https://codereview.appspot.com/41350043/diff/1/quickstart/tests/models/test_f...
> quickstart/tests/models/test_fields.py:318: # If the field is not required no
> errors are raised.
> On 2013/12/13 17:15:59, benji wrote:
> > Since you used a comma in the comment for the next test, you probably
intended
> > to have one here too.
> 
> Done.

[frankban, 2013-12-16 14:08:44]

*** Submitted:

Implement the missing model bits.

Thanks to Gary for his help in defining
this approach with me in several pre-imp
calls.

See the envs module docstring for a
description of the functions implemented
in this branch.

This branch also includes some 
fixes/improvements to the fields module,
e.g. the handling of default and unset values.

Tests: make check
No QA.

R=matthew.scott, benji
CC=
https://codereview.appspot.com/41350043

[frankban, 2013-12-16 15:02:39]

Thank you!