[06:01] <wallyworld_> jam: g'day, can haz mumble?
[07:11] <TheMue> Good morning.
[07:16] <fwereade_> mornings
[07:18] <TheMue> fwereade_: Thx for your reviews. And yes, so far I've concentrated on positive testing. I will harden it now.
[07:23] <TheMue> fwereade_: Btw, over night I found a little trouble with your regex for valid machine ids.
[07:23] <TheMue> fwereade_: Right now leading zeros are ok, like "000000001".
[07:24] <TheMue> fwereade_: I don't think that's wanted.
[07:24] <fwereade_> TheMue, hmm, interesting
[07:24] <fwereade_> TheMue, yeah, thank you, good catch
[07:24] <TheMue> fwereade_: Cheers.
[09:06] <fwereade_> morning rogpeppe1
[09:08] <TheMue> rogpeppe1: Good morning.
[09:35] <fwereade_> hm, I appear to be nutritionally challenged, need to pop out for a croissant or something, bbs
[09:44] <rogpeppe1> fwereade_, TheMue: morning!
[09:45] <TheMue> rogpeppe1: Hiay.
[09:46]  * TheMue has somewhat crooked fingers again this morning.
[10:20] <rogpeppe1> fwereade_, TheMue: if you've a moment at some point, i would quite like a review of a branch that fixes an earlier trunk-reverted branch. this CL is exactly the changes that were LGTM'd, submitted and reverted: https://codereview.appspot.com/6852081/. this CL holds the changes that i've made to fix it: https://codereview.appspot.com/6850087/
[10:24] <TheMue> rogpeppe1: *click*
[10:26] <TheMue> rogpeppe1: Could you please tell me more about the current state of the fake home?
[10:26]  * fwereade_ looks
[10:26] <rogpeppe1> TheMue: i'm not sure what you mean, sorry
[10:28] <TheMue> rogpeppe1: As far as I remember the discussion about the fake home has been controversal. But I may be wrong.
[10:28] <rogpeppe1> TheMue: in this case it's just a local helper function
[10:28] <TheMue> rogpeppe1: Ah, ok.
[10:28] <rogpeppe1> TheMue: the controversial thing was making it into a test fixture
[10:29] <TheMue> rogpeppe1: Yes, now I remember.
[10:29] <fwereade_> rogpeppe1, sorry, but I'm having a bit of difficulty orienting myself -- I haven't been paying close attention to your discussions
[10:30] <rogpeppe1> fwereade_: ok, brief summary:
[10:30] <rogpeppe1> fwereade_: the original branch made ca-cert and ca-private-key keys in the config.Config
[10:31] <rogpeppe1> fwereade_: with logic to read them from ~/.juju/<envname>-cert.pem and ~/.juju/<envname>-private-key.pem if necessary
[10:31] <rogpeppe1> fwereade_: (similar to the authorized-keys behaviour)
[10:31] <rogpeppe1> fwereade_: that branch was approved and submitted, but...
[10:32] <rogpeppe1> fwereade_: unfortunately it interacted badly with another parallel change, and a bad test meant i didn't catch it
[10:33] <rogpeppe1> fwereade_: so, after discussion with gustavo, this pair of CLs a) reapplies all the changes that were reverted and b) applies a fix
[10:33] <fwereade_> rogpeppe1, can we drill down into the bad interaction a little?
[10:34] <rogpeppe1> fwereade_: ok. the bad interaction was with juju.Bootstrap
[10:34] <rogpeppe1> fwereade_: which expected the old scheme of putting both certificate and key in the same file
[10:35] <rogpeppe1> fwereade_: the problem was that i was expecting juju.Bootstrap to generate a certificate that the config could read
[10:35] <rogpeppe1> fwereade_: but it didn't
[10:35] <rogpeppe1> fwereade_: (i did have an explicit test for that, but it wasn't working, which is the cause of all these problems)
[10:36] <rogpeppe1> fwereade_: so this is a temporary band-aid fix that gets this huge branch in without breaking trunk so i can fix juju.Bootstrap in the next iteration.
[10:37] <rogpeppe1> fwereade_: (the config stuff will remain as proposed)
[10:38] <rogpeppe1> fwereade_: i really don't to pile more changes into this CL if at all possible
[10:39] <fwereade_> rogpeppe1, I remain confused about whether we're expecting one file or two
[10:40] <fwereade_> rogpeppe1, that []byte in juju/bootstrap.go implies one; the config stuff implies 2
[10:40] <rogpeppe1> fwereade_: this CL leaves us in a half-way house - juju.Bootstrap expectes one; the config expects two
[10:40] <rogpeppe1> fwereade_: but nothing in this branch relies on juju.Bootstrap to generate a certificate
[10:40] <rogpeppe1> fwereade_: or to read a certificate
[10:41] <rogpeppe1> fwereade_: (because all tests explicitly pass in the cert/key pair)
[10:41] <fwereade_> rogpeppe1, ok... but what about actually running juju?
[10:42] <rogpeppe1> fwereade_: that's fine. we don't use juju.Bootstrap.
[10:42] <rogpeppe1> fwereade_: the cert/key pair isn't used anyway
[10:42] <rogpeppe1> yet
[10:43] <rogpeppe1> fwereade_: note the changes here: https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap.go
[10:43] <rogpeppe1> fwereade_: i have tested it live - it works
[10:43] <fwereade_> rogpeppe1, yeah, that was the source of my confusion really :)
[10:44] <rogpeppe1> fwereade_: yeah, the whole thing is a little confusing - two independent branches interacting badly.
[10:44] <rogpeppe1> fwereade_: (i'd abandoned the config change 'cos gustavo said it was crack, but he later changed his mind and said that was just what we wanted)
[10:46] <fwereade_> rogpeppe1, heh :)
[10:47] <dimitern> rogpeppe, mgz, jam: please have a look https://codereview.appspot.com/6858052/ - after fixing what was suggested
[10:48] <mgz> dimitern: sure.
[10:49] <fwereade_> rogpeppe, so... to follow up, you're going to cause [some bootstrap machinery] to generate cert/key where required, and make it mandatory again in config; and then subsequently start depending on it on the server side?
[10:50] <rogpeppe> fwereade_: yes to the first, but actually the cert will remain optional
[10:50] <rogpeppe> fwereade_: because otherwise we can't read a config file without first generating the certificates
[10:50] <fwereade_> rogpeppe, ok, but the cert will not be optional on the server side?
[10:51] <rogpeppe> fwereade_: no indeed. it'll be a required flag in cmd/jujud, and required when creating a cloudinit MachineConfig too.
[10:51] <fwereade_> (grumble grumble, user config and state config are not the same)
[10:52] <rogpeppe> fwereade_: yeah
[10:52] <fwereade_> rogpeppe, ok; and what you're really interested in is review of the second branch, on the basis that the first one has in fact already been LGTMed?
[10:52] <rogpeppe> fwereade_: precisely
[10:52] <mgz> hm, how do to I get the interdiff on cr? or should I just branch this locally...
[10:52] <rogpeppe> mgz: the interdiff?
[10:53] <rogpeppe> mgz: you can get a diff between any two stages in cr by adjusting the "Left" and "Right" popup menus
[10:53] <mgz> dimitern updated his merge proposal, I want to see what changed... I guess I can just use lp.
[10:54] <mgz> the whats?
[10:54] <dimitern> mgz: well, I did the changes in the same branch, pushed and then lbox propose - did I do it wrong?
[10:54] <fwereade_> mgz, look in the "Delta from patch set" column
[10:54] <rogpeppe> mgz: if you're in the "Side by Side Diff" page, there's are two buttons at the top left, labelled "Left" and "right"
[10:55] <mgz> dimitern: nope, I just don't know how to use cr
[10:55] <rogpeppe> mgz: Each one holds one iteration of the review. You can adjust as you like, then click "Go" to see the differences
[10:55] <mgz> there's only '1' listed in that column
[10:56] <fwereade_> mgz, so that is then the diff from patch set one, right?
[10:56] <rogpeppe> mgz: i see two: for instance: https://codereview.appspot.com/6858052/diff2/1:5002/environs/openstack/config.go
[10:57] <rogpeppe> davecheney: yo!
[10:57] <mgz> ah, so it's 0 otherwise
[10:57] <rogpeppe> mgz: "j" and "k" are convenient keyboard shortcuts for moving forward and backwards between files
[10:58] <fwereade_> mgz, yeah, if you expand Patch Set 1 you will see the column is empty
[10:58]  * rogpeppe never uses the "Delta from patch set" column, other than as informational content.
[10:59] <dimitern> cool! I was looking for that too - the delta column is great
[10:59] <rogpeppe> what i usually do is click on a comment, then adjust the "Right" button to point to the latest iteration.
[11:00] <rogpeppe> 'cos i'm almost always interested in what's changed since a given comment
[11:02] <wallyworld_> dimitern: mgz: jam: standup?
[11:03] <TheMue> rogpeppe: You've got a review.
[11:03] <niemeyer> Morning!
[11:03] <rogpeppe> TheMue: thanks!
[11:03] <rogpeppe> niemeyer: yo!
[11:03] <TheMue> niemeyer: Hiya.
[11:04] <dimitern> mgz, jam: wallyworld_ and me are in mumble
[11:05] <mgz> jam is off again today I think, I'll be with you guys shortly
[11:11] <wallyworld_> dimitern: mgz: https://pastebin.canonical.com/78914/
[11:12] <fwereade_> niemeyer, morning
[11:17] <TheMue> lunchtime
[11:17] <mgz> that odd "Get http://example.com/marker" bit is the same as dimitern had, but you're not getting the crash
[11:18] <niemeyer> mgz: This is an issue introduced in 1.0.3
[11:18] <niemeyer> mgz: and unfortunately there isn't a workaround
[11:19] <niemeyer> mgz: Even more unfortunately, I perceived that in advance of the release, and we fixed it
[11:19] <niemeyer> mgz: But luck wanted the bug to be cherrypicked for the release, and not the fix
[11:52] <rogpeppe> i'm having an extended lunch break today, back in about 3 hours
[11:56] <dimitern> rogpeppe: cool :)
[11:58] <niemeyer> rogpeppe: Enjoy
[11:58] <niemeyer> rogpeppe: We should catch up when you're back, to sync up on the branches that should go in
[11:59] <rogpeppe> niemeyer: yes please
[11:59] <rogpeppe> niemeyer: in case you manage to get to it before i return, here's a summary of the one i'm most concerned about:
[11:59] <rogpeppe>  this CL is exactly the changes that were LGTM'd, submitted and reverted: https://codereview.appspot.com/6852081/. this CL holds the changes that i've made to fix it: https://codereview.appspot.com/6850087/
[12:00] <niemeyer> rogpeppe: Ah, great.. so the former one is just the revert reverted :-)
[12:00] <rogpeppe> niemeyer: yeah
[12:00] <niemeyer> rogpeppe: Thanks, that makes things a lot easier
[12:01] <rogpeppe> niemeyer: i hoped it might help :-)
[12:05] <dimitern> fwereade_: ping
[12:06] <dimitern> niemeyer: so now, after having 2 positive reviews, how's the process to land the CL into lp:juju-core?
[12:07] <dimitern> niemeyer: just merge the branch into trunk and push it?
[12:09] <dimitern> TheMue: maybe you can help? ^^
[12:10] <TheMue> dimitern: You're working with lbox?
[12:10] <niemeyer> dimitern: No
[12:10] <niemeyer> dimitern: lbox submit
[12:10] <dimitern> niemeyer: I see, just that?
[12:10] <niemeyer> dimitern: That said, can you please wait a moment on that one
[12:10] <niemeyer> dimitern: Since this is the bootstrap, I'd like to follow at least the first few steps on it
[12:11] <dimitern> niemeyer: sure, I'd like that
[12:12] <niemeyer> dimitern: I'll look up right now so I don't block you
[12:12] <dimitern> niemeyer: thanks!
[12:12] <niemeyer> dimitern: What's "tenant-name"?
[12:13] <dimitern> niemeyer: that's one of openstack's auth credentials
[12:13] <dimitern> niemeyer: it's a bit controversial - sometimes called project-name, and in the api it's called tenant id
[12:13] <niemeyer> dimitern: Cool
[12:13] <niemeyer> dimitern: SO why do we go in the middle?
[12:14] <niemeyer> :)
[12:14] <niemeyer> dimitern: tenant-name isn't either of them :)
[12:14] <dimitern> niemeyer: but the env var is called OS_TENANT_NAME or NOVA_PROJECT_ID
[12:14] <niemeyer> dimitern: Wow, okay
[12:14] <mgz> it's a little more complicated...
[12:14] <niemeyer> Seriously diverse
[12:14] <mgz> what exactly it is depends across versions and deployments, we probably want some fancier logic here in future
[12:15] <niemeyer> mgz: Can you describe a bit more of the issue?
[12:16] <mgz> want the long fairy tale? :)
[12:16] <mgz> so, in the beginning there was nova
[12:16] <dimitern> niemeyer: from the openstack docs: "Credentials are a combination of your username, password, and what tenant (or project) your cloud is running under. " (http://docs.openstack.org/api/quick-start/content/)
[12:16] <niemeyer> Thanks, it would be useful to picture what we should do
[12:17] <mgz> it had code for handling authentication itself#
[12:18] <mgz> and that had support added for using 'project' as a sort of group id for resources, so multiple users could share control of a resource
[12:18] <mgz> with the growth of the project, various things that used to be in nova started getting pulled out into seperate codebases
[12:20] <mgz> the new auth stuff is keystone... which is a complicated history itself... the initial version wasn't accepted but got rewritten as keystone-lite just in time for being a core component of essex
[12:21] <mgz> for... not very good reasons, they use 'tenant' for the same concept, so you as a user are a member of one or more tenants
[12:21] <mgz> which are then the things that own particular resources, and when you request an access token, that token is (generally) associated with a particular tenant
[12:21] <mgz> so, all api calls using that token are scoped to that tenant
[12:22] <mgz> the confusing thing is 'tenant' was already in use as a term for a different concept, particularly with regards to multi-tenancy
[12:22] <niemeyer> Gosh :(
[12:22] <mgz> so, they keystone guys were threatening to rename back to 'project' again... but that's probably too disruptive now
[12:23] <mgz> the name vs id thing is simpler
[12:23] <mgz> the old way of refering to a project in nova was by an integer (basically a unique id straight from the db I think)
[12:24] <mgz> ...is that right?... it might have been a string
[12:24] <mgz> at any rate, the new style is all objects have a UUID associated
[12:25] <niemeyer> mgz: Okay, and how did "name" get in there?
[12:25] <mgz> and any that used to have a unique name still have that, and some api calls take a name rather than the UUID
[12:25] <niemeyer> mgz: Is it a uuid or something else?
[12:25] <mgz> it was originally a string name in the first versions of the code, so eg, my username is gz, the default tenant that gets created just for me is gz_project
[12:26] <mgz> that's nice because you can see at a glance what the tenant is about
[12:27] <mgz> but... when constructing urls and such like, the style is /object/UUID/stuff
[12:27] <niemeyer> OMG
[12:27] <niemeyer> :)
[12:28] <niemeyer> It's hard to believe such a trivial concept can get so involved
[12:28] <mgz> for keystone and tenants, this diversion doesn't matter too much, as there's basically only one api call juju needs to use (everything else is admin related)
[12:28] <mgz> and these days it accepts either the name or the id
[12:28] <mgz> you just need to know which you have.
[12:29] <mgz> the fun part only comes when supporting deployments based on older versions
[12:30] <niemeyer> mgz: I see
[12:30] <mgz> generally though, we always have one way of refering to a tenant, and should be able to tell from the envvar whether it's a name or an id.
[12:30] <niemeyer> mgz: So what's name? :)
[12:30] <niemeyer> mgz: "gz_project"?
[12:30] <mgz> yup.
[12:30] <niemeyer> mgz: and that's automatically assigned by the provider?
[12:30] <niemeyer> mgz: or can you customize it?
[12:31] <mgz> it's part of the keystone user account management stuff
[12:31] <mgz> so, the cloud admin sets it, and you'll get one in your account details from them
[12:33] <mgz> eg, on my hp cloud api keys page,
[12:34] <mgz> there's a "Tenant" heading that lists both an id (...which is an integer not a uuid, hp is special) and a name (which is <username>-default-tenant)
[12:36] <niemeyer> mgz: i see
[12:36] <niemeyer> mgz: What about "container"?
[12:36] <mgz> in what context?
[12:36] <niemeyer> mgz: Does it have such an involved history as well? :)
[12:36] <niemeyer> mgz: THe setting
[12:37] <dimitern> niemeyer: the container is the equivalent of control-bucket for aws
[12:37] <mgz> ah, I think I just used bucket-name for that in the python as there's no useful reason to distinguish it from S3
[12:38] <niemeyer> Can we use "control-bucket" then?
[12:38] <mgz> that's the thingy.
[12:38] <dimitern> mgz: but in the python version, the s3-compatible api is used, not swift where there are no buckets
[12:39] <mgz> dimitern: both are supported
[12:39] <mgz> and both use control-bucket for the key name as that's then less confusing (except for the person writing the swift api code)
[12:39] <mgz> I wonder what the benefit fo expecting the user to configure that is though
[12:39] <dimitern> mgz: exactly, the swift api uses containers instead of buckets
[12:40] <mgz> have had people having issues because they used "yourrandombucketname" literally... as did someone else
[12:40] <dimitern> niemeyer: sure, we can use "control-bucket", but won't it be confusing for people who know openstack swift?
[12:41] <mgz> dimitern: that can safely be assumed to be a minority of people :)
[12:41] <niemeyer> dimitern: I'd prefer to not confuse people that know juju
[12:41] <dimitern> niemeyer: ok, I can change container->controlBucket then
[12:41] <niemeyer> dimitern: I don't like control-bucket either, to be honest.. I hope we can kill it soon, on both providers
[12:41] <mgz> right, having it manually configured is... not pretty
[12:42] <niemeyer> mgz: Yeah, it's actually automatically generated in Python at the moment.. we should do that for openstack too
[12:42] <niemeyer> mgz: So a sample file is generate with the field pre-filed
[12:42] <mgz> the issue is it always appears in the example juju configs
[12:42] <niemeyer> mgz: Eventually, juju will not require any external storage
[12:42] <niemeyer> mgz: and then we can kill this for good
[12:42] <dimitern> niemeyer: apart from this, other things look ok to you?
[12:42] <mgz> people too often don't seem to use the pre-fill logic when setting up juju for the first time
[12:42] <niemeyer> dimitern: Btw, I just noticed you have a review from Roger pending
[12:42] <niemeyer> dimitern: I'm providing a review.. nothing large so far
[12:43] <dimitern> niemeyer: pending? he did it yesterday and I fixed what he proposed..
[12:44] <mgz> I think you can carry over the lgtm, it's just marked him as he looked at the previous version
[12:44] <niemeyer> dimitern: Yes, but once someone reviews your code and asks for changes, you should wait for a LGTM from them
[12:44] <niemeyer> dimitern: Or at least an explicit dismissal saying "wait for someone else"
[12:44] <niemeyer> dimitern: "LGTM" is the special key there
[12:44] <dimitern> niemeyer: I see, I just assumed he generally liked it, once I do the changes it will be "LGTM", maybe I'm wrong here
[12:44] <mgz> niemeyer: do you have a lgtm-and-I-trust-you-to-change-these-trivial-things-before-landing concept?
[12:44] <mgz> or are things generally always re-reviewed?
[12:45] <niemeyer> mgz: No, we do have that
[12:45] <niemeyer> mgz: "LGTM with the following:"
[12:45] <niemeyer> mgz: That assumes the reviewer trusts it to go in, assuming the suggestions are sensible and are applied
[12:45] <dimitern> niemeyer: Ok, I see, I'll ping roger later for re-review
[12:45] <niemeyer> mgz: Sometimes it doesn't quite fly because the author doesn't think they are sensible, which causes talks and re-reviews
[12:45] <niemeyer> mgz: But that's the exception
[12:45] <mgz> thanks.
[12:46] <niemeyer> No problem
[12:47] <dimitern> niemeyer: in the mean time, I started another related branch to implement private/public addresses in the provider
[12:47] <niemeyer> dimitern: Super
[12:47] <niemeyer> dimitern: You can even push that to review if you get to completion, by using -req
[12:47] <niemeyer> dimitern: on lbox
[12:47] <dimitern> niemeyer: yeah, that's my plan
[12:54] <niemeyer> dimitern: Review sent
[12:54] <niemeyer> dimitern: Very nice bootstrap
[12:54] <dimitern> niemeyer: thanks!
[13:15] <dimitern> niemeyer: updated as suggested https://codereview.appspot.com/6858052/
[13:17] <niemeyer> dimitern: Thanks!
[13:19] <dimitern> niemeyer: so now, as soon as roger gives his final LGTM, I can lbox submit it?
[13:20] <niemeyer> dimitern: Yep!
[13:20] <dimitern> super, thanks
[13:25] <dimitern> niemeyer: one more question about the lbox process - so, after I have a review I see I can reply to each inline comment (done or reply), once I do that I send my replies, should I do lbox propose again, or I need just to push the branch changes only?
[13:26] <niemeyer> dimitern: You need to do propose again
[13:26] <dimitern> ok
[13:26] <niemeyer> dimitern: Otherwise the diff in Rietveld isn't updated
[13:26] <dimitern> niemeyer: so I shouldn't worry I'm sending too many emails for a CL :)
[13:26] <niemeyer> dimitern: Note that if you write down drafts after the comments (done, reply, etc), you can do "lbox propose" and it'll publish your drafts as well
[13:27] <dimitern> niemeyer: nice! so I don't need to publish them from the site myself
[13:27] <niemeyer> dimitern: Thta's right
[13:29] <fwereade_> dimitern, sorry, just back from lunch
[13:33] <dimitern> fwereade_: np, just got some reviews flowing :)
[13:33] <fwereade_> dimitern, cool
[13:34] <dimitern> fwereade_: if you want to take a look: https://codereview.appspot.com/6858052/
[13:34]  * fwereade_ looks
[13:37] <fwereade_> dimitern, just a quick thought: can you at least check that the region != ""? or can even that be valid?
[13:37] <dimitern> fwereade_: good point, I can check for this
[13:37] <dimitern> fwereade_: I has to be set to something at least
[13:41] <fwereade_> dimitern, maybe also worth checking that auth-url is actually a valid URL?
[13:41] <dimitern> fwereade_: I thought about this, but was not sure of an easy way to parse/check it - anything in go like urlparse?
[13:42] <fwereade_> dimitern, net/url
[13:42] <dimitern> fwereade_: cool, I'll check it out
[13:42] <fwereade_> dimitern, has a Parse func
[13:43] <fwereade_> dimitern, and if I'm being really picky I might suggest positive tests for changing the values that can/should change... and I'm suspicious of making the firewall mode changeable
[13:44] <fwereade_> rogpeppe, TheMue, niemeyer: *should* firewall-mode be mutable? (and should it even maybe be common to all providers?)
[13:44] <dimitern> fwereade_: don't know about the firewall mode
[13:44] <fwereade_> dimitern, nor do I really, hence my question above :)
[13:47] <TheMue> fwereade_: Hard to say. For sure it would be implementable, but with a lot changes. And which reason?
[13:47] <TheMue> fwereade_: OK, maybe if we reach the limit of EC2 dynamically switch to global mode.
[13:47]  * TheMue thinks.
[13:52] <TheMue> fwereade_: Why do you think about making it changable?
[13:53] <fwereade_> TheMue, just because it appears that it is
[13:53] <fwereade_> TheMue, and I was wondering whetherthat was sane
[13:54] <TheMue> fwereade_: It is? Have to take a look.
[13:55] <fwereade_> TheMue, it appears to be in dimitern's CL; haven't looked at EC2
[13:55] <niemeyer> fwereade_: I think it shouldn't be allowed
[13:55] <niemeyer> fwereade_: One of the reasons why I didn't suggest restricting yet is because it's handy for tests to be able to change it
[13:56] <fwereade_> niemeyer, both are perfectly reasonable, but somewhat in conflict :)
[13:57] <fwereade_> niemeyer, do you have an opinion on making it part of config.Config rather than provider-specific?
[13:57] <niemeyer> fwereade_: They are in conflict indeed, and the behavior on changes isn't user friendly, so you're right.. the final implementation should reject changes
[13:58] <niemeyer> fwereade_: Making what part of config.Config? (it is already there?)
[13:58] <fwereade_> niemeyer, I don't think so; it seems to be in danger of being duplicated across ec2/openstack
[13:59] <fwereade_> niemeyer, (fwiw, ISTM that it's not *overwhelmingly* handy to change it in tests when we could have separate env setup for separate tests...)
[13:59] <niemeyer> fwereade_: FirewallMode and the respective settings are all in config.. what are you referring to, more exactly?
[14:00] <fwereade_> niemeyer, ok, perhaps I am smoking crack, I had expected that if it were handled in config.Config we wouldn't need to touch it in provider-specific configs
[14:00] <niemeyer> fwereade_: Sorry, I still don't know what you're referring to
[14:02] <fwereade_> niemeyer, I am talking about FirewallMode, but it appears I'm being dumb -- it is properly handled in config.Config, but it seems it also needs to be checked in Validate for each provider
[14:03] <niemeyer> fwereade_: Ah, yes, it does
[14:03] <niemeyer> fwereade_: Only because each provider offers its own idea of what is possible or not, and what's the default
[14:03] <niemeyer> fwereade_: But I think that's inherent to the problem
[14:03] <niemeyer> fwereade_: (IOW, we can't avoid it)
[14:03] <fwereade_> niemeyer, yeah, that makes sense
[14:04] <fwereade_> niemeyer, sorry noise :)
[14:04] <niemeyer> fwereade_: np at  all
[14:04] <fwereade_> so, dimitern, looks like the firewall handling is just fine
[14:04] <fwereade_> dimitern, niemeyer: but when we tighten it up and make it unchangeable we should be sure to do so in each Validate method
[14:05] <dimitern> fwereade_: cool, well I just replicated the ec2 tests there - I supposed should be there
[14:05] <fwereade_> dimitern, yeah, seems sane
[14:07] <TheMue> fwereade_: I also looked and it seems that ec2 allows to change/set it in SetConfig() of environ. But The overall EnvironmentProvider and Environment interfaces doen't provide that method. The …Provider only takes the Config at Open().
[14:15] <dimitern> so, when I have something for review, should I always post it here, or somebody will see it and pick it up?
[14:17] <fwereade_> TheMue, Environ certainly does expose SetConfig
[14:17] <fwereade_> dimitern, I don't think it ever does any harm to announce it here
[14:18] <dimitern> cool :) so here it is: https://codereview.appspot.com/6843106
[14:18] <TheMue> fwereade_: Ooops, indeed. *facepalm*
[14:19] <TheMue> fwereade_: Btw, review, I've hardened code and tests after your last review for the first CL and now doing the same for the second one.
[14:19] <TheMue> dimitern: I'll take a look.
[14:20] <fwereade_> TheMue, cool, I'll take a look now
[14:22] <dimitern> TheMue: 10x
[14:32] <TheMue> dimitern: You've got a review.
[14:32] <dimitern> TheMue: thank you!
[14:32] <TheMue> dimitern: YW.
[14:33]  * niemeyer => lunch
[14:33] <dimitern> TheMue: the EC2 docs describing metadata is the only reference document, also implemented in openstack, the first one is a mistake (forgot to change the comment) - will fix it
[14:34] <TheMue> dimitern: Ah, IC. Something learned. ;) Thanks.
[14:36] <fwereade_> TheMue, https://codereview.appspot.com/6852065/ reviewed
[14:36] <TheMue> fwereade_: Thanks.
[14:38] <TheMue> fwereade_: Good ones, will change it.
[14:38] <fwereade_> TheMue, cheers :)
[14:42] <dimitern> fwereade_: it seems url.Parse is too lenient - "some url" is valid for it and it parses it as Path="some url", other fields empty
[14:50] <fwereade_> dimitern, ha!
[14:51] <fwereade_> dimitern, I guess you could check for presence of other fields
[14:51] <fwereade_> dimitern, but I'm not too worried about it tbh
[14:52] <dimitern> fwereade_: ok, I'll dig in a bit more
[14:57] <rogpeppe> back
[15:03] <dimitern> rogpeppe: heya, can you please recheck my CL: https://codereview.appspot.com/6858052/
[15:04] <rogpeppe> dimitern: i'm on it
[15:04] <rogpeppe> dimitern: LGTM
[15:04] <rogpeppe> dimitern: get it submitted!
[15:04] <dimitern> rogpeppe: thanks! so now I can submit it
[15:04] <dimitern> yeah
[15:06] <dimitern> rogpeppe: and the other related one: https://codereview.appspot.com/6843106/
[15:11] <rogpeppe> dimitern: reviewed
[15:13] <dimitern> rogpeppe: thanks!
[15:13] <dimitern> rogpeppe: about whether it's too early - well, there are the tests, and we might forget about it much later - when we get to deploying stuff, that's why
[15:14] <rogpeppe> dimitern: you won't forget about it - your code will panic on the server side and it'll be obvious.
[15:15] <rogpeppe> dimitern: what might be less obvious is if the code is subtly broken for some reason and it doesn't work live - then you'll not be entirely sure which bit of the system is broken.
[15:16] <rogpeppe> dimitern: that's why i prefer to see code go in that can be tested for real (obviously it's not always possible, but in this case i think it might be worth it)
[15:16] <dimitern> rogpeppe: so you suggest to shelve it for now, but when will it be appropriate to land it?
[15:16] <rogpeppe> dimitern: when you've got a working bootstrap (i.e. something will actually call PrivateAddress or PublicAddress for real on the server side)
[15:16] <rogpeppe> dimitern: until then, it's fine to leave as a stub, i think
[15:17] <rogpeppe> dimitern: but if others think it should go in, i'm happy with that too.
[15:17] <dimitern> rogpeppe: I see, ok then I'll just fix the comments for now
[15:32] <TheMue> fwereade_: You've got a very critical eye (Do you say so?) - and that's very good. ;) I hope https://codereview.appspot.com/6852065/ now gets approved.
[15:32] <fwereade_> TheMue, it's one of those useful phrases that can be positive or negative :) I will take in the spirit in which it is intended ;)
[15:33] <TheMue> fwereade_: You, absolute positive meant. Is there a better phrase for it?
[15:33] <TheMue> Argh, my fingers again!
[15:33] <TheMue> s/You/Yes/
[15:34] <fwereade_> TheMue, hmm, I can't think of any way of expressing that quality that doesn't critically depend on the tone of voice ;)
[15:36] <TheMue> fwereade_: Maybe I have to use diacritical chars next time. :D
[15:37] <fwereade_> TheMue, LGTM :)
[15:37] <TheMue> fwereade_: *hug*
[15:38] <fwereade_> TheMue, I don't seem to be able to officially Approve it, but you have your 2 :)
[15:38] <TheMue> fwereade_: Ah, and instead of diacritics I meant phonetics.
[15:39] <fwereade_> TheMue, ah, yes, true, my pedantry isn't quite as advanced as that :)
[15:42] <TheMue> rogpeppe: Hmm, still the stderr. So far the lxc commands return error codes I return too as Go errors.
[15:42] <rogpeppe> TheMue: don't they print anything when something goes wrong?
[15:43] <rogpeppe> TheMue: for example, if i run lxc-start not as root, it prints "lxc-start: Not running with sufficient privilege"
[15:43] <rogpeppe> TheMue: but your code will return the error "exit code 255"
[15:43] <rogpeppe> TheMue: guess which one would be easier to debug? :-)
[15:44] <TheMue> rogpeppe: The 255. *lol*
[15:44] <TheMue> rogpeppe: OK, gotcha, will change it.
[15:45] <rogpeppe> TheMue: you might want to experiment with the lxc commands to see how to make the output into nicer Go errors. for example, you probably want to strip off the "lxc-[^:]*: " prefix.
[15:46] <TheMue> rogpeppe: Good idea, thx.
[15:52] <TheMue> rogpeppe: Any idea on how to skip a whole suite, not just one test?
[15:52] <rogpeppe> TheMue: call Skip inside SetUpSuite, i believe
[15:53] <TheMue> rogpeppe: Yes, just found it in the docs.
[15:53] <TheMue> rogpeppe: Thx.
[15:55]  * niemeyer respawns
[15:57] <TheMue> rogpeppe: Works.
[16:00] <niemeyer> rogpeppe: This is up for review, despite the fact it has been submitted already: https://codereview.appspot.com/6846066/
[16:01] <niemeyer> rogpeppe: What's its actual status?
[16:01] <rogpeppe> niemeyer: now back to Merged status
[16:03] <niemeyer> rogpeppe: Cheers
[16:05] <dimitern> niemeyer: I'd like your view on this as well please: https://codereview.appspot.com/6843106/
[16:05] <niemeyer> dimitern: Sounds good, just let me clean the queue for roger and will back to you
[16:06] <dimitern> niemeyer: sure, no rush
[16:29] <niemeyer> rogpeppe: Review sent
[16:29] <rogpeppe> niemeyer: thanks
[17:00] <dimitern> niemeyer: ping
[17:00] <niemeyer> dimitern: Pongus
[17:00] <dimitern> niemeyer: sorry :) ^^
[17:01] <niemeyer> dimitern: I'm on it
[17:01] <dimitern> niemeyer: thanks
[17:04] <niemeyer> dimitern: Done!
[17:05] <dimitern> niemeyer: cool, 10x
[17:07] <dimitern> niemeyer: how about rogpeppe's comment, i think he could right that it's too soon to land this
[17:08] <fwereade_> ok, I've got to dash
[17:08] <niemeyer> fwereade_: Have a good.. hmm.. dashing? :)
[17:08] <dimitern> fwereade_: see ya :)
[17:09] <fwereade_> niemeyer, I'm on holiday tomorrow but I will pop on for a brief discussion of the machine thing
[17:09] <niemeyer> dimitern: I thought you actually added the tests in the branch itself?
[17:09] <fwereade_> niemeyer, I think I will know in my own heart whether it's worth it sometime later, but I'll need to run it by you too :)
[17:09] <fwereade_> dimitern, I just remebered we're having an early dinner with someone, might ping you later and see if you're up for a drink
[17:09] <niemeyer> fwereade_: Sounds good.. we should have a voice call, just to sync up
[17:10] <dimitern> niemeyer: yeah, the tests with a locally served canned metadata work the same
[17:10] <dimitern> fwereade_: sgtm
[17:11] <fwereade_> niemeyer, sgtm, I'll grab yu when I can
[17:11] <fwereade_> gn all
[17:11] <niemeyer> fwereade_: Night!
[17:12] <niemeyer> dimitern: Yeah, I think it's fine.. rogpeppe?
[17:12] <rogpeppe> niemeyer: i kinda think it's a bit early but i don't mind much. it's going to be long time until this code is actually tested for real.
[17:13] <dimitern> rogpeppe: not too long i hope :) we're picking up speed
[17:13] <rogpeppe> dimitern: ok :-)
[17:13] <rogpeppe> dimitern: but seriously, you'll have done almost all the provider by the time this gets used
[17:14] <niemeyer> rogpeppe: Early for what?
[17:14] <dimitern> rogpeppe: well, basically all the code is there, but it's being refactored at the moment
[17:14] <niemeyer> rogpeppe: It's necessary code, with some testing alongside
[17:14] <rogpeppe> niemeyer: ok, fair enough
[17:15] <rogpeppe> dimitern: i see. in that case, LGTM
[17:15] <dimitern> rogpeppe: cool, 10x, i'm submitting it then
[17:31] <niemeyer> dimitern: ping
[17:31] <dimitern> niemeyer: pong
[17:31] <niemeyer> dimitern: I noticed that the suggested change wasn't done there
[17:31] <dimitern> niemeyer: which one?
[17:32] <niemeyer> dimitern: The one that was part of the review
[17:32] <dimitern> niemeyer: oops, sorry, how to revert it?
[17:32] <niemeyer> dimitern: Don't have to revert it.. just propose a new CL that does the trivial
[17:33] <dimitern> niemeyer: I see, ok i'm on it
[17:33] <niemeyer> dimitern: Thanks
[17:33] <niemeyer> dimitern: The change is actually not *that* important.. I'm mainly worried about getting used to the workflow
[17:33] <niemeyer> dimitern: So I think it's worth fixing
[17:33] <dimitern> niemeyer: sure, I think is very useful
[17:33] <dimitern> mapping the process :)
[17:35] <niemeyer> dimitern: Yeah :)
[17:43] <dimitern> niemeyer: it seems it worked: https://codereview.appspot.com/6843106/
[17:43] <TheMue> So, have to step out. Second CL will be updated tomorrow. Have a nice evening.
[17:43] <niemeyer> dimitern: Oops
[17:43] <niemeyer> dimitern: It kind of worked
[17:43] <niemeyer> dimitern: It should be a different banch
[17:43] <niemeyer> branch
[17:44] <niemeyer> dimitern: This one was submitted already
[17:44] <dimitern> niemeyer: I see
[17:44] <dimitern> niemeyer: so it won't work like this
[17:44] <niemeyer> dimitern: Nope
[17:44] <dimitern> niemeyer: I'll branch it and repost
[17:44] <niemeyer> dimitern: It's just a matter of changing the branch name, though
[17:46] <dimitern> niemeyer: with cobzr: bzr branch . new-name ?
[17:46] <dimitern> or checkout -b and push
[17:48] <niemeyer> dimitern: bzr branch -m old-name new-name
[17:48] <niemeyer> dimitern: Assuming cobzr
[17:51] <dimitern> niemeyer: yep, it worked: https://codereview.appspot.com/6782102
[18:07] <niemeyer> dimitern: Almost there
[18:07] <niemeyer> dimitern: ("cannot get %q: %v", uri, err)
[18:07] <dimitern> niemeyer: ok
[18:09] <dimitern> niemeyer: done
[18:10] <niemeyer> dimitern: LGTM with the fix
[18:10] <dimitern> niemeyer: thanks, should I submit it?
[18:10] <niemeyer> dimitern: Yes, please
[18:10] <niemeyer> dimitern: This is a follow up on the previous, and trivial as well
[18:10] <dimitern> niemeyer: ok
[18:13] <niemeyer> dimitern: Thanks!
[18:14] <dimitern> niemeyer: thank you! i had a proper kick-start today with the workflow :)
[18:15] <niemeyer> dimitern: Indeed, and well done :)
[18:16] <dimitern> niemeyer: i'm off for today then, thanks again, and good night!
[18:17] <niemeyer> dimitern: Thanks, you too!
[19:08] <rogpeppe> niemeyer: if you're still around, i think this should be better now. the file reading behaviour took a bit more thought than i expected. https://codereview.appspot.com/6850087
[19:08] <niemeyer> rogpeppe: Checking out
[19:12] <niemeyer> rogpeppe: Documentation still feels pretty hard to grasp
[19:12] <rogpeppe> niemeyer: darn
[19:12] <rogpeppe> niemeyer: i thought it was better now
[19:12] <niemeyer> rogpeppe: Do you have any automated realignment tool, btw?
[19:13] <niemeyer> rogpeppe: I've noticed the out of shape comments with some frequency
[19:13] <niemeyer> rogpeppe: Let me try to have a go at the docs
[19:13] <rogpeppe> niemeyer: the documentation for config.New or for the internal maybeReadFile function?
[19:13] <niemeyer> rogpeppe: The internal one
[19:13] <rogpeppe> niemeyer: yeah, i do. i just don't use it as much as i should
[19:14] <niemeyer> rogpeppe: Semantic also seem to have changed in the new version
[19:15] <rogpeppe> niemeyer: the semantics of maybeReadFile have changed, yes. that's because they didn't match the documented behaviour of config.New.
[19:15] <rogpeppe> niemeyer: (i realised that as i looked into it)
[19:16] <rogpeppe> niemeyer: actually, that reminds me - there are some tests i've forgotten to add.
[19:17] <rogpeppe> (testing what happens when both ca-cert and ca-cert-path are both set
[19:17] <rogpeppe> )
[19:18] <niemeyer> rogpeppe: Disabling the default behavior via setting things to empty strings is unlike anything else we have so far, I believe
[19:18] <niemeyer> rogpeppe: and I do recall fwereade being explicitly interested on that behavior
[19:19] <rogpeppe> niemeyer: i think it's important that we be able to specify a certificate only without having the private key read from a file
[19:19] <niemeyer> rogpeppe: Why?
[19:19] <niemeyer> rogpeppe: This is part of the configuration.. if you don't like the file, take it off
[19:19] <niemeyer> rogpeppe: That kind of fine tunning via vague rules is pretty unfriendly to actual users
[19:20] <rogpeppe> niemeyer: you might not have control of the name space. that's true of our tests for example
[19:20] <niemeyer> rogpeppe: Not to mention that it's really hard to grasp what that documentation is saying
[19:20] <niemeyer> rogpeppe: If this, but that, and not that other thing
[19:21] <niemeyer> rogpeppe: I don't understand..
[19:21] <niemeyer> rogpeppe: We have entire control of everything we do
[19:21] <niemeyer> rogpeppe: It's our code base, our disk, our data, our configuration
[19:21] <rogpeppe> niemeyer: we don't necessarily want to set up a fake home for every single test
[19:21] <niemeyer> rogpeppe: Yes, let's not do that.. (?)
[19:22] <rogpeppe> niemeyer: and a configuration is perfectly valid (and potentially more common) without the private key in the config
[19:23] <rogpeppe> niemeyer: if we don't allow this kind of behaviour, we're saying that there is no way to force the config to create a configuration with no private key without it hitting the disk
[19:24] <niemeyer> rogpeppe: No, what I am saying is that it feels bad to reuse behavior that in every other case works in a *different* way
[19:24] <rogpeppe> niemeyer: for example, i want to allow people to create an app that manages its own CA keys without storing them on disk
[19:25] <rogpeppe> niemeyer: ok. we could potentially allow a special value for an unspecified key or cert, such as "none".
[19:26] <niemeyer> rogpeppe: Coincidently, yaml has such a null value
[19:26] <rogpeppe> niemeyer: what do you think CACertPEM should return if there's no certificate specified?
[19:27] <rogpeppe> niemeyer: should we make it return a []byte rather than a string, perhaps?
[19:27] <niemeyer> rogpeppe: We can make it return ""?
[19:28] <rogpeppe> niemeyer: so CACertPEM returns "", but the attributes don't contain "" ?
[19:28] <niemeyer> rogpeppe: The attributes contain "", but CACertPEM doesn't return ""?
[19:28] <rogpeppe> niemeyer: i think i'd expect CACertPEM to return the same as attrs["ca-cert"]
[19:28] <niemeyer> rogpeppe: I'm not sure I understand the point you're making
[19:29] <niemeyer> rogpeppe: So you don't want to read it from disk?
[19:29] <rogpeppe> niemeyer: yup. i want to be able to create a config with, for instance, no cert and no key, and have it work independently of what the disk contents are
[19:30] <niemeyer> rogpeppe: What I mean is that the logic you're suggesting already doesn't make attrs["ca-cert" and CACertPEM be the same thing
[19:30] <rogpeppe> niemeyer: i also want configs to round-trip correctly in all circumstances
[19:30] <niemeyer> rogpeppe: And it also changes the behavior of what defaults do
[19:30] <niemeyer> rogpeppe: i'm finding it surprisingly difficult to get something that looks straightforward going
[19:31] <niemeyer> rogpeppe: We need something to get started
[19:31] <niemeyer> rogpeppe: Something that looks like what we already do
[19:31] <rogpeppe> niemeyer: i'm not sure my "special value" suggestion is any good
[19:31] <niemeyer> rogpeppe: Something that is understandable, well documented, etc
[19:31] <niemeyer> rogpeppe: Let's not do special value.. let's implement exactly the same thing we do with ssh..
[19:32] <niemeyer> rogpeppe: Just not enforcing its presence
[19:32] <rogpeppe> niemeyer: question: if i do config.New(otherConfig.AllAttrs()), should i always end up with the same thing i started with?
[19:32] <niemeyer> rogpeppe: Yes
[19:32] <rogpeppe> niemeyer: how do you plan to enable that?
[19:32] <niemeyer> rogpeppe: How do we do that today?
[19:32] <rogpeppe> niemeyer: we always require authorized-keys
[19:33] <rogpeppe> niemeyer: and that's the only current field we have that potential problem with
[19:34] <niemeyer> rogpeppe: Okay, and how's that a problem in the new world?
[19:35] <rogpeppe> niemeyer: if we create a config with no certificate, that's a valid certificate, but (assuming we don't allow the ""-means-unspecified behaviour), AllAttrs cannot return a value for the ca-cert attribute.
[19:35] <niemeyer> rogpeppe: There's a very good way to say that a key is not present in the configuration.. and that's to have the key not present in the configuration (!)
[19:35] <rogpeppe> niemeyer: if the key isn't present in the configuration, then we hit the disk
[19:36] <rogpeppe> niemeyer: which means that config.New(otherConfig.AllAttrs()) might *not* return the same configuration
[19:36] <niemeyer> rogpeppe: Okay
[19:36] <niemeyer> rogpeppe: So what's the problem with nulling it out?
[19:37] <rogpeppe> niemeyer: having an explicit null value in the map?
[19:37] <niemeyer> rogpeppe: Yes
[19:38] <rogpeppe> niemeyer: we could do that. but clients will just compare the return value of CACertPEM against "" anyway, so it doesn't seem worth it.
[19:38] <niemeyer> rogpeppe: This actually sounds bogus
[19:38] <niemeyer> rogpeppe: We've been pretty good at not allowing invalid values to be handled as valid ones
[19:39] <rogpeppe> niemeyer: i dunno. we guarantee that CACertPEM contains a valid certificate
[19:39] <niemeyer> rogpeppe: If CACertPEM can return an invalid certificate, we should return a pair
[19:39] <niemeyer> (cert, ok)
[19:39] <niemeyer> rogpeppe: Do we? Where?
[19:39] <rogpeppe> niemeyer: (if it's present, i mean)
[19:39] <niemeyer> rogpeppe: "" is not a valid certificate
[19:39] <niemeyer> rogpeppe: We allow that
[19:39] <rogpeppe> niemeyer: indeed. it seems a reasonable way to say "no certificate" to me
[19:40] <niemeyer> rogpeppe: It's also a reasonable way to saying "default", as far as our configuration goes
[19:40] <niemeyer> rogpeppe: It's poor engineering to have both side-by-side
[19:40] <rogpeppe> niemeyer: do we use "" to mean default anywhere?
[19:41] <niemeyer> rogpeppe: Read that function again please
[19:41] <rogpeppe> niemeyer: which function?
[19:41] <niemeyer> rogpeppe: The one you're suggesting changes to
[19:42] <rogpeppe> niemeyer: what about it?
[19:42] <niemeyer>         if c.m["default-series"].(string) == "" {
[19:42] <niemeyer>                 c.m["default-series"] = version.Current.Series
[19:42] <niemeyer>         }
[19:42] <niemeyer>         if path != "" || keys == "" {
[19:42] <niemeyer>                 c.m["authorized-keys"], err = authorizedKeys(path)
[19:42] <niemeyer>         switch firewallMode {
[19:42] <niemeyer>         case FwDefault,
[19:42] <niemeyer>         FwDefault FirewallMode = ""
[19:42] <rogpeppe> oh yeah
[19:43] <rogpeppe> :-)
[19:43] <niemeyer> rogpeppe: :-(
 niemeyer: do we use "" to mean default anywhere?
[19:43] <niemeyer> YEEEES
[19:43] <rogpeppe> gotcha
[19:44] <rogpeppe> niemeyer: ok, so... how about we make CACertPEM return []byte, and nil when not set.
[19:44] <rogpeppe> niemeyer: and have an entry in the map, nil when explicitly not set.
[19:44] <niemeyer> rogpeppe: No, (cert string/[]byte, ok bool), please
[19:45] <niemeyer> rogpeppe: nil in the map sounds fine
[19:46] <rogpeppe> niemeyer: i'm slightly concerned that it won't be obvious what type of thing is expected in the map (which is part of our public interface, after all)
[19:46] <niemeyer> rogpeppe: That's why we have an interface on that type
[19:46] <rogpeppe> niemeyer: there are quite a few tests that do a DeepEqual on the attributes returned from AllAttrs
[19:46] <niemeyer> rogpeppe: It doesn't have to be obvious.. people shouldn't be poking on the map
[19:46] <rogpeppe> niemeyer: maybe they're bogus
[19:46] <niemeyer> rogpeppe: Because that's our tests implementing it
[19:47] <niemeyer> rogpeppe: The tests are fine
[19:47] <niemeyer> rogpeppe: We're verifying that our logic is sane
[19:47] <rogpeppe> niemeyer: i'm talking about tests in state
[19:47] <niemeyer> rogpeppe: Me too
[19:47] <rogpeppe> niemeyer: ok
[19:47] <niemeyer> rogpeppe: I'd like to move forward.. that sounds like a pretty reasonable way to move forward
[19:48] <rogpeppe> niemeyer: what would the YAML-marshalled representation look like?
[19:48] <niemeyer> rogpeppe: ?
[19:48] <niemeyer> rogpeppe: I don't understand the question.. the YAML-marshalled representation will look like YAML
[19:48] <rogpeppe> niemeyer: of a ca-cert that's explicitly not present
[19:48] <rogpeppe> niemeyer: presumably yaml has null or something like it
[19:48] <niemeyer> rogpeppe: Yes, and it's called null :-)
[19:49] <rogpeppe> niemeyer: great
[19:49] <rogpeppe> niemeyer: ok, i'll see how it goes
[19:50] <niemeyer> rogpeppe: Thanks.. I think the defaults should be "", btw, because we'll be checking for that
[19:50] <niemeyer> rogpeppe: (rather than omit)
[19:50] <niemeyer> rogpeppe: Makes both the logic and the consistency better, I believe
[19:51] <rogpeppe> niemeyer: and we have the type as OneOf(String, nil)
[19:51] <rogpeppe> niemeyer: ?
[19:51] <niemeyer> rogpeppe: Right
[19:52] <niemeyer> rogpeppe: I think we'd need that anyway, or it'll barf on the nil
[19:52] <niemeyer> rogpeppe: (not there != nil)
[19:53] <rogpeppe> niemeyer: this all seems fine. sorry it's taken a while to discuss, but it is surprisingly subtle...
[19:53] <niemeyer> rogpeppe: I can see it is indeed
[19:53] <niemeyer> rogpeppe: Glad we found an alternative
[19:54]  * rogpeppe dives once more into the breach
[19:54] <niemeyer> :)
[19:54] <niemeyer> I'll step out to get some food meanwhile
[19:56] <rogpeppe> niemeyer: darn, there's no schema.Null
[19:56] <rogpeppe> niemeyer: i'll make one
[19:56] <rogpeppe> niemeyer: enjoy!
[19:56] <niemeyer> rogpeppe: Const(nil)
[19:56] <rogpeppe> niemeyer: argh! of course.
[21:03] <rogpeppe> niemeyer: done, i hope: https://codereview.appspot.com/6850087/