| bigjools_ | it's so much quieter here without thumper around | 01:54 |
|---|---|---|
| davecheney | too quiet ... | 01:55 |
| === bigjools_ is now known as bigjools | ||
| bigjools | davecheney: I am putting some logger.Infof() calls to debug stuff but they are not appearing, do I need to initialise anything first? | 03:36 |
| davecheney | bigjools: shouldn't | 03:41 |
| bigjools | oh ffs | 03:41 |
| davecheney | but from memory unless you pass -v, then you get nothing | 03:42 |
| bigjools | I just saw that I need a -v | 03:42 |
| davecheney | axw will know | 03:42 |
| bigjools | works with -v | 03:42 |
| davecheney | yeah, this is fucked | 03:42 |
| davecheney | he's fixing it | 03:42 |
| bigjools | good man | 03:42 |
| davecheney | in our defense, we inhertied this madness from pyjuju | 03:42 |
| davecheney | i don't know if that is the kind of defence that will stand up in court | 03:42 |
| axw | yeah just waiting for thumper to ready his changes | 03:43 |
| axw | it's a bit messy | 03:43 |
| davecheney | 'yes your honor, I saw someone else brutally murdered, so I figured it was ok ' | 03:43 |
| bigjools | one day, juju will evolve the perfect provider interface | 03:45 |
| === tasdomas_afk is now known as tasdomas | ||
| * bigjools juju bootstraps an azure environment and deploys juju-gui on it | 05:48 | |
| axw | bigjools: I'm looking into adding instance-specific state to juju status. Do you know if MAAS provides a way of getting the current state of a machine? | 06:21 |
| bigjools | axw: one sec | 06:22 |
| bigjools | axw: the nearest thing is that api lets you query all allocated nodes which returns quite detailed info for each node. | 06:26 |
| bigjools | so depends on what state you want? | 06:26 |
| axw | I don't know :) I guess I'd like to see what's available and work from there | 06:26 |
| axw | can you point me to some docs/code? | 06:26 |
| axw | bigjools: I'm after something comparable to gwacl.HostedServiceDescriptor.Status for Azure | 06:29 |
| bigjools | jtv1: can you remember if the maas api will return json for a single node if we GET its URL? | 06:30 |
| jtv1 | bigjools: IIRC it does. | 06:31 |
| === jtv1 is now known as jtv | ||
| bigjools | axw: ok so there's a status on the json that gets returned | 06:32 |
| bigjools | if you look in the maas source tree at the file src/maasserver/enum.py check out the NODE_STATUS enum | 06:32 |
| axw | bigjools: cool, thanks | 06:32 |
| bigjools | axw: if juju knows about it, it'll never see anything other than "reserved" though | 06:33 |
| axw | ah | 06:33 |
| bigjools | errr allocated I mean | 06:33 |
| bigjools | maas does not track the status of anything after the node gets given to a user | 06:34 |
| axw | bigjools: okey dokey. sounds like it mightn't be useful to add at all then | 06:34 |
| bigjools | so there's no way of knowing whether it's installing, powering up etc | 06:34 |
| bigjools | yeah | 06:35 |
| === tasdomas is now known as tasdomas_afk | ||
| rogpeppe | mornin' all | 07:39 |
| axw | rogpeppe: morning | 07:43 |
| rogpeppe | axw: hiya | 07:43 |
| * axw wonders how much lxc testing his laptop can take before carking it | 07:54 | |
| === tasdomas_afk is now known as tasdomas | ||
| dimitern | rogpeppe: ping | 08:55 |
| rogpeppe | dimitern: pong | 08:55 |
| dimitern | rogpeppe: hey, morning! | 08:56 |
| dimitern | rogpeppe: did you merge the agent entity stuff already? | 08:56 |
| rogpeppe | dimitern: yeah | 08:56 |
| dimitern | rogpeppe: cool, i'll take a look then | 08:56 |
| dimitern | rogpeppe: so AgentEntity will now need StatusSetter and Watcher(?) | 08:59 |
| rogpeppe | dimitern: seems reasonable | 09:00 |
| dimitern | rogpeppe: how should I call the interface that has Entiy.Watch() ? | 09:00 |
| dimitern | rogpeppe: and I also need EnsureDeader | 09:01 |
| rogpeppe | dimitern: ha, you can't have EntityWatcher. | 09:03 |
| dimitern | rogpeppe: exactly | 09:04 |
| dimitern | rogpeppe: Watchable? | 09:04 |
| * rogpeppe thinks | 09:04 | |
| dimitern | rogpeppe: WatchEntiter ?:) | 09:05 |
| rogpeppe | dimitern: i've already mentally rejected that :-) | 09:06 |
| dimitern | rogpeppe: yeah | 09:06 |
| rogpeppe | dimitern: EWatcher? | 09:07 |
| rogpeppe | dimitern: EntityWatcherInterface? | 09:07 |
| dimitern | rogpeppe: AgentEntityWatcher ? | 09:08 |
| rogpeppe | dimitern: thing is, it's not specific to agent entities | 09:08 |
| dimitern | rogpeppe: why? i think it is | 09:08 |
| rogpeppe | dimitern: it applies to anything that has Watch() *NotifyWatcher | 09:08 |
| rogpeppe | dimitern: e.g. Service | 09:08 |
| rogpeppe | dimitern: but... maybe that's ok | 09:09 |
| dimitern | rogpeppe: but a service is also managed by some agent after all | 09:09 |
| rogpeppe | dimitern: really? i don't think so. | 09:10 |
| dimitern | rogpeppe: well the uniter for example manages services inside the unit agent | 09:10 |
| rogpeppe | dimitern: it manages units *of* a service | 09:10 |
| rogpeppe | dimitern: well, one unit of a service | 09:10 |
| rogpeppe | dimitern: the service itself has no agent | 09:11 |
| dimitern | rogpeppe: yeah, true | 09:11 |
| dimitern | rogpeppe: but I don't think we'll need an interface for that | 09:11 |
| dimitern | rogpeppe: it's about reusing common code for agents | 09:11 |
| rogpeppe | dimitern: but still, the reason we're defining the interface is for AgentEntity, so i'm ok with calling it AgentEntityWatcher | 09:11 |
| dimitern | rogpeppe: there'll be a WatchService API call in the uniter | 09:12 |
| dimitern | rogpeppe: ok, AgentEntityWatcher then | 09:12 |
| rogpeppe | dimitern: + | 09:12 |
| rogpeppe | 1 | 09:12 |
| sidnei | here's my attempt at fixing https://bugs.launchpad.net/juju-core/+bug/1203816 https://codereview.appspot.com/12143043 | 09:27 |
| _mup_ | Bug #1203816: local provider should support use of local proxy <juju-core:Triaged> <https://launchpad.net/bugs/1203816> | 09:27 |
| dimitern | rogpeppe: you haven't changed getAuthFuncs to use method expressions yet? | 09:40 |
| rogpeppe | dimitern: no, i still need another LGTM on that CL | 09:41 |
| dimitern | rogpeppe: ah, ok | 09:41 |
| rogpeppe | dimitern: i'm also looking for reviews of https://codereview.appspot.com/12361043/ (mgz?) and https://codereview.appspot.com/12352044/ | 09:42 |
| dimitern | rogpeppe: will take a look in a bit | 09:45 |
| === _thumper_ is now known as thumper | ||
| * thumper waves from the IOM | 10:03 | |
| jamespage | davecheney, which release of juju-core should I be uploading to saucy? | 10:04 |
| dimitern | thumper: hey, how is it going there? | 10:05 |
| thumper | good so far, going through everyone's demos | 10:06 |
| thumper | also, raining | 10:06 |
| dimitern | :) cool | 10:06 |
| mgz | jamespage: 1.12 I believe | 10:08 |
| jamespage | mgz, ack - thats what I thought | 10:08 |
| davecheney | jamespage: correct | 10:15 |
| jamespage | davecheney, great -thanks for confirming - working that now | 10:32 |
| davecheney | jamespage: thanks mate | 10:32 |
| davecheney | this is the version we talked about in Oakland | 10:32 |
| davecheney | ie, the one we're hoping to feed into backports | 10:32 |
| dimitern | rogpeppe: one review done, i'm on your second CL now | 10:34 |
| dimitern | rogpeppe: can you take a look at https://codereview.appspot.com/12443044/ in the mean time? | 10:34 |
| rogpeppe | davecheney: i'd be interested in your input on https://codereview.appspot.com/12361043/ too, please | 10:35 |
| rogpeppe | dimitern: looking | 10:36 |
| dimitern | rogpeppe: second review done as well | 10:36 |
| rogpeppe | dimitern: i don't know if it's a nice catch until we get a lack of intermittent test failures for that test :-) | 10:37 |
| rogpeppe | dimitern: do you know if it had a bug report, BTW? i couldn't find one | 10:37 |
| dimitern | rogpeppe: which test failed? | 10:41 |
| davecheney | rogpeppe: i saw that | 10:41 |
| rogpeppe | dimitern: MachineSuite.TestWatchMachine | 10:41 |
| davecheney | where is the change to the README to explain how to use it ? | 10:41 |
| dimitern | rogpeppe: no, I don't think there's a bug for it | 10:41 |
| dimitern | rogpeppe: just checked | 10:41 |
| rogpeppe | davecheney: currently we're just putting it into the tree. mgz is going to change the bot to use it; then i plan to change godeps so it can use it | 10:42 |
| davecheney | rogpeppe: where is godeps ? | 10:43 |
| rogpeppe | davecheney: go get launchpad.net/godeps | 10:43 |
| dimitern | sidnei: you've got a review | 10:46 |
| sidnei | dimitern: lovely. i might be doing a few more changes, during testing i noticed the proxy is set up too late, talking with smoser about a proper fix | 10:47 |
| jamespage | davecheney, do you happen to know what happening re upgrade from pyjuju->juju-core | 10:47 |
| jamespage | first upgrade break: | 10:47 |
| jamespage | https://bugs.launchpad.net/ubuntu/+source/juju-core/+bug/1200878 | 10:47 |
| _mup_ | Bug #1200878: Upgrade breaks existing pyjuju deployment <apport-collected> <regression-release> <saucy> <juju-core (Ubuntu):Triaged> <https://launchpad.net/bugs/1200878> | 10:47 |
| dimitern | sidnei: I see, ok | 10:47 |
| dimitern | oh boy.. EnsureDeaderer | 10:52 |
| dimitern | or maybe DeathEnsurerer :) | 10:53 |
| rogpeppe | dimitern: DeadEnsurer? | 10:55 |
| dimitern | rogpeppe: that's for state? | 10:56 |
| rogpeppe | dimitern: is there a problem with changing the Machines field to Entities in the Machine.SetStatus call, BTW? i'm concerned about backward incompatibility. | 10:56 |
| rogpeppe | dimitern: yeah | 10:56 |
| dimitern | rogpeppe: that's why I added DEPRECATE(v1.14) to it | 10:57 |
| dimitern | rogpeppe: to the older struct | 10:57 |
| rogpeppe | dimitern: ah, ok | 10:57 |
| rogpeppe | dimitern: cool | 10:57 |
| dimitern | rogpeppe: ok i'll go with DeadEnsurer | 10:58 |
| sidnei | dimitern: indeed, the AddScripts was supposed to be a pre-req, wasn't merged yet. i'll submit it. | 11:00 |
| jam | just saying a quick hello to everyone. In a meeting, but back at work after vacation. | 11:01 |
| dimitern | sidnei: ok; you might run into conflicts with this CL afte the prereq lands | 11:01 |
| dimitern | jam: heyhey | 11:01 |
| sidnei | dimitern: in codereview? because bzr should handle it fine. | 11:01 |
| rogpeppe | dimitern: i'm not sure how your changes preserve API backward compatibility | 11:03 |
| dimitern | sidnei: ah, ok then; no, codereview doesn't matter - it's a bzr matter anyway | 11:03 |
| dimitern | rogpeppe: do you propose to have handling for both MachineSetStatus and SetStatus for compatibility? | 11:04 |
| rogpeppe | dimitern: how would that help? | 11:05 |
| dimitern | rogpeppe: it'll take both types of args | 11:06 |
| dimitern | rogpeppe: (machiner.SetStatus) | 11:06 |
| * dimitern fwiw i think even considering backwards compatibility with the API at this point is ridiculous | 11:07 | |
| rogpeppe | dimitern: it's something that we need to consider | 11:08 |
| rogpeppe | dimitern: if something updates the API server but not one of the clients, then this change will break the installation | 11:08 |
| jam | dimitern: my personal feeling, you don't have to preserve compat with 1.13, but we should preserve with things in 1.12. (At least, we need to start getting comfortable with how we are going to do these things going forward.) | 11:08 |
| rogpeppe | jam: hiya | 11:09 |
| rogpeppe | jam: i think i agree with that | 11:09 |
| dimitern | rogpeppe: so how about my proposal? | 11:09 |
| rogpeppe | dimitern: and i *think* 1.12 used the API machiner, didn't it? | 11:09 |
| dimitern | rogpeppe: handle both the old and new args types in SetStatus? | 11:09 |
| rogpeppe | dimitern: that seems reasonable. just add a Machines field in params.SetStatus, marked deprecated. | 11:10 |
| dimitern | rogpeppe: and read both? | 11:11 |
| rogpeppe | dimitern: if len(p.Entities) == 0 { p.Entities = p.Machines } | 11:12 |
| dimitern | rogpeppe: ok | 11:12 |
| rogpeppe | dimitern: i'm particularly +1 on jam's remark "we need to start getting comfortable with how we are going to do these things going forward" | 11:13 |
| rogpeppe | dimitern: it's important that we get experience maintaining API compatibility, see what works, what doesn't. | 11:14 |
| dimitern | rogpeppe: ok, I can agree with that | 11:15 |
| dimitern | rogpeppe: how can I get 1.12 tools to test the upgrade? | 11:26 |
| rogpeppe | dimitern: i should hope they'd be in the tools bucket | 11:26 |
| rogpeppe | dimitern: if not, you'll have to fetch the tgz and compile them yourself | 11:27 |
| dimitern | rogpeppe: I might need to have a quick chat with you about that, after the standup | 11:27 |
| rogpeppe | dimitern: ok | 11:27 |
| dimitern | mgz: standup? | 11:31 |
| dimitern | jam, thumper: I suppose you won't be joining as well | 11:32 |
| mgz | ta dimitern | 11:33 |
| jam | we will not | 11:37 |
| jam | lunch time for us | 11:37 |
| wallyworld | rogpeppe: https://codereview.appspot.com/12308044/ :-) | 11:37 |
| rogpeppe | wallyworld: ta! | 11:38 |
| sidnei | dimitern: cl updated | 11:49 |
| natefinch | rogpeppe: https://codereview.appspot.com/12347044/ It still needs tests, but I wanted a check to make sure the approach was ok, since it's a bit above and beyond what the bug calls for. It's a very small number of lines changed, however. | 12:02 |
| dimitern | sidnei: thanks, still LGTM | 12:03 |
| rogpeppe | natefinch: ah yes, i started looking at that and wasn't entirely sure, but got distracted while trying to think of a better alternative... | 12:04 |
| rogpeppe | natefinch: one question: why does ReadEnvirons return a no-env error only if path=="" ? | 12:04 |
| natefinch | rogpeppe: I went back and forth on that. The idea was that it only gives the error message if you're using the default environment... because otherwise it means you explicitly gave it a file you were expecting to exist, and the error message talks about using init, which won't help make your existing configuration work | 12:06 |
| rogpeppe | natefinch: hmm, that sounds like something that should be triggered at a different level (for example at the command level, where you know if you've been passed an environment or not) | 12:08 |
| rogpeppe | natefinch: i have an idea - i'll just check to see if it's viable, one mo | 12:08 |
| natefinch | rogpeppe: cool, thanks for the help. | 12:08 |
| rogpeppe | natefinch: i wonder if a better place to return the NoEnv error would be from juju.NewConnFromName | 12:18 |
| dimitern | natefinch: reviewed | 12:19 |
| rogpeppe | natefinch: and i'm not sure if we should pollute cmd/cmd.go with juju-command specific stuff (it's also used by jujud) | 12:19 |
| dimitern | rogpeppe: that's where the supercommand lives, no? | 12:19 |
| rogpeppe | dimitern: yes, but i don't thing the supercommand wants to know about stuff that cmd/juju is doing. it seems like mixing responsibilities | 12:20 |
| rogpeppe | natefinch: so i'd be tempted to do something like this, in each juju command that calls NewConnFromName: | 12:21 |
| rogpeppe | conn, err := juju.NewConnFromName(c.EnvName) | 12:21 |
| rogpeppe | if err != nil { | 12:21 |
| rogpeppe | return envOpenFailure(err) | 12:21 |
| rogpeppe | } | 12:21 |
| dimitern | rogpeppe: if it's already reading the env.yaml file there, the responsibility is there | 12:21 |
| rogpeppe | dimitern: it's not, is it? | 12:21 |
| natefinch | rogpeppe: yeah, I was trying to avoid changing every single command, but if cmd.go isn't juju specific, then yeah, we don't want to pollute that | 12:21 |
| rogpeppe | natefinch: and define envOpenFailure as: func envOpenFailure(err error) error {if err == juju.ErrNoEnvironmentFile {print message; return cmd.ErrSilent}; return err} | 12:22 |
| dimitern | rogpeppe: i'll not restrict EnsureDead to not alive entities then | 12:24 |
| natefinch | rogpeppe: Yeah, I guess I was ok letting the error go back up the stack, mostly so it still gets printed out, but I guess we can just print it out there, too. | 12:24 |
| natefinch | rogpeppe: (the file not found error) | 12:24 |
| rogpeppe | natefinch: i think there's no need to embed the original file-not-found error - we already know it's os.IsNotExist | 12:25 |
| natefinch | rogpeppe: I think it's helpful to know what file the system was trying to open and couldn't find. It helps if you typo the path of the file somewhere | 12:26 |
| rogpeppe | natefinch: fair enough | 12:29 |
| rogpeppe | natefinch: oh yes, you'll probably want envOpenFailure(err error, envName string) so that you can print the "Please create a configuration" message only if it's empty. | 12:30 |
| rogpeppe | natefinch: or you could even implement it as a method on EnvCommandBase | 12:30 |
| natefinch | rogpeppe: yep, sounds good | 12:31 |
| dimitern | mgz: https://codereview.appspot.com/12443044/ this is the first CL | 12:36 |
| dimitern | mgz: and this is the follow-up https://codereview.appspot.com/12461043 | 12:37 |
| dimitern | rogpeppe: ^^ | 12:37 |
| dimitern | rogpeppe: enter the DeadEnsurer :) | 12:37 |
| * rogpeppe feels uncomfortable about the spelling, but DeathEnsurer seems wrong too | 12:38 | |
| dimitern | rogpeppe: I'm open to suggestions | 12:38 |
| rogpeppe | dimitern: i have none. | 12:39 |
| natefinch | dimitern: If we have the opportunity to put a DeathEnsurer in the product, I think we should take it :) | 12:40 |
| natefinch | dimitern: Reaper? With a Reap() function? | 12:41 |
| dimitern | natefinch: :) alas we need to adhere to the predefined state interface | 12:41 |
| dimitern | natefinch: I proposed DeathEnsurer, but it's equally bad | 12:41 |
| natefinch | dimitern: Ahh, too bad. | 12:42 |
| dimitern | rogpeppe: 1.12 is reallt 1.11.something, right? | 13:02 |
| dimitern | really* | 13:02 |
| rogpeppe | dimitern: yeah | 13:02 |
| dimitern | rogpeppe: do you know which one exactly? | 13:02 |
| rogpeppe | dimitern: no. why do you want to know? | 13:02 |
| dimitern | rogpeppe: so I can wget it and test the upgrade | 13:03 |
| rogpeppe | dimitern: https://launchpad.net/juju-core/1.12/1.12.0/+download/juju-core_1.12.0-1.tar.gz | 13:03 |
| dimitern | rogpeppe: ah, ok, 10x | 13:03 |
| * rogpeppe goes for lunch | 13:07 | |
| dimitern | mgz: ping | 13:07 |
| mgz | hey dimitern | 13:07 |
| dimitern | mgz: hey, any chance to look at my CLs? | 13:07 |
| mgz | 088 and 089? | 13:08 |
| dimitern | mgz: yep | 13:08 |
| mgz | doing so now | 13:08 |
| dimitern | cheers! | 13:08 |
| mgz | I find "NewStatusSetter returns a new StatusSetter" a little funny as the first sentence of a doc comment | 13:12 |
| mgz | but can't really suggest an improvement.... | 13:12 |
| dimitern | that's what it does :) | 13:12 |
| mgz | dimitern: one down, will just grab some food them look at the other | 13:16 |
| dimitern | ok, thanks | 13:16 |
| dimitern | mgz: about the deprecation stuff - aiui we need to make sure the api params/calls are backwards compatible between versions, rather than removing them straight away - it'll make certain version-to-version upgrades problematic | 13:26 |
| dimitern | hence the deprecation - we guarantee at least the upgrade is possible from the previous release, but deprecated stuff will go away in the next | 13:28 |
| === teknico1 is now known as teknico | ||
| dimitern | and now.. AgentEntityWatcherer.. | 13:40 |
| dimitern | we defined a whole dictionary | 13:40 |
| natefinch | dimitern: I usually take the "add -er to an interface name" as a guideline, not a hard and fast rule, for exactly these problems | 13:50 |
| dimitern | natefinch: tell that to rogpeppe :) | 13:50 |
| rogpeppe | dimitern: i agree with natefinch | 13:51 |
| rogpeppe | dimitern: i hate the *erer names | 13:51 |
| dimitern | rogpeppe: you're the one who usually insists on using -er everywhere | 13:51 |
| dimitern | rogpeppe: I can remember at least 3 cases where I choose something else :) | 13:52 |
| rogpeppe | dimitern: only when reasonable. i would never have suggested Removerer (the first such name) | 13:52 |
| dimitern | rogpeppe: let me find the CL | 13:52 |
| natefinch | at some point it just looks ridiculous (pretty much any -erer) | 13:52 |
| rogpeppe | dimitern: i don't know who did Removerer - i laughed out loud when i saw it | 13:53 |
| rogpeppe | dimitern: looks like you and jam were responsible... https://codereview.appspot.com/11125043/diff/1/state/apiserver/common/remove.go#newcode17state/apiserver/common/remove.go:17 | 13:54 |
| dimitern | rogpeppe: ok, sorry it wasn't you - it was jam, sorry | 13:54 |
| rogpeppe | dimitern: you will be :-) :-) | 13:55 |
| dimitern | well, strictly speaking English is not my native language, so none of these names look especially funny or wrong to me :) | 13:56 |
| dimitern | it's just convention | 13:56 |
| fwereade | rogpeppe, dimitern: so, SetStatus change | 13:56 |
| fwereade | rogpeppe, dimitern: what's the deal? | 13:56 |
| dimitern | fwereade: it landed | 13:56 |
| dimitern | fwereade: oh? what | 13:57 |
| fwereade | dimitern, Machines vs Agents/Entities/whatever | 13:57 |
| rogpeppe | fwereade: i suggested that it was worth maintaining 1.12 compat | 13:57 |
| dimitern | fwereade: yes, and I did that | 13:57 |
| dimitern | fwereade: it's deprecated | 13:58 |
| fwereade | rogpeppe, for values of "worth" equal to "necessary" | 13:58 |
| rogpeppe | fwereade: which means that SetStatus needs to recognise the Machines argument, until we can deprecate iyt | 13:58 |
| fwereade | rogpeppe, dimitern: perfect | 13:58 |
| fwereade | rogpeppe, dimitern: just checking, there was uncertainty here | 13:58 |
| dimitern | fwereade: I even added a test for 1.12 compat and live tested an upgrade 1.12 -> tip | 13:58 |
| fwereade | dimitern, rogpeppe: lovely | 13:59 |
| rogpeppe | fwereade: in practice, i don't *think* it would be a problem if we didn't recognise the Machines argument, because environments tend to upgrade all at the same time | 13:59 |
| fwereade | rogpeppe, disagree | 13:59 |
| fwereade | rogpeppe, dimitern: please confirm which workers are currently using the api | 14:00 |
| rogpeppe | fwereade: in 1.12, i think it's the machiner | 14:00 |
| fwereade | rogpeppe, in trunk | 14:00 |
| dimitern | fwereade: upgrader, deployer and machiner | 14:00 |
| dimitern | rogpeppe: upgrader as well, right? | 14:00 |
| rogpeppe | dimitern: yup | 14:00 |
| rogpeppe | fwereade: i *think* this particular incompatibility is only a problem if there's an agent that's persistently using an earlier version. | 14:02 |
| fwereade | rogpeppe, I would prefer us not to half-ass compatibility even if we can get away with it today | 14:02 |
| rogpeppe | fwereade: i agree entirely, which was why i suggested the change | 14:02 |
| rogpeppe | fwereade: but it's interesting to consider what the actual ramifications of the incompatibility might be in *practice* | 14:03 |
| fwereade | rogpeppe, gtg, but I note in APIWorker | 14:16 |
| fwereade | runner := worker.NewRunner(allFatal, moreImportant) | 14:16 |
| rogpeppe | fwereade: yes, that will change very soon | 14:16 |
| fwereade | rogpeppe, cool, so long as we're aligned on it being important | 14:16 |
| rogpeppe | fwereade: indeed | 14:16 |
| === tasdomas is now known as tasdomas_afk | ||
| dimitern | mgz: ping | 14:33 |
| mgz | dimitern: second branch? :) | 14:33 |
| dimitern | mgz: yep :) | 14:33 |
| dimitern | rogpeppe: can you take a look as well? https://codereview.appspot.com/12461043/ | 14:33 |
| mgz | DeadEnsurer really is funny | 14:34 |
| rogpeppe | dimitern: looking | 14:34 |
| dimitern | it might as well be GrimReaper | 14:34 |
| rogpeppe | dimitern: i wonder if we might want to consider combining LifeGetter, StatusGetter, DeadEnsurer etc into a single type | 14:36 |
| rogpeppe | dimitern: i dunno actually, just a passing thought | 14:37 |
| dimitern | rogpeppe: I'll actually do something like this in the final CL | 14:37 |
| dimitern | rogpeppe: but they are good on their own, at least for now | 14:37 |
| dimitern | sweet.. now with the last CL the machiner API is reduced to a single constructor | 14:38 |
| dimitern | rogpeppe: thanks! | 14:58 |
| dimitern | rogpeppe, mgz: last one (for now) https://codereview.appspot.com/12464043 | 14:59 |
| mgz | dimitern: ta | 14:59 |
| sidnei | can i get one more review on https://codereview.appspot.com/12143043/ ? | 15:18 |
| dimitern | rogpeppe: now that we have a separate apiserver/agent, we'll get rid of apiserver/machine/agent.go and rename the path to apiserver/machiner, right (well, pending deprecation in 1.14). For the uniter, I'll create it in apiserver/uniter directly | 15:19 |
| rogpeppe | dimitern: i don't quite understand. rename which path? create what in apiserver/uniter? | 15:20 |
| dimitern | rogpeppe: rename apiserver/machine to apiserver/machiner, remove agent.go from apiserver/machine; create apiserver/uniter/uniter.go | 15:21 |
| rogpeppe | dimitern: sgtm | 15:22 |
| dimitern | rogpeppe: but the renaming and removing agent.go only in 1.14, right? | 15:24 |
| rogpeppe | dimitern: yeah | 15:24 |
| natefinch | dimitern, rogpeppe, whoever else: what's the best way to get bazaar to show diffs in an external diff tool? I'm a big fan of Beyond Compare, but a quick google gives me like 6 different ways to hook up external diff/conflict resolution... and none of them look particularly good. | 15:28 |
| rogpeppe | natefinch: i use qbzr | 15:28 |
| rogpeppe | natefinch: so there is some way of doing it, but i don't know more than that | 15:29 |
| natefinch | rogpeppe: thanks, that's a good place to start | 15:29 |
| dimitern | natefinch: +10 to qbzr | 15:29 |
| rogpeppe | natefinch: mgz is good on bzr | 15:29 |
| rogpeppe | dimitern: do you use any other qbzr feature than qdiff? | 15:29 |
| mgz | you can also just alias something with the --using flag | 15:30 |
| dimitern | rogpeppe: I use a few - qlog, qannotate, qdiff, q | 15:30 |
| dimitern | not just q :) | 15:30 |
| mgz | the reson the docs aren't completely clear on this is there are two scenarious you might want to use beyond compare | 15:30 |
| mgz | just looking at the current state of the tree, a la `bzr diff`, and resolving merges | 15:30 |
| mgz | and there's not unified config for the two | 15:31 |
| natefinch | mgz: pretty much any time I *can* use Beyond Compare, I do :) | 15:31 |
| dimitern | sidnei: ping rogpeppe and mgz directly :) | 15:32 |
| rogpeppe | dimitern: good point. | 15:32 |
| rogpeppe | sidnei: looking :-) | 15:32 |
| dimitern | rogpeppe: and then maybe look my third? :) | 15:33 |
| mgz | natefinch: so, for starts you just want somethig like `bzr alias bc="diff --using=/path/to/bc"` | 15:38 |
| natefinch | mgz: thanks | 15:43 |
| dimitern | mgz: I feel bad to bug you again.. | 15:47 |
| mgz | dimitern: it's okay, I keep wombling off onto other code | 15:48 |
| mgz | poking is justified | 15:48 |
| dimitern | :) ok, noted | 15:48 |
| === tasdomas_afk is now known as tasdomas | ||
| mgz | dimitern: why have you moved away from table-ish tests to a similar style but with five or so seperate condidiotns in one test case? | 15:58 |
| dimitern | mgz: which one are you refering to? | 15:58 |
| mgz | having the inputs x0..x5 at the top, then the expected results at the bottom of a long-ish test not obviously connected seems more confusing to me | 15:59 |
| mgz | TestEnsureDead and TestSetStatus in the previous mp | 16:00 |
| dimitern | mgz: what do you suggest? | 16:05 |
| rogpeppe | sidnei: reviewed | 16:05 |
| mgz | I'm just curious, those tests aren't so huge, but I thought it was the kind of thing you favoured the table driven style for | 16:05 |
| dimitern | mgz: well, imo table-driven tests are good for a lot of similar cases, which only slightly differ in their setup; this is a single test with multiple arguments | 16:06 |
| sidnei | rogpeppe: thanks! | 16:10 |
| sidnei | rogpeppe: http://paste.ubuntu.com/5951789/ | 16:19 |
| sidnei | or dimitern ^ | 16:19 |
| rogpeppe | sidnei: sorry, use DeepEquals | 16:19 |
| dimitern | sidnei: yeah | 16:20 |
| sidnei | that works, thanks! | 16:20 |
| sidnei | rogpeppe: next: ./apt.go:24: invalid method expression exec.Cmd.Output (needs pointer receiver: (*exec.Cmd).Output) | 16:23 |
| sidnei | oh, just did as the error message, works. | 16:24 |
| dimitern | niemeyer: ping | 16:25 |
| niemeyer | dimitern: Yo | 16:25 |
| dimitern | niemeyer: hey | 16:25 |
| dimitern | niemeyer: so no rpc/jsoncodec lines at all in all-machines.log? | 16:25 |
| niemeyer | dimitern: Yeah | 16:26 |
| dimitern | niemeyer: any DEBUG stuff at all? | 16:26 |
| niemeyer | dimitern: Yeah | 16:26 |
| dimitern | niemeyer: that's really strange.. can you check machine-1.log or some other machine log? | 16:27 |
| niemeyer | dimitern: The file is also very messy | 16:27 |
| niemeyer | dimitern: It's missing line breaks | 16:27 |
| niemeyer | dimitern: It has about 3 messages about rpc, all of them of the style: | 16:27 |
| niemeyer | INFO JUJU:jujud:machine rpc: discarding obtainer method reflect.Method{Name:"Kill" | 16:27 |
| dimitern | niemeyer: anything with "juju.worker.machiner" in there? (not "juju/worker/machiner") | 16:27 |
| dimitern | niemeyer: ah, should be printed once per method in the beginning - it's messy, yes | 16:28 |
| niemeyer | dimitern: Nothing at all | 16:29 |
| dimitern | niemeyer: can you paste the log somehow? it shouldn't be that huge | 16:29 |
| niemeyer | dimitern: Definitely.. just a sec | 16:30 |
| niemeyer | dimitern: Btw, I wouldn't be surprised if I have an out-of-date dependency.. but it's not clear which. I'd expect the agent stuff to be all within juju-core itself | 16:31 |
| dimitern | niemeyer: it is, yes | 16:31 |
| sidnei | rogpeppe: did you meant to suggest CombinedOutput? the comment is a bit unclear | 16:32 |
| rogpeppe | sidnei: ah, sorry missed your earlier remark | 16:33 |
| rogpeppe | sidnei: (*exec.Cmd).Output | 16:33 |
| niemeyer | % juju ssh 0 cat /var/log/juju/all-machines.log | pastebinit | 16:33 |
| niemeyer | http://paste.ubuntu.com/5951849/ | 16:33 |
| sidnei | rogpeppe: yes, past that now, onto the next comment about capturing stderr | 16:33 |
| niemeyer | dimitern: ^^^ | 16:33 |
| niemeyer | Hmm | 16:34 |
| niemeyer | Didn't quite work | 16:34 |
| rogpeppe | sidnei: it depends where the apt- command prints its error message to | 16:34 |
| rogpeppe | sidnei: perhaps it's safer to use CombinedOutput, unless it's cluttered with stdout stuff | 16:34 |
| dimitern | niemeyer: maybe it's a depedency actually | 16:34 |
| niemeyer | Weird.. piping isn't working | 16:34 |
| sidnei | rogpeppe: seems to be stderr: https://pastebin.canonical.com/95449/ | 16:35 |
| dimitern | niemeyer: you can try comparing the godeps generated dependencies.tsv file from here https://codereview.appspot.com/12361043/ to a run of godeps -t `{go list ./...}` in juju-core/ (get it from launchpad.net/godeps/ | 16:35 |
| * rogpeppe goes to look for his keyring for 2-factor auth | 16:35 | |
| sidnei | rogpeppe: sorry, pasting to public | 16:35 |
| niemeyer | dimitern: Holy crap.. it's one gigantic line! | 16:36 |
| dimitern | niemeyer: the log? | 16:36 |
| sidnei | rogpeppe: http://paste.ubuntu.com/5951862/ | 16:36 |
| rogpeppe | sidnei: thanks | 16:36 |
| rogpeppe | sidnei: it looks as if the error lines have a leading "E:" | 16:37 |
| niemeyer | dimitern: Yeah | 16:37 |
| rogpeppe | sidnei: if that's standard, perhaps we should use those for the error message. i wonder if we can have several. | 16:37 |
| sidnei | indeed. conversely it doesn't print any error on missing config, it just produces no output. | 16:37 |
| niemeyer | dimitern: It's breaking the editor as well | 16:37 |
| rogpeppe | sidnei: i guess we could split them and join them with semicolons or something | 16:38 |
| dimitern | niemeyer: hmm.. maybe take a look for anything suspicious in cloud-init-output.log ? | 16:38 |
| sidnei | rogpeppe: http://paste.ubuntu.com/5951872/ | 16:38 |
| niemeyer | dimitern: Please note that the system is actually working | 16:38 |
| dimitern | niemeyer: hmm.. perhaps try zipping the logs and passing it over U1 or something? | 16:39 |
| === tasdomas is now known as tasdomas_afk | ||
| rogpeppe | sidnei: i imagine there are other possible failure modes | 16:39 |
| dimitern | mgz, rogpeppe: poke https://codereview.appspot.com/12464043/ | 16:41 |
| sidnei | rogpeppe: i'll make it like the cmd() in git.go then. | 16:41 |
| rogpeppe | sidnei: yeah, that seems reasonable | 16:42 |
| rogpeppe | sidnei: thanks | 16:43 |
| natefinch | mgz: bzr diff --using seems to only show diffs serially, which is kind of annoying. Is there a way to just have it show them all in parallel? I don't see anything obvious in the man pages | 16:49 |
| mgz | not for diff I think, the equivalent for merge can be smarter | 16:52 |
| mgz | ...though that probably wants you to resolve everything sequentially | 16:53 |
| mgz | ah, maybe qbzr has better handling for this? | 16:54 |
| natefinch | qbzr looks like it should work... evidently some kinks to work out | 16:55 |
| natefinch | it's not opening the external diff I specified in the config by default, and if I switch to bcompare from the qdiff window, the command line seems not to expand environment variables | 16:56 |
| mgz | have to tried just running `bzr qdiff` with args? | 16:56 |
| mgz | modifying your alias to be qdiff rather than diff may be all you need | 16:57 |
| natefinch | qdiff doesn't seem to have using | 16:57 |
| mgz | hm, no, the flags are different | 16:58 |
| mgz | so, is through config only | 16:59 |
| natefinch | yeah, I can get qdiff to pop up the default diff, and then choose beyond compare from the list of external diffs... but it doesn't seem to want to pop up beyond compare by default | 17:01 |
| dimitern | rogpeppe: are you seious about the tests? | 17:02 |
| dimitern | serious | 17:02 |
| rogpeppe | dimitern: only if we're serious about the tests for any of the other of those methods. as they are all going away very soon (assuming my proposal is accepted), i have difficulty in suggesting seriously that you write the tests | 17:03 |
| dimitern | rogpeppe: yep | 17:03 |
| mgz | natefinch: as you may have gathered, I don't use external diff toos so am not clear on all the details... possibly qbzr doesn't support going straight to a subprocess for this | 17:03 |
| dimitern | rogpeppe: I'll shovel up most of these ifaces soon | 17:04 |
| mgz | looks like what we really want is the better qbzr code in bzr core anyway | 17:04 |
| rogpeppe | dimitern: that's what i'm doing now | 17:04 |
| natefinch | mgz: no problem, thanks for the help anyway | 17:04 |
| dimitern | rogpeppe: ah, even better then :) | 17:04 |
| rogpeppe | dimitern: i'm also making some changes to the names package that i hope you'll like | 17:04 |
| dimitern | rogpeppe: nice, can't wait to see them | 17:05 |
| dimitern | mgz: well, as we discovered with rogpeppe, Setterer is actually a word for some bizare profession, but Watcherer is just a proper name in some parts :) | 17:07 |
| natefinch | dimitern: have you gotten qdiff to work with external diff by default? | 17:08 |
| dimitern | natefinch: since I couldn't use my favorite meld, I didn't bother trying to setup external diff with bzr; qdiff seems fine for my uses | 17:11 |
| natefinch | dimitern: dang | 17:12 |
| * dimitern signs off for today | 17:19 | |
| dimitern | g'night all! | 17:20 |
| rogpeppe | dimitern: g'night | 17:20 |
| sidnei | rogpeppe: fixed 'em all | 17:21 |
| rogpeppe | sidnei: brilliant, thanks | 17:21 |
| sidnei | rogpeppe: should i poke someone else re: apt-config, or you're comfortable with approving? | 17:21 |
| rogpeppe | sidnei: it would be really good to get at least one person who knows about that stuff to have a sanity check | 17:22 |
| rogpeppe | sidnei: i don't really feel comfortable approving something that i don't know anything about | 17:22 |
| sidnei | there's not really much to know about it other than 'man apt-config' afaict | 17:23 |
| sidnei | rogpeppe: i've validated the approach with smoser fwiw | 17:23 |
| rogpeppe | sidnei: it would be nice if he could have a look at the code, but if it's not possible, then i guess it'll be ok | 17:24 |
| sidnei | rogpeppe: +1'd in private | 17:30 |
| rogpeppe | sidnei: cool | 17:30 |
| sidnei | rogpeppe: there will be a new feature in cloud-init itself so that apt_https_proxy is a valid config option, then we can remove the 99proxy-extra hack when that's out there. | 17:31 |
| rogpeppe | sidnei: cool | 17:31 |
| rogpeppe | sidnei: just doing a last once-over | 17:31 |
| mattyw | I seem to get permission denied when sshing to a unit and cd'ing to /var/lib/juju/agents/<unit-name>/charm, anyone else seen this? < - juju 1.13.0 | 17:32 |
| rogpeppe | sidnei: i hope that apt doesn't sometimes print warnings to stderr :-) | 17:33 |
| sidnei | i could look at the source, but meh. it's generally not very verbose. | 17:33 |
| rogpeppe | sidnei: reviewed | 17:35 |
| sidnei | rogpeppe: when putting }) in the newline, should i end the last line with ',' as well? | 17:37 |
| rogpeppe | sidnei: yes | 17:37 |
| rogpeppe | sidnei: (you need to) | 17:37 |
| sidnei | do i need to lbox propose again or just rv-submit? | 17:39 |
| sidnei | oh, apt-config output is case sensitive | 17:42 |
| * sidnei looks for case-insensitive flags on go regex | 17:42 | |
| sidnei | cool fixed. | 17:46 |
| sidnei | rogpeppe: sent for landing | 17:57 |
| * rogpeppe smells the sweet smell of frying onions and garlic from downstairs | 18:20 | |
| rogpeppe | time to stop for the day :-) | 18:20 |
| mgz | how very british :) | 18:20 |
| rogpeppe | mgz: pea risotto. lots of peas from the garden, mmm. | 18:21 |
| mgz | sounds great | 18:21 |
| rogpeppe | mgz: smells great :-) | 18:21 |
| rogpeppe | g'night all | 18:22 |
| mgz | later rog | 18:22 |
| davecheney | jamespage: no, sorry, i do not know about pyjuju -> go juju upgrades | 23:35 |
| davecheney | at one level, we always said there is no upgrade path for pyjuju-> go juju environments | 23:35 |
| davecheney | i am not sure if that was your question | 23:35 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!