Code review comment for lp:~dimitern/juju-core/openstack-stub-provider-config
Revision history for this message
| John A Meinel (jameinel) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
LGTM
Worth landing with some tweaks, and iterating from here.
https:/
File environs/
https:/
environs/
string {
On 2012/11/21 17:28:20, gz wrote:
> Using tenant-id vs. tenant-name is something we'll need to watch out
for later.
Just to follow up on this, "tenant-name" is what we'll get from the
environment, and what we would expect users to write themselves.
"tenant-id" would be something that we get from talking to the API.
So if we need to use it/store it we can, but ideally we would just go
with tenant name in the config.
https:/
File environs/
https:/
environs/
runner.
On 2012/11/21 17:26:12, rog wrote:
> this is ubiquitous enough that it probably doesn't need a comment.
I would also mention that I don't usually see 1-line functions. They
pretty much are always wrapped to 3 lines. Am I mistaken about that?
https:/
environs/
*C) {
On 2012/11/21 17:28:20, gz wrote:
> This is kind of ugly, and we'll need to do better for testing envvars
as there
> are a bunch of different ones beyond these. The tests for this in the
python
> code used a change_environment helper that you passed a dict of new
values to
> within the test which were then stashed and restored at the end.
If we switch to using CredentialsFromEnv, do we need a lot of env
variable testing here? It seems like permutation testing can be done at
the goose level, and then some sort of smoke test can be done here.
https:/
environs/
If we are going to go with table based tests instead of a bunch of named
tests, I'd like us to add summary: to the definitions, so that we have
some sort of name identifying what this permutation is trying to do.
Table based tests aren't that different from using a TestSuite that has
a common SetUp and TearDown and then you get a named test case for each
one. Hopefully these have less infrastructure making them easier to use,
but they do have the downside of not having human-sensible names unless
we put them there.
As a thought, I wonder if we might try writing this as TestSuite with
some helper setup functions, rather than as one big 'check' function.
And see which one is clearer. I can see where table based tests have
nice definitions. But you also tend to end up with a 'check' function
that mixes setup and assertions.
The 'check' function can become overloaded with lots of if checks,
because some cases want to test a failure mode, some want to test
setting a value, etc.
So there are bits I like (simple table definition), and bits I'm not
sure about (actual assertions are intermixed with a lot of setup code,
and many assertions are not actually valid for all of the rows in the
table.)
Is it worth trying?
https:/