=== davecheney is now known as davecheney95
jamwallyworld: /wave, how's it going today?06:10
wallyworldhi, going well. going to put up a mp soon. i have all tests except one working (but that fails in trunk also). i keep finding more refactoring to do but will draw the line so i can get something merged in. next thing to do is add a lot of missing tests06:11
jamwallyworld: sounds good. I fully support ratcheting the changes into trunk as much as possible.06:13
wallyworldyeah, i don't want to make it perfect but i also don't want to have duplicated code blocks and such. so it will be fairly clean06:14
wallyworldi'm wishing Go had infrastructure for IOC and such06:14
wallyworldthis is the type of project hat would really benefit from those patterns06:15
jamwallyworld: isn't that what interfaces are for?06:23
wallyworldsure, but you need an infrastructure to cleanly to the dependency injection06:23
wallyworldeg Spring in the Java world06:24
=== Guest84948 is now known as rogpeppe
=== rogpeppe is now known as rogpeppe1
=== rogpeppe1 is now known as rogpeppe2
=== rogpeppe2 is now known as rogpeppe3
=== rogpeppe3 is now known as rogpeppe
rogpeppemornin' all07:40
TheMuerogpeppe: Hiya.07:40
TheMueMy test just stopped my VM.07:45
fwereade__hey all07:55
TheMuefwereade__: Morning.07:57
rogpeppedavecheney95: sorry about trunk breakage. looks like i hadn't broken a test in the right way before checking that it passed.07:58
rogpeppedavecheney95: will fix asap07:58
rogpeppefwereade__: hiya07:59
davecheney95rogpeppe: it's not broken per 'se08:00
rogpeppedavecheney95: it is!08:00
davecheney95i just copied some of the other cert files that juju generated into the right place and it worked fine08:00
rogpeppedavecheney95: it's quite badly broken08:01
rogpeppedavecheney95: i've got some code that generates cert and key in a single file, but the new config stuff expects it in two08:01
davecheney95rogpeppe: orly;08:01
davecheney95i got it to work with only one08:02
rogpeppedavecheney95: i thought it was ok 'cos my tests would've caught a problem, but it was masked08:02
davecheney95anyhoo: flick me a CL and i'll give it a review08:02
davecheney95file stuff on disk is hard08:02
rogpeppedavecheney95: yeah, it'll work if the cert is already there08:02
rogpeppedavecheney95: it's just these big changes - i've got quite a bit up in the air currently08:03
davecheney95rogpeppe: yup, that is what I did, i copied a pem file that juju generated from a previos revision into the right place and it worked08:03
davecheney95which was promising08:03
rogpeppedavecheney95: hopefully it'll all be reconciled soon - the CL will be a temporary patch before the code goes away08:03
* davecheney95 is uncomfortable about the cheques we're writiing for the juju tool to do as a setup agent08:03
* davecheney95 proposes a new command, juju setup08:04
davecheney95which is also run by default in some situatoins08:04
davecheney95-EFACEPALM ?08:06
TheMuedavecheney95: Exactly! *grmblx*08:07
TheMueToo dumb to use a self written command correct.08:07
wallyworldjam: i need a little bzr advice. i have ditched my usual co-location workflow and directory layout to try cobzr. i have set in location.conf "public_branch:policy = appendpath" but this causes .bzr/cobzr to appear in the branch paths. what's the correct config?08:17
jamwallyworld: I don't use cobzr, so I can't say for sure. One option is to use 'appendpath' but set the containing location to include .bzr/cobzr08:18
jamSo instead of [/path/to/branches] you use [/path/to/branches/.bzr/cobzr]08:18
jamwallyworld: I think abentley worked on a 'just take the last name' rather than appendpath08:18
wallyworldah ok, will try thanks. i should have just stuck to what worked :-)08:18
jambut I don't remember how that ended up.08:18
jamAlso, you might try something with nick, let me look at the code abit.08:19
jamwallyworld: just to mention, I just use a lightweight checkout pointing to my normal locations for branches.08:19
jamwhich gives you the same functionality as cobzr, but the branches don't exist in a subdir.08:19
wallyworldyes, that's what i normally do too08:20
wallyworldi think i'll ditch cobzr and revert to that08:20
wallyworldjam: actually, your idea aboce to add .bzr/cobzr to the containing locations seems to work08:22
rogpeppedavecheney95: are you talking about the certificate generation stuff?08:23
rogpeppedavecheney95: (which, BTW is more broken than i realised, dammit - the fix isn't quite as trivial as i want)08:24
rogpeppewallyworld: FWIW i use cobzr but i've never touched location.conf AFAIR08:25
davecheney95rogpeppe: all i can say is what was in the email08:25
davecheney95i went to use a environment i hadn't used about 10 revisions ago08:25
davecheney95and it whinged that I didn't have the magic certificate poop08:25
wallyworldrogpeppe: i use to to save me typing the full urls each time i push etc08:25
rogpeppedavecheney95: that's because i broke it, not necessarily because it's fundamentally the wrong design08:25
rogpeppewallyworld: ah, that sounds useful actually08:26
wallyworldrogpeppe: it's quite awesome. i just type "bzr push" and it knows what to do08:26
wallyworldi can pastebin my relevant config if you like08:27
rogpeppedavecheney95: that happens for me too - i just rely on lbox to work out where it wants to push to, then it remembers is08:27
rogpeppewallyworld: please08:27
wallyworldok, sec08:27
rogpeppewallyworld: that 2nd last remark should've been addressed to you.08:27
wallyworldok. i'm just trying lbox now for the first time08:28
rogpeppedavecheney95: BTW the certificate generation stuff is moving from the juju package to the environs package08:28
wallyworldwhy don't we use lp for mps? isn't that the company standard? did we get an exception?08:28
rogpeppewallyworld: we do08:29
rogpeppewallyworld: but we use codereview too08:29
rogpeppewallyworld: because it's considerably awesomer :-)08:29
wallyworldok, i'm still ramping up on juju processes08:29
davecheney95wallyworld: i should have mention, ping me if you need help in your timezone08:29
wallyworldi'll see real soon now what's it's like :-)08:29
wallyworlddavecheney95: thanks, will do08:29
davecheney95your timezone is like my timezone, except we're not scared of confusing the livestock08:29
* wallyworld wishes we had daylight saving08:30
* davecheney95 wishes we didn't08:30
jamwallyworld: really? The 2h time shift with the rest of the world tends to be very disruptive08:30
davecheney95i'm already getting grief about the tuesday meeting 'cos it's slipped to 11pm08:30
jampoolie had it, and it was quite annoying08:30
davecheney95it's great to get 2 hours more with the yanks08:30
rogpeppedavecheney95: what time is it for you now?08:30
wallyworldexcepting collaboration, i really like it08:30
wallyworld18:30 for me08:31
davecheney95when I worked at the trading company, daylight saving sucked08:31
davecheney95we had to work til 9pm to cover the indian market08:31
wallyworldit makes it really nice for family time at the end of the day08:31
davecheney95wallyworld: which part of brisvegas are you in ?08:32
wallyworldfig tree pocket08:32
wallyworldwestern suburbs08:32
davecheney95wallyworld: latte sipping inner west sydney08:32
wallyworldoh, that's east08:33
wallyworldbalmain maybe08:33
wallyworldi don't know sydney that well08:33
davecheney95ashfield, almost the same08:33
davecheney95bit further south from the river08:33
rogpeppedavecheney95: hmm, good thing you can work a bit late often, 'cos otherwise our working days have no overlap at all, unless i get up at 6.30am08:34
wallyworldrogpeppe: https://pastebin.canonical.com/78813/   - i just have it set up for goose right now. i could promote the common config items like public_branch:policy to a [/home/ian/juju] section08:35
wallyworldwhen i add juju-core or other projects08:35
wallyworldso now bzr info gives:08:36
wallyworldRelated branches:08:36
wallyworld  public branch: bzr+ssh://bazaar.launchpad.net/~wallyworld/goose/client-using-identity08:36
wallyworld    push branch: bzr+ssh://bazaar.launchpad.net/~wallyworld/goose/client-using-identity08:36
wallyworld  parent branch: .bzr/cobzr/master08:36
wallyworld  submit branch: bzr+ssh://bazaar.launchpad.net/~gophers/goose/trunk08:36
wallyworldso stuff it cares about is set correctly08:36
rogpeppewallyworld: cool, thanks08:36
rogpeppewallyworld: and you put that in ~/.bzr/locations.conf ?08:37
rogpeppeha, that's funny08:38
rogpeppei did actually set up a locations.conf file once08:39
jamrogpeppe: in the non-go world, I have all of my branches roughly mirroring launchpad as: ~/dev/$PROJECT/$BRANCH, which lets me set a policy like the one above, except it works across all of my projects, rather than configuring each one.08:39
rogpeppejam: yeah, i did have that setup originally08:39
rogpeppejam: my current locations.conf has a push_location of  lp:~rogpeppe/ensemble :-)08:40
jamthat sounds like it has been a while :)08:40
rogpeppejam: blast from the past08:40
wallyworldsave me looking up docs, if lbox says gofmt is sad, do i have to run gofmt against each file individually, or do we have a tool to them all at once?08:43
rogpeppewallyworld: in the root juju directory, run "go fmt ./..."08:44
wallyworldcool, thanks08:44
rogpeppewallyworld: or, from any directory, run "go fmt launchpad.net/juju-core/..."08:44
wallyworldi was typing gofmt08:44
rogpeppewallyworld: or to do it in the current directory only, run "go fmt"08:45
wallyworldworked, thanks08:45
wallyworldi didn't appreciate the difference between gofmt and go fmt08:45
rogpeppewallyworld: (the go command always uses the package in the current directory by default)08:45
rogpeppewallyworld: gofmt allows access to a few features that go fmt doesn't08:45
rogpeppewallyworld: for example the -r flag (which can be awesome BTW)08:46
rogpeppewallyworld: go fmt does actually just call gofmt, i think08:46
wallyworldgofmt  seemed to print the file to stdout08:46
rogpeppewallyworld: yeah, that's the default. although you can use gofmt -w too08:46
wallyworldiw works with ./... ?08:47
rogpeppewallyworld: no.08:47
jamwallyworld: I'm pretty sure gofmt doesn't support './...' but 'go fmt' does08:47
wallyworldok, thanks08:47
rogpeppewallyworld: the go tool is the one that understands package wildcards etc08:47
rogpeppewallyworld: gofmt just knows about files08:48
rogpeppewallyworld: i almost always use go fmt08:48
rogpeppewallyworld: except when i'm editing - then i'll often pipe a section of a file (or the whole thing) through gofmt08:48
wallyworldi might look at setting up my IDE to use it in the background08:49
wallyworldi already have code inspections but not fully go fmt compliant08:49
rogpeppewallyworld: for instance if you use vim, you can do :1,$!gofmt08:50
rogpeppewallyworld: i know various people set their editors up to automatically gofmt on save08:50
wallyworldi use Intellij with a golang plugin08:50
wallyworldi'll look into it08:50
jamrogpeppe: yeah, I tried to set up vim with it, but my simple method meant that every write triggered a reload of the working buffer, shoving me all the way to the top of the file. Loosing my place made it impractical.08:51
rogpeppewallyworld: and i'm pretty sure that some people (fwreade__ ?) change tabs to spaces on load, and back to tabs on save.08:51
wallyworldi'm just waiting on integrated debugging to be supported. i really miss that from Java and Python08:51
jamSo instead, I just do "go fmt ./... && go test ./..." so every test run reformats.08:51
rogpeppejam: yeah i can see how that might be annoying08:51
jamwallyworld: ISTR that gdb works with go code08:51
wallyworldi really dislike the use of tabs - everyone i mentioned it to is quite incredulous08:52
jam*sigh* too easy to get that one wrong08:52
rogpeppewallyworld: what don't you like about them?08:52
wallyworldi've not used gdb - will have to learn, i rely on my ide08:52
jamwallyworld: well, I would guess if gdb works, many ides might use that on the backend.08:53
rogpeppeyeah, gdb does work (i checked once) but i never use it08:53
jamWhich means you could have intellij integration earlier than later.08:53
wallyworldrogpeppe: i can't quite put it into words easily why i dislike tabs, no other language i know of ides it08:53
jamwallyworld: I worked on a project in C/C++ that mandated tabs so that people could configure their preferred indent.08:53
jamMost python projects mandate no tabs.08:54
wallyworldand Java too08:54
rogpeppewallyworld: i've used tabs for a very long time, so i'm always interested to know why people don't like them08:54
rogpeppejam: with python it's understandable, because getting the indentation right is crucial08:54
jamrogpeppe: yeah, the main problem in python is that you *can* mix spaces and tabs, and that way lies madness. :)08:55
rogpeppewallyworld: i like the fact it's just a single character to type to add or remove a level of indentation08:55
wallyworldrogpeppe: i guess because the spacing becomes variable for everyone, depending on their setup08:55
rogpeppejam: yeah08:55
wallyworldrogpeppe: my IDe does all that - i hit tab and it inserts saces on thre fly08:55
jamrogpeppe: sure but I use << >> in vim anyway, so indentation is a simple select and move regardless the characters used to indent.08:55
wallyworldand it auto indents according to the code style in use08:55
rogpeppewallyworld: yeah - that's a problem if people rely on beautiful code alignment. you can't do that in go though :-)08:55
wallyworldso i never type lots of spaces08:55
wallyworldrogpeppe: and the preference for tabs = 8 is madness IMO08:56
wallyworldway too much08:56
davecheney95wallyworld: for python, probably08:56
rogpeppewallyworld: don't you need to type a few delete characters to remove a level of indentation08:56
wallyworldnope, IDe does it08:57
davecheney95but it could be a million, it just depends what your editor is set too08:57
jamrogpeppe: I know vim can be configured to know when you are deleting indent chars vs regular spaces.08:57
wallyworldit knows whatr the code style is and handles all that08:57
jamHowever, I use <<, I imagine wally does "shift + tab"08:57
jamISTR doing that in VC in the long distant past.08:57
wallyworldBackspace deletes all the white space unti lthe next indentation block08:57
wallyworldjust like if tabs were used08:58
rogpeppewallyworld: i don't think go has a preference for tabstop=8 particular08:58
wallyworldi think i read it somewhere08:58
wallyworldbut i've crammed a lot of Go docs in the past few days08:58
rogpeppewallyworld: personally, i'm a heretic because i use a proportionally spaced font08:58
jamwallyworld: so you can always set your tab stop if 8 is too much. Which isn't true if you use spaces. For *me* I just adapt between them.08:59
jamrogpeppe: there is a slight preference if go fmt ends up aligning something with spaces on a continuation line, etc.08:59
wallyworldjam: yeah, but as you said, it may mess with alignment08:59
jamI thought, I could be wrong.08:59
rogpeppejam: yeah. i don't mind much though.08:59
jamwallyworld: occasional mis alignment could be a lot better than constant extra depth for you08:59
rogpeppejam: i don't find it affects readability08:59
wallyworldrogpeppe: why proportional spaced font?09:00
rogpeppewallyworld: but i get more readable text per pixel with a proportionally spaced font09:00
jamrogpeppe: does it do fixed width spaces for aligned tables?09:00
rogpeppejam: yeah, i can switch trivially if i need to09:01
rogpeppejam: aligned tables are pretty rare though09:01
jamrogpeppe: except for all of the inline comments in structs :) and `json:content` stuff09:01
rogpeppejam: yeah. but the alignment there is just prettification. it doesn't make much difference if it's not aligned.09:02
rogpeppejam: unlike a table09:02
rogpeppeif i used an equivalent font size in fixed with, i'd see much less code. and i like the way proportionally spaced text looks.09:03
rogpeppeanyway, excuse me, i need to un-break trunk :-)09:03
wallyworldjam: i just ran lbox and it created the lp mp, but i got this error: "error: Failed to send patch set to codereview: diff is empty"09:13
wallyworldlp can take a little time to generate the code diff09:14
wallyworldhow do i send the diff to codereview once lp has calculated it?09:14
jamwallyworld: sounds odd, offhand sounds like an incorrect submit branch.09:14
jamwallyworld: lbox computes the diff and proposes it for you09:14
rogpeppewallyworld: what does "bzr diff --old lp:goose/trunk" print?09:14
wallyworldjam: the lp mp says the submit branch is https://code.launchpad.net/~gophers/goose/trunk09:14
wallyworldjam: that command prints the diff as expected09:15
rogpeppehmm, odd09:15
rogpeppewallyworld: what does lbox propose -v print?09:16
wallyworldlp also shows the preview diff, let me type the the above and see09:16
wallyworldrogpeppe: that time it send the diff to codereview, or so it says. it also marked the mp as neews review since i created initially as wip09:18
rogpeppewallyworld: that sounds like it all worked09:18
rogpeppewallyworld: did it print a codereview url?09:18
wallyworldyeah, i have no idea what happened the first time09:18
wallyworldrogpeppe: ah no, my apologies - it still failed: error: Failed to send patch set to codereview: diff is empty09:19
rogpeppewallyworld: could you paste the lbox output?09:19
wallyworldrogpeppe: https://pastebin.canonical.com/78814/09:20
wallyworldrogpeppe: i have to go and put my son to bed and read him a story, i'll be back in a bit09:21
jamwallyworld: Diffing branches for codereview: /home/ian/juju/go/src/launchpad.net/goose => /home/ian/juju/go/src/launchpad.net/goose/.bzr/cobzr/lbox-805004725/lbox09:25
jamIt looks like lbox is treating your goose checkout as the submit target.09:25
jamAnd doing a diff of the code against itself.09:25
jamobviously that is an empty diff.09:25
jamI don't know where lbox gets the URL of the branch to diff against.09:25
jamI would have thought that would be in .lbox, which uses "propose -cr -for lp:goose"09:26
jamMaybe it is looking up something else in your config?09:26
jamrogpeppe: do you know where lbox gets the target to diff against?09:26
rogpeppejam: it should get it from the -for flag, which in this case is set in .lbox09:27
rogpeppejam: it looks like that worked too, but i'm not sure why it changed.09:30
rogpeppejam: it looks like the FetchRemote logic is failing somehow09:34
rogpeppejam: erm, no it doesn't - it all looks ok to me09:35
jamthe fact that it is diffing against 'lbox' something makes me think it is fetching the remote branch to a temp location and then trying to diff against it, but it also feels like it might be failing to get the actual files to diff against for some reason.09:36
rogpeppejam: hmm, it worked fine for me: https://codereview.appspot.com/685805009:41
rogpeppeexcept that testservices/identityservice/util.go needed gofmting, which makes me suspicious09:41
rogpeppei'm not sure how the lbox check could have succeeded if that branch really was current09:42
davecheney95rogpeppe: speaking of gofmt09:45
davecheney95there are some small changes, and larger ones coming that make gofmt 1.0 and gofmt 1.1 incompatible09:45
rogpeppedavecheney95: really? in what way?09:46
davecheney95there is a small change already that affects the padding of comments on struct members09:46
davecheney95and gri is about to commit a change that makes09:46
davecheney95for i := range s { i++ }09:46
rogpeppedavecheney95: orly?!09:47
davecheney95which is going to upset our .lbox.check hook09:47
rogpeppedavecheney95: oh, you mean the formatting09:47
rogpeppedavecheney95: i thought you meant that you could influence a range variable within the body09:47
davecheney95the syntax is not changing, just the canonical format09:47
rogpeppedavecheney95: i guess we'll have to standardise on 1.0.3 gofmt09:48
davecheney95rogpeppe: i agree09:48
rogpeppedavecheney95: until 1.1 is released09:48
davecheney95although it'll be 1.0.209:48
davecheney95'cos that is what is in precise or quantal09:48
rogpeppedavecheney95: same difference, i think09:48
davecheney95for this, yes09:48
rogpeppedavecheney95: only problem is: can we get the tip go tool to use a different gofmt?09:49
rogpeppedavecheney95: i.e. does it just use the gofmt in $PATH or not?09:49
wallyworldrogpeppe: jam: thanks for looking, i'll poke around and see what might be happening. certainly something must be correctly setup since launchpad's mp diff is there ok09:49
davecheney95go space format from memory09:50
davecheney95for people like me that live on tip09:50
rogpeppewallyworld: i did the propose myself, and it worked, except that it required a gofmt, which seems odd09:50
davecheney95we could add optoinal support for a GO_1_x path (making up a name)09:50
wallyworldhmmm. when i first did it it complained and then i fixed all the issues09:50
davecheney95review request: https://codereview.appspot.com/6851081/09:51
davecheney95^ i think this is gettig pretty resonable09:51
jamwallyworld: I should have a review for you in a bit.09:51
wallyworldjam: using the lp diff?09:51
rogpeppedavecheney95: go fmt just uses gofmt from $PATH, so there's no difficulty chopping and changing if we need to09:53
jamwallyworld: yeah, I'm still perfectly comfortable reviewing from LP.09:54
davecheney95rogpeppe: nice, I can make that work09:54
jamand/or from a raw branch when necesasry.09:54
jamI have to go in about 20min, so I'd rather get you a review than nothing today.09:55
jamwallyworld: review submitted. Probably the start of a conversation, though at this point, we can land what you are comfortable with and iterate from there, rather than iterating pre landing.10:15
wallyworldjam: ok, will look. thanks10:16
rogpeppedavecheney95: you've got a review10:27
dimiternwallyworld: a tiny comment from me as well, not really a full review (since I did the original code, more or less I don't think will be appropriate to review).10:31
wallyworlddimitern: thanks, i'll look after replying to jam's lengthy review10:32
wallyworldwhich i'm doing now10:32
dimiternwallyworld: feel free to blame the initial ugliness on me :)10:33
wallyworldprototype code!10:33
wallyworldnot meant to be perfect10:33
wallyworlddimitern: there is that one failing test - any idea?10:33
dimiternwallyworld: not really, I struggled with it a bit, but even though the Put was ok and no error reported, Get returned empty data and also no error... probably a sleep of a few secs is in order - the eventual consistency thing of the cloud10:35
wallyworldcould be. will be nice to know10:36
wallyworldso we can plan around it for other things10:36
dimiternwallyworld: also, it seems you're not yet a member of The Go Language Gophers team and you should be10:39
wallyworldah, haven't checked that. thanks, i'll follow up10:39
niemeyerGood morning ladies and gentlemen10:59
TheMueBinjour Monsieur Niemeyer11:00
niemeyerTheMue: Merci beaucoup11:06
niemeyer"beaucoup" must be my favorite word in French11:06
niemeyerI picture imagined language designers saying "Hey, why don't we just throw a few more letters there, just for the fun of it?"11:09
dimiternniemeyer: yeah, the same applies to irish, scottish and manx :)11:10
TheMueniemeyer: Hehe, indeed, just discovered the number of vowels in "beaucoup".11:13
rogpeppedimitern: perhaps you could put your initial commit up for review before we put ian's changes on top?11:21
rogpeppeniemeyer: yo!11:22
niemeyerrogpeppe: Heya11:22
dimiternrogpeppe: well, since it's already in trunk, how can I do this?11:22
rogpeppedimitern: hmm, good question. erase trunk and start again? :-)11:22
dimiternrogpeppe: done! :D11:22
dimiternrogpeppe: that's what I meant yesterday by wanting someone to take a look at goose/client/client.go first11:23
rogpeppedimitern: yeah, it would be a good idea i think11:24
dimiternrogpeppe: I'd really appreciate if you take a look then11:26
rogpeppedimitern: the only problem with starting from scratch is that wallyworld's branch becomes divergent, but i don't think that's too much problem actually. apart from jam's review. gotta start somewhere tho.11:26
rogpeppedimitern: will do when i see the CL11:27
mgzrogpeppe: that does make sense for future, but as this is mostly initial experimental hacking that will get replaced in pretty short order11:30
rogpeppemgz: i'm not sure. it looks to me like the core of something that will shortly be expanded, with no time to go back to get the core right.11:31
rogpeppemgz: there are lots of things there that can easily be got right from the word "go"11:32
mgzdo I have to hit you for that pun? :)11:32
* rogpeppe was blithely oblivious11:32
* rogpeppe apologises11:33
rogpeppei dunno, maybe it doesn't need a review, but... niemeyer, what thinkest thou?11:34
niemeyerrogpeppe: I tend to agree with you11:34
niemeyerIf it's indeed scratch, it can be moved aside11:35
dimiternrogpeppe, mgz: yeah, I'll feel better once we have a routine CL/review/land process in place, but the code now in trunk will be there for a short while and be reviewed in chunks11:35
wallyworldniemeyer: hey can you add me to gophers so i can push to goose trunk?11:35
niemeyerEven more if there's no review process in place11:35
niemeyerThis is a major eye-opener11:35
dimiternniemeyer: absolutely, I agree, so how do we handle the gap between the no-review code and the refactoring of it, which is reviewed as usual in steps11:37
rogpeppedimitern: i think we should add code to a blank trunk, in steps11:37
mgzniemeyer: there is review, it's just we're integrating the two initial stabs dimitern and jam did during UDS11:37
mgzwhich were not reviewed, they just got written11:37
mgzso, jam had some comments about dimitern's code from then, which wallyworld is refactoring into the common set now11:38
niemeyerAlthough, I'll leave that to you guys.. the important thing is that there's certainty that the code that is in trunk was looked over and is sensible and tested11:38
mgzso, it's essentially the same as having an unmerged feature branch that some parts of which are now being cherrypicked it, and that cherrypick is being reviewed11:39
rogpeppeniemeyer: unfortunately you were totally right about your config misgiving last night. a test wasn't working properly, and as a result trunk is now broken in a way that's not easy to fix. am v sorry about this.11:40
niemeyerrogpeppe: If we can't fix it promptly, we should revert it so other people can continue to use it11:47
rogpeppeniemeyer: i think reverting it is the best plan (i do have a fix, but it's ugly and bigger than i'd like)11:48
rogpeppeniemeyer: how would you go about reverting it? apply a reverse patch?11:48
niemeyerrogpeppe: Right11:49
mgzrogpeppe: then in your feature branch, merge trunk after the revert but keep the existing tree state, so it's ready to reland11:50
rogpeppemgz: would you do that just by copying the tree out and back in? or is there a clever way to merge without updating the tree?11:52
niemeyerYeah, the obvious "re-apply" doesn't work after the reverse11:52
niemeyerwallyworld: That's done11:52
mgzrogpeppe: basically switch to feature, then bzr merge trunk (or whatever branch layout you're using), then `bzr revert .`11:52
wallyworldniemeyer: thanks!11:53
mgzand commit11:53
mgzso, you keep the merge marker but reject the trunk changes11:53
rogpeppemgz: doh! of course11:53
rogpeppemgz: i was thinking that reverting would lose the merge merker11:53
mgzif there are intermediate things on trunk that need saving, you want to do it in two steps, but there shouldn't be in this case11:53
niemeyermgz: I don't think that works11:54
niemeyerBecause revert reverts11:54
niemeyerIncluding the merge11:54
mgzif you pass . it reverts the tree contents, not the merge marker11:54
wallyworldmgz: dimitern: my changes should be pushed11:55
niemeyermgz: Ah, I missed the .11:55
dimiternwallyworld: thanks11:55
rogpeppeme too11:55
mgzwallyworld: thanks!11:55
wallyworldi'll address the refactorings tomorrow11:55
rogpeppemgz: that's a very subtle distinction!11:56
rogpeppehmm, my irc client says it's dead, but i still saw wallyworld's messages11:56
mgzright, it's less obvious than the inverse which is `bzr revert --forget-merges` which keeps the tree state but loses the marker11:56
rogpeppeniemeyer: https://codereview.appspot.com/684507012:01
rogpeppeperhaps i should just submit it - it's entirely automatic12:02
niemeyerrogpeppe: Yep, sounds sane12:06
niemeyerrogpeppe: I'm wondering about the best plan to get that in now.. it'd be nice to have that branch mostly unchanged when it comes back, so we can more easily get it in12:07
rogpeppeniemeyer: i agree, but it's not an easy fix12:07
rogpeppeniemeyer: trying to make a fix made me realise a few issues with my current plan12:08
niemeyerrogpeppe: Sure, but it can be done on top of it, rather than *in* it12:08
rogpeppeniemeyer: i'm not sure.12:08
niemeyerrogpeppe: ?12:08
rogpeppeniemeyer: it's difficult to make the original cert generation work properly12:08
rogpeppeniemeyer: because you need to know the name of the environment to generate the certificate, but you can't know the name of the environment until you've parsed it.12:09
rogpeppeniemeyer: and you can't parse it without the certificate12:09
niemeyerrogpeppe: I still don't understand how that makes it hard to make the change on top of the branch, rather than mixing stuff up12:10
rogpeppeniemeyer: it's not easy to make that branch pass tests.12:10
rogpeppeniemeyer: (when the tests aren't broken)12:10
rogpeppeniemeyer: it's quite chicken-and-eggy12:11
niemeyerrogpeppe: I see12:11
rogpeppeniemeyer: my ugly fix was to generate the certificate into a file with a known name12:12
rogpeppeniemeyer: then the generation can be separated from the parsing12:12
rogpeppeniemeyer: in the new scheme, we can make the config parse error return the name of the environment in the error, so we at least know the correct name to use to generate the cert12:15
rogpeppeniemeyer: a much easier alternative would be to use the same root cert for all environments12:15
niemeyerrogpeppe: Much easier yet non-reasonable, I think12:17
rogpeppeniemeyer: yeah, i thought so12:17
niemeyerrogpeppe: Where is that config parsing error taking place?12:18
rogpeppeniemeyer: the first line of BootstrapCommand.Run12:18
rogpeppeniemeyer: well, that's currently.12:19
rogpeppeniemeyer: the plan is to have it handled somewhere within environs.NewFromName12:19
rogpeppeniemeyer: but that's another issue actually12:19
rogpeppeniemeyer: in our current scheme, we parse all environments when we read the environments.yaml file12:20
niemeyerrogpeppe: I'm kind of lost in the overall plan I think.. why do we need an error telling us about a name that we have as a variable?12:20
niemeyerrogpeppe: Kind of12:20
niemeyerrogpeppe: What you're saying is that we parse the environments.yaml file when we read the environments.yaml file, which seems self-defining12:21
rogpeppeniemeyer: yeah. but there are two levels of parsing - reading the environments.yaml file itself and turning each section into a *config.Config12:22
rogpeppeniemeyer: currently we do both12:22
rogpeppeniemeyer: and NewFromName simply calls ReadEnvironsBytes and returns one of the environs12:22
niemeyerrogpeppe: So, what's the issue?12:23
rogpeppeniemeyer: one issue is that ReadEnvironsBytes doesn't know the name that we have as a variable12:24
rogpeppeniemeyer: so we'd need to change the scheme we use in some way12:24
niemeyerrogpeppe: Why?12:24
niemeyerrogpeppe: I think ReadEnvironsBytes is fine as it is12:24
niemeyerrogpeppe: What are you thinking of doing?12:24
rogpeppeniemeyer: how can ReadEnvironsBytes work without generating a certificate if needed for each environment section?12:25
niemeyerrogpeppe: To build the certificates12:25
rogpeppeniemeyer: i'm thinking of doing the config parsing at a later stage.12:25
niemeyerrogpeppe: You're assuming other things that I can't tell yet12:25
rogpeppeniemeyer: so then we can always call ReadEnvironsBytes even if the certificates aren't in place yet12:25
niemeyerrogpeppe: It works today, so without knowledge of what other changes you have in mind, I can easily say that it doesn't have to do anything12:25
niemeyerrogpeppe: We can do that today12:26
rogpeppeniemeyer: how? the CA cert is required by the config.12:26
niemeyerrogpeppe: By not making it required by the config, for example.. this is the change you've just reverted12:26
rogpeppeniemeyer: we could do that. but it is really required in the end...12:27
rogpeppeniemeyer: perhaps that's ok though. we can insert checks in all the relevant places.12:30
niemeyerrogpeppe: Where do we want to generate the certificates?12:30
rogpeppeniemeyer: our plan, i think, was to do it in NewFromName. but actually i'm not sure that's right. i think it should happen on bootstrap only.12:31
niemeyerrogpeppe: NewFromName is definitely not the right place.. this is a trivial wrapper12:32
rogpeppeniemeyer: in my branch fix this morning, i added a function in juju: GenerateCACertificateIfNecessary12:32
* niemeyer observes rogpeppe getting Javaeske12:32
rogpeppeniemeyer: which actually seemed like a reasonable way of doing things12:32
rogpeppeniemeyer: yeah, i know. it was only a temporary name :-)12:32
rogpeppeniemeyer: if the Environ we get back from NewFromName might not have a root CA cert, where *might* a good place to generate it be?12:34
niemeyerrogpeppe: I think it should have a root cert for sure12:35
niemeyerrogpeppe: And config.New may also require the availability of the cert, actually12:35
niemeyerrogpeppe: The logic you had in place looks good12:35
rogpeppeniemeyer: in juju?12:36
niemeyerrogpeppe: ?12:36
rogpeppeniemeyer: sorry, which logic?12:36
niemeyerrogpeppe: config.New?12:36
rogpeppeniemeyer: ok, cool12:36
rogpeppeniemeyer: yeah, i think it's ok12:36
rogpeppeniemeyer: the problem being that we need to generate the certificate some time before calling config.New12:37
rogpeppeniemeyer: and i don't think we can do it before calling NewFromName (assuming that's what cmd/juju does in bootstrap)12:37
niemeyerrogpeppe: I think we should just change type environ to be struct { attrs }12:38
rogpeppeniemeyer: ooh, radical12:38
rogpeppeniemeyer: isn't that a huge change?12:39
rogpeppeniemeyer: assuming you mean environs.Environ12:39
niemeyerrogpeppe: I don't12:39
rogpeppeniemeyer: oh yeah12:40
rogpeppeniemeyer: phew12:40
rogpeppeniemeyer: yes, i agree12:40
rogpeppeniemeyer: that was my suggestion earlier12:40
niemeyerrogpeppe: There are still issues, though12:40
niemeyerrogpeppe: Oh, I hadn't noticed the suggestion before12:41
rogpeppe[12:25:29] <rogpeppe> niemeyer: i'm thinking of doing the config parsing at a later stage.12:41
rogpeppe[12:25:32] <niemeyer> rogpeppe: You're assuming other things that I can't tell yet12:41
rogpeppe[12:25:51] <rogpeppe> niemeyer: so then we can always call ReadEnvironsBytes even if the certificates aren't in place yet12:41
niemeyerrogpeppe: I see.. you mean something else by parsing, okay12:41
rogpeppeniemeyer: yeah, sorry. it's checking rather than parsing.12:42
niemeyerrogpeppe: It's still not enough, though12:42
rogpeppeniemeyer: no, we need something that actually generates the certs12:42
niemeyerrogpeppe: More than that, we need a place to generate the certs12:43
niemeyerrogpeppe: ReadEnvironsBytes is purely in memory12:43
rogpeppeniemeyer: i was wondering about environs.BootstrapEnviron()12:43
rogpeppeniemeyer: something explicitly in environs that returns an environment suitable for bootstrapping, generating certs if necessary12:43
rogpeppeBootstrapEnviron isn't right tho12:44
niemeyerrogpeppe: How many Bootstrap functions do we need? :)12:44
rogpeppeit sounds too active12:44
rogpeppeniemeyer: yeah12:44
rogpeppeniemeyer: this is something we only want to happen at bootstrap time though.12:45
niemeyerrogpeppe: Yeah, that's something interesting to take into account12:47
niemeyerIt's fine to error in other situations12:47
rogpeppeniemeyer: i'm wondering how things would be if we put all the $HOME-related logic into the juju package12:48
rogpeppeniemeyer: then it would be logical that juju.Bootstrap could generate certificates and write them to $HOME.12:49
niemeyerrogpeppe: Feels like fiddling without solving the underlying issue12:49
niemeyerrogpeppe: juju.Bootstrap is already bogus, I think12:49
niemeyerrogpeppe: It should be environs.Bootstrap12:49
niemeyerrogpeppe: We're loosing sight of what that package was supposed to be12:49
rogpeppeniemeyer: environs.Bootstrap works for me12:49
rogpeppeniemeyer: in which case, perhaps we could have environs.BootstrapWithName which would generate the certificates if necessary12:51
niemeyerrogpeppe: The problem isn't the name12:55
niemeyerrogpeppe: We have the environment name12:55
rogpeppeniemeyer: but we can't get an Environ without its certificate, so the existing juju.Bootstrap interface doesn't work12:56
rogpeppeniemeyer: so BootstrapWithName would read environments.yaml, generate certificates if necessary, then call environs.Bootstrap12:57
* rogpeppe has thought of a very quick and easy way of getting that branch in without breaking trunk12:58
niemeyerrogpeppe: BootstrapWithNameAndMaybeWithoutCertificates12:58
rogpeppeniemeyer: arf12:58
niemeyerrogpeppe: I'm pretty convinced that config.New shouldn't barf if there are no certificates12:58
rogpeppeniemeyer: hmm, so that logic is wrong now?12:59
niemeyerrogpeppe: There's no such logic now?12:59
rogpeppeniemeyer: true, it's just been reverted12:59
rogpeppeniemeyer: i meant the logic you said looks good earlier13:00
niemeyerrogpeppe: The points that should barf are the points that need the certificate (novel idea!)13:00
rogpeppeniemeyer: that seems fine13:01
niemeyerrogpeppe: environs.Bootstrap shouldn't even try to create the certs13:01
rogpeppeniemeyer: that still doesn't solve to problem of who *does* try to create the certs though13:01
niemeyerrogpeppe: The bootstrap command can do that by itself, I believe13:01
niemeyerrogpeppe: Which is one of the few control points that can actually relate BuildCerts (or whatever) to $HOME to the fact that Bootstrap is in progress13:02
rogpeppeniemeyer: i'm not sure. i'd prefer not to duplicate that logic in every piece of client code that calls Bootstrap13:02
rogpeppeniemeyer: but actually, maybe most client code (builddb included) should not actually be calling Bootstrap13:03
niemeyerrogpeppe: Hmm.. good point13:03
niemeyerrogpeppe: I mean the former one, not the latter13:04
dimiternmgz: I'm back13:07
niemeyerrogpeppe: What about having environs.Bootstrap taking a certsDir parameter?13:09
niemeyerrogpeppe: "" defaults to ~/.juju/13:09
niemeyerrogpeppe: Use for writing only.. never reads from it13:09
rogpeppeniemeyer: we'd want a way of doing an environs.Bootstrap *without* generating certificates13:10
niemeyerrogpeppe: Why?13:11
rogpeppeniemeyer: because we might want to use it in an environment with no writable directories.13:12
niemeyerrogpeppe: Erm?13:12
niemeyerrogpeppe: Do you have a realistic use case?13:12
rogpeppeniemeyer: i'm thinking of platforms like google appengine (not that that's currently a realistic target of course)13:13
niemeyerrogpeppe: I don't think that even makes sense.. if you don't have a way to write the certificate down to disk, the certificate should be provided upfront otherwise it's simply not usable13:14
rogpeppeniemeyer: i suppose you can always provide a nonsense name in that case13:15
niemeyerrogpeppe: Why do we need a nonsense name?13:15
rogpeppeniemeyer: because you need to call environs.Bootstrap even if you're providing the cert up front13:15
rogpeppeniemeyer: and it requires a name13:15
niemeyerrogpeppe: I don't understand the obsession with the name.. it doesn't require a name.. it requires an environment, that has a name, at all times13:16
niemeyerrogpeppe: environ Environ13:16
rogpeppeniemeyer: sorry, the certDir name13:16
niemeyerrogpeppe: LOL, okay13:16
niemeyerrogpeppe: It's a red-herring really.. if there are zero paths writable, it can't work without a certificate, and that's fine.13:17
rogpeppeniemeyer: so presumably environs.Bootstrap would use SetConfig to set the certificate on the environment passed in13:17
rogpeppeniemeyer: i can imagine a case where you might not want to provide a path (e.g. running as root and wanting no side-effects)13:18
niemeyerrogpeppe: Seems pretty fictitious13:18
niemeyerrogpeppe: You're bootstrapping but want no side effects? :-)13:18
rogpeppeniemeyer: there are all kinds of places i could save stuff that are not on disk13:19
niemeyerrogpeppe: juju needs a certificate to work.. that's the only invariant we have13:19
rogpeppeniemeyer: or, not on the local filesystem at any rate13:19
niemeyerrogpeppe: If people don't want juju to generate the certificate for them, it's their problem to generate it13:19
rogpeppeniemeyer: yeah, true13:19
rogpeppeniemeyer: alternatively, forget about the "" meaning ~/.juju thing, and define environs.JujuDir = os.Getenv("HOME") + "/.juju"13:20
rogpeppe.or ConfigDir or whatever13:21
rogpeppeniemeyer: i think that would be my preference - more explicit13:21
rogpeppeniemeyer: another possibility: provide an interface{WriteFile(name string, data []byte)error}, and when nil, use a type that writes to ~/.juju13:25
rogpeppeniemeyer: i like that even better - nil is slightly easier to type than "" :-)13:25
rogpeppeniemeyer: and it makes it easier to test too13:25
rogpeppeniemeyer: func Bootstrap(env Environ, certWriter FileWriter) error13:27
rogpeppeniemeyer: also makes it easy to encrypt the certificate/key if desired13:28
rogpeppealthough of course, then we'd want a converse in config.New13:30
niemeyerrogpeppe: Sounds good, but the certWriter should be func(name string, data []byte) error13:31
niemeyerrogpeppe: What converse?13:31
rogpeppeniemeyer: ok.13:31
rogpeppeniemeyer: ReadFile13:31
niemeyerrogpeppe: I don't think that's necessary13:31
rogpeppeniemeyer: not for the time being anyway13:32
rogpeppeniemeyer: at some point it would be nice to be able to hook into the ubuntu keyring mechanism13:32
rogpeppeniemeyer: (i'm not sure how that works at all)13:32
rogpeppeniemeyer: so that people can avoid storing their juju environment keys in plaintext13:33
mgzdimitern: can you glance over a branch for me?13:43
dimiternmgz: sure13:44
mgzdimitern: https://code.launchpad.net/~gz/goose/client_identity_refactor_fix/+merge/13539413:44
dimiternmgz: pretty straightforward, LGTM13:47
mgzthanks! will land.13:48
rogpeppeniemeyer: here's the config CL updated so that it's not broken anymore. (i made certificates optional in config and it doesn't use juju.Bootstrap in cmd/juju) https://codereview.appspot.com/6846066/14:32
rogpeppeniemeyer: i guess i'll need to create another merge request, but the changes are more obvious in this one14:33
rogpeppemgz: i'm not sure your technique of merging, then reverting works that well14:35
rogpeppemgz: because the changes i'd reverted to have already been applied in trunk, so they'll be ignored when i come to merge.14:36
rogpeppemgz: i think i should've just copied the tree14:36
mgz rogpeppe: I did give a warning, there's an extra step if there have been intermediate unmerged landings14:39
rogpeppemgz: i'm not sure exactly what you mean by "intermediate unmerged landings"14:40
mgzrereading what you wrote, I'm not sure I understand what your problem was either...14:40
mgzhave you tried a test merge to trunk locally? it will probably just work.14:41
rogpeppemgz: i'll try14:41
TheMuerogpeppe: You've seen, I reproposed our both CLs after fixes based on your reviews.14:41
mgzthe issue with just merging trunk and reverting is you need to check that the tree changes you're discarding with that are only the ones from the trunk reversion of the feature14:42
mgzif there have been changes made to trunk since the feature branch last merged trunk, but before the trunk merge that reverted the feature merge, you need to be careful not to chuck them out14:43
rogpeppemgz: ah, i see14:43
mgzdepending on how you normally work, that's either very unlikely, or will pretty much always be the case14:44
rogpeppemgz: yeah, it seems to work. i was slightly confused by the way the CL on codereview didn't show all the diffs14:44
rogpeppemgz: that would've been the case if there'd been any significant time, 'cos i do tend to merge trunk as i go along14:44
mgzand what you need in that case is just to merge trunk from just before the revert on trunk, keep those changes, then merge and discard the next trunk revsion that contains the revert14:45
rogpeppemgz: luckily, i don't *think* that was an issue14:45
mgzthe diff when relanding on trunk will be clear regardless14:45
TheMuefwereade__: I'm currently in https://codereview.appspot.com/6852080/. You removed the empty line between standard and our packages in the import. So the sorting changes. Is that wanted?14:51
fwereade_TheMue, whoops, those changes were not conscious14:51
fwereade_TheMue, are we standardising on that behaviour?14:51
TheMuefwereade_: Btw, so far it looks like a great simplification.14:51
TheMuefwereade_: I'm not sure, I only recognized it twice now.14:52
fwereade_TheMue, I've noticed it once or twice and have no very string feelings either way -- but my own habit is to do a single block of imports, so sometimes I "fix" them without noticing14:54
rogpeppefwereade_: where i've seen the two groups, i've thought it looked nice15:04
rogpeppefwereade_: i don't think we need to apply a standard too rigorously15:05
fwereade_rogpeppe, yeah, I'm not actually in opposition, just occasionally thoughtless15:05
TheMuefwereade_, rogpeppe: I don't need that kind of rule too.15:06
TheMuefwereade_, rogpeppe: But indeed, it looks nice. ;)15:06
rogpeppeniemeyer: here's the full CL for the config fixes. https://codereview.appspot.com/684310015:19
rogpeppeniemeyer: https://codereview.appspot.com/6846066/ makes it much easier to grasp15:19
niemeyerrogpeppe: Brilliant, thank you!15:22
niemeyerrogpeppe: So, is the 66 still valid now?15:23
rogpeppeniemeyer: yes, but i'm not sure about submitting a CL twice...15:23
niemeyerrogpeppe: I has a single LGTM15:23
rogpeppeniemeyer: i'd use it on 310015:24
rogpeppeniemeyer: but they're the same thing15:24
rogpeppeniemeyer: the branch is identical15:24
niemeyerrogpeppe: Okay, I se15:24
niemeyerrogpeppe: So it's just the 310015:25
rogpeppeniemeyer: yeah15:25
niemeyerrogpeppe: Superb, thanks15:25
rogpeppeniemeyer: except the 66 shows just the changes that you need to see15:25
niemeyerrogpeppe: I'll have a look once I'm back from my daily driver duties, which are finishing today luckily15:25
rogpeppeniemeyer: np15:25
niemeyerrogpeppe: I don't get that part15:26
rogpeppeniemeyer: it's unexpected but useful in this case15:26
rogpeppeniemeyer: presumably it's something to do with lbox's logic. i don't pretend to understand.15:26
niemeyerrogpeppe: I've already reviewed the 66, and that's not it15:26
niemeyerrogpeppe: Well, we should, because that's a monster branch15:26
niemeyerrogpeppe: Saying it's the same as a tiny branch isn't clear15:27
rogpeppeniemeyer: the launchpad branches *are* the same15:27
niemeyerrogpeppe: If it's the same let's make it the same15:27
rogpeppeniemeyer: the CL is just shown differently in both cases15:27
rogpeppeniemeyer: it is really a monster branch15:27
niemeyerrogpeppe: Yes, because you reverted tons of changes, and this branch is adding them back!15:27
rogpeppeniemeyer: yeah15:27
rogpeppeniemeyer: i don't know why the 066 isn't showing the monster15:28
niemeyerrogpeppe: Because it was already submitted, and was done previously15:28
niemeyerrogpeppe: Why is it adding the changes back?15:28
rogpeppeniemeyer: but it's useful because it shows just the changes i've made on top of the original15:28
niemeyerrogpeppe: I don't get it, sorry15:29
niemeyerrogpeppe: You've just reverted tons of changes, and now is proposing a branch that adds it back exactly as it was15:29
rogpeppeniemeyer: no exactly as it was. the 066 branch shows you the changes that i made on top of the original (unreverted) changes15:29
niemeyerrogpeppe: And asking me to review it, and saying you don't know what's going on.. I'm not very comfortable15:29
mgzniemeyer: do you just want the original branch as a prerequiste so you can see what's changed?15:30
mgzclearly the branch itself will contain much the same changes (plus whatever fixup was needed), so the default diff will15:30
rogpeppemgz: hmm, maybe that'll work. except that... i'm not sure it can.15:30
niemeyerrogpeppe: The 66 is the 66.. it shows the changes you made that *went in*15:31
niemeyerrogpeppe: It was submitted15:31
rogpeppeniemeyer: no, i've reproposed it15:31
rogpeppeniemeyer: which might've been the wrong thing to do, admittedly :-)15:31
niemeyerrogpeppe: Yes, the same branch, wiht the same changes15:31
niemeyerrogpeppe: Including what was reverted15:31
rogpeppeniemeyer: i'm not sure i understand.15:32
rogpeppeniemeyer: we've got one branch which, now, includes all the unrevert changes, and a few changes i made on top of those.15:32
mgzniemeyer: the reverted stuff does still want to land, it was reverted so it could be fixed up, not because the change was rejected15:33
mgzso, the new proposal naturally includes many of the same things as the original proposal that got reverted15:33
rogpeppeyeah, that's it15:33
niemeyerYeah, so it's not the same thing15:34
rogpeppethe new proposal is almost the same as the old proposal. the 066 branch conveniently says exactly how it's different15:34
niemeyermgz: <rogpeppe> niemeyer: the launchpad branches *are* the same15:34
niemeyerSorry guys, I really have to leave now15:34
niemeyerI'll be back later15:34
mgzI think he means the branch (name/location) is the same, it now includes a few more changes over last time though15:34
rogpeppemgz: no, they really are the same15:36
rogpeppemgz: http://paste.ubuntu.com/1375040/15:36
rogpeppemgz: well, with that one change (i'd accidentally reverted dave cheney's trunk change)15:36
mgzthat's a big paste...15:37
rogpeppemgz: oops!15:37
mgzokay, so you did basically what I would have, but in a slightly different order15:37
rogpeppemgz: i accidentally pastebinned my entire shell window15:38
mgzyou've got one proposal with basically the previous changes, and then another one on with that as a prereq and specifically has been fixed since last landing?15:38
rogpeppemgz: this is what i meant to paste: http://paste.ubuntu.com/1375042/15:39
mgzyou can fix the accidental trunk revert by just cherrypicking that rev again I think15:39
rogpeppemgz: i think i'll do that. it might make it easier to explain.15:39
rogpeppemgz: currently i have no prereq in the fix branch15:40
rogpeppemgz: i did a patch to fix the accidental trunk revert, which was probably the wrong approach15:40
mgzthat's also fine.15:40
rogpeppemgz: but i thought it wasn't too much problem for two lines...15:40
mgzprovided it's fixed in the feature branch, it doesn't matter that it briefly went walkies15:41
rogpeppemgz: i thought so too15:41
rogpepperight, i'll try again with a prereq15:42
mgzyeah, and just refer the inital proposal to the identical previous one, and mention fixups are following15:42
* rogpeppe wishes launchpad prereqs were more flexible15:43
TheMuefwereade_: You've got a review.15:54
rogpeppemgz: ha, now i've got a nice branch with prerequisite (https://codereview.appspot.com/6843101), but the original branch isn't showing any changes, because of course it's been submitted. gah!15:55
dimiternhey, my first CL in juju-core! :) https://code.launchpad.net/~dimitern/juju-core/openstack-stub-provider-config/+merge/135454 - anybody could take a look?15:55
rogpeppedimitern: can you propose it in codereview?15:55
dimiternrogpeppe: lbox is broken for me15:55
rogpeppedimitern: what happens?15:56
TheMuefwereade_: So you could review https://codereview.appspot.com/6852065/ and https://codereview.appspot.com/6853075/ for me.15:56
fwereade_TheMue, a pleasure15:56
dimiternit worked all the way until rietveld push, then: panic: runtime error: invalid memory address or nil pointer dereference15:56
TheMuerogpeppe: I've changed ^^ after your reviews.15:56
dimiternrogpeppe: wanna see the full paste?15:56
rogpeppedimitern: go on then15:56
TheMuefwereade_: Cheers.15:56
dimiterndimitern: http://paste.ubuntu.com/1375072/15:57
rogpeppedimitern: hmm, this looks suspicious to me: http://paste.ubuntu.com/1375082/16:01
mgzdimitern: do you want me to review on launchpad, or are you going to put up on cr too?16:01
mgz...I guess the answer to that question depends on if you can get lbox fixed16:02
dimiternmgz: exactly, I'd rather have lbox working16:05
rogpeppedimitern: could you try this:16:05
rogpeppedimitern: at launchpad.net/goetveld/rietveld/auth.go:18116:05
rogpeppedimitern: change the logf line to this:16:05
rogpeppelogf("Login on %s successful; error %v (%T)", rietveldURL, err, err)16:05
rogpeppedimitern: then run lbox propose -v16:06
dimiternrogpeppe: ok16:06
rogpeppedimitern: after go installing lbox16:06
rogpeppedimitern: (make sure you're using the lbox from your GOPATH, not /usr/bin/lbox)16:06
dimiternrogpeppe: ok, so it still fails, but at least with some other error: http://paste.ubuntu.com/1375096/16:11
mramm2Anybody need anything before I sign out for the evening? If not I'll see you all after thanksgiving.16:11
dimiternrogpeppe: I swear I authorized rietveld to use my @canonical account few days ago, and it seems it forgot, because initially login failed, but then I went to appspot and enabled it again, the result is in the last paste16:12
mramm2I was able to get a lot of organizational work done on today, and will be back working Monday16:12
mramm2monday/tuesday next week I'll be working on setting up blueprints for this cycle16:13
mramm2so I may ask for some help from folks on that that here and there -- just an FYI ;)16:13
dimiternmramm2: have a good thanksgiving ;)16:14
rogpeppedimitern: are you sure that's using the lbox you just changed?16:14
dimiternrogpeppe: well, I changed the code in goetveld, did go install launchpad.net/lbox and then run lbox propose -v16:14
rogpeppedimitern: are you sure you're not running /usr/bin/lbox?16:15
dimiternrogpeppe: dimitern@kubrik:~/work/juju-core$ which lbox16:15
rogpeppedimitern: ok, how about inserting a line after the client.Get in the same function:16:16
rogpeppelogf("client.Get returned %#v, %#v", r, err)16:16
rogpeppedimitern: 'cos i can't quite see how it can get to call Cookies without printing the "Login successful" message.16:17
dimiternrogpeppe: I'm doing it now16:17
dimitern2012/11/21 17:18:16 RIETVELD client.Get returned (*http.Response)(nil), &url.Error{Op:"Get", URL:"http://example.com/marker", Err:(*errors.errorString)(0xf8400a26d0)}16:18
rogpeppedimitern: you might check the mtime on the lbox binary to verify that it has actually been rebuilt too16:18
dimitern2012/11/21 17:18:16 RIETVELD Login on https://codereview.appspot.com successful; error Get http://example.com/marker: redirect blocked (*url.Error)16:18
dimitern2012/11/21 17:18:16 RIETVELD Login failed: Get http://example.com/marker: redirect blocked16:18
dimiternrogpeppe: ^^ it is rebuilt16:19
rogpeppedimitern: could i see the whole paste?16:20
dimiternrogpeppe: sure - http://paste.ubuntu.com/1375114/16:20
dimiternrogpeppe: what's that redirect blocked crap? it seems the same error is carried over from a previous call16:25
rogpeppedimitern: i dunno.16:26
dimiternrogpeppe: william is mentioning something similar happened to him before, but then he used lbox from some ppa and was fixed16:27
rogpeppedimitern: ah, i wonder if there are two url fetches happening concurrently...16:27
rogpeppedimitern: that would explain a lot16:27
dimiternrogpeppe: hmm.. ok, how to fix that?16:28
rogpeppedimitern: well, it *should* be just fine16:28
rogpeppedimitern: but it explains why i'm seeing the Login failed message after the Login successful message16:28
dimiternrogpeppe: i c16:29
dimiternrogpeppe: why lbox keeps asking me to login each time? any way to cache this?16:30
rogpeppedimitern: out of interest, i wonder if it makes a difference if you're using a different go version16:30
rogpeppedimitern: what does "go version" print?16:31
dimiterngo version go1.0.316:31
dimiternrogpeppe: I got it from golang.org and have it locally installed, rather than from apt16:31
rogpeppedimitern: hmm, 1.0.3 *should* be fine16:32
rogpeppedimitern: how about trying to replace the end of the function with this: http://paste.ubuntu.com/1375133/16:32
dimiternrogpeppe: will do16:32
rogpeppedimitern: that should stop the crash, but i'm thinking it won't fix your problem :-)16:32
rogpeppedimitern: in fact, here's a better thing, that should confirm the concurrent hypothesis too: http://paste.ubuntu.com/1375137/16:34
dimiternrogpeppe: it seems there is an auth issue16:34
rogpeppedimitern: yeah, your authorisation is failing somehow16:35
rogpeppedimitern: in a way not anticipated by lbox...16:35
dimiternrogpeppe: I applied the last patch and retrying now16:36
dimiternrogpeppe: http://paste.ubuntu.com/1375145/16:37
dimiternrogpeppe: this time, it didn't crash (and the last patch didn't - as you said), but it asked me twice for creds16:38
rogpeppedimitern: ah, i think i understand now16:38
rogpeppedimitern: there were two concurrent auth requests. the auth caching will block out one of them16:39
rogpeppedimitern: then one crashes and the other manages to step in and do something before the panic hits16:39
dimiternrogpeppe: hmm..16:39
rogpeppedimitern: something like that, anyway16:39
dimiternrogpeppe: do you see a solution?16:39
rogpeppedimitern: i'd be interested to see if it fails against go tip16:40
rogpeppedimitern: try going to your $GOROOT/src, running hg update tip, then all.bash16:41
dimiternrogpeppe: ok16:41
rogpeppedimitern: then go install launchpad.net/lbox again16:42
dimiternrogpeppe: there's no .hg repo there16:42
rogpeppedimitern: oh i thought you'd done an hg get from golang.org16:42
dimiternrogpeppe: no, it seems I got a tarball16:43
rogpeppedimitern: you might wanna do that. it's useful to have.16:43
dimiternI can check it out in another dir and change paths to use it - feels safer anyway16:43
rogpeppedimitern: see https://code.google.com/p/go/source/checkout16:44
rogpeppedimitern: yeah, i keep two go roots around16:44
rogpeppedimitern: but it's actually safer to always build a given tree against the same GOROOT16:44
rogpeppedimitern: so that mtime comparison works properly16:45
dimiternrogpeppe: ok, I'm checking out go tip, then I'll run ./all.bash in src and then get to lbox, right?16:45
rogpeppedimitern: yeah. you can usually interrupt after the tests have begun in all.bash BTW16:46
rogpeppedimitern: it makes rebuilding quite a bit faster16:46
dimiternrogpeppe: ok16:46
rogpeppedimitern: although it's worth running the tests at least once, probably16:46
rogpeppedimitern: they only take a couple of minutes16:46
dimiternrogpeppe: couple of minutes i can handle :) when it's not 2h16:47
rogpeppedimitern: yeah, we're spoilt with go16:49
dimiternrogpeppe: it's done - all tests passed, and I changed $GOROOT to point to go-tip, but left $GOPATH the old value, so I don't have to move everything16:59
dimiternrogpeppe: then did go install launchpad.net/lbox, and now I'm doing lbox propose -v again17:00
rogpeppedimitern: that should work17:00
dimiternrogpeppe: yes! it worked this time!17:02
rogpeppedimitern: ah, i thought that might be the case17:02
rogpeppedimitern: there's some weird shit going on with http requests, and i know gustavo's unhappy with something about it in 1.0.317:03
dimiternrogpeppe: now would you care to review? :) https://codereview.appspot.com/6858052/17:03
rogpeppedimitern: looking17:03
dimiternrogpeppe: hmm, but now, once I have lbox compiled and running, I should switch back to go 1.0.3, right?17:03
rogpeppedimitern: personally i run against tip most of the time17:04
rogpeppedimitern: but i test against 1.0.3 too17:04
dimiternrogpeppe: I'll try to do that as well then17:04
rogpeppedimitern: there's actually no reason you can't switch back to 1.0.317:05
rogpeppedimitern: just don't recompile lbox against it17:05
dimiternrogpeppe: yeah, for sure I won't :) now that I have it working finally17:05
TheMueSo, have to leave, no lunch, but now is dinnertime. CU tomorrow.17:12
rogpeppedimitern: you have a review17:26
dimiternrogpeppe: thanks!17:26
rogpeppeniemeyer: here's another version of the CL, hopefully easier to understand what's going on now: https://codereview.appspot.com/684310117:27
rogpeppeniemeyer: it has the old branch as a prereq17:28
rogpeppeniemeyer: (you can see the contents of the old branch by clicking on the comments)17:28
mgzdimitern: also reviewed.17:29
dimiternmgz: thanks! 2 reviews now - so I'll fix what was suggested17:29
rogpeppeniemeyer: hmm, maybe you'll still be unhappy.17:30
rogpeppemgz: secret attrs is for attributes that can't be pushed in cloudinit, and will be pushed over a secure connection on the first connection after bootstrap17:31
dimiternmgz: what I didn't get is are you generally happy or sad with the review :)17:31
mgzdimitern: it's a stub, so most of the comments are just comments :)17:32
dimiternmgz: yes, but the config is basically as it will be17:33
mgzI'd rename the config vars now though17:33
dimiternmgz: fair enough, that I'll do yes17:33
rogpeppeyeah, we try to go for python config compatibility except where necessary17:33
mgzidentity-url -> auth-url and probably tenant-id -> project-name (but that one we'll want to fiddle with later anyway, as want to cope with being given either name or id)17:34
dimiternmgz: so the CredsFromEnv in the refactored code from ian is now in goose?17:35
mgzit predates that, but the client code now uses that identity code, yes17:36
mgzI note rogpeppe also commented on the envvar overriding in your config_test.go file (module? what's the right term in go?), so that may be worth tackling now as well17:37
rogpeppemgz: file17:37
rogpeppemgz, dimitern: yeah, if there are potentially more env vars to come, it's definitely worth doing17:38
dimiternmgz, rogpeppe: so tenant-id -> project-name or tenant-name17:38
rogpeppedimitern: i'm sorry, i'm not familiar with the openstack config names17:38
mgzdimitern: it's a long story :)17:39
dimiternrogpeppe: well the env var is OS_TENANT_NAME17:39
mgz"tenant" is what the thing is called in the keystone code at present, but they keep threatening to rename it17:39
rogpeppemgz: if in doubt, do what python does now17:39
dimiternso I'll change it to tenant-name17:39
mgzit used to be called project, and may again at some point, because it's not actually a tenant17:39
mgzand whenever people talk about, for instance, multi-tenancy in openstack they mean a different kind of 'tenant'17:40
mgzso, it's an overloaded term, which is why I used 'project-name' in the original implementation (the having both name an id is an additional complication...)17:41
rogpeppeniemeyer: finally, here's a version that you might possibly be happy with: https://codereview.appspot.com/6850087/17:43
mgzsee for instance this line in your novarc:17:43
mgzexport NOVA_PROJECT_ID="${OS_TENANT_NAME}"17:43
rogpeppeniemeyer: it has the original branch as a prereq (in a different CL, so all the changes are visible)17:44
dimiternmgz: not sure i want to couple the openstack/config with the goose/identity just yet though17:45
mgzdimitern: that's probably fine to leave for later, but I'd add some comments about what goes where17:46
dimiternmgz: won't it be better to leave the dummyAuth for now, until ..17:46
dimiternmgz: yeah, just started writing the same thing17:46
dimiternok, I'll go back to my place and work on the CL some more, see you guys tomorrow!17:48

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!