| thumper | davecheney: hmm, went to the gym before the LGTM | 00:34 |
|---|---|---|
| davecheney | shit happens | 00:36 |
| thumper | davecheney: changes being proposed now if you want to LGTM the actual review... | 00:44 |
| thumper | davecheney: just give it a minute | 00:44 |
| davecheney | changes look good, i trust ya | 00:45 |
| davecheney | it was only nit picking stuff | 00:45 |
| thumper | davecheney: so submitting now should pick the right reviewers? | 00:45 |
| thumper | davecheney: one time before it failed to do so | 00:45 |
| thumper | and had "R=" in the commit message | 00:45 |
| davecheney | thumper: looks like that thing is unreliable | 00:45 |
| davecheney | i don't think anyone cares | 00:45 |
| davecheney | or looks | 00:45 |
| davecheney | or both | 00:45 |
| * thumper cares | 00:45 | |
| thumper | kinda | 00:45 |
| davecheney | that makes one then | 00:45 |
| * thumper does the switch to trunk, merge and run tests prior to submitting | 00:46 | |
| thumper | :) | 00:46 |
| davecheney | thumper: so you switch to trunk then merge your branch before pushing ? | 00:47 |
| davecheney | i do the other way around | 00:47 |
| davecheney | i merge trunk into my branch, test, then push if it passes | 00:47 |
| thumper | if I switch to trunk then merge it, and it works, it is all good | 00:48 |
| thumper | then I revert, and switch and submit | 00:48 |
| thumper | kinda the same | 00:48 |
| davecheney | horses for courses | 00:48 |
| thumper | but my way avoids bringing in the extra merge | 00:48 |
| davecheney | meh, that isn't the slow part | 00:48 |
| thumper | heh, no | 00:48 |
| thumper | davecheney: if you have time, i'd love to chat about the upgrade process | 00:49 |
| thumper | as in 'juju upgrade' | 00:49 |
| thumper | do you know much about it? | 00:49 |
| davecheney | a little, dating back to august last year (2nd lisbon sprint) | 00:49 |
| davecheney | i don't think much has changed since then | 00:49 |
| davecheney | want me to grab moi headset ? | 00:50 |
| thumper | sure, 5 min? | 00:51 |
| thumper | as in, in 5 min? | 00:51 |
| davecheney | kk | 00:51 |
| thumper | hangout? | 00:54 |
| davecheney | sure | 00:58 |
| davecheney | lemmie try | 00:58 |
| wallyworld | thumper: davecheney: want the easiest review ever? +1/0 https://codereview.appspot.com/8064043 | 02:25 |
| jam | wallyworld: for your patch, the idea is that we should retry only for IsNotFound errors, but exit on everything else? | 03:44 |
| wallyworld | jam: yes | 03:44 |
| wallyworld | that's how ec2 does it too | 03:44 |
| jam | wallyworld: I would probably find it easier to read with inverted if logic | 03:44 |
| jam | if !errors.IsNotFound(err) { return err} | 03:45 |
| jam | otherwise LGTM | 03:45 |
| wallyworld | ok, i copied the logic style used by ec2 for consistency | 03:45 |
| jam | wallyworld: yeah, but consistency with *cough* is not necessarily worth keeping. | 03:45 |
| wallyworld | sure :-) | 03:45 |
| jam | i would be happy if you wanted to tweak the ec2 side as well | 03:46 |
| wallyworld | will do | 03:46 |
| wallyworld | jam: i reworked the debug log branch also, as per william's review, less code now | 03:46 |
| jam | wallyworld: to make sure I understand: https://codereview.appspot.com/7749048/patch/6001/7004 | 03:49 |
| jam | that change is because you can't use the nil constructor | 03:49 |
| jam | because 'sshCmd' is an interface. | 03:49 |
| jam | so it doesn't know which one to do. | 03:49 |
| jam | The only thing I might say is "could we just check for nil?" | 03:49 |
| wallyworld | jam: go doesn't allow default values in struct definitions sadly | 03:49 |
| jam | so Init does something like "if s.sshCmd == nil {sshCmd = XXX}" | 03:50 |
| jam | wallyworld: right, but you set it in Init if it isn't set yet. | 03:50 |
| wallyworld | jam: i did that, but SetFlags() is called before init, so the nil check needed to go there, hence i aborted that way of doing it because it wasn't obvious to the reader where the nil check needed to go | 03:50 |
| jam | anyway, I'm not to unhappy with just specifying it at register. | 03:50 |
| jam | wallyworld: yeah, I wondered about that. SetFlags vs Init... good enough for me. | 03:51 |
| wallyworld | ie the expected place Init() wasn't the right place at all | 03:51 |
| wallyworld | so best to be explicit | 03:51 |
| jam | you could hide it under a method call | 03:51 |
| jam | DebugLogCommand.sshCmd() which would do it. | 03:51 |
| wallyworld | i could, but then debug log would be the only command with a New() | 03:51 |
| jam | but yeah | 03:51 |
| wallyworld | hmm, i could have used a method like you suggest | 03:52 |
| jam | wallyworld: I was meaning a method on DebugLogCommand. But it is a fair amount of *stuff* just to avoid setting it for registration. | 03:52 |
| jam | wallyworld: so certainly, think about it, if you like it more than what you have you can change it, but it seems very much 6-of-one half-dozen of another. | 03:52 |
| jam | Neither is clearly better. | 03:53 |
| wallyworld | yeah, agreed | 03:53 |
| wallyworld | it's landed now, so not really worth changing unless really needed | 03:53 |
| jam | wallyworld: anything left on your plate that hasn't landed? | 03:55 |
| jam | wallyworld: and did you get a chance to look at sync-log? | 03:55 |
| wallyworld | jam: writing tests for the invalid region stuff. will propose soon | 03:56 |
| wallyworld | remind me, what was sync-log again? | 03:56 |
| jam | wallyworld: sorry not sync-log, sync-tools | 04:02 |
| jam | I was mixing my work with yours | 04:02 |
| jam | it is "get the official posted binaries, upload them to my local space" | 04:02 |
| wallyworld | jam: ah, balls, i haven't looked yet. sorry. will do it now. i was going to do it before you came on line and clearly i didn't expect you to log on till later :-) | 04:04 |
| jam | wallyworld: I'm not officially here for another hour at least | 04:04 |
| wallyworld | that's what i thought :-) | 04:05 |
| wallyworld | jam: i made the change to that branch without first seeing your comment. you happy with my version? | 05:32 |
| jam | wallyworld: change to which branch? | 05:32 |
| wallyworld | sorry, the retries one | 05:32 |
| wallyworld | https://codereview.appspot.com/8064043/ | 05:33 |
| jam | wallyworld: yeah, break is fine vs return | 05:34 |
| wallyworld | cool, landing now | 05:34 |
| wallyworld | i've pushed my other branch with the error messages, so might even get to land it tonight | 05:34 |
| wallyworld | jam: i thought about the \n, but the common convention *seems* to be just a long string. i'll tweak it and see how it turns out | 06:41 |
| jam | wallyworld: I like the amount of context, but it is a little hard to read | 06:41 |
| jam | our error formatting putting in a bunch of "caused-by" doesn't help either. | 06:41 |
| jam | wallyworld: as for convention, the convertion isn't to give super-helpful error messages with info for the user, either. | 06:41 |
| wallyworld | jam: i agree that it's hard to read for sure | 06:41 |
| jam | so I feel comfortable breaking convention here :) | 06:42 |
| wallyworld | jam: caused by messages should all come after a \n imho | 06:42 |
| jam | wallyworld: yeah, I'd probably agree with that | 06:43 |
| wallyworld | but i'll just do this one thing perhaps so i can land it before i go | 06:43 |
| jam | wallyworld: it isn't a blocker, your patch is better in than out, etc. | 06:43 |
| jam | but if you can tweak it, I think we can make it better. | 06:43 |
| wallyworld | that's what she said | 06:43 |
| jam | wallyworld: I'm pretty sure that is "in and out" . | 06:44 |
| wallyworld | lol | 06:44 |
| * wallyworld biab | 06:45 | |
| dimitern | morning all | 07:37 |
| rogpeppe | dimitern: hiya | 07:40 |
| dimitern | rogpeppe: yo | 07:41 |
| TheMue | *: morning | 07:46 |
| * thumper is back | 07:55 | |
| dimitern | can someone send me the HO link? | 07:56 |
| dimitern | i found it | 08:01 |
| thumper | fwereade_: I'm looking for something next week... | 09:07 |
| thumper | fwereade_: how about I take https://bugs.launchpad.net/juju-core/+bug/1131608 | 09:07 |
| _mup_ | Bug #1131608: deployed series is arbitrary <juju-core:New for fwereade> < https://launchpad.net/bugs/1131608 > | 09:07 |
| fwereade_ | thumper, hum, ok, let me think a sec -- and I *think* you've actually basically fixed that one | 09:07 |
| fwereade_ | thumper, modulo openstack | 09:08 |
| thumper | fwereade_: in fact, part 3 of that bug solution is done | 09:08 |
| thumper | fwereade_: ok, I'll let you think on it | 09:08 |
| thumper | fwereade_: and pick something on Tuesday next week | 09:08 |
| fwereade_ | thumper, yeah, I did 1 and 2, you did 3 | 09:08 |
| thumper | Monday is a public holiday here too | 09:08 |
| fwereade_ | thumper, I do have an idea | 09:08 |
| thumper | fwereade_: feel free to email me :) | 09:08 |
| * thumper is off now | 09:08 | |
| fwereade_ | thumper, which is to look into the cards surrounding machine status and reporting back of provisioning errors | 09:08 |
| thumper | 10 pm before 4 days off \o/ | 09:08 |
| fwereade_ | thumper, but I shall mail you | 09:08 |
| fwereade_ | thumper, enjoy your holiday :) | 09:09 |
| thumper | fwereade_: awesome, ta | 09:09 |
| dimitern | fwereade_: so now upgrade-charm is done (up to py-juju parity) what next? | 09:10 |
| dimitern | fwereade_: or can just pick some other card | 09:10 |
| fwereade_ | dimitern, it is probably best to pick some other card | 09:10 |
| fwereade_ | dimitern, trying to swarm is probably a good idea | 09:11 |
| fwereade_ | dimitern, since you have picked up some experience with state | 09:11 |
| dimitern | ok, any ideas where? | 09:11 |
| fwereade_ | dimitern, I might lean towards the "add machine status field" | 09:11 |
| fwereade_ | dimitern, which is in blue's TODO | 09:11 |
| dimitern | i'll take a look | 09:11 |
| rogpeppe | fwereade_: i'd appreciate it if you could take a look at the Relation embedded in Endpoint CL at some point, please. i *think* it worked out pretty well, but YMMV. https://codereview.appspot.com/8055044/ | 09:13 |
| fwereade_ | rogpeppe, sweet, I will take a look | 09:14 |
| rogpeppe | fwereade: ping | 10:24 |
| rogpeppe | fwereade: " | 10:24 |
| rogpeppe | Not sure what the distinction is between Limit and Optional -- they're equally | 10:24 |
| rogpeppe | ignored AFAICT, so we should probably include both or neither. | 10:25 |
| rogpeppe | " | 10:25 |
| rogpeppe | fwereade: we do include both AFAICS | 10:25 |
| fwereade | rogpeppe, ah, sorry, let me reread | 10:27 |
| fwereade | rogpeppe, yep, I'm an idiot, please disregard | 10:28 |
| rogpeppe | fwereade: np | 10:28 |
| dimitern | fwereade: what are the possible machine statuses? are they basically the same as machine agent statuses (as with unit statuses)? | 10:28 |
| fwereade | dimitern, hum, not necessarily | 10:36 |
| fwereade | dimitern, I don't think "installed" is meaningful | 10:36 |
| fwereade | dimitern, all the others might be | 10:36 |
| dimitern | fwereade: ok, thanks | 10:40 |
| dimitern | fwereade: so this comment is wrong? // UnitStatus represents the status of the unit agent. | 10:40 |
| fwereade | dimitern, well -- there is a fuzzy distinction between the "unit" and the "unit agent" | 10:45 |
| fwereade | dimitern, and there's an additional complication in that agent presence is conflated with unit status (see Unit.Status) | 10:46 |
| dimitern | fwereade: so it's better to say "UnitStatus represents the status of a unit." and similarly for a machine status | 10:46 |
| fwereade | dimitern, yeah, sgtm | 10:46 |
| fwereade | dimitern, btw, since you're hitting that | 10:47 |
| dimitern | fwereade: and specific status value have a relation to the agent? cf. UnitStarted UnitStatus = "started" // Agent is running properly | 10:47 |
| fwereade | dimitern, it would prbably be valuable, as a first step, to move the "down" stuff out of Unit.Status and put it in cmd/juju/status instead | 10:47 |
| fwereade | dimitern, not really | 10:47 |
| fwereade | dimitern, "Agent is running properly" applied to every status except pending and down | 10:48 |
| fwereade | dimitern, and down isn't really a status | 10:48 |
| dimitern | fwereade: i'm thinking of moving all of unit status in state/status.go | 10:48 |
| fwereade | dimitern, no firm opinion either way on that, follow your heart :) | 10:49 |
| dimitern | fwereade: cheers | 10:49 |
| dimitern | fwereade: and istm i'll just s/agent/unit in those comments | 10:49 |
| TheMue | lunchtime | 11:10 |
| jam | niemeyer: blog.labix.org is currently offline | 11:18 |
| niemeyer | jam: I noticed.. great timing | 11:18 |
| niemeyer | jam: The whole server is down an inaccessible | 11:18 |
| jam | that's a hosted site/ | 11:18 |
| jam | ? | 11:19 |
| niemeyer | jam: Yeah, dreamhost | 11:19 |
| niemeyer | Already filed a support request | 11:19 |
| jam | niemeyer: its back up | 11:23 |
| niemeyer | jam: Phew | 11:23 |
| niemeyer | jam: I'll wait a bit until making it public, to make sure it won't happen again | 11:24 |
| jam | wallyworld: are you aronud for standup? | 11:32 |
| dimitern | mgz: https://codereview.appspot.com/8051043/ | 11:40 |
| mgz | ta | 11:41 |
| rogpeppe | fwereade: " | 11:43 |
| rogpeppe | Let's keep that terminology out of charm, I think it's only meaningful at | 11:43 |
| rogpeppe | runtime. | 11:43 |
| rogpeppe | " | 11:43 |
| rogpeppe | fwereade: we already use the term "endpoint" in the RelationScope type doc | 11:43 |
| rogpeppe | fwereade: should i remove it from there too? | 11:43 |
| fwereade | rogpeppe, good point -- yes, I think so | 11:43 |
| fwereade | rogpeppe, does that make it unpleasant? | 11:44 |
| rogpeppe | fwereade: i'm don't *think* so. | 11:44 |
| fwereade | rogpeppe, cool, thanks | 11:45 |
| rogpeppe | fwereade: if we think of a "relation" as a single-ended thing | 11:45 |
| rogpeppe | fwereade: it's a pity we don't have two terms for "one end of a relation" and "the relation that joins these things together" | 11:45 |
| fwereade | rogpeppe, agreed, but the terminology seems to be reasonably well bedded in already | 11:46 |
| fwereade | rogpeppe, without too much confusion, hopefully | 11:46 |
| rogpeppe | fwereade: yeah | 11:47 |
| rogpeppe | fwereade: certainly in charm, there's no concept of the double-ended relation anyway | 11:47 |
| fwereade | rogpeppe, yeah, indeed | 11:47 |
| === benji___ is now known as benji | ||
| rogpeppe | fwereade: here's some preliminary work to make state/watcher amenable to a faster sync cycle: https://codereview.appspot.com/8078043/ | 12:31 |
| wallyworld | dimitern: jam: https://codereview.appspot.com/8079043 | 12:39 |
| wallyworld | sorry i missed the suggestions the first time | 12:39 |
| dimitern | wallyworld: no worries, it looks much better now - LGTM | 12:43 |
| dimitern | thanks | 12:44 |
| dimitern | fwereade: ping | 12:44 |
| wallyworld | thanks, i must have been smoking my crack pipe to have missed it, sorry | 12:44 |
| dimitern | ;) | 12:44 |
| jam | wallyworld: reviewed | 12:48 |
| wallyworld | thanks, looking | 12:48 |
| dimitern | jam: my point with using %s instead of %q was that ["compute, object-store"] looks uglier than [compute, object-store], and esp. [... one of these regions may be suitable instead: "reg1, reg2, reg3"] (ILTM as the region name suggested is "reg1, reg2, reg3" | 12:50 |
| wallyworld | jam: the wrapping with the error you pasted into the code review - i don't get that in my terminal | 12:51 |
| wallyworld | i get | 12:51 |
| wallyworld | error: failed to GET object provider-state from container juju-50548874395ac32d8896f65ab33f3e17 | 12:51 |
| wallyworld | caused by: cannot create service URLs | 12:51 |
| wallyworld | caused by: the configured region "region-a.geo-1" does not allow access to all required services, namely: compute, object-store | 12:51 |
| wallyworld | access to these services is missing: compute | 12:51 |
| wallyworld | one of these regions may be suitable instead: az-1.region-a.geo-1, az-2.region-a.geo-1, az-3.region-a.geo-1 | 12:51 |
| jam | wallyworld: that specific wrapping was probably codereview: http://paste.ubuntu.com/5655074/ | 12:51 |
| jam | right | 12:51 |
| jam | which is why it is 'uneven' | 12:52 |
| jam | Not worth worrying about. | 12:52 |
| wallyworld | yeah. at least each caused by is on a new line etc, so it's not tooooo hard to read | 12:52 |
| wallyworld | better than a long string | 12:52 |
| jam | certainly | 12:52 |
| jam | just not 'ideal' | 12:52 |
| jam | but good enough for now, I believe | 12:52 |
| wallyworld | certainly better than what it is replacing | 12:53 |
| === teknico is now known as teknico_away | ||
| jam | yep | 12:53 |
| jam | and quite helpful | 12:53 |
| wallyworld | let's see what users think :-) | 12:54 |
| * rogpeppe struggles with the crackful WatchUnits tests | 12:56 | |
| jam | dimitern: so, it appears that goamz.S3 requires an aws.Auth, and it *always* signs the request, even when your key is "". And that, in turn, is rejected by Amazon, even though the bucket is public (auth is checked first0. | 13:06 |
| jam | so I'm thinking just to add a way to supply ec2 creds, even though we "shouldn't" need them. | 13:06 |
| dimitern | jam: it looks like a bug in goamz | 13:07 |
| jam | dimitern: well, it is something to fix in the whole stack, which seems out of scope for the benefit. | 13:07 |
| dimitern | jam: but maybe it's worth a bug report at least to keep it in mind | 13:07 |
| jam | as you have to have a way to get creds to accept not having auth entries | 13:07 |
| jam | rogpeppe: I believe you can land: https://codereview.appspot.com/7925045/ | 13:19 |
| rogpeppe | jam: i believe you might be right | 13:19 |
| jam | I was going to LGTM, but its already been done | 13:19 |
| jam | rogpeppe: it seems like you might get the framing wrong if you don't disconnect. Is there an advantage of leaving the connection open? https://codereview.appspot.com/7518052/ | 13:23 |
| jam | it seems like while there might be a small one, it shouldn't be "normal operation" to get UnexpectedEOF | 13:23 |
| rogpeppe | jam: yeah, i should probably not return nil on UnexpectedEOF actually | 13:24 |
| rogpeppe | jam: the advantage of leaving the connection open is that the client gets some feedback on what went wrong. | 13:24 |
| jam | rogpeppe: can we write the response and then disconnect? | 13:25 |
| rogpeppe | jam: in the actual api, ReadRequestBody does not actually read anything | 13:25 |
| jam | I do think we want to respond | 13:25 |
| jam | I agree we want to respond | 13:25 |
| jam | My concern is just that if we get a partial read, then we might not know where the next message starts | 13:25 |
| rogpeppe | jam: i'm wondering about allowing the codec to return a special error code that signifies it's not a terminal error | 13:25 |
| rogpeppe | jam: we shouldn't get an error on a partial read | 13:26 |
| jam | rogpeppe: isn't that UnexpectedEOF? | 13:26 |
| rogpeppe | jam: in the UnexpectedEOF case, we do bomb out | 13:26 |
| rogpeppe | jam: the issue is other requests that may or may not signify a terminal error | 13:27 |
| rogpeppe | jam: the thing is that the codec *knows* whether it's a recoverable error or not | 13:28 |
| rogpeppe | jam: and when it is, i'd prefer not to drop the connection | 13:28 |
| jam | rogpeppe: sure | 13:28 |
| rogpeppe | jam: (this was a request from the GUI team, BTW) | 13:29 |
| jam | properly formed messages (even when they are telling you "I can't do that") don't have to close the conn | 13:29 |
| rogpeppe | jam: the problem is that a message can be syntactically correct (e,g. well-formed JSON) but have some problem with the param types (e.g. null instead of string) | 13:29 |
| jam | rogpeppe: sure. My concern is specifically about malformed messages, which seemed EOF related. | 13:30 |
| rogpeppe | jam: the behaviour at EOF and UnexpectedEOF is still pretty much what it was | 13:30 |
| rogpeppe | jam: i'm more concerned about issues like spurious closing braces | 13:31 |
| jam | rogpeppe: well, I'd be fine closing a conn if you had a spurious closing brace. | 13:31 |
| jam | If you're sending stuff that badly formatted, you can connect again. | 13:31 |
| rogpeppe | jam: yeah, but this change might make it so that doesn't happen | 13:32 |
| rogpeppe | jam: *if* ReadRequestBody actually read some data | 13:32 |
| rogpeppe | jam: (which it doesn't, except in the rpc tests) | 13:32 |
| rogpeppe | jam: my thought is that if such a thing happens, and the connection really is borked, then the next read-header will deal with the issue | 13:33 |
| jam | rogpeppe: as long as it doesn't become some sort of security hole (user-data getting interpreted as request data, etc) but I think the change is fine. | 13:34 |
| rogpeppe | jam: i can't currently see any problem with doing that | 13:34 |
| jam | Just having the conversation about it. | 13:34 |
| rogpeppe | jam: yeah, it's a good conversation to have | 13:34 |
| rogpeppe | jam: i'm glad you brought it up, because it's the central possible issue in this change | 13:34 |
| rogpeppe | jam: if the user data could force a syntax error in the framing data, then i think the client is broken. | 13:35 |
| jam | rogpeppe: sure, though you can always have a custom client that lets you send broken requests. | 13:37 |
| jam | HTTP apis aren't sacred :) | 13:37 |
| jam | anyway, | 13:37 |
| jam | that is a bit remote | 13:37 |
| rogpeppe | jam: that's true, but if that's true, then they can make up requests as they need - no need to leverage this method of getting user data to be interpreted as request data | 13:38 |
| jam | yeah | 13:38 |
| rogpeppe | s/that's true/they want to/ | 13:38 |
| rogpeppe | second one | 13:38 |
| dimitern | anybody seen "Modifiers and non-modifiers cannot be mixed" from mgo? tried searching whole or parts of it in all go code, including mgo, but couldn't find anything | 13:38 |
| rogpeppe | dimitern: sounds like it might be an error from mongod itself | 13:39 |
| rogpeppe | dimitern: yeah: ./src/mongo/db/ops/update.cpp:40 | 13:39 |
| dimitern | rogpeppe: yeah, seems so. hmm weird.. | 13:39 |
| rogpeppe | omg c++ | 13:40 |
| rogpeppe | dimitern: http://paste.ubuntu.com/5655182/ | 13:41 |
| rogpeppe | dimitern: "e.eoo()" ?! | 13:41 |
| dimitern | rogpeppe: what's e.eoo() ? | 13:41 |
| dimitern | :) | 13:41 |
| rogpeppe | dimitern: my question exactly | 13:42 |
| rogpeppe | dimitern: EOO == "end of object", i think | 13:42 |
| rogpeppe | dimitern: ah, you're mixing "$x" with "x" | 13:43 |
| rogpeppe | dimitern: the crucial bit is: e.fieldName()[ 0 ] != '$' | 13:43 |
| dimitern | rogpeppe: I can't see how.. I dumped the doc and the key and there's no $ anywhere | 13:43 |
| rogpeppe | dimitern: have you dumped the entire mongo op? | 13:44 |
| dimitern | rogpeppe: doing it now | 13:45 |
| dimitern | >>>> trying to set status of "u#wordpress/0" to &state.unitStatusDoc{Status:"error", StatusInfo:"test-hook failed"}; txn.Op{C:"statuses", Id:"u#wordpress/0", Assert:"d+", Insert:interface {}(nil), Update:(*state.unitStatusDoc)(0xf8401dd720), Remove:false} | 13:46 |
| dimitern | that's what I have printed just before | 13:46 |
| dimitern | rogpeppe: can I somehow get a log of what mongo is receiving? | 13:49 |
| rogpeppe | dimitern: i'd probably put a printf somewhere in mgo | 13:50 |
| dimitern | rogpeppe: good idea | 13:50 |
| rogpeppe | dimitern: are you getting this when running a transaction? | 13:50 |
| dimitern | rogpeppe: yes | 13:50 |
| dimitern | rogpeppe: insert is fine, update fails | 13:50 |
| dimitern | rogpeppe: in the same collection | 13:51 |
| rogpeppe | dimitern: what does the dump of the whole transaction ops look like? | 13:51 |
| dimitern | rogpeppe: I haven't changed anything else, just added a single op to an exiting transaction | 13:51 |
| rogpeppe | dimitern: sure, but i'd still like to see it | 13:51 |
| dimitern | rogpeppe: just a sec | 13:52 |
| dimitern | rogpeppe: http://paste.ubuntu.com/5655208/ | 13:54 |
| rogpeppe | dimitern: looks fishy to me | 13:55 |
| rogpeppe | dimitern: what's that Name:"$ne" doing in there? | 13:55 |
| dimitern | rogpeppe: I think I start to get it - Update: someinterface is not the same as wrapping it up in a D{{...}} | 13:55 |
| dimitern | rogpeppe: that's notDeadDoc assert | 13:56 |
| dimitern | rogpeppe: ah! | 13:56 |
| dimitern | rogpeppe: got it | 13:56 |
| rogpeppe | dimitern: cool | 13:56 |
| rogpeppe | dimitern: see, told you it was worth dumping in total :-) | 13:56 |
| dimitern | rogpeppe: or maybe not.. | 13:56 |
| === wedgwood_away is now known as wedgwood | ||
| dimitern | rogpeppe: so it used to be {C:u.st.units.Name, Id: u.doc.Name, Assert: notDeadDoc, Update: D{{"status", "somestatus"}}} and I removed the update but wanted to keep the assert | 13:58 |
| rogpeppe | dimitern: there's still an Update in there | 13:58 |
| rogpeppe | dimitern: which looks possibly bogus | 13:58 |
| dimitern | rogpeppe: yeah, but the status is moved into a separate collection where's the update now | 13:58 |
| rogpeppe | dimitern: all the other Update instances have D{{...}} but this just has a unitStatusDoc | 13:59 |
| dimitern | rogpeppe: well, unitStatusDoc is interface{} to the setStatusOp (opaque) | 13:59 |
| dimitern | rogpeppe: and I need to insert/update whatever's there | 14:00 |
| dimitern | rogpeppe: should I wrap it up in a D{{something}}? | 14:00 |
| rogpeppe | dimitern: what's the type of unitStatusDoc ? | 14:00 |
| dimitern | rogpeppe: unitStatusDoc is the type - struct {Status, StatusInfo string} | 14:01 |
| rogpeppe | dimitern: BTW, i recommend dumping with spew.Dump - it formats it nicely and shows all depths | 14:01 |
| rogpeppe | dimitern: that doesn't look like something you can pass directly to Update | 14:01 |
| dimitern | rogpeppe: actually no, there are 2 doc types: unitStatusDoc and machineStatusDoc - both have 2 fields (+ the implicit _id) and one has Status UnitStatus, the other Status MachineStatus (both are type *Status string) | 14:02 |
| rogpeppe | dimitern: i think perhaps you want D{{"$set", unitStatusDoc}} | 14:02 |
| dimitern | rogpeppe: ah, ok I'll try this 10x | 14:03 |
| dimitern | rogpeppe: it worked! tyvm | 14:04 |
| dimitern | fwereade: ping | 14:32 |
| dimitern | fwereade: I prefer not to change annotations and constraints in this CL to use the globalKeyer, if you don't mind | 14:42 |
| dimitern | and there it is - https://codereview.appspot.com/8087043/ | 14:44 |
| fwereade | dimitern, heyhey | 15:21 |
| dimitern | fwereade: hey, it's basically done - not sure about machine status logic; can you take a look please? | 15:22 |
| dimitern | fwereade: all tests pass, it seems none of the uniter ones depend on unit changes on status | 15:22 |
| fwereade | dimitern, there are a few things to talk about, not necessarily line-focused; might make sense to do so in a hangout? | 15:23 |
| dimitern | fwereade: sure, I'll start one | 15:23 |
| dimitern | fwereade: https://plus.google.com/hangouts/_/555b884bebd0c196b62a3b05ec40cb1ba95a5c19?authuser=0&hl=en | 15:24 |
| Makyo | Can I request another reviewer for https://codereview.appspot.com/8018043/ ? It's big, but mostly just a name refactor. | 15:34 |
| jam | Makyo: is it possible to split it into *just* a name refactor and then another patch for logic changes, that is usually a lot easier to review, though I'll still give this one a look if you want | 16:05 |
| jam | otherwise each line I have to read carefully to see if there is more than just a mechanical change | 16:06 |
| jam | though to be fair, everything so far has been only mechanical renames | 16:07 |
| Makyo | jam, not easily, but yes, it's at least 95% mechanical, with the only logic changes being to get rid of entityNamer/namedEntity interfaces in state tests, as those are now available through the Tagger interface in state/state.go | 16:09 |
| jam | Makyo: can you point out the specific files, so I can be careful when looking at them | 16:09 |
| jam | state/state.go and state/?_test.go ? | 16:09 |
| Makyo | jam, yes, let me see if I can narrow it down further. | 16:10 |
| jam | Makyo: so one place here: https://codereview.appspot.com/8018043/patch/12001/13026 | 16:10 |
| jam | you rename entityId => tag | 16:10 |
| jam | where everything else was "entityName" | 16:10 |
| jam | is Id something different? | 16:10 |
| Makyo | jam, no, same as entityName | 16:10 |
| jam | Makyo: so I think you mean here: https://codereview.appspot.com/8018043/patch/12001/13030 | 16:12 |
| jam | where you got rid of the namedEntity interface | 16:13 |
| jam | for a public state.Tagger interfac | 16:13 |
| jam | (presumably providing the logically same interface) | 16:13 |
| Makyo | jam, state/assign_test.go, state/apiserver/api_test.go, state/state_test.go | 16:13 |
| Makyo | jam, yep. | 16:13 |
| jam | reviewed | 16:20 |
| Makyo | jam, Thank you :) | 16:20 |
| rogpeppe | gosh that was a hard bug to find | 17:18 |
| rogpeppe | eod. am off now. happy weekends all. see y'all on tuesday. | 18:11 |
| === wedgwood is now known as wedgwood_away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!