Code review comment for lp:~axwalk/juju-core/sanity-check-constraints
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
https:/
File cmd/juju/
https:/
cmd/juju/
conn.Environ.
On 2013/09/11 14:43:35, fwereade wrote:
> We really shouldn't use conn.Environ. We should probably unexport it,
even.
Sigh, sorry (again). Will fix.
https:/
File cmd/juju/
https:/
cmd/juju/
errors.
On 2013/09/11 12:05:59, jameinel wrote:
> You don't clean up after setting this value, which will give errors in
strange
> ways for other tests (I think).
> You need something like:
> defer func() { dummy.SanityChe
I do, it's just not obvious from here. It's cleaned up in dummy.Reset,
which gets called in the JujuConnSuite's TearDownTest. This is similar
to how dummy.Listen is used.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:329: machineId = strings.
On 2013/09/11 14:43:35, fwereade wrote:
> Yeah, I'm pretty sure we don't want this. :s later in the string
should not be
> turned into /s.
> And I remain really nervous about having this logic right here,
regardless...
> can we break it out a little, and write it in terms of "placement
directives",
> in which the bit before the first : determines the handler for the bit
> afterwards? And thus have an explicit lxc handler, ready to add the
others?
> I do appreciate it's just a code move, but even if you don't fix it
today please
> be sure to document that it's a good-enough-for-now hack at the above
> functionality.
I have added a comment stating this is due for refactoring into
placement directives, and changes in logic should consider that first.
When I've got my current work done, I'll have a look into it.
https:/
juju/conn.go:342: }
On 2013/09/11 12:05:59, jameinel wrote:
> My first thought on reading this was that you don't need to validate
> containerType if you got "3/lxc/2" because that machine must already
exist.
> But looking closer, I'm a bit confused by the logic here. It looks
like you get
> really weird behavior if you do: lxc:2:3
> It actually looks like you could do: lxc:2:lxc:3 to create a new lxc
on machine
> 2/lxc/3, though if we really want to support that, we'd use the syntax
> lxc:2/lxc/3
> (I don't really understand why we have the strings.
> Anyway, I realize you just moved the code. Though I don't fully know
why you had
> to move it.
I didn't have to, I just die a little bit inside when I see code in
loops that doesn't need to be.
https:/
File provider/
https:/
provider/
On 2013/09/11 12:05:59, jameinel wrote:
> It is really unfortunate that it has to be written this way.
> But from what I can tell, you can't take the address of a const, and
you can't
> use &instance.
> You can do it as:
> container := instance.LXC
> cons.Container = &container
> I don't know if that is actually cleaner, though. (I'm just generally
not a big
> fan of new())
Six of one... I've changed it. I don't mind either way.
https:/
File provider/
https:/
provider/
On 2013/09/11 12:05:59, jameinel wrote:
> I don't think we actually support nesting LXC's in the local provider.
So we
> should probably check and return an error here, rather than allowing
it.
Done.
https:/