| === davecheney is now known as davecheney95 | ||
| jam | wallyworld: /wave, how's it going today? | 06:10 |
|---|---|---|
| wallyworld | hi, 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 tests | 06:11 |
| jam | wallyworld: sounds good. I fully support ratcheting the changes into trunk as much as possible. | 06:13 |
| wallyworld | yeah, 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 clean | 06:14 |
| wallyworld | i'm wishing Go had infrastructure for IOC and such | 06:14 |
| wallyworld | this is the type of project hat would really benefit from those patterns | 06:15 |
| jam | wallyworld: isn't that what interfaces are for? | 06:23 |
| wallyworld | sure, but you need an infrastructure to cleanly to the dependency injection | 06:23 |
| wallyworld | eg Spring in the Java world | 06:24 |
| TheMue | Morning | 07:01 |
| === 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 | ||
| rogpeppe | mornin' all | 07:40 |
| TheMue | rogpeppe: Hiya. | 07:40 |
| TheMue | Iiirks! | 07:45 |
| TheMue | My test just stopped my VM. | 07:45 |
| fwereade__ | hey all | 07:55 |
| TheMue | fwereade__: Morning. | 07:57 |
| rogpeppe | davecheney95: sorry about trunk breakage. looks like i hadn't broken a test in the right way before checking that it passed. | 07:58 |
| rogpeppe | davecheney95: will fix asap | 07:58 |
| rogpeppe | fwereade__: hiya | 07:59 |
| davecheney95 | rogpeppe: it's not broken per 'se | 08:00 |
| rogpeppe | davecheney95: it is! | 08:00 |
| davecheney95 | i just copied some of the other cert files that juju generated into the right place and it worked fine | 08:00 |
| rogpeppe | davecheney95: it's quite badly broken | 08:01 |
| rogpeppe | davecheney95: i've got some code that generates cert and key in a single file, but the new config stuff expects it in two | 08:01 |
| davecheney95 | rogpeppe: orly; | 08:01 |
| davecheney95 | i got it to work with only one | 08:02 |
| rogpeppe | davecheney95: i thought it was ok 'cos my tests would've caught a problem, but it was masked | 08:02 |
| davecheney95 | anyhoo: flick me a CL and i'll give it a review | 08:02 |
| davecheney95 | file stuff on disk is hard | 08:02 |
| rogpeppe | davecheney95: yeah, it'll work if the cert is already there | 08:02 |
| rogpeppe | davecheney95: it's just these big changes - i've got quite a bit up in the air currently | 08:03 |
| davecheney95 | rogpeppe: yup, that is what I did, i copied a pem file that juju generated from a previos revision into the right place and it worked | 08:03 |
| davecheney95 | which was promising | 08:03 |
| rogpeppe | davecheney95: hopefully it'll all be reconciled soon - the CL will be a temporary patch before the code goes away | 08:03 |
| * davecheney95 is uncomfortable about the cheques we're writiing for the juju tool to do as a setup agent | 08:03 | |
| * davecheney95 proposes a new command, juju setup | 08:04 | |
| davecheney95 | which is also run by default in some situatoins | 08:04 |
| TheMue | Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargh! | 08:06 |
| TheMue | ErrorSitsInFrontOfTheScreenException | 08:06 |
| TheMue | *ashamed* | 08:06 |
| davecheney95 | -EFACEPALM ? | 08:06 |
| TheMue | davecheney95: Exactly! *grmblx* | 08:07 |
| TheMue | Too dumb to use a self written command correct. | 08:07 |
| wallyworld | jam: 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 |
| jam | wallyworld: 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/cobzr | 08:18 |
| jam | So instead of [/path/to/branches] you use [/path/to/branches/.bzr/cobzr] | 08:18 |
| jam | wallyworld: I think abentley worked on a 'just take the last name' rather than appendpath | 08:18 |
| wallyworld | ah ok, will try thanks. i should have just stuck to what worked :-) | 08:18 |
| jam | but I don't remember how that ended up. | 08:18 |
| wallyworld | ok | 08:19 |
| jam | Also, you might try something with nick, let me look at the code abit. | 08:19 |
| jam | wallyworld: just to mention, I just use a lightweight checkout pointing to my normal locations for branches. | 08:19 |
| jam | which gives you the same functionality as cobzr, but the branches don't exist in a subdir. | 08:19 |
| wallyworld | yes, that's what i normally do too | 08:20 |
| wallyworld | i think i'll ditch cobzr and revert to that | 08:20 |
| wallyworld | jam: actually, your idea aboce to add .bzr/cobzr to the containing locations seems to work | 08:22 |
| rogpeppe | davecheney95: are you talking about the certificate generation stuff? | 08:23 |
| rogpeppe | davecheney95: (which, BTW is more broken than i realised, dammit - the fix isn't quite as trivial as i want) | 08:24 |
| rogpeppe | wallyworld: FWIW i use cobzr but i've never touched location.conf AFAIR | 08:25 |
| davecheney95 | rogpeppe: all i can say is what was in the email | 08:25 |
| davecheney95 | i went to use a environment i hadn't used about 10 revisions ago | 08:25 |
| davecheney95 | and it whinged that I didn't have the magic certificate poop | 08:25 |
| wallyworld | rogpeppe: i use to to save me typing the full urls each time i push etc | 08:25 |
| rogpeppe | davecheney95: that's because i broke it, not necessarily because it's fundamentally the wrong design | 08:25 |
| rogpeppe | wallyworld: ah, that sounds useful actually | 08:26 |
| wallyworld | rogpeppe: it's quite awesome. i just type "bzr push" and it knows what to do | 08:26 |
| wallyworld | i can pastebin my relevant config if you like | 08:27 |
| rogpeppe | davecheney95: that happens for me too - i just rely on lbox to work out where it wants to push to, then it remembers is | 08:27 |
| rogpeppe | it | 08:27 |
| rogpeppe | wallyworld: please | 08:27 |
| wallyworld | ok, sec | 08:27 |
| rogpeppe | wallyworld: that 2nd last remark should've been addressed to you. | 08:27 |
| wallyworld | ok. i'm just trying lbox now for the first time | 08:28 |
| rogpeppe | davecheney95: BTW the certificate generation stuff is moving from the juju package to the environs package | 08:28 |
| wallyworld | why don't we use lp for mps? isn't that the company standard? did we get an exception? | 08:28 |
| rogpeppe | wallyworld: we do | 08:29 |
| rogpeppe | wallyworld: but we use codereview too | 08:29 |
| rogpeppe | wallyworld: because it's considerably awesomer :-) | 08:29 |
| wallyworld | ok, i'm still ramping up on juju processes | 08:29 |
| davecheney95 | wallyworld: i should have mention, ping me if you need help in your timezone | 08:29 |
| wallyworld | i'll see real soon now what's it's like :-) | 08:29 |
| wallyworld | davecheney95: thanks, will do | 08:29 |
| davecheney95 | your timezone is like my timezone, except we're not scared of confusing the livestock | 08:29 |
| * wallyworld wishes we had daylight saving | 08:30 | |
| * davecheney95 wishes we didn't | 08:30 | |
| wallyworld | lol | 08:30 |
| jam | wallyworld: really? The 2h time shift with the rest of the world tends to be very disruptive | 08:30 |
| davecheney95 | i'm already getting grief about the tuesday meeting 'cos it's slipped to 11pm | 08:30 |
| jam | poolie had it, and it was quite annoying | 08:30 |
| davecheney95 | it's great to get 2 hours more with the yanks | 08:30 |
| rogpeppe | davecheney95: what time is it for you now? | 08:30 |
| wallyworld | excepting collaboration, i really like it | 08:30 |
| davecheney95 | 19:30 | 08:31 |
| wallyworld | 18:30 for me | 08:31 |
| davecheney95 | when I worked at the trading company, daylight saving sucked | 08:31 |
| davecheney95 | we had to work til 9pm to cover the indian market | 08:31 |
| wallyworld | it makes it really nice for family time at the end of the day | 08:31 |
| davecheney95 | wallyworld: which part of brisvegas are you in ? | 08:32 |
| wallyworld | fig tree pocket | 08:32 |
| wallyworld | western suburbs | 08:32 |
| wallyworld | you? | 08:32 |
| davecheney95 | wallyworld: latte sipping inner west sydney | 08:32 |
| wallyworld | bondi? | 08:32 |
| wallyworld | oh, that's east | 08:33 |
| wallyworld | balmain maybe | 08:33 |
| wallyworld | i don't know sydney that well | 08:33 |
| davecheney95 | ashfield, almost the same | 08:33 |
| davecheney95 | bit further south from the river | 08:33 |
| rogpeppe | davecheney95: 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.30am | 08:34 |
| wallyworld | rogpeppe: 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] section | 08:35 |
| wallyworld | when i add juju-core or other projects | 08:35 |
| wallyworld | so now bzr info gives: | 08:36 |
| wallyworld | Related branches: | 08:36 |
| wallyworld | public branch: bzr+ssh://bazaar.launchpad.net/~wallyworld/goose/client-using-identity | 08:36 |
| wallyworld | push branch: bzr+ssh://bazaar.launchpad.net/~wallyworld/goose/client-using-identity | 08:36 |
| wallyworld | parent branch: .bzr/cobzr/master | 08:36 |
| wallyworld | submit branch: bzr+ssh://bazaar.launchpad.net/~gophers/goose/trunk | 08:36 |
| wallyworld | so stuff it cares about is set correctly | 08:36 |
| rogpeppe | wallyworld: cool, thanks | 08:36 |
| wallyworld | np | 08:37 |
| rogpeppe | wallyworld: and you put that in ~/.bzr/locations.conf ? | 08:37 |
| wallyworld | ~/.bazaar | 08:37 |
| wallyworld | locations.conf | 08:37 |
| rogpeppe | ha, that's funny | 08:38 |
| rogpeppe | i did actually set up a locations.conf file once | 08:39 |
| jam | rogpeppe: 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 |
| rogpeppe | jam: yeah, i did have that setup originally | 08:39 |
| rogpeppe | jam: my current locations.conf has a push_location of lp:~rogpeppe/ensemble :-) | 08:40 |
| jam | that sounds like it has been a while :) | 08:40 |
| rogpeppe | jam: blast from the past | 08:40 |
| wallyworld | save 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 |
| rogpeppe | wallyworld: in the root juju directory, run "go fmt ./..." | 08:44 |
| wallyworld | cool, thanks | 08:44 |
| rogpeppe | wallyworld: or, from any directory, run "go fmt launchpad.net/juju-core/..." | 08:44 |
| wallyworld | i was typing gofmt | 08:44 |
| rogpeppe | wallyworld: or to do it in the current directory only, run "go fmt" | 08:45 |
| wallyworld | worked, thanks | 08:45 |
| wallyworld | i didn't appreciate the difference between gofmt and go fmt | 08:45 |
| rogpeppe | wallyworld: (the go command always uses the package in the current directory by default) | 08:45 |
| wallyworld | ok | 08:45 |
| rogpeppe | wallyworld: gofmt allows access to a few features that go fmt doesn't | 08:45 |
| rogpeppe | wallyworld: for example the -r flag (which can be awesome BTW) | 08:46 |
| rogpeppe | wallyworld: go fmt does actually just call gofmt, i think | 08:46 |
| wallyworld | gofmt seemed to print the file to stdout | 08:46 |
| wallyworld | files | 08:46 |
| rogpeppe | wallyworld: yeah, that's the default. although you can use gofmt -w too | 08:46 |
| wallyworld | iw works with ./... ? | 08:47 |
| wallyworld | -w | 08:47 |
| rogpeppe | wallyworld: no. | 08:47 |
| jam | wallyworld: I'm pretty sure gofmt doesn't support './...' but 'go fmt' does | 08:47 |
| wallyworld | ok, thanks | 08:47 |
| rogpeppe | wallyworld: the go tool is the one that understands package wildcards etc | 08:47 |
| rogpeppe | wallyworld: gofmt just knows about files | 08:48 |
| rogpeppe | wallyworld: i almost always use go fmt | 08:48 |
| wallyworld | ok | 08:48 |
| rogpeppe | wallyworld: except when i'm editing - then i'll often pipe a section of a file (or the whole thing) through gofmt | 08:48 |
| wallyworld | i might look at setting up my IDE to use it in the background | 08:49 |
| wallyworld | i already have code inspections but not fully go fmt compliant | 08:49 |
| rogpeppe | wallyworld: for instance if you use vim, you can do :1,$!gofmt | 08:50 |
| rogpeppe | wallyworld: i know various people set their editors up to automatically gofmt on save | 08:50 |
| wallyworld | i use Intellij with a golang plugin | 08:50 |
| wallyworld | i'll look into it | 08:50 |
| jam | rogpeppe: 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 |
| rogpeppe | wallyworld: and i'm pretty sure that some people (fwreade__ ?) change tabs to spaces on load, and back to tabs on save. | 08:51 |
| wallyworld | i'm just waiting on integrated debugging to be supported. i really miss that from Java and Python | 08:51 |
| jam | So instead, I just do "go fmt ./... && go test ./..." so every test run reformats. | 08:51 |
| rogpeppe | jam: yeah i can see how that might be annoying | 08:51 |
| jam | wallyworld: ISTR that gdb works with go code | 08:51 |
| wallyworld | i really dislike the use of tabs - everyone i mentioned it to is quite incredulous | 08:52 |
| jam | Losing | 08:52 |
| jam | *sigh* too easy to get that one wrong | 08:52 |
| rogpeppe | wallyworld: what don't you like about them? | 08:52 |
| wallyworld | i've not used gdb - will have to learn, i rely on my ide | 08:52 |
| jam | wallyworld: well, I would guess if gdb works, many ides might use that on the backend. | 08:53 |
| rogpeppe | yeah, gdb does work (i checked once) but i never use it | 08:53 |
| jam | Which means you could have intellij integration earlier than later. | 08:53 |
| wallyworld | rogpeppe: i can't quite put it into words easily why i dislike tabs, no other language i know of ides it | 08:53 |
| wallyworld | does | 08:53 |
| jam | wallyworld: I worked on a project in C/C++ that mandated tabs so that people could configure their preferred indent. | 08:53 |
| jam | Most python projects mandate no tabs. | 08:54 |
| wallyworld | and Java too | 08:54 |
| jam | IME | 08:54 |
| rogpeppe | wallyworld: i've used tabs for a very long time, so i'm always interested to know why people don't like them | 08:54 |
| rogpeppe | jam: with python it's understandable, because getting the indentation right is crucial | 08:54 |
| jam | rogpeppe: yeah, the main problem in python is that you *can* mix spaces and tabs, and that way lies madness. :) | 08:55 |
| rogpeppe | wallyworld: i like the fact it's just a single character to type to add or remove a level of indentation | 08:55 |
| wallyworld | rogpeppe: i guess because the spacing becomes variable for everyone, depending on their setup | 08:55 |
| rogpeppe | jam: yeah | 08:55 |
| wallyworld | rogpeppe: my IDe does all that - i hit tab and it inserts saces on thre fly | 08:55 |
| jam | rogpeppe: sure but I use << >> in vim anyway, so indentation is a simple select and move regardless the characters used to indent. | 08:55 |
| wallyworld | and it auto indents according to the code style in use | 08:55 |
| rogpeppe | wallyworld: yeah - that's a problem if people rely on beautiful code alignment. you can't do that in go though :-) | 08:55 |
| wallyworld | so i never type lots of spaces | 08:55 |
| wallyworld | rogpeppe: and the preference for tabs = 8 is madness IMO | 08:56 |
| wallyworld | way too much | 08:56 |
| davecheney95 | wallyworld: for python, probably | 08:56 |
| rogpeppe | wallyworld: don't you need to type a few delete characters to remove a level of indentation | 08:56 |
| rogpeppe | ? | 08:56 |
| wallyworld | nope, IDe does it | 08:57 |
| davecheney95 | but it could be a million, it just depends what your editor is set too | 08:57 |
| jam | rogpeppe: I know vim can be configured to know when you are deleting indent chars vs regular spaces. | 08:57 |
| wallyworld | it knows whatr the code style is and handles all that | 08:57 |
| jam | However, I use <<, I imagine wally does "shift + tab" | 08:57 |
| jam | ISTR doing that in VC in the long distant past. | 08:57 |
| wallyworld | Backspace deletes all the white space unti lthe next indentation block | 08:57 |
| wallyworld | just like if tabs were used | 08:58 |
| rogpeppe | wallyworld: i don't think go has a preference for tabstop=8 particular | 08:58 |
| rogpeppe | y | 08:58 |
| rogpeppe | ly | 08:58 |
| wallyworld | i think i read it somewhere | 08:58 |
| wallyworld | but i've crammed a lot of Go docs in the past few days | 08:58 |
| rogpeppe | wallyworld: personally, i'm a heretic because i use a proportionally spaced font | 08:58 |
| jam | wallyworld: 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 |
| wallyworld | really???? | 08:59 |
| jam | rogpeppe: there is a slight preference if go fmt ends up aligning something with spaces on a continuation line, etc. | 08:59 |
| wallyworld | jam: yeah, but as you said, it may mess with alignment | 08:59 |
| jam | I thought, I could be wrong. | 08:59 |
| rogpeppe | jam: yeah. i don't mind much though. | 08:59 |
| jam | wallyworld: occasional mis alignment could be a lot better than constant extra depth for you | 08:59 |
| rogpeppe | jam: i don't find it affects readability | 08:59 |
| wallyworld | rogpeppe: why proportional spaced font? | 09:00 |
| rogpeppe | wallyworld: but i get more readable text per pixel with a proportionally spaced font | 09:00 |
| jam | rogpeppe: does it do fixed width spaces for aligned tables? | 09:00 |
| rogpeppe | jam: yeah, i can switch trivially if i need to | 09:01 |
| rogpeppe | jam: aligned tables are pretty rare though | 09:01 |
| jam | rogpeppe: except for all of the inline comments in structs :) and `json:content` stuff | 09:01 |
| rogpeppe | jam: yeah. but the alignment there is just prettification. it doesn't make much difference if it's not aligned. | 09:02 |
| rogpeppe | jam: unlike a table | 09:02 |
| jam | sure | 09:02 |
| rogpeppe | if 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 |
| rogpeppe | anyway, excuse me, i need to un-break trunk :-) | 09:03 |
| wallyworld | jam: 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 |
| wallyworld | lp can take a little time to generate the code diff | 09:14 |
| wallyworld | how do i send the diff to codereview once lp has calculated it? | 09:14 |
| jam | wallyworld: sounds odd, offhand sounds like an incorrect submit branch. | 09:14 |
| jam | wallyworld: lbox computes the diff and proposes it for you | 09:14 |
| rogpeppe | wallyworld: what does "bzr diff --old lp:goose/trunk" print? | 09:14 |
| wallyworld | jam: the lp mp says the submit branch is https://code.launchpad.net/~gophers/goose/trunk | 09:14 |
| wallyworld | jam: that command prints the diff as expected | 09:15 |
| rogpeppe | hmm, odd | 09:15 |
| rogpeppe | wallyworld: what does lbox propose -v print? | 09:16 |
| wallyworld | lp also shows the preview diff, let me type the the above and see | 09:16 |
| wallyworld | rogpeppe: 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 wip | 09:18 |
| rogpeppe | wallyworld: that sounds like it all worked | 09:18 |
| rogpeppe | wallyworld: did it print a codereview url? | 09:18 |
| wallyworld | yeah, i have no idea what happened the first time | 09:18 |
| wallyworld | rogpeppe: ah no, my apologies - it still failed: error: Failed to send patch set to codereview: diff is empty | 09:19 |
| rogpeppe | wallyworld: could you paste the lbox output? | 09:19 |
| wallyworld | rogpeppe: https://pastebin.canonical.com/78814/ | 09:20 |
| wallyworld | rogpeppe: i have to go and put my son to bed and read him a story, i'll be back in a bit | 09:21 |
| jam | wallyworld: Diffing branches for codereview: /home/ian/juju/go/src/launchpad.net/goose => /home/ian/juju/go/src/launchpad.net/goose/.bzr/cobzr/lbox-805004725/lbox | 09:25 |
| jam | It looks like lbox is treating your goose checkout as the submit target. | 09:25 |
| jam | And doing a diff of the code against itself. | 09:25 |
| jam | obviously that is an empty diff. | 09:25 |
| jam | I don't know where lbox gets the URL of the branch to diff against. | 09:25 |
| jam | I would have thought that would be in .lbox, which uses "propose -cr -for lp:goose" | 09:26 |
| jam | Maybe it is looking up something else in your config? | 09:26 |
| jam | rogpeppe: do you know where lbox gets the target to diff against? | 09:26 |
| rogpeppe | jam: it should get it from the -for flag, which in this case is set in .lbox | 09:27 |
| rogpeppe | jam: it looks like that worked too, but i'm not sure why it changed. | 09:30 |
| rogpeppe | jam: it looks like the FetchRemote logic is failing somehow | 09:34 |
| rogpeppe | jam: erm, no it doesn't - it all looks ok to me | 09:35 |
| jam | the 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 |
| rogpeppe | jam: hmm, it worked fine for me: https://codereview.appspot.com/6858050 | 09:41 |
| rogpeppe | except that testservices/identityservice/util.go needed gofmting, which makes me suspicious | 09:41 |
| rogpeppe | i'm not sure how the lbox check could have succeeded if that branch really was current | 09:42 |
| davecheney95 | rogpeppe: speaking of gofmt | 09:45 |
| davecheney95 | there are some small changes, and larger ones coming that make gofmt 1.0 and gofmt 1.1 incompatible | 09:45 |
| rogpeppe | davecheney95: really? in what way? | 09:46 |
| davecheney95 | there is a small change already that affects the padding of comments on struct members | 09:46 |
| davecheney95 | and gri is about to commit a change that makes | 09:46 |
| davecheney95 | for i := range s { i++ } | 09:46 |
| davecheney95 | valid | 09:46 |
| rogpeppe | davecheney95: orly?! | 09:47 |
| davecheney95 | which is going to upset our .lbox.check hook | 09:47 |
| rogpeppe | davecheney95: oh, you mean the formatting | 09:47 |
| davecheney95 | yes | 09:47 |
| rogpeppe | davecheney95: i thought you meant that you could influence a range variable within the body | 09:47 |
| davecheney95 | the syntax is not changing, just the canonical format | 09:47 |
| rogpeppe | davecheney95: i guess we'll have to standardise on 1.0.3 gofmt | 09:48 |
| davecheney95 | rogpeppe: i agree | 09:48 |
| rogpeppe | davecheney95: until 1.1 is released | 09:48 |
| davecheney95 | although it'll be 1.0.2 | 09:48 |
| davecheney95 | 'cos that is what is in precise or quantal | 09:48 |
| rogpeppe | davecheney95: same difference, i think | 09:48 |
| davecheney95 | for this, yes | 09:48 |
| rogpeppe | davecheney95: only problem is: can we get the tip go tool to use a different gofmt? | 09:49 |
| rogpeppe | davecheney95: i.e. does it just use the gofmt in $PATH or not? | 09:49 |
| wallyworld | rogpeppe: 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 ok | 09:49 |
| davecheney95 | go space format from memory | 09:50 |
| davecheney95 | for people like me that live on tip | 09:50 |
| rogpeppe | wallyworld: i did the propose myself, and it worked, except that it required a gofmt, which seems odd | 09:50 |
| davecheney95 | we could add optoinal support for a GO_1_x path (making up a name) | 09:50 |
| wallyworld | hmmm. when i first did it it complained and then i fixed all the issues | 09:50 |
| davecheney95 | review request: https://codereview.appspot.com/6851081/ | 09:51 |
| davecheney95 | ^ i think this is gettig pretty resonable | 09:51 |
| jam | wallyworld: I should have a review for you in a bit. | 09:51 |
| wallyworld | jam: using the lp diff? | 09:51 |
| davecheney95 | eww | 09:52 |
| rogpeppe | davecheney95: go fmt just uses gofmt from $PATH, so there's no difficulty chopping and changing if we need to | 09:53 |
| jam | wallyworld: yeah, I'm still perfectly comfortable reviewing from LP. | 09:54 |
| davecheney95 | rogpeppe: nice, I can make that work | 09:54 |
| jam | and/or from a raw branch when necesasry. | 09:54 |
| wallyworld | \o/ | 09:54 |
| jam | I have to go in about 20min, so I'd rather get you a review than nothing today. | 09:55 |
| wallyworld | thanks | 09:55 |
| jam | wallyworld: 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 |
| wallyworld | jam: ok, will look. thanks | 10:16 |
| rogpeppe | davecheney95: you've got a review | 10:27 |
| davecheney95 | ty | 10:31 |
| dimitern | wallyworld: 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 |
| wallyworld | dimitern: thanks, i'll look after replying to jam's lengthy review | 10:32 |
| wallyworld | which i'm doing now | 10:32 |
| dimitern | wallyworld: feel free to blame the initial ugliness on me :) | 10:33 |
| wallyworld | prototype code! | 10:33 |
| wallyworld | not meant to be perfect | 10:33 |
| dimitern | yeah | 10:33 |
| wallyworld | dimitern: there is that one failing test - any idea? | 10:33 |
| dimitern | GetObject? | 10:33 |
| wallyworld | yeah | 10:34 |
| wallyworld | TestObjects | 10:34 |
| dimitern | wallyworld: 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 cloud | 10:35 |
| wallyworld | could be. will be nice to know | 10:36 |
| wallyworld | so we can plan around it for other things | 10:36 |
| dimitern | wallyworld: also, it seems you're not yet a member of The Go Language Gophers team and you should be | 10:39 |
| wallyworld | ah, haven't checked that. thanks, i'll follow up | 10:39 |
| niemeyer | Good morning ladies and gentlemen | 10:59 |
| TheMue | Binjour Monsieur Niemeyer | 11:00 |
| TheMue | s/Binjour/Bonjour/ | 11:00 |
| niemeyer | TheMue: Merci beaucoup | 11:06 |
| niemeyer | "beaucoup" must be my favorite word in French | 11:06 |
| niemeyer | I 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 |
| dimitern | niemeyer: yeah, the same applies to irish, scottish and manx :) | 11:10 |
| TheMue | niemeyer: Hehe, indeed, just discovered the number of vowels in "beaucoup". | 11:13 |
| rogpeppe | dimitern: perhaps you could put your initial commit up for review before we put ian's changes on top? | 11:21 |
| rogpeppe | niemeyer: yo! | 11:22 |
| niemeyer | rogpeppe: Heya | 11:22 |
| dimitern | rogpeppe: well, since it's already in trunk, how can I do this? | 11:22 |
| rogpeppe | dimitern: hmm, good question. erase trunk and start again? :-) | 11:22 |
| dimitern | rogpeppe: done! :D | 11:22 |
| dimitern | rogpeppe: that's what I meant yesterday by wanting someone to take a look at goose/client/client.go first | 11:23 |
| rogpeppe | dimitern: yeah, it would be a good idea i think | 11:24 |
| dimitern | rogpeppe: I'd really appreciate if you take a look then | 11:26 |
| rogpeppe | dimitern: 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 |
| rogpeppe | dimitern: will do when i see the CL | 11:27 |
| mgz | rogpeppe: that does make sense for future, but as this is mostly initial experimental hacking that will get replaced in pretty short order | 11:30 |
| rogpeppe | mgz: 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 |
| rogpeppe | mgz: there are lots of things there that can easily be got right from the word "go" | 11:32 |
| mgz | do I have to hit you for that pun? :) | 11:32 |
| * rogpeppe was blithely oblivious | 11:32 | |
| * rogpeppe apologises | 11:33 | |
| rogpeppe | i dunno, maybe it doesn't need a review, but... niemeyer, what thinkest thou? | 11:34 |
| niemeyer | rogpeppe: I tend to agree with you | 11:34 |
| niemeyer | If it's indeed scratch, it can be moved aside | 11:35 |
| dimitern | rogpeppe, 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 chunks | 11:35 |
| wallyworld | niemeyer: hey can you add me to gophers so i can push to goose trunk? | 11:35 |
| niemeyer | Even more if there's no review process in place | 11:35 |
| niemeyer | This is a major eye-opener | 11:35 |
| dimitern | niemeyer: 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 steps | 11:37 |
| rogpeppe | dimitern: i think we should add code to a blank trunk, in steps | 11:37 |
| mgz | niemeyer: there is review, it's just we're integrating the two initial stabs dimitern and jam did during UDS | 11:37 |
| mgz | which were not reviewed, they just got written | 11:37 |
| niemeyer | rogpeppe++ | 11:38 |
| mgz | so, jam had some comments about dimitern's code from then, which wallyworld is refactoring into the common set now | 11:38 |
| niemeyer | Although, 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 tested | 11:38 |
| mgz | so, 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 reviewed | 11:39 |
| rogpeppe | niemeyer: 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 |
| rogpeppe | pwd | 11:40 |
| niemeyer | rogpeppe: If we can't fix it promptly, we should revert it so other people can continue to use it | 11:47 |
| rogpeppe | niemeyer: 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 |
| rogpeppe | niemeyer: how would you go about reverting it? apply a reverse patch? | 11:48 |
| niemeyer | rogpeppe: Right | 11:49 |
| mgz | rogpeppe: then in your feature branch, merge trunk after the revert but keep the existing tree state, so it's ready to reland | 11:50 |
| rogpeppe | mgz: 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 |
| niemeyer | Yeah, the obvious "re-apply" doesn't work after the reverse | 11:52 |
| niemeyer | wallyworld: That's done | 11:52 |
| mgz | rogpeppe: basically switch to feature, then bzr merge trunk (or whatever branch layout you're using), then `bzr revert .` | 11:52 |
| wallyworld | niemeyer: thanks! | 11:53 |
| mgz | and commit | 11:53 |
| mgz | so, you keep the merge marker but reject the trunk changes | 11:53 |
| rogpeppe | mgz: doh! of course | 11:53 |
| rogpeppe | mgz: i was thinking that reverting would lose the merge merker | 11:53 |
| mgz | if there are intermediate things on trunk that need saving, you want to do it in two steps, but there shouldn't be in this case | 11:53 |
| niemeyer | mgz: I don't think that works | 11:54 |
| niemeyer | Because revert reverts | 11:54 |
| niemeyer | Including the merge | 11:54 |
| mgz | if you pass . it reverts the tree contents, not the merge marker | 11:54 |
| wallyworld | mgz: dimitern: my changes should be pushed | 11:55 |
| niemeyer | mgz: Ah, I missed the . | 11:55 |
| dimitern | wallyworld: thanks | 11:55 |
| rogpeppe | me too | 11:55 |
| mgz | wallyworld: thanks! | 11:55 |
| wallyworld | np | 11:55 |
| wallyworld | i'll address the refactorings tomorrow | 11:55 |
| rogpeppe | mgz: that's a very subtle distinction! | 11:56 |
| rogpeppe | hmm, my irc client says it's dead, but i still saw wallyworld's messages | 11:56 |
| mgz | right, it's less obvious than the inverse which is `bzr revert --forget-merges` which keeps the tree state but loses the marker | 11:56 |
| rogpeppe | niemeyer: https://codereview.appspot.com/6845070 | 12:01 |
| rogpeppe | perhaps i should just submit it - it's entirely automatic | 12:02 |
| niemeyer | rogpeppe: Yep, sounds sane | 12:06 |
| niemeyer | rogpeppe: 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 in | 12:07 |
| rogpeppe | niemeyer: i agree, but it's not an easy fix | 12:07 |
| rogpeppe | niemeyer: trying to make a fix made me realise a few issues with my current plan | 12:08 |
| niemeyer | rogpeppe: Sure, but it can be done on top of it, rather than *in* it | 12:08 |
| rogpeppe | niemeyer: i'm not sure. | 12:08 |
| niemeyer | rogpeppe: ? | 12:08 |
| rogpeppe | niemeyer: it's difficult to make the original cert generation work properly | 12:08 |
| rogpeppe | niemeyer: 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 |
| rogpeppe | niemeyer: and you can't parse it without the certificate | 12:09 |
| niemeyer | rogpeppe: I still don't understand how that makes it hard to make the change on top of the branch, rather than mixing stuff up | 12:10 |
| rogpeppe | niemeyer: it's not easy to make that branch pass tests. | 12:10 |
| rogpeppe | niemeyer: (when the tests aren't broken) | 12:10 |
| rogpeppe | niemeyer: it's quite chicken-and-eggy | 12:11 |
| niemeyer | rogpeppe: I see | 12:11 |
| rogpeppe | niemeyer: my ugly fix was to generate the certificate into a file with a known name | 12:12 |
| rogpeppe | niemeyer: then the generation can be separated from the parsing | 12:12 |
| rogpeppe | niemeyer: 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 cert | 12:15 |
| rogpeppe | niemeyer: a much easier alternative would be to use the same root cert for all environments | 12:15 |
| niemeyer | rogpeppe: Much easier yet non-reasonable, I think | 12:17 |
| rogpeppe | niemeyer: yeah, i thought so | 12:17 |
| niemeyer | rogpeppe: Where is that config parsing error taking place? | 12:18 |
| rogpeppe | niemeyer: the first line of BootstrapCommand.Run | 12:18 |
| rogpeppe | niemeyer: well, that's currently. | 12:19 |
| rogpeppe | niemeyer: the plan is to have it handled somewhere within environs.NewFromName | 12:19 |
| rogpeppe | niemeyer: but that's another issue actually | 12:19 |
| rogpeppe | niemeyer: in our current scheme, we parse all environments when we read the environments.yaml file | 12:20 |
| niemeyer | rogpeppe: 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 |
| niemeyer | rogpeppe: Kind of | 12:20 |
| niemeyer | rogpeppe: What you're saying is that we parse the environments.yaml file when we read the environments.yaml file, which seems self-defining | 12:21 |
| rogpeppe | niemeyer: yeah. but there are two levels of parsing - reading the environments.yaml file itself and turning each section into a *config.Config | 12:22 |
| rogpeppe | niemeyer: currently we do both | 12:22 |
| rogpeppe | niemeyer: and NewFromName simply calls ReadEnvironsBytes and returns one of the environs | 12:22 |
| niemeyer | rogpeppe: So, what's the issue? | 12:23 |
| rogpeppe | niemeyer: one issue is that ReadEnvironsBytes doesn't know the name that we have as a variable | 12:24 |
| rogpeppe | niemeyer: so we'd need to change the scheme we use in some way | 12:24 |
| niemeyer | rogpeppe: Why? | 12:24 |
| niemeyer | rogpeppe: I think ReadEnvironsBytes is fine as it is | 12:24 |
| niemeyer | rogpeppe: What are you thinking of doing? | 12:24 |
| rogpeppe | niemeyer: how can ReadEnvironsBytes work without generating a certificate if needed for each environment section? | 12:25 |
| niemeyer | rogpeppe: To build the certificates | 12:25 |
| rogpeppe | niemeyer: i'm thinking of doing the config parsing at a later stage. | 12:25 |
| niemeyer | rogpeppe: You're assuming other things that I can't tell yet | 12:25 |
| rogpeppe | niemeyer: so then we can always call ReadEnvironsBytes even if the certificates aren't in place yet | 12:25 |
| niemeyer | rogpeppe: 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 anything | 12:25 |
| niemeyer | rogpeppe: We can do that today | 12:26 |
| rogpeppe | niemeyer: how? the CA cert is required by the config. | 12:26 |
| niemeyer | rogpeppe: By not making it required by the config, for example.. this is the change you've just reverted | 12:26 |
| rogpeppe | niemeyer: we could do that. but it is really required in the end... | 12:27 |
| rogpeppe | niemeyer: perhaps that's ok though. we can insert checks in all the relevant places. | 12:30 |
| niemeyer | rogpeppe: Where do we want to generate the certificates? | 12:30 |
| rogpeppe | niemeyer: 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 |
| niemeyer | rogpeppe: NewFromName is definitely not the right place.. this is a trivial wrapper | 12:32 |
| rogpeppe | niemeyer: in my branch fix this morning, i added a function in juju: GenerateCACertificateIfNecessary | 12:32 |
| * niemeyer observes rogpeppe getting Javaeske | 12:32 | |
| rogpeppe | niemeyer: which actually seemed like a reasonable way of doing things | 12:32 |
| rogpeppe | niemeyer: yeah, i know. it was only a temporary name :-) | 12:32 |
| rogpeppe | niemeyer: 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 |
| niemeyer | rogpeppe: I think it should have a root cert for sure | 12:35 |
| niemeyer | rogpeppe: And config.New may also require the availability of the cert, actually | 12:35 |
| niemeyer | rogpeppe: The logic you had in place looks good | 12:35 |
| rogpeppe | niemeyer: in juju? | 12:36 |
| niemeyer | rogpeppe: ? | 12:36 |
| rogpeppe | niemeyer: sorry, which logic? | 12:36 |
| niemeyer | rogpeppe: config.New? | 12:36 |
| rogpeppe | niemeyer: ok, cool | 12:36 |
| rogpeppe | niemeyer: yeah, i think it's ok | 12:36 |
| rogpeppe | niemeyer: the problem being that we need to generate the certificate some time before calling config.New | 12:37 |
| rogpeppe | niemeyer: and i don't think we can do it before calling NewFromName (assuming that's what cmd/juju does in bootstrap) | 12:37 |
| niemeyer | rogpeppe: I think we should just change type environ to be struct { attrs } | 12:38 |
| rogpeppe | niemeyer: ooh, radical | 12:38 |
| rogpeppe | niemeyer: isn't that a huge change? | 12:39 |
| rogpeppe | niemeyer: assuming you mean environs.Environ | 12:39 |
| niemeyer | rogpeppe: I don't | 12:39 |
| rogpeppe | niemeyer: oh yeah | 12:40 |
| rogpeppe | niemeyer: phew | 12:40 |
| rogpeppe | niemeyer: yes, i agree | 12:40 |
| rogpeppe | niemeyer: that was my suggestion earlier | 12:40 |
| niemeyer | rogpeppe: There are still issues, though | 12:40 |
| niemeyer | rogpeppe: Oh, I hadn't noticed the suggestion before | 12: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 yet | 12:41 |
| rogpeppe | [12:25:51] <rogpeppe> niemeyer: so then we can always call ReadEnvironsBytes even if the certificates aren't in place yet | 12:41 |
| niemeyer | rogpeppe: I see.. you mean something else by parsing, okay | 12:41 |
| rogpeppe | niemeyer: yeah, sorry. it's checking rather than parsing. | 12:42 |
| niemeyer | rogpeppe: It's still not enough, though | 12:42 |
| rogpeppe | niemeyer: no, we need something that actually generates the certs | 12:42 |
| niemeyer | rogpeppe: More than that, we need a place to generate the certs | 12:43 |
| niemeyer | rogpeppe: ReadEnvironsBytes is purely in memory | 12:43 |
| rogpeppe | niemeyer: i was wondering about environs.BootstrapEnviron() | 12:43 |
| rogpeppe | niemeyer: something explicitly in environs that returns an environment suitable for bootstrapping, generating certs if necessary | 12:43 |
| rogpeppe | BootstrapEnviron isn't right tho | 12:44 |
| niemeyer | rogpeppe: How many Bootstrap functions do we need? :) | 12:44 |
| rogpeppe | it sounds too active | 12:44 |
| rogpeppe | niemeyer: yeah | 12:44 |
| rogpeppe | niemeyer: this is something we only want to happen at bootstrap time though. | 12:45 |
| niemeyer | rogpeppe: Yeah, that's something interesting to take into account | 12:47 |
| niemeyer | It's fine to error in other situations | 12:47 |
| rogpeppe | niemeyer: i'm wondering how things would be if we put all the $HOME-related logic into the juju package | 12:48 |
| rogpeppe | niemeyer: then it would be logical that juju.Bootstrap could generate certificates and write them to $HOME. | 12:49 |
| niemeyer | rogpeppe: Feels like fiddling without solving the underlying issue | 12:49 |
| niemeyer | rogpeppe: juju.Bootstrap is already bogus, I think | 12:49 |
| niemeyer | rogpeppe: It should be environs.Bootstrap | 12:49 |
| niemeyer | rogpeppe: We're loosing sight of what that package was supposed to be | 12:49 |
| rogpeppe | niemeyer: environs.Bootstrap works for me | 12:49 |
| rogpeppe | niemeyer: in which case, perhaps we could have environs.BootstrapWithName which would generate the certificates if necessary | 12:51 |
| niemeyer | rogpeppe: The problem isn't the name | 12:55 |
| niemeyer | rogpeppe: We have the environment name | 12:55 |
| rogpeppe | niemeyer: but we can't get an Environ without its certificate, so the existing juju.Bootstrap interface doesn't work | 12:56 |
| rogpeppe | niemeyer: so BootstrapWithName would read environments.yaml, generate certificates if necessary, then call environs.Bootstrap | 12:57 |
| * rogpeppe has thought of a very quick and easy way of getting that branch in without breaking trunk | 12:58 | |
| niemeyer | rogpeppe: BootstrapWithNameAndMaybeWithoutCertificates | 12:58 |
| rogpeppe | niemeyer: arf | 12:58 |
| niemeyer | rogpeppe: I'm pretty convinced that config.New shouldn't barf if there are no certificates | 12:58 |
| rogpeppe | niemeyer: hmm, so that logic is wrong now? | 12:59 |
| niemeyer | rogpeppe: There's no such logic now? | 12:59 |
| rogpeppe | niemeyer: true, it's just been reverted | 12:59 |
| rogpeppe | niemeyer: i meant the logic you said looks good earlier | 13:00 |
| niemeyer | rogpeppe: The points that should barf are the points that need the certificate (novel idea!) | 13:00 |
| rogpeppe | niemeyer: that seems fine | 13:01 |
| niemeyer | rogpeppe: environs.Bootstrap shouldn't even try to create the certs | 13:01 |
| rogpeppe | niemeyer: that still doesn't solve to problem of who *does* try to create the certs though | 13:01 |
| niemeyer | rogpeppe: The bootstrap command can do that by itself, I believe | 13:01 |
| niemeyer | rogpeppe: Which is one of the few control points that can actually relate BuildCerts (or whatever) to $HOME to the fact that Bootstrap is in progress | 13:02 |
| rogpeppe | niemeyer: i'm not sure. i'd prefer not to duplicate that logic in every piece of client code that calls Bootstrap | 13:02 |
| rogpeppe | niemeyer: but actually, maybe most client code (builddb included) should not actually be calling Bootstrap | 13:03 |
| niemeyer | rogpeppe: Hmm.. good point | 13:03 |
| niemeyer | rogpeppe: I mean the former one, not the latter | 13:04 |
| dimitern | mgz: I'm back | 13:07 |
| niemeyer | rogpeppe: What about having environs.Bootstrap taking a certsDir parameter? | 13:09 |
| niemeyer | rogpeppe: "" defaults to ~/.juju/ | 13:09 |
| niemeyer | rogpeppe: Use for writing only.. never reads from it | 13:09 |
| niemeyer | Used | 13:09 |
| rogpeppe | niemeyer: we'd want a way of doing an environs.Bootstrap *without* generating certificates | 13:10 |
| niemeyer | rogpeppe: Why? | 13:11 |
| rogpeppe | niemeyer: because we might want to use it in an environment with no writable directories. | 13:12 |
| niemeyer | rogpeppe: Erm? | 13:12 |
| niemeyer | rogpeppe: Do you have a realistic use case? | 13:12 |
| rogpeppe | niemeyer: i'm thinking of platforms like google appengine (not that that's currently a realistic target of course) | 13:13 |
| niemeyer | rogpeppe: 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 usable | 13:14 |
| rogpeppe | niemeyer: i suppose you can always provide a nonsense name in that case | 13:15 |
| niemeyer | rogpeppe: Why do we need a nonsense name? | 13:15 |
| rogpeppe | niemeyer: because you need to call environs.Bootstrap even if you're providing the cert up front | 13:15 |
| rogpeppe | niemeyer: and it requires a name | 13:15 |
| niemeyer | rogpeppe: 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 times | 13:16 |
| niemeyer | rogpeppe: environ Environ | 13:16 |
| rogpeppe | niemeyer: sorry, the certDir name | 13:16 |
| niemeyer | rogpeppe: LOL, okay | 13:16 |
| niemeyer | rogpeppe: 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 |
| rogpeppe | niemeyer: so presumably environs.Bootstrap would use SetConfig to set the certificate on the environment passed in | 13:17 |
| rogpeppe | niemeyer: 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 |
| niemeyer | rogpeppe: Seems pretty fictitious | 13:18 |
| niemeyer | rogpeppe: You're bootstrapping but want no side effects? :-) | 13:18 |
| rogpeppe | niemeyer: there are all kinds of places i could save stuff that are not on disk | 13:19 |
| niemeyer | rogpeppe: juju needs a certificate to work.. that's the only invariant we have | 13:19 |
| rogpeppe | niemeyer: or, not on the local filesystem at any rate | 13:19 |
| niemeyer | rogpeppe: If people don't want juju to generate the certificate for them, it's their problem to generate it | 13:19 |
| rogpeppe | niemeyer: yeah, true | 13:19 |
| rogpeppe | niemeyer: alternatively, forget about the "" meaning ~/.juju thing, and define environs.JujuDir = os.Getenv("HOME") + "/.juju" | 13:20 |
| rogpeppe | .or ConfigDir or whatever | 13:21 |
| rogpeppe | niemeyer: i think that would be my preference - more explicit | 13:21 |
| rogpeppe | niemeyer: another possibility: provide an interface{WriteFile(name string, data []byte)error}, and when nil, use a type that writes to ~/.juju | 13:25 |
| rogpeppe | niemeyer: i like that even better - nil is slightly easier to type than "" :-) | 13:25 |
| rogpeppe | niemeyer: and it makes it easier to test too | 13:25 |
| rogpeppe | niemeyer: func Bootstrap(env Environ, certWriter FileWriter) error | 13:27 |
| rogpeppe | niemeyer: also makes it easy to encrypt the certificate/key if desired | 13:28 |
| rogpeppe | although of course, then we'd want a converse in config.New | 13:30 |
| niemeyer | rogpeppe: Sounds good, but the certWriter should be func(name string, data []byte) error | 13:31 |
| niemeyer | rogpeppe: What converse? | 13:31 |
| rogpeppe | niemeyer: ok. | 13:31 |
| rogpeppe | niemeyer: ReadFile | 13:31 |
| niemeyer | rogpeppe: I don't think that's necessary | 13:31 |
| rogpeppe | niemeyer: not for the time being anyway | 13:32 |
| rogpeppe | niemeyer: at some point it would be nice to be able to hook into the ubuntu keyring mechanism | 13:32 |
| rogpeppe | niemeyer: (i'm not sure how that works at all) | 13:32 |
| rogpeppe | niemeyer: so that people can avoid storing their juju environment keys in plaintext | 13:33 |
| mgz | dimitern: can you glance over a branch for me? | 13:43 |
| dimitern | mgz: sure | 13:44 |
| mgz | dimitern: https://code.launchpad.net/~gz/goose/client_identity_refactor_fix/+merge/135394 | 13:44 |
| dimitern | mgz: pretty straightforward, LGTM | 13:47 |
| mgz | thanks! will land. | 13:48 |
| niemeyer | Lunch! | 13:59 |
| rogpeppe | niemeyer: 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 |
| rogpeppe | niemeyer: i guess i'll need to create another merge request, but the changes are more obvious in this one | 14:33 |
| rogpeppe | mgz: i'm not sure your technique of merging, then reverting works that well | 14:35 |
| rogpeppe | mgz: 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 |
| rogpeppe | mgz: i think i should've just copied the tree | 14:36 |
| mgz | rogpeppe: I did give a warning, there's an extra step if there have been intermediate unmerged landings | 14:39 |
| rogpeppe | mgz: i'm not sure exactly what you mean by "intermediate unmerged landings" | 14:40 |
| mgz | rereading what you wrote, I'm not sure I understand what your problem was either... | 14:40 |
| mgz | have you tried a test merge to trunk locally? it will probably just work. | 14:41 |
| rogpeppe | mgz: i'll try | 14:41 |
| TheMue | rogpeppe: You've seen, I reproposed our both CLs after fixes based on your reviews. | 14:41 |
| mgz | the 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 feature | 14:42 |
| mgz | if 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 out | 14:43 |
| rogpeppe | mgz: ah, i see | 14:43 |
| mgz | depending on how you normally work, that's either very unlikely, or will pretty much always be the case | 14:44 |
| rogpeppe | mgz: yeah, it seems to work. i was slightly confused by the way the CL on codereview didn't show all the diffs | 14:44 |
| rogpeppe | mgz: that would've been the case if there'd been any significant time, 'cos i do tend to merge trunk as i go along | 14:44 |
| mgz | and 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 revert | 14:45 |
| rogpeppe | mgz: luckily, i don't *think* that was an issue | 14:45 |
| mgz | the diff when relanding on trunk will be clear regardless | 14:45 |
| TheMue | fwereade__: 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 conscious | 14:51 |
| fwereade_ | TheMue, are we standardising on that behaviour? | 14:51 |
| TheMue | fwereade_: Btw, so far it looks like a great simplification. | 14:51 |
| TheMue | fwereade_: 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 noticing | 14:54 |
| rogpeppe | fwereade_: where i've seen the two groups, i've thought it looked nice | 15:04 |
| rogpeppe | fwereade_: i don't think we need to apply a standard too rigorously | 15:05 |
| fwereade_ | rogpeppe, yeah, I'm not actually in opposition, just occasionally thoughtless | 15:05 |
| TheMue | fwereade_, rogpeppe: I don't need that kind of rule too. | 15:06 |
| TheMue | fwereade_, rogpeppe: But indeed, it looks nice. ;) | 15:06 |
| rogpeppe | niemeyer: here's the full CL for the config fixes. https://codereview.appspot.com/6843100 | 15:19 |
| rogpeppe | niemeyer: https://codereview.appspot.com/6846066/ makes it much easier to grasp | 15:19 |
| niemeyer | rogpeppe: Brilliant, thank you! | 15:22 |
| niemeyer | rogpeppe: So, is the 66 still valid now? | 15:23 |
| rogpeppe | niemeyer: yes, but i'm not sure about submitting a CL twice... | 15:23 |
| niemeyer | rogpeppe: I has a single LGTM | 15:23 |
| rogpeppe | niemeyer: i'd use it on 3100 | 15:24 |
| rogpeppe | niemeyer: but they're the same thing | 15:24 |
| rogpeppe | niemeyer: the branch is identical | 15:24 |
| niemeyer | rogpeppe: Okay, I se | 15:24 |
| niemeyer | e | 15:24 |
| niemeyer | rogpeppe: So it's just the 3100 | 15:25 |
| rogpeppe | niemeyer: yeah | 15:25 |
| niemeyer | rogpeppe: Superb, thanks | 15:25 |
| rogpeppe | niemeyer: except the 66 shows just the changes that you need to see | 15:25 |
| niemeyer | rogpeppe: I'll have a look once I'm back from my daily driver duties, which are finishing today luckily | 15:25 |
| rogpeppe | niemeyer: np | 15:25 |
| niemeyer | rogpeppe: I don't get that part | 15:26 |
| rogpeppe | niemeyer: it's unexpected but useful in this case | 15:26 |
| rogpeppe | niemeyer: presumably it's something to do with lbox's logic. i don't pretend to understand. | 15:26 |
| niemeyer | rogpeppe: I've already reviewed the 66, and that's not it | 15:26 |
| niemeyer | rogpeppe: Well, we should, because that's a monster branch | 15:26 |
| niemeyer | rogpeppe: Saying it's the same as a tiny branch isn't clear | 15:27 |
| rogpeppe | niemeyer: the launchpad branches *are* the same | 15:27 |
| niemeyer | rogpeppe: If it's the same let's make it the same | 15:27 |
| rogpeppe | niemeyer: the CL is just shown differently in both cases | 15:27 |
| rogpeppe | niemeyer: it is really a monster branch | 15:27 |
| niemeyer | rogpeppe: Yes, because you reverted tons of changes, and this branch is adding them back! | 15:27 |
| rogpeppe | niemeyer: yeah | 15:27 |
| rogpeppe | niemeyer: i don't know why the 066 isn't showing the monster | 15:28 |
| niemeyer | rogpeppe: Because it was already submitted, and was done previously | 15:28 |
| niemeyer | rogpeppe: Why is it adding the changes back? | 15:28 |
| rogpeppe | niemeyer: but it's useful because it shows just the changes i've made on top of the original | 15:28 |
| niemeyer | rogpeppe: I don't get it, sorry | 15:29 |
| niemeyer | rogpeppe: You've just reverted tons of changes, and now is proposing a branch that adds it back exactly as it was | 15:29 |
| rogpeppe | niemeyer: no exactly as it was. the 066 branch shows you the changes that i made on top of the original (unreverted) changes | 15:29 |
| rogpeppe | s/no/not/ | 15:29 |
| niemeyer | rogpeppe: And asking me to review it, and saying you don't know what's going on.. I'm not very comfortable | 15:29 |
| mgz | niemeyer: do you just want the original branch as a prerequiste so you can see what's changed? | 15:30 |
| mgz | clearly the branch itself will contain much the same changes (plus whatever fixup was needed), so the default diff will | 15:30 |
| rogpeppe | mgz: hmm, maybe that'll work. except that... i'm not sure it can. | 15:30 |
| niemeyer | rogpeppe: The 66 is the 66.. it shows the changes you made that *went in* | 15:31 |
| niemeyer | rogpeppe: It was submitted | 15:31 |
| rogpeppe | niemeyer: no, i've reproposed it | 15:31 |
| rogpeppe | niemeyer: which might've been the wrong thing to do, admittedly :-) | 15:31 |
| niemeyer | rogpeppe: Yes, the same branch, wiht the same changes | 15:31 |
| niemeyer | rogpeppe: Including what was reverted | 15:31 |
| rogpeppe | niemeyer: i'm not sure i understand. | 15:32 |
| rogpeppe | niemeyer: we've got one branch which, now, includes all the unrevert changes, and a few changes i made on top of those. | 15:32 |
| mgz | niemeyer: the reverted stuff does still want to land, it was reverted so it could be fixed up, not because the change was rejected | 15:33 |
| mgz | so, the new proposal naturally includes many of the same things as the original proposal that got reverted | 15:33 |
| rogpeppe | yeah, that's it | 15:33 |
| niemeyer | Yeah, so it's not the same thing | 15:34 |
| rogpeppe | the new proposal is almost the same as the old proposal. the 066 branch conveniently says exactly how it's different | 15:34 |
| niemeyer | mgz: <rogpeppe> niemeyer: the launchpad branches *are* the same | 15:34 |
| niemeyer | Sorry guys, I really have to leave now | 15:34 |
| niemeyer | I'll be back later | 15:34 |
| mgz | I think he means the branch (name/location) is the same, it now includes a few more changes over last time though | 15:34 |
| rogpeppe | mgz: no, they really are the same | 15:36 |
| rogpeppe | mgz: http://paste.ubuntu.com/1375040/ | 15:36 |
| rogpeppe | mgz: well, with that one change (i'd accidentally reverted dave cheney's trunk change) | 15:36 |
| mgz | that's a big paste... | 15:37 |
| rogpeppe | mgz: oops! | 15:37 |
| mgz | okay, so you did basically what I would have, but in a slightly different order | 15:37 |
| rogpeppe | mgz: i accidentally pastebinned my entire shell window | 15:38 |
| mgz | you'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 |
| rogpeppe | mgz: this is what i meant to paste: http://paste.ubuntu.com/1375042/ | 15:39 |
| mgz | you can fix the accidental trunk revert by just cherrypicking that rev again I think | 15:39 |
| rogpeppe | mgz: i think i'll do that. it might make it easier to explain. | 15:39 |
| rogpeppe | mgz: currently i have no prereq in the fix branch | 15:40 |
| rogpeppe | mgz: i did a patch to fix the accidental trunk revert, which was probably the wrong approach | 15:40 |
| mgz | that's also fine. | 15:40 |
| rogpeppe | mgz: but i thought it wasn't too much problem for two lines... | 15:40 |
| mgz | provided it's fixed in the feature branch, it doesn't matter that it briefly went walkies | 15:41 |
| rogpeppe | mgz: i thought so too | 15:41 |
| rogpeppe | right, i'll try again with a prereq | 15:42 |
| mgz | yeah, and just refer the inital proposal to the identical previous one, and mention fixups are following | 15:42 |
| * rogpeppe wishes launchpad prereqs were more flexible | 15:43 | |
| TheMue | fwereade_: You've got a review. | 15:54 |
| rogpeppe | mgz: 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 |
| dimitern | hey, 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 |
| rogpeppe | dimitern: can you propose it in codereview? | 15:55 |
| dimitern | rogpeppe: lbox is broken for me | 15:55 |
| rogpeppe | dimitern: what happens? | 15:56 |
| TheMue | fwereade_: So you could review https://codereview.appspot.com/6852065/ and https://codereview.appspot.com/6853075/ for me. | 15:56 |
| fwereade_ | TheMue, a pleasure | 15:56 |
| dimitern | it worked all the way until rietveld push, then: panic: runtime error: invalid memory address or nil pointer dereference | 15:56 |
| TheMue | rogpeppe: I've changed ^^ after your reviews. | 15:56 |
| dimitern | rogpeppe: wanna see the full paste? | 15:56 |
| rogpeppe | dimitern: go on then | 15:56 |
| TheMue | fwereade_: Cheers. | 15:56 |
| dimitern | dimitern: http://paste.ubuntu.com/1375072/ | 15:57 |
| rogpeppe | dimitern: hmm, this looks suspicious to me: http://paste.ubuntu.com/1375082/ | 16:01 |
| mgz | dimitern: 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 fixed | 16:02 |
| dimitern | mgz: exactly, I'd rather have lbox working | 16:05 |
| rogpeppe | dimitern: could you try this: | 16:05 |
| rogpeppe | dimitern: at launchpad.net/goetveld/rietveld/auth.go:181 | 16:05 |
| rogpeppe | dimitern: change the logf line to this: | 16:05 |
| rogpeppe | logf("Login on %s successful; error %v (%T)", rietveldURL, err, err) | 16:05 |
| rogpeppe | dimitern: then run lbox propose -v | 16:06 |
| dimitern | rogpeppe: ok | 16:06 |
| rogpeppe | dimitern: after go installing lbox | 16:06 |
| rogpeppe | dimitern: (make sure you're using the lbox from your GOPATH, not /usr/bin/lbox) | 16:06 |
| dimitern | rogpeppe: ok, so it still fails, but at least with some other error: http://paste.ubuntu.com/1375096/ | 16:11 |
| mramm2 | Anybody need anything before I sign out for the evening? If not I'll see you all after thanksgiving. | 16:11 |
| dimitern | rogpeppe: 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 paste | 16:12 |
| mramm2 | I was able to get a lot of organizational work done on today, and will be back working Monday | 16:12 |
| mramm2 | monday/tuesday next week I'll be working on setting up blueprints for this cycle | 16:13 |
| mramm2 | so I may ask for some help from folks on that that here and there -- just an FYI ;) | 16:13 |
| dimitern | mramm2: have a good thanksgiving ;) | 16:14 |
| rogpeppe | dimitern: are you sure that's using the lbox you just changed? | 16:14 |
| dimitern | rogpeppe: well, I changed the code in goetveld, did go install launchpad.net/lbox and then run lbox propose -v | 16:14 |
| rogpeppe | dimitern: are you sure you're not running /usr/bin/lbox? | 16:15 |
| dimitern | rogpeppe: dimitern@kubrik:~/work/juju-core$ which lbox | 16:15 |
| dimitern | /home/dimitern/work/go/bin/lbox | 16:15 |
| rogpeppe | hrmph | 16:15 |
| rogpeppe | dimitern: ok, how about inserting a line after the client.Get in the same function: | 16:16 |
| rogpeppe | logf("client.Get returned %#v, %#v", r, err) | 16:16 |
| rogpeppe | dimitern: 'cos i can't quite see how it can get to call Cookies without printing the "Login successful" message. | 16:17 |
| dimitern | rogpeppe: I'm doing it now | 16:17 |
| dimitern | 2012/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 |
| rogpeppe | dimitern: you might check the mtime on the lbox binary to verify that it has actually been rebuilt too | 16:18 |
| dimitern | 2012/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 |
| dimitern | 2012/11/21 17:18:16 RIETVELD Login failed: Get http://example.com/marker: redirect blocked | 16:18 |
| dimitern | rogpeppe: ^^ it is rebuilt | 16:19 |
| rogpeppe | dimitern: could i see the whole paste? | 16:20 |
| dimitern | rogpeppe: sure - http://paste.ubuntu.com/1375114/ | 16:20 |
| dimitern | rogpeppe: what's that redirect blocked crap? it seems the same error is carried over from a previous call | 16:25 |
| rogpeppe | dimitern: i dunno. | 16:26 |
| dimitern | rogpeppe: william is mentioning something similar happened to him before, but then he used lbox from some ppa and was fixed | 16:27 |
| rogpeppe | dimitern: ah, i wonder if there are two url fetches happening concurrently... | 16:27 |
| rogpeppe | dimitern: that would explain a lot | 16:27 |
| dimitern | rogpeppe: hmm.. ok, how to fix that? | 16:28 |
| rogpeppe | dimitern: well, it *should* be just fine | 16:28 |
| rogpeppe | dimitern: but it explains why i'm seeing the Login failed message after the Login successful message | 16:28 |
| dimitern | rogpeppe: i c | 16:29 |
| dimitern | rogpeppe: why lbox keeps asking me to login each time? any way to cache this? | 16:30 |
| rogpeppe | dimitern: out of interest, i wonder if it makes a difference if you're using a different go version | 16:30 |
| rogpeppe | dimitern: what does "go version" print? | 16:31 |
| dimitern | go version go1.0.3 | 16:31 |
| dimitern | rogpeppe: I got it from golang.org and have it locally installed, rather than from apt | 16:31 |
| rogpeppe | dimitern: hmm, 1.0.3 *should* be fine | 16:32 |
| rogpeppe | dimitern: how about trying to replace the end of the function with this: http://paste.ubuntu.com/1375133/ | 16:32 |
| dimitern | rogpeppe: will do | 16:32 |
| rogpeppe | dimitern: that should stop the crash, but i'm thinking it won't fix your problem :-) | 16:32 |
| rogpeppe | dimitern: in fact, here's a better thing, that should confirm the concurrent hypothesis too: http://paste.ubuntu.com/1375137/ | 16:34 |
| dimitern | rogpeppe: it seems there is an auth issue | 16:34 |
| rogpeppe | dimitern: yeah, your authorisation is failing somehow | 16:35 |
| rogpeppe | dimitern: in a way not anticipated by lbox... | 16:35 |
| dimitern | rogpeppe: I applied the last patch and retrying now | 16:36 |
| dimitern | rogpeppe: http://paste.ubuntu.com/1375145/ | 16:37 |
| dimitern | rogpeppe: this time, it didn't crash (and the last patch didn't - as you said), but it asked me twice for creds | 16:38 |
| rogpeppe | dimitern: ah, i think i understand now | 16:38 |
| rogpeppe | dimitern: there were two concurrent auth requests. the auth caching will block out one of them | 16:39 |
| rogpeppe | dimitern: then one crashes and the other manages to step in and do something before the panic hits | 16:39 |
| dimitern | rogpeppe: hmm.. | 16:39 |
| rogpeppe | dimitern: something like that, anyway | 16:39 |
| dimitern | rogpeppe: do you see a solution? | 16:39 |
| rogpeppe | dimitern: i'd be interested to see if it fails against go tip | 16:40 |
| rogpeppe | dimitern: try going to your $GOROOT/src, running hg update tip, then all.bash | 16:41 |
| dimitern | rogpeppe: ok | 16:41 |
| rogpeppe | dimitern: then go install launchpad.net/lbox again | 16:42 |
| dimitern | rogpeppe: there's no .hg repo there | 16:42 |
| rogpeppe | dimitern: oh i thought you'd done an hg get from golang.org | 16:42 |
| dimitern | rogpeppe: no, it seems I got a tarball | 16:43 |
| rogpeppe | dimitern: you might wanna do that. it's useful to have. | 16:43 |
| dimitern | I can check it out in another dir and change paths to use it - feels safer anyway | 16:43 |
| rogpeppe | dimitern: see https://code.google.com/p/go/source/checkout | 16:44 |
| rogpeppe | dimitern: yeah, i keep two go roots around | 16:44 |
| rogpeppe | dimitern: but it's actually safer to always build a given tree against the same GOROOT | 16:44 |
| rogpeppe | dimitern: so that mtime comparison works properly | 16:45 |
| dimitern | rogpeppe: ok, I'm checking out go tip, then I'll run ./all.bash in src and then get to lbox, right? | 16:45 |
| rogpeppe | dimitern: yeah. you can usually interrupt after the tests have begun in all.bash BTW | 16:46 |
| rogpeppe | dimitern: it makes rebuilding quite a bit faster | 16:46 |
| dimitern | rogpeppe: ok | 16:46 |
| rogpeppe | dimitern: although it's worth running the tests at least once, probably | 16:46 |
| rogpeppe | dimitern: they only take a couple of minutes | 16:46 |
| dimitern | rogpeppe: couple of minutes i can handle :) when it's not 2h | 16:47 |
| rogpeppe | dimitern: yeah, we're spoilt with go | 16:49 |
| dimitern | rogpeppe: 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 everything | 16:59 |
| dimitern | rogpeppe: then did go install launchpad.net/lbox, and now I'm doing lbox propose -v again | 17:00 |
| rogpeppe | dimitern: that should work | 17:00 |
| dimitern | rogpeppe: yes! it worked this time! | 17:02 |
| rogpeppe | dimitern: ah, i thought that might be the case | 17:02 |
| rogpeppe | dimitern: there's some weird shit going on with http requests, and i know gustavo's unhappy with something about it in 1.0.3 | 17:03 |
| dimitern | rogpeppe: now would you care to review? :) https://codereview.appspot.com/6858052/ | 17:03 |
| rogpeppe | dimitern: looking | 17:03 |
| dimitern | rogpeppe: hmm, but now, once I have lbox compiled and running, I should switch back to go 1.0.3, right? | 17:03 |
| rogpeppe | dimitern: personally i run against tip most of the time | 17:04 |
| rogpeppe | dimitern: but i test against 1.0.3 too | 17:04 |
| dimitern | rogpeppe: I'll try to do that as well then | 17:04 |
| rogpeppe | dimitern: there's actually no reason you can't switch back to 1.0.3 | 17:05 |
| rogpeppe | dimitern: just don't recompile lbox against it | 17:05 |
| dimitern | rogpeppe: yeah, for sure I won't :) now that I have it working finally | 17:05 |
| TheMue | So, have to leave, no lunch, but now is dinnertime. CU tomorrow. | 17:12 |
| rogpeppe | dimitern: you have a review | 17:26 |
| dimitern | rogpeppe: thanks! | 17:26 |
| rogpeppe | niemeyer: here's another version of the CL, hopefully easier to understand what's going on now: https://codereview.appspot.com/6843101 | 17:27 |
| rogpeppe | niemeyer: it has the old branch as a prereq | 17:28 |
| rogpeppe | niemeyer: (you can see the contents of the old branch by clicking on the comments) | 17:28 |
| mgz | dimitern: also reviewed. | 17:29 |
| dimitern | mgz: thanks! 2 reviews now - so I'll fix what was suggested | 17:29 |
| rogpeppe | niemeyer: hmm, maybe you'll still be unhappy. | 17:30 |
| rogpeppe | mgz: 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 bootstrap | 17:31 |
| dimitern | mgz: what I didn't get is are you generally happy or sad with the review :) | 17:31 |
| mgz | dimitern: it's a stub, so most of the comments are just comments :) | 17:32 |
| dimitern | mgz: yes, but the config is basically as it will be | 17:33 |
| mgz | I'd rename the config vars now though | 17:33 |
| dimitern | mgz: fair enough, that I'll do yes | 17:33 |
| rogpeppe | yeah, we try to go for python config compatibility except where necessary | 17:33 |
| mgz | identity-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 |
| dimitern | mgz: so the CredsFromEnv in the refactored code from ian is now in goose? | 17:35 |
| mgz | it predates that, but the client code now uses that identity code, yes | 17:36 |
| mgz | I 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 well | 17:37 |
| rogpeppe | mgz: file | 17:37 |
| rogpeppe | mgz, dimitern: yeah, if there are potentially more env vars to come, it's definitely worth doing | 17:38 |
| dimitern | mgz, rogpeppe: so tenant-id -> project-name or tenant-name | 17:38 |
| rogpeppe | dimitern: i'm sorry, i'm not familiar with the openstack config names | 17:38 |
| mgz | dimitern: it's a long story :) | 17:39 |
| dimitern | rogpeppe: well the env var is OS_TENANT_NAME | 17:39 |
| mgz | "tenant" is what the thing is called in the keystone code at present, but they keep threatening to rename it | 17:39 |
| rogpeppe | mgz: if in doubt, do what python does now | 17:39 |
| dimitern | so I'll change it to tenant-name | 17:39 |
| mgz | it used to be called project, and may again at some point, because it's not actually a tenant | 17:39 |
| mgz | and whenever people talk about, for instance, multi-tenancy in openstack they mean a different kind of 'tenant' | 17:40 |
| mgz | so, 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 |
| rogpeppe | niemeyer: finally, here's a version that you might possibly be happy with: https://codereview.appspot.com/6850087/ | 17:43 |
| mgz | see for instance this line in your novarc: | 17:43 |
| mgz | export NOVA_PROJECT_ID="${OS_TENANT_NAME}" | 17:43 |
| rogpeppe | niemeyer: it has the original branch as a prereq (in a different CL, so all the changes are visible) | 17:44 |
| dimitern | mgz: not sure i want to couple the openstack/config with the goose/identity just yet though | 17:45 |
| mgz | dimitern: that's probably fine to leave for later, but I'd add some comments about what goes where | 17:46 |
| dimitern | mgz: won't it be better to leave the dummyAuth for now, until .. | 17:46 |
| dimitern | mgz: yeah, just started writing the same thing | 17:46 |
| dimitern | ok, I'll go back to my place and work on the CL some more, see you guys tomorrow! | 17:48 |
| mgz | later! | 17:48 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!