[00:38] is -r not implemented for relation-set yet with juju-core? [00:56] would people please stop adding ~juju to more freaking teams [00:56] * bigjools adds another procmail entry line [04:37] wallyworld_: I'm trying to add some debug logging to imagemetadata, to help track down failures to find matches. [04:38] you poor bastard [04:38] In the outer getImageIdsPath() loop, there's a condition: can't load package: package launchpad.net/juju-core: no Go source files in /home/jtv/go/src/launchpad.net/juju-core [04:38] Ahem, not that one. [04:38] Ignore that one; old buffer contents. It's a Go error that means "you put an option on the command line in the wrong place." [04:39] The code I mean is: [04:39] if metadata.DataType != imageIds { [04:39] imageIds is "image-ids" i think [04:39] Is that a search (i.e. true for at most, and ideally exactly, one iteration)? Or a filter (i.e. true for any number of iterations)? [04:39] Yes, it is. [04:39] ISTM it's meant as a search. [04:40] the simplestreams contains data belonging to different data types [04:40] it is meant to iterate over them and only look at image-ids data types [04:40] FWIW it's usually a good idea to extract these search loops, so that you get explicit "not found" conditions instead of just arriving at the end of the big loop without success. [04:40] So it's a filter, not a search? [04:40] yes, i guess you could say that [04:41] Thanks. I'll apply loop fission. [04:41] it's a filter built into the loop [04:41] The code will be easier to follow, but also have explicitly recognizable, reportable "nothing found" conditions. [04:42] the not found condition hasn't been important [04:42] I find these things usually improve a lot with loop fission. [04:42] maybe it is now [04:42] It's bloody well important to me! [04:42] For debugging. [04:42] Just about the only error condition is "sorry, no luck." A nightmare to debug. [04:43] the idea is that there can be many metadata files and if one doesn't suit, it just looks at the next one [04:43] no need to bother the user with such trivialities [04:43] but for debugging, i see your point [04:44] And I yours, w.r.t. the user. [04:44] Well, except for cases maybe where "not found" errors are quietly skipped. :) [04:45] that's desired behaviour sometimes :-) [04:55] I can see that -- but I think even non-dev users need to be able to get at debug output sometimes. [04:56] agreed [04:56] i just never added any cause it sorta just worked [04:56] so i didn't have an itch to scratc [04:56] h [04:58] * jtv scratches === tasdomas_afk is now known as tasdomas === tasdomas is now known as tasdomas_afk === tasdomas_afk is now known as tasdomas [07:36] mornin' all [07:38] rogpeppe: morning [07:38] axw: hiya [07:39] axw: simplifying the patterns is a great idea, BTW. that had also occurred to me as a possibility [07:40] rogpeppe: cool [07:40] axw: we can easily expand the syntax later if people have the need [07:40] okey dokey [07:41] rogpeppe: LGTM stands then? I wanted to give you a chance to respond before I went ahead and merged [07:41] axw: i'm just having a last look through it [07:41] ok [07:42] fwereade: thanks for the review; no additional feedback needed, if you're happy with the latest changes [07:44] axw, it looked great yesterday and you're more than competent to make the changes I suggested ;) [07:44] heh thanks :) [07:45] axw: shouldn't validatePattern allow a slash ? [07:45] validatePattern is called on either side of the slash [07:46] rogpeppe: see the strings.Split in newUnitMatcher [07:47] axw: ah i'd missed that, yes. [07:52] axw: reviewed [07:53] rogpeppe: ta [08:18] axw: are you around? [08:19] thumper: I am [08:19] I'm thinking about what to get you to look at [08:19] I'll have a chat with jam and fwereade and get back to you :) [08:20] thumper: okey dokey, thanks [08:20] I am tempted to leave you going through all the high bugs thinking that at this rate, we'll be all done real soon now :) [08:20] but that is a bit mean [08:20] thumper: et al, is there a bug for the juju-plugins fuckup ? [08:21] davecheney: I don't think so, jcastro was pinged by people with private clouds going "huh? fubared" [08:21] davecheney: feel free to file one [08:21] kk [08:23] https://docs.google.com/a/canonical.com/document/d/1XPzmRAM3W7oYYmCGVYB5nR1DZyB16t8qPF68xxmWZho/edit# [08:23] release notes for 1.13.1 [08:23] jcastro: did anyone log an issue for the juju-metadata snafu ? [08:23] if not, no worries [08:24] * davecheney waves to anonymous frog === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: dimitern | Bugs: 4 Critical, 82 High - https://bugs.launchpad.net/juju-core/ [08:34] hi folks, we're having a problem with CPU constraints and juju-core 1.13.0-1~1594. --constraints "cpu-cores=1 mem=4G arch=amd64" gets 4 core machine [08:35] is this a known issue? couldn't see anything in the bugs list on LP [08:36] oops [08:36] fwereade: your surprise was well warranted ;) [08:36] I missed a test [08:36] (wordpress endpoints) [08:37] mthaddon: the constraints are the minimum values you want, not the maximum [08:37] dimitern: is there some way of specifying maximum values as well? [08:38] mthaddon: perhaps there are no 1 core x64 machines with 4G ram [08:38] mthaddon: not that I know of [08:38] mthaddon: even py-juju I think didn't support that [08:38] in pyjuju you could specify an instance-type which would achieve what we're trying to do here [08:39] mthaddon: in juju-core, the algorithm is to satisfy your given constraints to the best possible, even exceeding them, if not possible to meet them all [08:39] and in this case there very much is an instance type defined that matches the constraints we want exactly [08:40] mthaddon: yeah, that's being discussed and should be coming soon [08:40] dimitern: any idea of what "soon" means here? [08:41] mthaddon: not with certainty, but most likely a couple of weeks or so after the IoM sprint [08:41] which ends this week [08:43] dimitern: if we can show that there is an instance satisfying the constraints (cpu-cores=1 mem=4G arch=amd64) and nothing more that's available, would we be more likely to get that bug fixed or better to wait for the ability to specify an instance-type? this is a critical issue for us as to whether we can deploy on juju-core or need to stick with pyjuju [08:44] mthaddon: try specifying them incrementally, don't overspecify - first try ram, then cores or cpu-power, at last arch [08:44] mthaddon: what provider are you using? [08:44] dimitern: if I can boot an instance with the exact constraints outside of juju, but then juju-core can't surely that'd be a good enough test of a bug? [08:44] rogpeppe: openstack [08:45] mthaddon: hmm, it's possible that the simplestreams data is erroneous [08:46] mthaddon: since juju starts filtering the instance by ram, and 4G defined as 1024*1024*1024, you might want to try specifying something a bit lower [08:47] mthaddon: but by all means, if you want, file a bug as well, describing what you expect as a constraints selection behavior [08:49] dimitern: we have plenty of flavors that should match - http://paste.ubuntu.com/5961886/ [08:50] rogpeppe: is that something we'd need to fix? how would we verify that? I'm not familiar enough with simplestreams yet to know how this could affect this issue [08:50] mthaddon: let me check the exact matching [08:51] mthaddon: i'm looking into the code at the moment [08:51] cool, thx guys [08:52] mthaddon: hmm, it would be interesting to have some debugging prints in instances.getMatchingInstanceTypes [08:52] rogpeppe: or Tracef [08:52] dimitern: that's what i mean [08:53] rogpeppe: hey, re your suggestions to use params.ServiceUpdate as the only argument for Client.ServiceUpdate. I have no problem in doing that, but that would be the only client call like that. E.g. ServiceDeploy, which takes almost the same number of arguments, does not use params in its signature. [08:54] frankban: yeah, ServiceDeploy is pushing the boundaries [08:54] frankban: but i think ServiceUpdate is slightly different, as all the parameters are optional [08:54] rogpeppe: good point, I'll make the change [08:55] frankban: thanks [08:57] rogpeppe: should I use *params.ServiceUpdate? [08:58] frankban: that's a good question. on balance, i think no. others' opinions may vary. [08:58] rogpeppe: I'll use the value then, thank you [08:58] frankban: i might use a pointer if it was useful to be able to omit all parameters [08:59] rogpeppe: it is not, the service name is not optional [08:59] frankban: yeah [08:59] frankban: and it wouldn't be that useful anyway [09:00] yeah [09:00] is this juju-core's way of telling me there's no environment booted? http://paste.ubuntu.com/5961915/ [09:00] mthaddon: lol [09:00] mthaddon: it's entirely possible [09:00] * mthaddon files a bug about that [09:00] mthaddon: there may already be one [09:00] k, will check [09:01] https://bugs.launchpad.net/juju-core/+bug/1028053 [09:01] <_mup_> Bug #1028053: running juju status on a non bootstrapped environment gives confusing results [09:01] mthaddon: this one is being worked on I think [09:02] * mthaddon nods - obviously not very high priority, but good to have logged as a bug === 17WAB9WQY is now known as jam [09:38] INFO juju.environs.sync sync.go:166 download 2882kB, uploading download 2882kB, uploading [09:38] love it [09:39] dimitern, rogpeppe: re-proposed the branch with the requested changes. I am going to approve it unless you want to take another look [09:39] frankban: i'll just take a very quick look [09:40] rogpeppe: cool thanks [09:42] frankban: done [09:43] rogpeppe: great, proceeding [10:02] rogpeppe: merged, and filed bug 1210076 [10:02] <_mup_> Bug #1210076: Expose minUnits in the CLI [10:03] wallyworld_: you wouldn't be available to review my logging branch by any chance? https://codereview.appspot.com/12655043 [10:14] https://bugs.launchpad.net/juju-core/+bug/1210086 [10:14] <_mup_> Bug #1210086: CPU instance constraints not working as expected

[10:20] rogpeppe, natefinch: https://codereview.appspot.com/12658043 UniterAPI [10:22] sorry, left out a piece of debugging code, reproposing without it now [10:24] dimiternlooking [10:33] dimitern: reviewed [10:33] rogpeppe: cheers [10:35] rogpeppe: strictly speaking, these tests are not the same and even if some logic is the same (setup/teardown/asserts), the thing that's being tested is different [10:37] natefinch: ping [10:41] thumper: hey, I need a review on a small branch if you have 5m? https://codereview.appspot.com/12658043/ [10:41] * thumper looks [10:51] And anyone free to review this one? https://codereview.appspot.com/12655043 [10:51] jtv: looking [10:51] jtv: me too [10:52] jtv: i was thinking earlier that something like this might be a good idea [10:53] Thanks guys [10:53] Well, Roger, here you go. :) I'm sure you'll be able to make some good suggestions then. [11:07] thumper: thanks! [11:09] noodles775: feel like taking a look at mthaddon's bug above ^? [11:09] * noodles775 looks [11:11] Sure - sounds interesting :) [11:11] jtv: reviewed [11:14] Thanks dimitern [11:16] jtv: reviewed [11:20] mthaddon: can you paste the full list of instance types? [11:21] mthaddon: also the one that got selected [11:23] sidnei: I don't have the actual one that got selected, would need to confirm with thedac if he has that in history - the full list of instance types coming up (it's massive) [11:24] mthaddon: ok. i think you said it selected the 4cores one instead of a 1core one [11:24] yep [11:32] rogpeppe: standup? [11:56] rogpeppe: https://bugs.launchpad.net/juju-core/+bug/1210086/comments/1 makes sense? [11:56] <_mup_> Bug #1210086: CPU instance constraints not working as expected

[11:58] sidnei: yeah, that looks plausible [11:59] sidnei: it should probably try to minimise all constraint attributes in the absence of a monetary cost [12:24] rvba: you have a review: https://codereview.appspot.com/12251044/ === tasdomas is now known as tasdomas_afk [12:42] rogpeppe: what do you suggest, changing byCost to take the other attributes into account or changing the openstack environ to use a combination of ram and cpu cores? [12:43] sidnei: just having a look [12:45] sidnei: i think it would probably be better to change byCost [12:45] sidnei: that way providers don't have to worry about fitting a multidimensional value into a scalar [12:46] rogpeppe: and change openstack to have nil as default cost instead of ram, makes sense? [12:46] sidnei: yes, i think so [12:46] ack, on it. [12:47] sidnei: we will need to decide on precedence [12:47] sidnei: i *think* we should probably weigh ram higher than cores [12:47] sidnei: so, in order of precedence (each only considered if the previous values are equal): cost, ram, cores [12:48] sounds good to me [12:48] sidnei: cool [13:01] rogpeppe: to double check, lowest cost but highest ram and highest cores? [13:02] sidnei: no, i think lowest of all of them [13:02] sidnei: we want to pick the slimmest machine that fits the constraints, i think [13:03] rogpeppe: i'd think if two machines have the same cost and one has more ram i'd pick the one with more ram, but i could do that by tweaking the constraints anyway, so fine. [13:03] sidnei: yeah [13:04] sidnei: when monetary cost is implemented, it will tend to pick least ram/cores, so this is just extending that heuristic [13:10] rogpeppe, mgz: next CL https://codereview.appspot.com/12661043/ - uniter client-side API [13:13] dimitern: LGTM [13:14] rogpeppe: thanks [13:14] rogpeppe: would cpu-power come before or after cpu-cost? [13:14] sidnei: cpu-cost? [13:16] rogpeppe: sorry, cpu-cores [13:17] sidnei: your guess is as good as mine. perhaps before, as fast cpus are more expensive in general? [13:17] k [13:21] rogpeppe: hi there. have you had a chance to look at my latest changes for the control bucket stuff? === tasdomas_afk is now known as tasdomas [13:22] wallyworld_: i have; i really think this should probably go on hold until we can have a proper discussion about it along with fwereade [13:22] :-( [13:22] wallyworld_: i know, but i think this taps into something a bit deeper that it's worth sorting out earlier rather than later [13:23] ok then [13:23] wallyworld_: i'm really sorry - i've been thinking a lot about the issue over the last couple of days [13:23] i'm following the design document [13:24] wallyworld_: can you point me to the design doc again please - i always lose Drive links [13:24] * wallyworld_ looks it up [13:25] https://docs.google.com/a/canonical.com/document/d/1ncsNzDHauV_9Fwsm59GjX0-O-_g48UYnObefwtBStG0/edit [13:26] william was also ok with the approach i started with - get rid of control bucket, admin secret,and also the keys (in the followup branches) [13:26] * rogpeppe wishes google drive remembered what documents i'd viewed [13:26] it can do [13:27] goto docs.google.com [13:27] and your recently viewed ones are there [13:27] plus you can put them in folders etc [13:28] wallyworld_: i've viewed that doc recently and it isn't there [13:28] * wallyworld_ shrugs [13:29] mgz: ping [13:29] hmm, it is now [13:29] wallyworld_: anyway [13:29] wallyworld_: the design doc says this: [13:29] $ juju bootstrap myenv1 --template experimental [13:29] # generate admin-secret, CA cert/key, and control-bucket [13:30] wallyworld_: the problem i have is that your proposal generates these values even when you're not bootstrapping [13:30] wallyworld_: for instance, if you haven't bootstrapped an environment, and you call juju status, it'll generate these values, which i think is wrong [13:32] wallyworld_: i think your proposal is nice and non-disruptive to most juju code, but i think something more is needed [13:32] dimitern: hey [13:33] rogpeppe: well, it will generate the attrs whenever NewFromName is called ie whenever a new env is required to be constructed [13:33] wallyworld_: exactly [13:34] so whats the issue there? [13:34] a new env requires certains attrs [13:34] some come from yaml, others are generated [13:34] wallyworld_: i think the provider should have the chance to verify that the new bucket can actually be created [13:34] then next time an env is used, the attrs are there [13:34] mgz: https://codereview.appspot.com/12661043/ . [13:35] ? [13:35] wallyworld_: and there might be other attributes that we want to put in there that are not random, but obtained by querying the provider [13:36] rogpeppe: status won't work without being bootstrapped. the next bootstrap can verify the control bucket etc. but if we want the new attrs done on bootstrap, that will likely mean major code changes [13:37] wallyworld_: i don't think the changes are too bad - i've already got an experimental branch that makes ec2 work in that kind of way [13:38] wallyworld_: BTW, i've just realised your changes will break sync-tools [13:38] i guess it depends on what is considered the lifecycle of an env and what is allowed to create one as opposed to just using one [13:38] wallyworld_: yeah, absolutely [13:38] wallyworld_: that's the crux of the issue [13:38] sync-tools breakage is bad [13:39] wallyworld_: here's my current thoughts on how we might change the EnvironProvider interface: http://paste.ubuntu.com/5962612/ [13:40] dimitern: lgtm [13:40] wallyworld_: both the bootstrap and sync-tools commands would call Prepare rather than Open, and save any extra attributes in .environments [13:40] mgz: thanks! [13:41] prepare vs open is good - separates out the semantics [13:41] open would fail is prepare weren't called first [13:42] wallyworld_: yeah [13:42] wallyworld_: when calling Open we'd pass it all the config attributes including local ones [13:43] wallyworld_: actually that needs to be true for Prepare too [13:43] wallyworld_: and we're delete the local attributes when destroy-environment is called [13:43] or have open merge them and complain if they aren't there [13:43] wallyworld_: i think it can be done outside the provider [13:44] NewFromName and the associated Read functions would need looking at [13:45] wallyworld_: yes [13:45] a bit of work there [13:46] wallyworld_: not too much i think actually [13:46] wallyworld_: we don't have to change the semantics of NewFromName [13:46] wallyworld_: we'd add something like PrepareFromName (bad name!) [13:46] wallyworld_: very little code is actually directly involved in bootstrapping [13:48] i'll think about it tomorrow. quite late here now. i actually took sick day today. i got a broken cheek bone from being king hit from behind. had to get xrays and go to the police etc etc [13:48] it happened during a soccer game of all things [13:49] wallyworld_: woah [13:49] wallyworld_: you poor bugger [13:49] it's not too bad. i can't chew without a fair bit of pain. makes eating kind hard [13:49] wallyworld_: BTW here are the changes to environs/ec2 required for the above interface. http://paste.ubuntu.com/5962643/ [13:50] wallyworld_: soup! [13:50] thanks, i'll look in detail and see what i can add to my branch [13:50] * wallyworld_ hopes the ocr can look at his other simplestreams branch [13:51] * wallyworld_ goes to take some more painkillers and heads to bed [13:52] wallyworld_: mend soon! [13:52] wallyworld_: g'night [13:52] * rogpeppe goes for some lunch [13:52] see ya, thanks for discussion [13:53] wallyworld_: np, sorry for pushing back in the review [13:54] i'm planning to take the day off tomorrow, BTW [13:54] dimitern, nate-finch, wallyworld_: ^ [13:54] rogpeppe: ok [13:59] rogpeppe: cool, have fun with the long weekend [14:05] rogpeppe: https://codereview.appspot.com/12569044 [14:07] dimitern: ^ [14:07] sidnei: looking [14:13] sidnei: reviewed === BradCrittenden is now known as bac [14:53] can i get a second eye on https://codereview.appspot.com/12569044/ ? im getting someone to copy over the flavors from prodstack to canonistack so i can do some manual testing soonish. [14:54] rogpeppe: ^^ ? [14:56] dimitern: just back from lunch [14:56] sidnei: lookin [14:56] g [14:58] dimitern: i need to upload-tools and bootstrap with the newest to test right? [15:03] sidnei: yeah, but before that, make sure you do "go install ." inside cmd/juju and cmd/jujud [15:03] dimitern: and what's the syntax for sync-tools? [15:04] sidnei: juju sync-tools --help ? :) [15:04] sidnei: you don't need sync-tools if you're doing --upload-tools [15:05] dimitern: ah, bootstrap --upload-tools, i was looking for an upload-tools command [15:05] sidnei: yeah - when in doubt, use juju help commands and --help on a command - it's really good [15:07] sidnei: reviewed [15:07] sidnei: do you need the flavors on canonistack to be able to test? [15:07] (if so I can try and get that pushed through) [15:08] mthaddon: i think jjo is on it, let me check again [15:08] i see a couple dozen are already added, i guess the script is running [15:10] sidnei: if, while testing you see the issue mthaddon reported earlier is resolved, can you please link your MP to the bug as well? [15:10] already did [15:12] sidnei: great, thanks! [15:18] rogpeppe: you have a few branches ready for landing [15:21] rogpeppe: if cpu power is only set in one of the instances then it fallsback to cpu cores, right? [15:22] sidnei: cpu power and cpu cores are not linked in any way, and cpu power has only ec2 [15:22] sidnei: i think that if cpu power is only set in one of the instances, the one with no cpu power set will win [15:23] sidnei: because 0 is less than a positive cpu power [15:23] sidnei: or... [15:23] sidnei: are you asking about desired behaviour? [15:23] sidnei: if so, yes [15:23] rogpeppe: per your suggested if either side is nil we skip the cpu power check [15:23] sidnei: yeah [15:24] rogpeppe: ok, just double checking [15:27] rogpeppe: done && pushed, doing manual verification. [15:41] manual verification worked as expected, so landing it. [15:43] dimitern: thanks for the trivial... I assume that means it doesn't need a second LGTM, so I did the s/'/" and submitted - you should be able to just approve the MP. [15:43] dimitern: btw, there were 4 bugs, so I chose one and marked the other 3 dups. [15:43] noodles775: great, will do now [15:44] noodles775: approved [15:51] dimitern: what's the process for creating a tarball? i thought it was in the makefile but apparently not [15:51] sidnei: mgz might answer that better [15:53] dimitern: alternatively, i guess i just need to tar up $GOPATH/bin? [15:53] sidnei: there's a script that generates the tarball + all dependencies, but I don't know where it is and the process is not documented clearly [15:54] sidnei: no, you'll need the sources as well - wget a tarball and see what's inside [16:01] guys, where do I find the procedure to upgrade to go 1.1? [16:01] sidnei: why do you need a tarball anyway? isn't --upload-tools enough? [16:01] teknico: sudo add-apt-repository ppa:james-page/golang-backports && sudo apt-get update && sudo apt-get install golang [16:01] dimitern: i need to get a trunk client into a canonistack instance to run tests in jenkins. [16:02] dimitern: thanks :-) [16:02] teknico: then remove all binaries in $GOPATH/linux_*/ and rebuild & retest everything [16:02] teknico: oops - I meant $GOPATH/pkg/linux_*/ :) [16:03] sidnei: I see [16:03] dimitern: yep, and bin/ too for good measure [16:03] teknico: yeah [16:03] teknico: won't hurt [16:05] sidnei: what are you trying to do exactly? [16:06] mgz: getting a binary built and shipped to a remote instance to run deployment tests from the cloud [16:06] just make a tarball? running the thingy under scripts/ does that [16:06] mgz: although i could just as easily build the binary over there [16:07] you can probably just use --upload-tools? [16:07] or, if it's not for testing trunk, just use sync-tools [16:08] then you can pull the files out of your control-bucket [16:08] the tarball is nice because you can be exactly sure about what have been built and reproduce it [16:08] if you don't need that though (eg, for a one off test), then the existing dev methods are probably fine [16:09] mgz: yeah, i'll go for building from tip directly. [16:13] hi guys, who controls when PPAs get built for https://launchpad.net/~juju/+archive/devel/+packages ? [16:14] er, when packages get built for that PPA, I mean [16:16] mthaddon: seems to be https://code.launchpad.net/~dave-cheney/+recipe/juju-core [16:17] seems they are built on https://code.launchpad.net/~juju/+archive/experimental by default [16:18] yeah, looks like it [16:18] sidnei: great, which revno do we need to wait for? [16:18] so can we just request a build using the button there? [16:18] thedac: we need 1621 [16:19] (or later) [16:19] i can trigger a build it seems [16:19] woohoo [16:19] cool [16:19] I'll port that to precise-cat as soon as it builds === Guest79504 is now known as amithkk [16:20] thedac: https://code.launchpad.net/~juju/+archive/experimental/+builds?build_text=&build_state=all it's where it's at [16:21] thanks [16:36] thedac: source package is built, since i guess that's all you need [16:36] sidnei: thanks [16:45] rogpeppe: for the record, the import cycle I was having mysteriously went away after I finished some rework from code review suggestions. Weird, but I'll take it [16:46] nate-finch: it may have been to do with state object files, i suppose [16:46] s/state/stale/! [16:47] rogpeppe: yeah, I was thinking that too [17:02] fwereade: got a minute? [17:06] dimitern, rogpeppe: https://codereview.appspot.com/12546043/ mostly minor code review changes [17:09] natefinch: looking [17:16] nate-finch: reviewed === flaviami_ is now known as flaviamissi_ [17:18] dimitern: why are we renaming gocheck to gc? I tend to try to use the original name unless there's a conflict [17:18] nate-finch: convention [17:19] nate-finch: because gocheck and juju-core/checkers are used a lot, we decided to alias them to gc and jc respectively [17:19] nate-finch: it helps to keep the code consistent [17:19] dimitern: fair enough [17:20] nate-finch: originally, gocheck was imported as . but that's bad, just like from x import * in python, so we decided to avoid that and be explicit [17:20] nate-finch: because "gc" is better than nothing at all, but still reasonably terse [17:21] nate-finch: not all of the codebase is converted to the new style: 1) separating stdlib, third-party and juju-core imports by blanks; 2) using gc and jc for gocheck and checkers; 3) prefer longer names if possible, rather than 1-2 letters vars [17:22] nate-finch: exception for 3) (most of the time) method receivers, like func (s *someSuite) are usually ok [17:22] dimitern: yeah import as . is really bad. Glad we're moving away from it. [17:23] but it's better to have machiner, err := state.Machine(id) than m, e := state.Machine(id) [17:23] dimitern: yep [17:24] nate-finch: most of these are not really "set in stone", but like highly recommended "best practices" [17:26] * rogpeppe finds "m" just fine for a local variable referring to a machine [17:26] nate-finch: reviewed [17:27] rogpeppe: but "machine" is even better :) [17:27] dimitern: personally, i don't think so [17:27] dimitern: i think extra verbosity can make code harder to read [17:27] rogpeppe: aha.. ok :) [17:28] rogpeppe: if, we're talking about takeThisVeryLongAndMeaninglessIdentifierHere, I agree [17:29] rogpeppe: otherwise, for short and meaningful idents, like machine, or wordpressUnit, or assertStopsCleanly := func() .. I disagree [17:30] dimitern: russ cox said it best: the length of an identifier should be proportional to its information content [17:31] rogpeppe: yeah, and unless it's a loop counter, it'd better be a word at least [17:31] rogpeppe: I agree with you and russ. I use single letters most of the time... sometimes up to 3 letters if it's a good abbreviation of the word (like ctx for context) [17:32] dimitern: if i see m := x.Machine(), it's a fair bet that the Machine method is returning a *Machine :-) [17:32] nate-finch: you should try to convince thumper then :) [17:32] nate-finch: this is not idiomatic Go territory :-) [17:33] rogpeppe: I'm glad it isn't :) [17:33] rogpeppe: that's too bad. I like almost everything in idiomatic Go [17:33] * rogpeppe finds the standard Go library very easy to read [17:33] nate-finch: yeah, me too; it's one of the things that makes me like the language as a whole. [17:34]