=== marcoceppi|away is now known as marcoceppi|trave
davecheneythumper: ping04:30
davecheneyoh dear, no thumper04:30
davecheneyhas anyone tried runnig multiple local providers on the same machine, under _different_ user accounts ?04:31
axwdavecheney: no, but it's *meant* to work according to various comments int he code... have you tried it and it doesn't?04:48
davecheneyit does not04:48
davecheneyit may work for multiple local environments all owned by a single user04:48
davecheneybut does not work when multiple users try to each have a local environment04:49
axwdavecheney: I would've thought hte opposite :)   it namespaces things on the username04:49
davecheneyaxw: there is only one mongodb04:49
davecheneyit binds to
davecheneyand always binds to the same port04:49
davecheneythat is not configurable04:49
axwtrue - there's probably a TODO in there someone04:49
davecheneydon't forget, we're using 1.1204:50
davecheneydid somethig change in the last 2 weeks ?04:50
axwsorry, don't know04:50
axwuhh actually, the port is configurable04:51
axwun moment04:51
axwat least on the service registraiton side anyway04:52
davecheneyahh, it's in config/config04:53
davecheneyaxw: it doens't do shit04:55
davecheneystate-port doens't do anything04:55
axwdavecheney: you bootstrapped anew? I just did, and it worked. I'm on trunk tho04:55
davecheneyit is checked, validated then ignored04:55
davecheneyaxw: as two different users ?04:55
axwdavecheney: well no, but it picked it up, created the service with that port, and connected to it04:56
axwdavecheney: I'll create a new user and try. Should be able to set state-port, storage-port and shared-storage-port to unique values and have it work tho04:57
davecheneyyeah, that is what I am trying04:57
davecheneyit worked!04:58
davecheneythank heavens for that04:58
davecheneyshitballs that was a close one05:06
davecheneywe only have one machine to demo on05:06
bigjoolsdavecheney has been learning swearing from Debra Morgan05:09
jamhi all05:09
davecheneySUCK BAG!05:09
axwjam: hiya05:09
jamdavecheney: I'm so sorry :)05:10
jamoh wait, ECONTEXT :)05:10
davecheneylagging lag is laggy05:10
jamwallyworld: poke. Just checking if you're feeling up to a 1:1 today05:11
davecheneywhat does 'no CA certificat in environment configuration' mean ?05:32
davecheneynever mind05:33
davecheneyok, next problem05:39
davecheneyhow to I change the api server address05:39
davecheneyapi-port I guess05:39
wallyworld_jam: not sure if you saw my last message, i got disconnected, i'm ok for 1:105:46
jamwallyworld_: I didn't. Thanks05:48
davechen1yis there a way to disable the automatic apt-get update behavior ?05:57
davechen1yie, when we start a machine, it always does apt-get update -y05:57
jamdavechen1y: it is one of the flags we past to cloud-init05:58
jamdavechen1y: cloudinit/option.go apt_update: true05:59
jamsorry, apt_update, yes, yes06:00
jamwallyworld_: I'm there whenever you're ready06:00
davechen1yprobably can't change it without doing a release06:01
wallyworld_jam: one sec, i need to install the plugin, since it was lost when my system died06:02
jamdavechen1y: I'm pretty sure it was requested (and intentional) behavior. Given we even had a work around when the apt_update flag was broken.06:02
jamwallyworld_: np06:02
davechen1yjam: fair enough06:03
davechen1ydoes anyone know if it is possible to remove a single subordinate unit06:06
jamdavecheney: "remove a single subordinate unit", you mean I want X on all units of Y, except for machine-Z ?07:06
jamwallyworld_: did you see bug #121114707:10
_mup_Bug #1211147: Deploying service to bootstrap node causes debug-log to spew messages <juju-core:Triaged> <https://launchpad.net/bugs/1211147>07:10
rogpeppemornin' all07:31
=== ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: - | Bugs: 4 Critical, 85 High - https://bugs.launchpad.net/juju-core/
dimiternmgz: hey08:49
dimiternhttps://codereview.appspot.com/12698043/ ?08:49
mgzis that one of the ones I have a tab open for already...08:50
dimiternwell, I'm waiting from friday :)08:51
rogpeppedimitern: looking08:57
dimiternrogpeppe: thanks08:57
noodles775I'm just doing a small refactor for a bug, but am looking at environs/state.go:BootstrapState.StateInstances - why is that a slice? (Are there multiple state instances for some providers, or for the future?)09:00
noodles775And more specifically, is it OK/intended that StateInfo will return as soon as it has the hostname of one state instance?09:02
mgzdimitern: a couple of queries09:02
dimiternmgz: well I could've called it canModifyOrRead - because it's the same check, but once it's for reading, once for writing09:02
mgzI don't follow09:03
dimiternmgz: what's not clear?09:04
mgzthe function that returns is actually a can-anything-happen, despite being called getCanRead?09:05
mgzthe other functions call the returnvalue canRead but otherwise use it identically09:06
dimiternsome functions, like SetPublicAddress are "write" requests, while others like SubordinateNames are "read" requests09:06
dimiternboth have the same check - "are you the owner of that entity?"09:07
mgzbut we call the function getter for that check "getCanRead"?09:07
dimiternI agree the name should probably be canAccess or something09:07
mgzhaving the return value sometimes named canRead and sometimes named canModify isn't really helpful09:08
dimiternI named the returned function after it's actual role in the method - read or modify09:08
dimiternwill it be better if I call it getCanAccess and canAccess respectively?09:11
mgzseems like an improvement to me09:11
rogpeppedimitern: reviewed09:13
dimiternrogpeppe: cheers09:13
rogpeppedimitern: +1 to canAccess09:14
rogpeppewallyworld_: ping09:15
jammgz: /wave09:24
mgzhey jam!09:24
rogpeppejam: welcome back!09:25
jamrogpeppe: hey09:25
=== flaviamissi_ is now known as flaviamissi
=== wedgwood is now known as Guest52531
dimiternmgz, rogpeppe: https://codereview.appspot.com/1274804310:26
rogpeppedimitern: reviewed10:34
dimiternrogpeppe, mgz: thanks!10:34
dimiternrogpeppe: I don't really get your comment here https://codereview.appspot.com/12748043/diff/1/state/api/params/internal.go#newcode3510:37
rogpeppedimitern: oh, what doesn't make sense?10:38
dimiternrogpeppe: aren't we going to introduce a backwards-incompatible change that way?10:39
rogpeppedimitern: i don't think so10:39
dimiternrogpeppe: changing all results with Error to be omitempty10:39
rogpeppedimitern: how would it be backwardly incompatible?10:39
rogpeppedimitern: you'll get exactly the same result, won't you?10:40
dimiternrogpeppe: not in javascript10:40
rogpeppedimitern: nothing is using any of these calls from javascript10:40
dimiternrogpeppe: having {Result: x, Error: null} is different from {Result: x}10:41
rogpeppedimitern: i don't see your point. none of the agent API calls is used by anything except Go.10:41
dimiternrogpeppe: or anything that's relying on it to be JSON10:41
dimiternrogpeppe: how about third-party agents?10:41
rogpeppedimitern: these calls are exclusively for our own use (currently anyway)10:42
rogpeppedimitern: we don't support third-party agents.10:42
dimiternrogpeppe: and can you guarantee nobody else is using them?10:42
dimiternrogpeppe: there were already some python helpers using the api10:42
rogpeppedimitern: yes, the client API is our only public API10:43
rogpeppedimitern: if anyone is using the agent API, they'd have to be parsing our agent config files, getting the password, and pretending to be a machine agent or whatever10:43
dimiternrogpeppe: and that's going to be like that forever?10:43
rogpeppedimitern: not necessarily10:43
rogpeppedimitern: but it is now10:44
dimiternrogpeppe: hmm.. ok, but istm this is a change that's not for this CL10:44
rogpeppedimitern: and so we don't care about non-Go backward compatibility in the agent API at the moment10:44
rogpeppedimitern: sure10:44
rogpeppedimitern: but you can at least do it for StringsResult10:44
rogpeppedimitern: well actually10:44
rogpeppedimitern: you don't even need to do it for StringsResult10:44
rogpeppedimitern: you can add the Error field10:45
dimiternrogpeppe: ah.. ok I got you10:45
rogpeppedimitern: and even without omitempty it will still be backwardly compatible10:45
dimiternrogpeppe: you're saying rename StringsErrorResults to StringsResults and add an Error field to StringsResult10:45
rogpeppedimitern: yes10:45
dimiternrogpeppe: ok then, will do10:45
rogpeppedimitern: thanks10:45
dimiternrogpeppe: https://codereview.appspot.com/12748043/ - looks good?11:04
rogpeppedimitern: looking11:04
dimiternrogpeppe: just noticed a typo in StringsResults doc comment11:05
rogpeppedimitern: +111:05
rogpeppedimitern: looks great, thanks11:06
dimiternrogpeppe: cheers11:06
rogpeppedimitern: we could actually benefit in a few places from a way to induce an error talking to the underlying mongo11:06
dimiternrogpeppe: oh, yeah11:08
rogpeppedimitern: one way to do it might be to make all the tests actually connect to mongo through an intermediate relay that the tests could close when desired11:08
rogpeppedimitern: but that would probably disrupt lots of other test machinery which expects to be able to talk to state after a test has completed11:09
dimiternrogpeppe: or introduce hooks into the mgo connections?11:10
rogpeppedimitern: that only helps if the code you're trying to test is using the same *State that the test has access to11:11
rogpeppedimitern: the fact that State is using mgo is pretty much hidden outside of the state package.11:11
rogpeppedimitern: i guess we could have a global function, state.SetQueryError(err error) that sets the error returned when anything makes a mongo query.11:13
rogpeppedimitern: and similarly for transactions11:13
rogpeppedimitern: anyway, something to think on11:14
dimiternrogpeppe: sounds plausable11:14
rogpeppedimitern: standup?11:32
dimiternjam: ^^11:33
davecheneyreported on #juju, http://pastebin.com/FUri1dx311:44
davecheneyany ideas ?11:44
rogpeppewallyworld_: any chance of a chat at some point?12:00
wallyworld_rogpeppe: sorry i wasn't around before, was at soccer. did you want to catch up? give me 5?12:00
rogpeppewallyworld_: np12:00
rogpeppewallyworld_: i'll stay in this call for a bit; probably 10 minutes or so?12:01
davecheneysounds like the OP has deployed to his maas nodes12:03
davecheneythen turned them off12:03
wallyworld_rogpeppe: start a new hangout if the other one is busy? https://plus.google.com/hangouts/_/e4864938922c332329d0d696d9cb67f50b0d04f4?authuser=112:05
=== gary_poster|away is now known as gary_poster
dimiternrogpeppe, natefinch, mgz: next in line https://codereview.appspot.com/12756043/12:16
noodles775Hi people, I've got 3 branches needing reviews if anyone has time - the first just needs feedback from rogpeppe, but the other two haven't been looked at yet:12:24
noodles775https://codereview.appspot.com/12659043/ https://codereview.appspot.com/12755043/ https://codereview.appspot.com/12603047/12:24
rogpeppedimitern, noodles775: sorry, was in a call12:27
davecheneyquestion from the field12:27
davecheneydoes anyone have an objection to me adding config keys to expose the AptProxy and AptUpdate/AptUpgrade items in cloudinit12:28
davecheneyi can explain why we need such things, but not in this channel12:28
dimiterndavecheney: i think sidnei was working on something similar12:29
davecheneydimitern: or'ly12:29
davecheneyi'll talk to im12:29
davecheneywe sort of need it for the thing that marco and I are doing this week12:29
davecheneydimitern: what hours does sidney work ?12:30
dimiterndavecheney: he's usually around at this time or couple of hours later12:31
dimiterndavecheney: utc-312:32
davecheneydimitern: right o12:32
davecheneyi might hack something in anyway, we need it tomorrow12:32
davecheneybut i'll check with S12:32
dimiterndavecheney: i believe that's what i was referring to https://codereview.appspot.com/12143043/12:33
davecheneyfuck yeah12:34
davecheneysomeone has done all the work for me12:34
* davecheney goes to try it 12:35
davecheneyohhh, interesting, he's changed the lxc provider as well ...12:35
davecheneydid this already land ?12:35
davecheneyit's there now12:35
dimiternyes that one did land last week12:36
* davecheney smacks head12:37
davecheneyof course, we don't see (closed) on our CLs when they land12:37
dimiternnoodles775: looking the last two of yours12:37
dimiterndavecheney: yep, I only know it because I reviewed it and I can see it in my list12:38
dimiternI have the habit of manually closing them when then land12:38
dimiternnoodles775: there was never more than one state instance12:39
dimiternnoodles775: there are plans for ha support, but nothing done yet12:39
* davecheney wonders if we should backport that to 1.12.112:39
davecheneyit's a lot of work12:39
davecheneyand it isn't a *bugfix*12:40
dimiternthere's a linked bug, but not about that - about the local provider/lxc12:40
dimiternnoodles775: and about hp cloud account, you can ask mgz or wallyworld_ - we have one you can use perhaps12:42
noodles775dimitern: k, so BootstrapState.StateInstances is a slice just for the future plans? Do you want me to then s/WaitForFirstDNSName/WaitForDNSNames/ then (since there is only ever one state server, it'll leave the helper more general)12:44
noodles775dimitern: Re the hp cloud account, that'd be great to test - but no problems if not (if someone can verify the branch, that'd be enough for now).12:44
dimiternnoodles775: still looking, will get back to you12:45
dimiternnoodles775: so, not sure why you need WaitForFirstDNSName at all?12:50
dimiternnoodles775: can you expand a bit?12:51
noodles775dimitern: because that's what the existing code in trunk was doing - environs/state.go:StateInfo (or am I misreading)?12:52
noodles775It polls until it gets the hostname of a (well, the really) state server.12:53
natefinchmgz: gotta go for a bit, I'll be back around 15-20 past the hour12:54
dimiternnoodles775: yeah, it's written to accept more than one hostname, but in reality it's always one12:54
mgznoodles775: which branch? ...eh, I'll just look at all three12:54
mgznatefinch: sure, poke me when you're back12:54
noodles775Thanks mgz (the last two, dimitern is looking now too)12:54
dimiternnoodles775: so WaitDNSNames is used by the providers internally12:55
dimiternnoodles775: to implement StateInfo12:56
noodles775dimitern: Yeah, the fact that BootstrapState.StateInstances is a slice makes it hard to code otherwise (I mean, I could just take the 0th index, but...)12:56
rogpeppenoodles775: i'm not convinced by https://codereview.appspot.com/12755043/12:56
mgzick, I'm not a fan of the dummy provider12:57
rogpeppenoodles775: it sounds like a bug in the provider that HP cloud uses (openstack ?)12:57
noodles775rogpeppe: Maybe, see jam's comment on the related bug too.12:58
noodles775rogpeppe: but do you mean you wouldn't want to keep polling when the provider returns ErrNoInstances?12:58
dimiternnoodles775: yeah, the more I read it, the more I think the approach is not right12:58
dimiternnoodles775: ErrNoInstances is expected - they take time to start12:59
rogpeppenoodles775: yeah. in fact, i wouldn't poll at all when calling Instances12:59
rogpeppenoodles775: i don't think jam's comment is relevant12:59
rogpeppenoodles775: as we're not calling AllInstances here12:59
rogpeppenoodles775: (unless you mean a different comment12:59
noodles775No, I meant that one - I hadn't checked the change referred to (only saw the comment this morning).13:00
rogpeppenoodles775: i think the original code looks ok13:00
noodles775So I'm a bit confused - dimitern, you're saying that ErrNoInstances is expected because they take time to start, but that means we'd want to keep polling there...13:01
noodles775rogpeppe: It could be - without an HP account I didn't first reproduce the reported bug.13:01
dimiternnoodles775: yes, that's what we do13:01
mgznoodles775: I think the key is this used to work, it's just a regression13:01
rogpeppenoodles775: dimitern isn't quite right there13:01
mgzwe don't want new polling code to resolve the issue, just to work out what broke and undo it13:02
dimiternrogpeppe: ok, care to explain better please?13:02
rogpeppedimitern: Environ.Instances is expected to poll when it can't find the instances13:02
rogpeppedimitern: and, at least in ec2, that's what it does13:02
rogpeppedimitern: so you'll only get ErrNoInstances or ErrPartialInstances once it's already polled for long enough to be certain13:03
dimiternrogpeppe: hmm.. ok13:03
dimiternrogpeppe: and the openstack provider does the same, no?13:03
rogpeppei do think that environs.LongAttempt is a smell13:03
* rogpeppe checks13:04
* noodles775 looks for recent changes adding ErrNoInstances13:04
rogpeppenoodles775: it's quite possible that shortAttempt is too short in HP openstack13:04
dimiternand that's a recent change13:05
noodles775mgz: it's not new polling code - I'm just extending the errors that are allowed while polling in StateInfo.13:05
mgznoodles775: I see new public functions in environs/polling.go13:07
noodles775mgz: yep, but they're used in environs.StateInfo with the same (intended) functionality as was already there (I was just trying to make StateInfo both explicit and simpler, but may have failed).13:08
rogpeppei think there should probably be a separate package (environs/utils?) that exposes functions for the providers themselves to use. i think that having, for example, environs.Bootstrap (intended to be called by clients of environs) and environs.StateInfo (intended to be called back by providers) is confusing13:09
rogpeppenoodles775: i don't see anything wrong with the current implementation of environs.StateInfo13:09
dimiternthat's a whole different talk I think rogpeppe13:09
rogpeppedimitern: i agree13:10
rogpeppedimitern: just an aside13:10
rogpeppenoodles775: one other thing: what was the motivation behind the changes to the dummy provider?13:11
noodles775rogpeppe: Yep, nothing wrong with it - I just thought that that was where we'd need to also allow ErrNoInstances to avoid this bug, but as you said, a better place may be in openstack/provider.go (the shortAttempt).13:12
noodles775rogpeppe: The broken-error? To be able to test the behaviour of StateInfo (ie. that it still timed out if ErrNoInstances was raised, rather than passing the error up straight away).13:12
rogpeppenoodles775: ah, i see.13:14
noodles775(which ended up being encapsulated by WaitForInstances and so tested there)13:14
rogpeppenoodles775: probably not necessary if we end up just raising openstack.shortAttempt13:15
dimiternrogpeppe: perhaps not all of it, but just for that test?13:15
noodles775rogpeppe: yep - I think the whole branch won't be necessary, but was a good learning exercise :-)13:15
rogpeppenoodles775: :-)13:16
rogpeppedimitern: ?13:16
dimiternrogpeppe: openstack.shortAttepmt13:16
rogpeppedimitern: the bug was encountered for real, not in a test13:17
rogpeppenoodles775: do you remember just how long it *did* take before the command actually succeeded?13:17
dimiternrogpeppe: hmm.. so the shortAttempt used by the code you mean, not the one in tests? aren't these two linked?13:18
rogpeppenoodles775: the bug report shows that it took at least 17 seconds13:18
rogpeppedimitern: yes13:18
rogpeppedimitern: they're not really linked13:18
rogpeppedimitern: except in the live tests, i guess13:19
noodles775rogpeppe: Yes - I've not seen the bug in action (don't have HP creds), so am goin on the 17seconds there (the LongAttempt is 3 minutes)13:19
rogpeppenoodles775: i'd go for 20 seconds at least13:20
rogpeppenoodles775: looks like HP's consistency is really very eventual :-)13:21
noodles775rogpeppe: OK, so I'll do a second (much simpler) branch which just updates the shortAttempt there :-)13:21
rogpeppenoodles775: thanks. it seems a pity (if you've got a genuine error you'll have to wait for 20 seconds before it tells you) but i suppose that's the price we pay for distributed databases13:22
rogpeppei wonder if it might be useful to allow the timeout to be adjusted for different openstack providers13:23
dimiternwe need more live testing instead of making obscure tweakable settings like that13:25
sidneidavecheney: there's a couple bugs i've uncovered about the apt-proxy stuff IRL mainly when the host is precise, i have a bug open and a plan to fix it.13:25
davecheneysidnei: right13:25
davecheneyso we are using a raring host for precise machines13:26
davecheneyis that the bug ?>13:26
davecheneyrogpeppe: on a long enough timeline, HP will be eventually consistent13:26
sidneidavecheney: nope, the one bug i found is taht if the host is precise, apt-config dump dumps the whole config without filtering, which is then shoved into /etc/apt/apt.conf.d/99proxy in the container13:26
rogpeppedavecheney: is HP more tardy in general than other providers?13:27
dimiternrogpeppe, mgz, natefinch: poke https://codereview.appspot.com/12756043/13:27
sidneiwhere as in raring/saucy (havent checked others) it is filtered down to the args you provide after 'dump'13:27
davecheneysidnei: ewww13:27
rogpeppedimitern: i will get there :-)13:27
dimiternrogpeppe: thanks :)13:27
sidneidavecheney: a separate issue that hazmat brought up is that it requires configuring the host to use apt proxy so it's not automatic. pyjuju was simply checking if port 3142 (apt-cacher-ng) is up and listening and then adding that as the apt proxy without any extra config.13:28
davecheneysidnei: yeah, i wanted to add a config property to environs/config.go13:29
davecheneywhich exposed the cloud-init apt-proxy key13:29
davecheneybut reliable13:29
sidneidavecheney: that was one of the options i brought up on the list, but there were conflicting replies.13:30
davecheneysidnei: well, this may not get merged, but I need something simple13:30
rogpeppenoodles775: i think your fix in https://codereview.appspot.com/12603047 is a reasonable one, but i don't think we can do it13:31
noodles775rogpeppe: Yeah, as per the description there, I wasn't happy with it. Do you have a better way forward, or what other issues do you see?13:33
rogpeppenoodles775: currently it's valid to call "juju get myservice --debug"13:33
rogpeppenoodles775: your change will break that usage AFAICS13:34
rogpeppenoodles775: one possibility is to change the Command interface so that individual commands can request that flags are not allowed to be interspersed.13:34
sidneidavecheney: im definitely +1 on an explicit option, be it in the environ config or as i've implemented and landed, by changing the host's apt config. while the automatic option like in pyjuju is better from 'i just want to get started using juju quickly' pov, it's also a bit... surprising.13:34
davecheneysidnei: i'll try your patch tomorrow13:35
davecheneyi think we can change the host so it passes throught the right values13:35
noodles775rogpeppe: yeah - that sounds simpler than the other suggestion I had there on the description.13:35
sidneitks, i'll put up a bug fix for precise shortly.13:35
rogpeppenoodles775: for example, add an AllowInterspersedFlags() bool method13:36
rogpeppenoodles775: to the Command interface13:36
noodles775rogpeppe: great - I'll try that out. Thanks for the feedback.13:37
rogpeppenoodles775: there are a couple of other possibilities too13:37
rogpeppenoodles775: i haven't decided which i dislike least :-)13:37
rogpeppenoodles775: if the suggestion above is 1)13:38
rogpeppenoodles775: then:13:38
rogpeppenoodles775: 2) create another interface: type NonInterspersedFlagsCommand interface {NonInterspersedFlags()} and make SuperCommand.Init test dynamically if the command implements that.13:39
rogpeppenoodles775: 3) add another argument to cmd.Register13:39
rogpeppenoodles775: the down side with 1) is that, though simple, it requires that method to be added to every single command.13:39
rogpeppenoodles775: although...13:40
rogpeppenoodles775: hmm, one mo13:40
rogpeppenoodles775: ha, we've already got CommandBase! perfect.13:40
noodles775yeah - I thought BaseCommand could do som...13:40
noodles775Excellent, that makes 1) the winner then right?13:41
rogpeppenoodles775: yup13:41
* rogpeppe starts looking at dimitern's review13:42
natefinchmgz: sorry, back finally. Kids make everything take twice as long.13:42
sidneitrivial-ish review: https://codereview.appspot.com/1275804414:00
davecheneysidnei: your apt-config based proxy is working well14:01
davecheneyit's makeing the hotel wifi much more apporachable14:01
sidneidavecheney: good to know. see cl above for the precise fix.14:02
* davecheney looks14:02
dimiternsidnei: looking14:02
davecheneyoh, i see what's happehned14:02
rogpeppedimitern: how about something like this for factoring out the endless duplication? http://paste.ubuntu.com/5977395/14:03
rogpeppedimitern: i think i can implement that in an hour or so14:03
rogpeppedimitern: and i think it applies to pretty much all our API calls14:04
=== Guest52531 is now known as wedgwood
dimiternrogpeppe: and the implementation will basically be a lot of hardly readable reflection magic?14:06
rogpeppedimitern: well, at least it can be well tested and it will save us 1000s of lines of code and days of implementation time14:06
dimiternrogpeppe: how about getCanX? it just seems too generic to be useful14:06
rogpeppedimitern: one mo, i'll paste an example of how i envisage it would be used14:07
dimiternrogpeppe: we need at least one for Get and one for Set - they'll do different things14:07
dimiternrogpeppe: maybe more that 2 will be needed14:08
rogpeppedimitern: really?14:08
rogpeppedimitern: why the difference?14:08
dimiternrogpeppe: one returns something other than an error + optional error or bool to something, and the other, just an error and takes a bunch of args14:09
rogpeppedimitern: i don't see the difference. they both take some parameters and return some results14:09
dimiternrogpeppe: how about providing a bunch of helpers + callbacks and do the rest inline?14:10
dimiternrogpeppe: like prepareResult, checkAuth, processEntities14:11
rogpeppedimitern: here's how it might look for CharmURL, for example: http://paste.ubuntu.com/5977431/14:11
dimiternrogpeppe: and customizing each such helper to work with different results/params14:12
dimiternrogpeppe: that's sounds like forcing us to use reflection with it's performance issues14:12
rogpeppedimitern: i'd be interested to see a concrete proposal14:12
rogpeppedimitern: do you know that performance is a problem here?14:13
dimiternrogpeppe: where's the auth in the example?14:13
rogpeppedimitern: in the func returned by getUnitFunc14:13
dimiternrogpeppe: I'll need a more concrete example then, sorry14:14
rogpeppedimitern: ok, hold on14:14
rogpeppedimitern: here's a sample implementation of getUnitFunc in terms of the existing UniterAPI primitives: http://paste.ubuntu.com/5977456/14:17
dimiternrogpeppe: also, fwiw I don't think this approach is any faster than what we have now, it'll just reduce code duplication at the expense of having to look at several places to see what's going on14:17
dimiternrogpeppe: that'll work except for NotFoundErr, which needs to be converted to ErrPerm, but I begin to see your point14:18
rogpeppedimitern: reducing the number of lines of code from 26 to 7 seems like a pretty good win to me14:19
rogpeppedimitern: over however many API calls14:19
rogpeppedimitern: plus the fact that we can potentially reduce the amount of test code14:19
rogpeppedimitern: because we can be sure that the details of the bulk call are dealt with correctly14:20
dimiternrogpeppe: how can we do that?14:20
rogpeppedimitern: at the moment there's a significant amount of logic in each bulk API call that needs to be tested14:21
rogpeppedimitern: if each bulk API call contains a single call to BulkCall, surely we can make our tests rely on that and test only that the14:22
rogpeppedimitern: call is made with the expected parameters.14:22
dimiternrogpeppe: are you getting at not writing individual tests for each call?14:22
rogpeppedimitern: no, but at the moment each test needs to pass in a vector with several arguments in, each designed to test an aspect of the logic14:23
rogpeppedimitern: we'd just need to pass a single-element vector containing one argument, and check that the single expected result is returned14:23
dimiternrogpeppe: yeah, and I'm sure we can do something about reducing the size of tests, without losing code coverage14:24
rogpeppedimitern: exactly14:24
natefinchrogpeppe, dimitern: for what it's worth, I think code generation would make the code less error prone than it is now, and more readable than some really generic thing like the BulkCall Roger posted14:24
dimiternnatefinch: I was thinking of a way to declaratively generate both code and tests for the api calls14:25
rogpeppenatefinch: i'm a bit reluctant to go down that road14:25
rogpeppenatefinch: although if we've factored things out to use reflection, it would not be hard to change things to go that route in the future if desired.14:26
natefinchrogpeppe: I'm reluctant to go down the road of writing a function that takes 4 empty interface arguments and needs 5 paragraphs of comments to explain it :)14:27
dimiternrogpeppe: surely, generated code can be optimized to run faster than a generic all-in-one code relying on reflection magic14:27
rogpeppedimitern: performance isn't the issue here14:27
rogpeppedimitern: we haven't even started trying to optimise stuff yet14:27
rogpeppedimitern: beyond hand-wavy "bulk API calls will be faster" remarks14:28
dimiternrogpeppe: using reflection several times for each call will surely degrade performance, don't you agree?14:28
rogpeppedimitern: sure14:28
rogpeppedimitern: so will using many features that we're using all the time14:28
rogpeppedimitern: currently for each API call we're making several round trips to mongo14:29
rogpeppedimitern: *that*'s the place to start optimising stuff14:29
rogpeppedimitern: a few reflection calls are neglible compared to a network round trip14:29
dimiternrogpeppe: sure, but adding more less performant code on top of it won't improve things much in the long run14:30
rogpeppedimitern: first rule of optimisation: measure first14:30
dimiternrogpeppe: exactly, that's why we need a proof of concept for both14:30
rogpeppenatefinch: i agree with your reservations about the reflect call. but automatically generated code has its own down sides.14:32
rogpeppedimitern: i'm not convinced that macros are better than reflection in general14:33
dimiternrogpeppe: macros?14:34
rogpeppedimitern: code that writes code14:34
dimiternrogpeppe: well, code that's generated and compiled, rather than dynamically reflected at run-time14:35
natefinchrogpeppe: definitely. Neither is ideal. But I think generated code has better tradeoffs.  the code it creates is a lot easier to read, understand and debug than reflection-heavy generic code14:35
dimiternnatefinch: oh, that's +114:36
mgzrogpeppe: does goamz use lbox workflow for landing things?14:36
rogpeppemgz: yes14:36
rogpeppenatefinch: i'm not sure. i think that 50 lines of reflection magic to reduce code by 75% elsewhere can be worth it (think fmt.Printf for example)14:38
rogpeppenatefinch: i'd be interested to see how the code might look using gocog, for example, though14:46
* dimitern has to step out for a bit14:47
natefinchrogpeppe: I'll whip something up, so we'll at least have some real code to look at and complain about :)14:47
sidneirogpeppe: if you could review https://codereview.appspot.com/12758044/14:51
rogpeppesidnei: looking14:51
rogpeppesidnei: looking15:09
rogpeppesidnei: i mean, reviewed!15:09
dimiternrogpeppe: hey15:23
rogpeppedimitern: yo15:23
dimiternrogpeppe: how about to offer you a deal? :) let's keep that duplicated way until we finish the uniter client and server side, so we can actually refactor the uniter to use it15:23
dimiternrogpeppe: then let's discuss how to minimize the duplication - reflection or code generation, i don't mind, let's discuss it then15:24
rogpeppedimitern: my aim was to reduce the amount of work you need to do now15:24
dimiternrogpeppe: because now I have certain momentum and would like to keep it up15:24
rogpeppedimitern: keep on going15:25
dimiternrogpeppe: it's no trouble really15:25
dimiternrogpeppe: thanks!15:25
dimiternnatefinch, mgz, rogpeppe: I still need 2 reviews on https://codereview.appspot.com/12756043/15:27
rogpeppedimitern: oh yes, sorry, i got some way through, then distracted15:27
dimiternno worries15:27
rogpeppedimitern: reviewed15:32
dimiternrogpeppe: cheers15:32
dimiternmgz: ?15:38
mgzdimitern: looking15:42
sidneirogpeppe: fixed, thanks for the review. shall i land?15:48
rogpeppesidnei: i'll just take a quick last look15:48
rogpeppesidnei: LGTM, thanks. sorry, i had an unpublished comment suggesting the use of the m flag; hope you didn't have to look too hard for it!15:49
sidneirogpeppe: no, was a google + two clicks away :)15:49
sidneispent more time figuring out why \s? didn't work :)15:50
rogpeppesidnei: FWIW golang.org/pkg/regexp/syntax is an easy quick ref15:50
rogpeppesidnei: why didn't it work?15:50
mgzdimitern: reviewed, favour extraction to review the branch I have up please :015:50
sidneibecause it's the wrong syntax, it should have been \s*15:50
rogpeppesidnei: ah, i hadn't noticed that15:51
dimiternmgz: thanks - sure, can I have a link?15:51
sidneirogpeppe: although, hum the page at http://golang.org/pkg/regexp/syntax/ say it should work15:52
mgzdimitern: 12752043 :)15:52
sidnei"x?             zero or one x, prefer one" "x*             zero or more x, prefer more"15:52
rogpeppesidnei: what were you trying to match?15:53
dimiternmgz: sneaky :)15:53
sidneirogpeppe: zero or more spaces, but i actually had two in the test i did.15:53
rogpeppesidnei: ah, so it's right that it didn't work15:53
* sidnei blames on timezone15:54
dimiternmgz: reviewed16:01
rogpeppedimitern, natefinch: FWIW, here's the magic: http://play.golang.org/p/BzS1Kn8peV16:16
rogpeppedimitern: all the type inspection code can be factored out and done only once for each combination of types16:17
dimiternrogpeppe: will look more carefully a bit later16:32
sidneirogpeppe: reminder that im waiting on your ack.16:42
rogpeppesidnei: no need to wait - I already gave you my LGTM16:42
rogpeppesidnei: thanks anyway :-)16:43
dimiternrogpeppe, mgz: last of the unit ops https://codereview.appspot.com/1277604316:43
sidneirogpeppe: sorry i was looking for one in the cl and missed the one on irc. :)16:46
rogpeppesidnei: np16:47
rogpeppedimitern: reviewed16:52
dimiternrogpeppe: cheers16:53
dimiternmgz, natefinch: second review?16:55
mgzdimitern: sec17:01
mgzdimitern: done, alos responded to you comments on review17:08
dimiternmgz: thanks!17:09
=== flaviamissi is now known as flaviamissi_
natefinchmgz: back17:33
mgznatefinch: hey, with you in a sec17:39
natefinchmgz: no rush17:39
natefincharg... ubuntu, why do you hate me so?  I can get either the microphone or the headphones of my headset to work, but not both at the same time.  It's like whatever tab of the sound settings I'm on is what ubuntu decides will work, when I plug in the USB cord. :/17:48
* rogpeppe feels for natefinch17:52
rogpeppetime to go17:52
rogpeppeg'night all17:52
natefinchrogpeppe: g'night17:52
rogpeppenatefinch: see ya!17:52
=== tasdomas_afk is now known as tasdomas
=== flaviamissi_ is now known as flaviamissi
=== tasdomas is now known as tasdomas_afk
=== flaviamissi is now known as flaviamissi_
=== andreas__ is now known as ahasenack

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