>>>>> Martin Packman writes:

    > Review: Approve
    > +Default values can be acquired from environment variables for backwards
    > +compatibility during a transition period.

    > So, I disagree somewhat here as we've discussed before. For
    > development, I like not having to copy secret values into a config
    > file to test against different clouds, and I don't see how that
    > changes.

Ha, I failed to fully explain what I have in mind here, sorry.

So, the idea is that you setup a test cloud as part of your dev env and,
separately, fire the test suite.

The test suite checks the availability of various test clouds and inject
the corresponding scenarios to exercise them.

Roughly you do (imaginary commands):

  start-cloud essex-precise
  make test
  hack
  make test
  stop-cloud essex-precise
  hack, edit test cloud config file
  start-cloud essex-precise
  make test

Obviously if you have more than one test cloud, relying on env vars is a
no-go.

And if/when we have such test clouds, manual testing should not be
necessary anymore (or become so awkward we abandon it ;). 

Additionally, running bin/awsome without any prior sourcing:

vila:~/src/awsome/trunk :) $ bin/awsome
Traceback (most recent call last):
  File "bin/awsome", line 43, in <module>
    credentials = Credentials.from_config(conf)
  File "/home/vila/src/awsome/trunk/awsome/users.py", line 58, in from_config
    self = cls(conf.get('aws_access_key'), conf.get('aws_secret_key'))
  File "/home/vila/src/awsome/trunk/awsome/users.py", line 25, in __init__
    AWSCredentials.__init__(self, access_key, secret_key)
  File "/home/vila/lib/python/txaws/credentials.py", line 34, in __init__
    raise ValueError("Could not find %s" % ENV_ACCESS_KEY)
ValueError: Could not find AWS_ACCESS_KEY_ID

This demonstrates (don't let the 'from_config' distract you, this is an
old bug also encountered before this mp) that all of us have been
running our manual tests in sourced environments without realizing
*this* issue (there may be others).

'all' here includes me even if --os_auth_scheme=passthrough doesn't
require to set any access key... and is a valid setup for awsome on a
server.

The trap is easy to fall into :-/

Far too easy.

The comment is a reminder of the above issues/missing features, we can
continue to discuss ;)

    > +                if not isinstance(value, str):
    > +                    raise AssertionError(
    > +                    'Callable default values should be strings')

    > Assertions without any context at all are a bit annoying.

I've added the option name.

    > +class MultilineVersion(argparse.Action):

    > Slightly confusing copying this in in this mp as it's only removed
    > from bin/awsome in the followup, but not harmful.

Yeah, that was the easiest way to separate the mps.

    > +def bool_from_string(string):
    > +    val = None
    > +    if isinstance(string, str):
    > +        try:
    > +            val = _valid_boolean_strings[string.lower()]
    > +        except KeyError:
    > +            pass
    > +    return val

    > I'd use a more compact spelling avoiding the isinstance check:

    >     try:
    >         return _valid_boolean_strings[string.lower()]
    >     except (AttributeError, KeyError):
    >         return None

    > Or `str.lower(string)` catching TypeError if you really want to be picky about the type.

I'll take the later.

    > +def set_env_variable(name, value):
    > ...
    > +def overrideEnv(test, name, value):
    > ...
    > +def overrideAttr(test, obj, attr_name, new=_unitialized_attr):

    > I'd stick all these in awsome.tests so we have a one place with
    > all the extra testing bits we needed to bring in.

/me nods

I wanted some feedback before doing that, I'm glad we agree ;)

    > +    def test_first_default_value_from_env_wins(self):
    > +        opt = config.Option('foo', default='bar',
    > +                            default_from_env=['NO_VALUE', 'FOO', 'BAZ'])

    > Not isolated from NO_VALUE being set in the test process.

Wow, good catch, bogus upstream too. test_default_value_from_env also
suffers from that.

    > +        self.assertNotEquals(None, option_help)
    > +        # Come on, think about the user, he really wants to know what the
    > +        # option is about
    > +        self.assertIsNot(None, option_help)

    > First assertion is meaningless given the second, nothing ordinary
    > compares equal to None except itself.

Ahem, also bogus upstream, probably there since day one ;) I suspect I
thought like you but forgot to remove the first assertion when writing
the second one.

    > +    def test_error(self):

    > Not wild about this test, overriding standard streams and catching
    > SystemExit is a little dodgy in tests.

Yeah, me neither at first.

    > Would prefer some refactoring to avoid that, but I suspect
    > argparse doesn't make our lives easy here

Exactly. When I came to realize that argparse was calling stderr.write()
followed by sys.exit() though, the test as it's written is the way to
go.

    > so it's not that important.

Not that important but I think the rationale above explains why the test
should be written this way for now (an alternative would be to override
ArgumentParser.error() and ArgumentParser.exit() which seems far too
heavy weight).

I've pushed the corresponding fixes.