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