| wallyworld_ | jam: g'day, can haz mumble? | 06:01 |
|---|---|---|
| TheMue | Good morning. | 07:11 |
| fwereade_ | mornings | 07:16 |
| TheMue | fwereade_: Thx for your reviews. And yes, so far I've concentrated on positive testing. I will harden it now. | 07:18 |
| 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:23 |
| 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. | 07:24 |
| fwereade_ | morning rogpeppe1 | 09:06 |
| TheMue | rogpeppe1: Good morning. | 09:08 |
| fwereade_ | hm, I appear to be nutritionally challenged, need to pop out for a croissant or something, bbs | 09:35 |
| rogpeppe1 | fwereade_, TheMue: morning! | 09:44 |
| TheMue | rogpeppe1: Hiay. | 09:45 |
| * TheMue has somewhat crooked fingers again this morning. | 09:46 | |
| 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:20 |
| TheMue | rogpeppe1: *click* | 10:24 |
| 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:26 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:31 |
| rogpeppe1 | fwereade_: unfortunately it interacted badly with another parallel change, and a bad test meant i didn't catch it | 10:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| 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:36 |
| rogpeppe1 | fwereade_: (the config stuff will remain as proposed) | 10:37 |
| rogpeppe1 | fwereade_: i really don't to pile more changes into this CL if at all possible | 10:38 |
| fwereade_ | rogpeppe1, I remain confused about whether we're expecting one file or two | 10:39 |
| 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:40 |
| rogpeppe1 | fwereade_: (because all tests explicitly pass in the cert/key pair) | 10:41 |
| fwereade_ | rogpeppe1, ok... but what about actually running juju? | 10:41 |
| 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:42 |
| 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:43 |
| 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:44 |
| fwereade_ | rogpeppe1, heh :) | 10:46 |
| === rogpeppe1 is now known as rogpeppe | ||
| dimitern | rogpeppe, mgz, jam: please have a look https://codereview.appspot.com/6858052/ - after fixing what was suggested | 10:47 |
| mgz | dimitern: sure. | 10:48 |
| 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:49 |
| 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:50 |
| 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:51 |
| 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:52 |
| 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:53 |
| 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:54 |
| 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:55 |
| 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:56 |
| 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:57 |
| 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:58 | |
| 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. | 10:59 |
| rogpeppe | 'cos i'm almost always interested in what's changed since a given comment | 11:00 |
| wallyworld_ | dimitern: mgz: jam: standup? | 11:02 |
| 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:03 |
| dimitern | mgz, jam: wallyworld_ and me are in mumble | 11:04 |
| mgz | jam is off again today I think, I'll be with you guys shortly | 11:05 |
| wallyworld_ | dimitern: mgz: https://pastebin.canonical.com/78914/ | 11:11 |
| fwereade_ | niemeyer, morning | 11:12 |
| 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:17 |
| niemeyer | mgz: This is an issue introduced in 1.0.3 | 11:18 |
| niemeyer | mgz: and unfortunately there isn't a workaround | 11:18 |
| 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:19 |
| rogpeppe | i'm having an extended lunch break today, back in about 3 hours | 11:52 |
| dimitern | rogpeppe: cool :) | 11:56 |
| 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:58 |
| 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/ | 11:59 |
| 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:00 |
| rogpeppe | niemeyer: i hoped it might help :-) | 12:01 |
| dimitern | fwereade_: ping | 12:05 |
| dimitern | niemeyer: so now, after having 2 positive reviews, how's the process to land the CL into lp:juju-core? | 12:06 |
| dimitern | niemeyer: just merge the branch into trunk and push it? | 12:07 |
| dimitern | TheMue: maybe you can help? ^^ | 12:09 |
| 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:10 |
| dimitern | niemeyer: sure, I'd like that | 12:11 |
| 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:12 |
| 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:13 |
| 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:14 |
| niemeyer | mgz: Can you describe a bit more of the issue? | 12:15 |
| 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:16 |
| mgz | it had code for handling authentication itself# | 12:17 |
| 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:18 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| 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:24 |
| 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:25 |
| mgz | that's nice because you can see at a glance what the tenant is about | 12:26 |
| mgz | but... when constructing urls and such like, the style is /object/UUID/stuff | 12:27 |
| niemeyer | OMG | 12:27 |
| niemeyer | :) | 12:27 |
| 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:28 |
| mgz | the fun part only comes when supporting deployments based on older versions | 12:29 |
| 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:30 |
| 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:31 |
| mgz | eg, on my hp cloud api keys page, | 12:33 |
| 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:34 |
| 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:36 |
| 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:37 |
| 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:38 |
| 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:39 |
| 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:40 |
| 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:41 |
| 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:42 |
| dimitern | niemeyer: pending? he did it yesterday and I fixed what he proposed.. | 12:43 |
| 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:44 |
| 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:45 |
| niemeyer | No problem | 12:46 |
| 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:47 |
| niemeyer | dimitern: Review sent | 12:54 |
| niemeyer | dimitern: Very nice bootstrap | 12:54 |
| dimitern | niemeyer: thanks! | 12:54 |
| dimitern | niemeyer: updated as suggested https://codereview.appspot.com/6858052/ | 13:15 |
| niemeyer | dimitern: Thanks! | 13:17 |
| dimitern | niemeyer: so now, as soon as roger gives his final LGTM, I can lbox submit it? | 13:19 |
| niemeyer | dimitern: Yep! | 13:20 |
| dimitern | super, thanks | 13:20 |
| 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:25 |
| 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:26 |
| dimitern | niemeyer: nice! so I don't need to publish them from the site myself | 13:27 |
| niemeyer | dimitern: Thta's right | 13:27 |
| fwereade_ | dimitern, sorry, just back from lunch | 13:29 |
| dimitern | fwereade_: np, just got some reviews flowing :) | 13:33 |
| fwereade_ | dimitern, cool | 13:33 |
| dimitern | fwereade_: if you want to take a look: https://codereview.appspot.com/6858052/ | 13:34 |
| * fwereade_ looks | 13:34 | |
| 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:37 |
| 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:41 |
| fwereade_ | dimitern, net/url | 13:42 |
| dimitern | fwereade_: cool, I'll check it out | 13:42 |
| fwereade_ | dimitern, has a Parse func | 13:42 |
| 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:43 |
| 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:44 |
| 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:47 | |
| TheMue | fwereade_: Why do you think about making it changable? | 13:52 |
| fwereade_ | TheMue, just because it appears that it is | 13:53 |
| fwereade_ | TheMue, and I was wondering whetherthat was sane | 13:53 |
| TheMue | fwereade_: It is? Have to take a look. | 13:54 |
| 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:55 |
| fwereade_ | niemeyer, both are perfectly reasonable, but somewhat in conflict :) | 13:56 |
| 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:57 |
| 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:58 |
| 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? | 13:59 |
| 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:00 |
| 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:02 |
| 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:03 |
| 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:04 |
| dimitern | fwereade_: cool, well I just replicated the ec2 tests there - I supposed should be there | 14:05 |
| fwereade_ | dimitern, yeah, seems sane | 14:05 |
| 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:07 |
| dimitern | so, when I have something for review, should I always post it here, or somebody will see it and pick it up? | 14:15 |
| 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:17 |
| dimitern | cool :) so here it is: https://codereview.appspot.com/6843106 | 14:18 |
| TheMue | fwereade_: Ooops, indeed. *facepalm* | 14:18 |
| 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:19 |
| fwereade_ | TheMue, cool, I'll take a look now | 14:20 |
| === edamato is now known as edamato-lunch | ||
| dimitern | TheMue: 10x | 14:22 |
| TheMue | dimitern: You've got a review. | 14:32 |
| dimitern | TheMue: thank you! | 14:32 |
| TheMue | dimitern: YW. | 14:32 |
| * 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:33 |
| TheMue | dimitern: Ah, IC. Something learned. ;) Thanks. | 14:34 |
| fwereade_ | TheMue, https://codereview.appspot.com/6852065/ reviewed | 14:36 |
| TheMue | fwereade_: Thanks. | 14:36 |
| TheMue | fwereade_: Good ones, will change it. | 14:38 |
| fwereade_ | TheMue, cheers :) | 14:38 |
| 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:42 |
| fwereade_ | dimitern, ha! | 14:50 |
| 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:51 |
| dimitern | fwereade_: ok, I'll dig in a bit more | 14:52 |
| rogpeppe | back | 14:57 |
| dimitern | rogpeppe: heya, can you please recheck my CL: https://codereview.appspot.com/6858052/ | 15:03 |
| 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:04 |
| dimitern | rogpeppe: and the other related one: https://codereview.appspot.com/6843106/ | 15:06 |
| rogpeppe | dimitern: reviewed | 15:11 |
| 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:13 |
| rogpeppe | dimitern: you won't forget about it - your code will panic on the server side and it'll be obvious. | 15:14 |
| 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:15 |
| 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:16 |
| 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:17 |
| === edamato-lunch is now known as edamato | ||
| 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:32 |
| 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:33 |
| 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:34 |
| TheMue | fwereade_: Maybe I have to use diacritical chars next time. :D | 15:36 |
| fwereade_ | TheMue, LGTM :) | 15:37 |
| TheMue | fwereade_: *hug* | 15:37 |
| 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:38 |
| fwereade_ | TheMue, ah, yes, true, my pedantry isn't quite as advanced as that :) | 15:39 |
| 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:42 |
| 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:43 |
| TheMue | rogpeppe: The 255. *lol* | 15:44 |
| TheMue | rogpeppe: OK, gotcha, will change it. | 15:44 |
| 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:45 |
| TheMue | rogpeppe: Good idea, thx. | 15:46 |
| 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:52 |
| TheMue | rogpeppe: Yes, just found it in the docs. | 15:53 |
| TheMue | rogpeppe: Thx. | 15:53 |
| * niemeyer respawns | 15:55 | |
| TheMue | rogpeppe: Works. | 15:57 |
| niemeyer | rogpeppe: This is up for review, despite the fact it has been submitted already: https://codereview.appspot.com/6846066/ | 16:00 |
| niemeyer | rogpeppe: What's its actual status? | 16:01 |
| rogpeppe | niemeyer: now back to Merged status | 16:01 |
| niemeyer | rogpeppe: Cheers | 16:03 |
| 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:05 |
| dimitern | niemeyer: sure, no rush | 16:06 |
| niemeyer | rogpeppe: Review sent | 16:29 |
| rogpeppe | niemeyer: thanks | 16:29 |
| dimitern | niemeyer: ping | 17:00 |
| niemeyer | dimitern: Pongus | 17:00 |
| dimitern | niemeyer: sorry :) ^^ | 17:00 |
| niemeyer | dimitern: I'm on it | 17:01 |
| dimitern | niemeyer: thanks | 17:01 |
| niemeyer | dimitern: Done! | 17:04 |
| dimitern | niemeyer: cool, 10x | 17:05 |
| dimitern | niemeyer: how about rogpeppe's comment, i think he could right that it's too soon to land this | 17:07 |
| fwereade_ | ok, I've got to dash | 17:08 |
| niemeyer | fwereade_: Have a good.. hmm.. dashing? :) | 17:08 |
| dimitern | fwereade_: see ya :) | 17:08 |
| 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:09 |
| dimitern | niemeyer: yeah, the tests with a locally served canned metadata work the same | 17:10 |
| dimitern | fwereade_: sgtm | 17:10 |
| fwereade_ | niemeyer, sgtm, I'll grab yu when I can | 17:11 |
| fwereade_ | gn all | 17:11 |
| niemeyer | fwereade_: Night! | 17:11 |
| 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:12 |
| 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:13 |
| 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:14 |
| rogpeppe | dimitern: i see. in that case, LGTM | 17:15 |
| dimitern | rogpeppe: cool, 10x, i'm submitting it then | 17:15 |
| 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:31 |
| 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:32 |
| 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:33 |
| niemeyer | dimitern: Yeah :) | 17:35 |
| 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:43 |
| 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:44 |
| dimitern | niemeyer: with cobzr: bzr branch . new-name ? | 17:46 |
| dimitern | or checkout -b and push | 17:46 |
| niemeyer | dimitern: bzr branch -m old-name new-name | 17:48 |
| niemeyer | dimitern: Assuming cobzr | 17:48 |
| dimitern | niemeyer: yep, it worked: https://codereview.appspot.com/6782102 | 17:51 |
| niemeyer | dimitern: Almost there | 18:07 |
| niemeyer | dimitern: ("cannot get %q: %v", uri, err) | 18:07 |
| dimitern | niemeyer: ok | 18:07 |
| dimitern | niemeyer: done | 18:09 |
| 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:10 |
| niemeyer | dimitern: Thanks! | 18:13 |
| dimitern | niemeyer: thank you! i had a proper kick-start today with the workflow :) | 18:14 |
| niemeyer | dimitern: Indeed, and well done :) | 18:15 |
| dimitern | niemeyer: i'm off for today then, thanks again, and good night! | 18:16 |
| niemeyer | dimitern: Thanks, you too! | 18:17 |
| 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:08 |
| 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:12 |
| 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:13 |
| niemeyer | rogpeppe: Semantic also seem to have changed in the new version | 19:14 |
| 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:15 |
| rogpeppe | niemeyer: actually, that reminds me - there are some tests i've forgotten to add. | 19:16 |
| rogpeppe | (testing what happens when both ca-cert and ca-cert-path are both set | 19:17 |
| rogpeppe | ) | 19:17 |
| 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:18 |
| 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:19 |
| 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:20 |
| 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:21 |
| rogpeppe | niemeyer: and a configuration is perfectly valid (and potentially more common) without the private key in the config | 19:22 |
| 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:23 |
| 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:24 |
| rogpeppe | niemeyer: ok. we could potentially allow a special value for an unspecified key or cert, such as "none". | 19:25 |
| 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:26 |
| rogpeppe | niemeyer: should we make it return a []byte rather than a string, perhaps? | 19:27 |
| niemeyer | rogpeppe: We can make it return ""? | 19:27 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:31 |
| 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:32 |
| rogpeppe | niemeyer: and that's the only current field we have that potential problem with | 19:33 |
| niemeyer | rogpeppe: Okay, and how's that a problem in the new world? | 19:34 |
| 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:35 |
| 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:36 |
| rogpeppe | niemeyer: having an explicit null value in the map? | 19:37 |
| niemeyer | rogpeppe: Yes | 19:37 |
| 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:38 |
| 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:39 |
| 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:40 |
| 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:41 |
| 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:42 |
| rogpeppe | :-) | 19:43 |
| niemeyer | rogpeppe: :-( | 19:43 |
| niemeyer | <rogpeppe> niemeyer: do we use "" to mean default anywhere? | 19:43 |
| niemeyer | YEEEES | 19:43 |
| rogpeppe | gotcha | 19:43 |
| 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:44 |
| niemeyer | rogpeppe: nil in the map sounds fine | 19:45 |
| 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:46 |
| 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:47 |
| 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:48 |
| rogpeppe | niemeyer: great | 19:49 |
| rogpeppe | niemeyer: ok, i'll see how it goes | 19:49 |
| 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:50 |
| rogpeppe | niemeyer: and we have the type as OneOf(String, nil) | 19:51 |
| rogpeppe | niemeyer: ? | 19:51 |
| niemeyer | rogpeppe: Right | 19:51 |
| niemeyer | rogpeppe: I think we'd need that anyway, or it'll barf on the nil | 19:52 |
| niemeyer | rogpeppe: (not there != nil) | 19:52 |
| 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:53 |
| * rogpeppe dives once more into the breach | 19:54 | |
| niemeyer | :) | 19:54 |
| niemeyer | I'll step out to get some food meanwhile | 19:54 |
| 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. | 19:56 |
| rogpeppe | niemeyer: done, i hope: https://codereview.appspot.com/6850087/ | 21:03 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!