[02:08] huwshimi: I'm going to powerdown, need anything before I do? [02:28] hatch: I think I'm good for the moment [02:28] thanks [02:32] okee have a good weekend [07:24] mornin' all [07:24] urulama: hiya [07:41] rogpeppe1: Morning [07:41] huwshimi: hiya [07:46] rogpeppe1: hi [07:46] rogpeppe1: trying to do the review, but get NMIs alll the time :) [07:46] urulama: :-) [07:47] urulama: you need to sort out your processor architecture [07:51] rogpeppe1: yes, a stone brick model looks good [08:08] rogpeppe1: i am still a bit puzzled about the UploadCharm/Bundle func and the need for it ... archive.go has it's own methods, which do not use Upload* functions but archive_test does ... [08:09] urulama: it's so that tests can create charms with known URLs, without going through the v4 API upload mechanism [08:10] urulama: (which will get considerably more complex if we add challenge-response stuff) [08:11] urulama: for example, all the tests in api_test.go use it [08:11] urulama: it means that those tests can be independent of the API upload logic [08:11] rogpeppe1: exactly, i'm just saying that tests are there and provide feeling that all is fine, but that might be false positives ... maybe noting in archive_test.go that, would be useful ... [08:12] rogpeppe1: all clear on why the funcs are there and why used, just saying, that we need to make sure that we do not end forgetting about proper archive tests [08:12] urulama: the only archive test that uses UploadCharm is TestArchiveGet [08:12] urulama: that's because it was written before the API upload code existed [08:13] urulama: it should probably be changed to use the API upload, i guess, as it's part of that suite [08:14] yes. that's what i was asking. [08:14] i'll make a note [08:14] urulama: ah, ok. you used plural tests, which i took to mean you thought all the archive tests suffered from that issue [08:15] rogpeppe1: sorry, my head is a bit of a jello after these weeks :) [08:15] urulama: np at all [08:15] urulama: hope it's all gone ok [08:15] rogpeppe1: extremely well, i think [08:17] urulama: good to hear [08:20] rogpeppe1: so, now that nothing uses store.UploadCharm and is not part of store_test.go ... could we move UploadCharm/Bundle to test code? [08:20] rogpeppe1: or do we expect those to be useful in the future? [08:20] urulama: api_test.go uses UploadCharm [08:20] urulama: that's in the v4 package [08:21] urulama: and i think it's useful for it to be independent of the API upload mechanism [08:22] urulama: as you can probably see from the code, UploadCharm uses almost the same code paths as AddCharm - it just creates the blob as well as adding the entry [08:26] rogpeppe1: ok, thinking about the names of the api. Code (functional point of view) is ok. ... Add* (adding just entry in db) and Upload* which is Add+blob ... feels redundant in a way ... not saying that it's wrong. Just feels redundant or that naming is wrong. [08:27] urulama: yeah, you have a good point there. [08:27] urulama: AddCharmWithArchive ? [08:27] better by all means [08:28] urulama: will change it [09:32] "I really don't like the magic "blobhash" undocumented string here." [09:32] urulama: did you delete that comment? [09:33] rogpeppe1: i did [09:33] urulama: ah, ok [09:38] rogpeppe1: we use .series in referece to indicate it's a bundle? [09:39] urulama: yes [09:40] rogpeppe1: wouldn't it be cleaner if there would be a .type attribure? [09:40] urulama: we discussed this quite a while ago [09:40] rogpeppe1: before my time or in my time? [09:40] urulama: hmm, perhaps a little before your time [09:41] rogpeppe1: ok, so it's an agreement with core? [09:41] urulama: yes [09:41] urulama: by using series, we have a nice way of representing the "bundle-ness" of a url in the url itself [09:42] ok. [09:42] fine [09:47] except for the flat namespce which could be either? [09:48] namespace [09:53] rick_h__: that's true. but we wouldn't be able to tell then anyway. it's just like resolving other aspects of the charm or bundle url [10:01] rogpeppe1: yea, nvm [10:09] rogpeppe1: how is it going? [10:09] frankban: just running the tests on the juju-core charm.v2 change before attempting to land it again... [10:10] frankban: and making requested changes to our charmstore PR [10:10] frankban: not bad altogether [10:10] frankban: you? [10:11] rogpeppe1: I have a branch ready for the serveArchiveFile endpoint. Waiting for changes on our charmstore branch, looking at the review [10:11] frankban: nice! [10:33] urulama: have you finished your review? [11:15] rogpeppe1: he ran to lunch [11:16] rogpeppe1: will poke him when he's back [11:16] rick_h__: np [11:16] rick_h__: i'm not really expecting much activity, just a faint hope :-) [11:30] frankban, jrwren, dimitern, TheMue: https://github.com/juju/blobstore/pull/12 [11:35] rogpeppe1, looking [11:35] dimitern: thanks [11:35] rogpeppe1, first impression: you need a better PR description :) [11:35] dimitern: the title doesn't count? [11:37] rogpeppe1, well, I'd like to see *why* it's needed maybe? [11:37] dimitern: ok, doing that [11:42] rogpeppe1, ta [11:42] rogpeppe1, you've got a review [11:42] dimitern: thanks! [11:45] * frankban lunches [11:48] "Yes, as a record in a Zombies collection." [11:48] urulama: i don't understand that comment, sorry. [11:58] rogpeppe1: oh, sorry, trying to be quick and on more than one place ... so. that's just a reminder for me. [11:58] urulama: ha ha [11:58] urulama: i see [11:59] rogpeppe1: what i had in mind is that todo should have implementation that stores sha of that blob into zombies collection (new one) maybe, so that cleanup can be easy [11:59] rogpeppe1: what do you think? [11:59] rogpeppe1: easieer that searching for non-referenced ones maybe [11:59] urulama: i think that if the blob removal fails, it's almost certainly because the mongodb connection has gone away, so we're just adding more complexity for little likely gain [12:00] urulama: because we're likely not to be ablr to add to the zombies collection either [12:00] urulama: searching for non-referenced ones is pretty straightforward actually [12:00] urulama: (i actually implemented it already elsewhere) [12:00] rogpeppe1: agree on all points. i'll remove the comments [12:01] urulama: cool [12:11] * urulama sees 400 lines of tests for archive ... goes into the "that code works, the test pass, all is good" lazy mode for a moment :) [12:30] urulama: :-) [12:58] frankban: the charm upload branch has now landed [12:58] frankban: next up: https://github.com/juju/charmstore/pull/63 [12:59] urulama, jrwren: https://github.com/juju/charmstore/pull/63 [12:59] * rogpeppe1 goes for some lunch [13:01] rogpeppe1: cool, looking [13:01] rogpeppe1, urulama, jrwren: https://github.com/juju/charmstore/pull/64 [13:02] frankban, rogpeppe1: 63 & 64 so soon :) [13:02] heh [13:35] morning [13:38] hatch: morning. Care to talk about https://bugs.launchpad.net/juju-gui/+bug/1342853 ? [13:40] oh I already added it into the comments i Guess heh [13:43] I don't have anything to add beyond that comment, did you have any questions? [13:43] Yeah… seems like I ought to be pursuing the band-aid fix for now, right? [13:47] yeah - I'm not entirely sure how that's going to work though because right now whwnever the ecs is updated it fires an event which the deployer bar listens for [13:47] so I'm not sure the best approach to debouce that so you can consolidate them [13:48] if (n % 2 === 0) { return; } [13:48] you could consolidate on the end on render - but the little notification that shows up, will say 1 unit added [13:48] hah what? [13:48] I'll just drop every other event ;-) [13:50] haha [13:55] hatch: i also did more playing around with left-aligned text in the charm yesterday. Posted my comments on the PR, but tldr is that I'm sticking with center-aligned for the time being. [13:55] yeah? You don't think it looks funny being truncated with all that extra space on the right? [13:57] There really isn't very much extra space. [13:58] You could squeeze in an extra character depending on what letters are used in the name [13:58] Some names are going to leave more space, others less. [13:59] For example, here's what worst case scenario looks like with the current truncation limit: http://cl.ly/image/462m1L1f3908 [14:00] This is more typical: http://cl.ly/image/003Z0w0p0K04 [14:01] And to me, it doesn't look like extra space, but rather balanced. If I didn't know better, I'd just assume that space on the right was reserved for some other indicator/icon [14:02] Aside from that, I can only add one more character to the "typical" name before it starts squeezing the indicator and charm padding. [14:02] http://cl.ly/image/2n37173w0J0T [14:03] oh wait a second, so the worst case scenario is still possible? [14:03] If they name their charm using the widest characters in a font, yes. [14:03] Possible, but unlikely. [14:03] rogpeppe1: could you take a look at https://github.com/juju/charmstore/pull/64 ? thanks [14:04] rogpeppe1, jrwren: also https://github.com/juju/charmstore/pull/65 (quick) [14:05] so that's not very good then [14:05] It's the worst way to truncate, except for all the others. [14:05] measuring the width using that d3 script didn't work? [14:05] it seems like that is the best way using a variable width font [14:06] frankban: nice catch. [14:06] hatch: it would be a very complicated solution. [14:07] The simple solution that works for 90% is better here. [14:08] You can't just calculate the width of the text, which the d3 plugin did… you also have to know the width of the container, which we don't until long after the text is added. [14:08] what makes it complicated? take the width of the icon, the width of the blue circle, add to the width of the text, if it's too long, truncate the width of the text - and loop [14:09] Aside from that, the text gets wrapped in the parens, so you'd have to unwrap those, truncate, and then re-wrap [14:09] and since it scales linearly you don't need to do it on scaling, only once on render [14:09] well not really, you'd just use the original value and truncate it [14:10] (some-valu… vs. (some-valu…) [14:10] I would just much rather do it the best way and not have to deal with any potential fall out from doing it the easy way [14:10] IMO this is the best way. [14:10] Simple wins. [14:11] It works just fine for all but the most extreme scenario. [14:11] hatch: I am running the charm functional tests in a precise env [14:12] frankban thanks - it fails in the legacy server tests [14:12] frankban did you merge in the develop trunk to the precise trunk? [14:12] hatch: oh, so that's the problem, right? [14:12] yeah it says that there isn't an environment connection [14:13] hatch: Look at it another way… assuming we did things by calculating the width, all that it would get us, in the case of "block-storage-broker" is (block-stor…) instead of (block-sto…) - and we'd have to maintain quite a bit more code for one more character. [14:13] hatch: there so need to do that, we have a trusty release, and the code is the same, we just need to push that into the precise branch (when the tests pass) [14:14] frankban right - but those tests do not pass when running on precise [14:14] hatch: ok, I'll see them fail [14:14] I tried taking the trusty code and deploying it via precise and the same failure [14:14] did you notice the commits/diffs? [14:14] hatch: no [14:15] kadams54 I'm not really worried about getting extra characters, I'm worried about the situation when a charm name spills into the icon [14:15] hatch: I presume the tests pass when using a trusty env, correct? [14:15] frankban correct [14:16] hatch: ok, so maybe there is a regression on the legacy server on the legacy platform with the legacy code... [14:17] lol! [14:17] hatch: If someone names their charm "mmmmmmmmmmmm", I can live with that happening. [14:17] that's just the pyjuju stuff right? [14:17] kadams54 but it's not even that - if you look at "hadoop-mapreduce" the truncated name touches the uncommitted icon [14:18] frankban: looking [14:21] kadams54 so yes truncating is working for the majority of the situations but it's not very resilient [14:21] even when things aren't crazy [14:21] and people have called their video games AAAaaaaaAAAaaaaa :D [14:21] so mmmmmmmm might not be too far off [14:23] hatch: you have an affinity for solutions that require architectural changes. [14:23] ;-) [14:23] man chrome in ubuntu really does not like rendering svg's properly [14:24] Chrome anywhere does not. Check out the clipping on the icon: http://cl.ly/image/0b0G3r2I3A2i [14:24] oops.... [14:25] kadams54 well I'd just rather not land something that could come back to bite us later because it doesn't go all the way [14:26] This is a pretty simple change, even more so because it consolidates logic (adding the parens) that had been scattered. [14:26] If it bites us, then we fix it. [14:26] I envision a bug "service names sometimes under icon" [14:26] The bit isn't going to be a big one. [14:27] and then we go "didn't we already fix that?" [14:27] That could be a generalized argument against incremental change [14:28] except this is not an incremental change, it's a partial fix [14:28] I don't think we are going to agree here :) [14:29] Probably not :-) [14:29] incremental change, partial fix… to-may-to, to-mah-to [14:30] ok add a note to the limitation to the PR and land it [14:30] One of the reasons for incremental change is that you don't invest a slew of time and code into fixing something the wrong way. [14:33] jujugui: need a second review/QA at https://github.com/juju/juju-gui/pull/480 [14:40] kadams54: I looked at it. [14:40] jrwren: thanks [14:43] frankban any luck? :) [14:44] hatch: I saw the legacy test failing, trying to deploy it manually [14:44] ok thanks - when I deployed it manually it seemed to work just fine [14:49] jujugui call in 11 kanban now [14:51] frankban I remember there was some discussion about deleting machines with services on them. ATM we can hit the trash can button on the machine but then committing fails because it has a service on it. Did we ever decide what the proper course of action was there? [14:56] hatch: I don't remember. it's worth asking luca: I'd expect something like a warning message somewhere (at least) [14:58] yeah will do, I could see a popup asking if they also want to remove the units on the machine, and if that leaves the service without machines....then ask if they want to remove the service as well [14:58] jujugui call in 2 [15:00] jujugui call now [15:07] rogpeppe1 you must love our quick standups :D [15:07] hatch: oh yes [15:07] i have become convinced that the entirety of durham has crap for internet this week. [15:07] hatch: the juju-core standups were often interminable [15:07] hatch: ISTM that the legacy server is broken [15:08] rogpeppe1 haha, hardly a standup then :D [15:08] hatch: indeed [15:08] frankban so is legacy server pyjuju? [15:08] I'm not really sure what legacy server even is [15:08] hatch: I see websocket errors in the js console: Error during WebSocket handshake: Unexpected response code: 404 [15:09] hatch: no, the legacy server is haproxy + apache2 [15:09] ohh [15:09] hatch: the regular server is the shiny python/tornado app, which always works ;-) [15:09] frankban ok well if you take the develop-trunk and merge it into the precise-trunk you'll see a massive diff - maybe something in there busted it - tbh I didn't think we made any charm changes since the last release... [15:10] lol oh u peeps and your pythons [15:11] heh [15:16] hatch: ugm, I don't see so many changes in the charm, mostly documentation and a new feature for testing locally (which seems broken, but it's not related to the current problem) [15:17] uhm even [15:17] but I don't even recall those docs being updated.... [15:17] I'm wondering if something got committed incorectly [15:17] incorrectly [15:18] hatch: I think the diff is sane, investigating on the ec2 machine about why websockets are broken [15:18] allllright [15:18] hatch: btw, yay functional tests, this is a real problem [15:18] when do people use the legacy server? [15:18] but yay! [15:21] bbiab: heading out for a run. [15:23] enjoy [15:34] hatch: jujucharms.com runs on the legacy server :-) [15:35] of course it does [15:35] lol [15:35] hatch: but would not hit this problem since it uses sandbox mode, so no real websockets [15:36] so....does any real user use it? I don't even think it's enabled by default [15:37] you'd have to explicitly turn it on, correct? [15:37] hatch: yes [15:38] I wonder if we couldn't just 'ignore' it [15:38] I mean, if it's going to be a ton of work to fix [15:38] hatch: I don't know about real users. we dropped support for the legacy server on trusty. we still need to support it on precise [15:38] ohh that's why it passes fine on trusty hah [15:38] hatch: I guess so [15:39] hatch: we skip that test on trusty [15:55] hatch: haproxy sends a 404 to websocket upgrade requests :-/ [15:55] whaaa since when [15:56] hatch: weird, the config file is unchanged from months, the haproxy version is the same... [15:56] yeah.... [15:57] hatch: and I can connect to the juju websocket from the gui machine [15:59] that is odd, so it's just haproxy that's causing the issue [16:13] hatch: this is the haproxy configuration, do you see anything smelling? [16:13] http://pastebin.ubuntu.com/7989894/ [16:13] looking [16:14] frankban don't we now use wss ? [16:14] I don't see a handler for wss only for ws [16:15] hatch: not sure I understood [16:15] use_backend juju if { path /ws }, we use the juju backend based on the request path [16:16] maybe I'm misreading it because I thought we use wss: protocol [16:17] hatch: we use secure websocket indeed [16:18] so does it need to redirect /wss or does the /ws handle that too? [16:18] I'm just thinking that might be why it's 404ing [16:18] grasping at straws really... [16:19] hatch: /ws is just a path, not a protocol, the GUI uses that path for the websocket [16:19] yeah ok [16:19] in that case, I see nothing wrong with this config [16:24] hatch: and this is the log of a single wss request: http://pastebin.ubuntu.com/7989981/ not really helpful [16:25] frankban right - it looks like the /ws is 404ing [16:25] but you said you could hit that directly right? [16:26] hatch: no, I said I can hit directly the juju API address, which does not use /ws. In the haproxy conf, we strip away /ws before proxying to juju [16:26] ohh [16:27] hatch: juju.srvrep at line 9 makes me think the juju backend is actually invoked [16:27] hmm - of course it doesn't show the request it's making [16:27] can you throw a log in there? [16:33] hatch: not sure [16:34] hatch: I am already running it in debug mode [16:41] hatch: ok it seems reqrep ^([^\ ]*)\ /ws/ \1\ / is not working [16:41] damn regex....of curse it's the regex that you can't read.... :D [16:41] but that regexp is the same from ages [16:46] yeah I know.... [16:47] I really have no idea [16:47] thats why I passed it to you in hopes that you will have better luck :D [16:49] hatch: it worked!!! [16:49] whaaaat, what did you change? [16:49] hatch: replacing that regexp with the magical words... [16:49] lol [16:49] reqrep ^GET\ /ws\ HTTP/1.1 GET\ /\ HTTP/1.1 [16:49] oh I'm totally at a loss as to why it failed NOW [16:49] hah [16:51] hatch: we should always ask why it failed... almost always... I am near my EOD, I can leave it to you if you want, otherwise I'll hack a patch on Monday [16:51] well I think you're having better luck :) [16:54] hatch: ok, I have a fix at least, will investigate further for the cause. is it possible that HTTP/1.1 was not sent before and it is now? [16:54] that's very odd... [16:54] but yeah at least there is a fix now [17:00] done for the day, have a great weekend all! [17:02] cya frankban have a good weekend [17:02] thanks for finding the issue [17:18] jujugui can someone remind me what a removed-but-uncommitted relation looks like? [17:21] Hmm… [17:21] hatch: just gave that a whirl and it looks the same as an un-removed relationship. [17:22] The change is listed in the change log, but there's no visual indicator on the relationship. Maybe something to bring up with design? [17:23] It has been and they gave an answer, I just can't seem to find it in my email or in the designs heh [17:24] Blue line with status notification [17:24] Found the e-mail [17:25] (I think) [17:25] Subject: Representing removed relations on the juju-gui-peeps list, Luca's reply was on July 1 [17:25] * rogpeppe1 is done for the week [17:25] g'night all [17:25] Now then, that said… when you have a moment I'd like to chat more on my card. [17:25] and happy weekends [17:26] ok so everything needs a 'deleted' flag in their model then [17:26] cya rogpeppe1 have a good one [17:26] rogpeppe1: ditto [17:26] yeah I got moments [17:26] standup? [17:26] (Friday room) [17:28] sure joining [19:18] jujugui: need reviews and QA on https://github.com/juju/juju-gui/pull/485 [19:18] hatch: ^ that's the deployer bar consolidation stuff [19:29] kadams54 on it [19:29] I've already placed my bets on which lines of code will get hatch comments ;-) [19:30] lol so why don't you change them then :P [19:30] Gotta keep you honest :-) [19:38] CI build seems like YUI didn't load on the IE10 run… *sigh* [19:42] kadams54 review done [19:42] did I do what you thought I was gona do? :) [19:45] No, I lose. [19:45] But your suggestion is good. [19:45] lol......yay? [19:45] ok now I'm wondering, what did you think I was gona do [19:46] say* [19:46] I thought for sure I'd get in trouble over the single letter variable names here: https://github.com/juju/juju-gui/pull/485/files#diff-e6baa739d19ae967f00d894884a065acR709 [19:47] Though I feel like I'm just giving this away for whoever the second reviewer is ;-) [19:48] haha - I was going to actually comment on moving the var declaration outside of the loop AND change the var name but I figured you had to re-write it based on my other comment anyways :P [19:49] I'm not against single var names when it's easy to tell what they are for [19:49] but everyone else is haha [19:50] I find that kind of funny because I picked it up from the pythonistas I knew. [19:51] I figure if the var doesn't follow a convention (like e for event with YUI code) or is used more than 3-5 lines from its declaration or is used with other single char vars then it should be renamed [19:51] ...I lead a simple life.... [19:51] lol [19:52] I thought this team was pro-single character variable names. [19:53] Maybe we have enough to swing the balance. [19:53] ;-) [19:57] haha [19:57] jrwren lol definitely no [19:58] jrwren: just use "e" everywhere for events. It's all good. [19:59] I'm even getting out voted over that one [20:00] no reason to pre-minify code really [20:00] Outvoted, out-shmoted. [20:00] Sure there is. Programmers are lazy. [20:01] I actually had no clue until the most recent IRC conversation. No one's objected to my use of "e" in code reviews. [20:01] right, because of my lobbying for the e convention for events [20:02] it's a YUI convention [20:02] I also use i, j, k for iterators [20:02] but don't do for loops often heh [20:04] I hope not. I miss that vim plugin that calls 1 length variable names an error. [20:05] you use multi char vars for iterators? [20:05] no. [20:05] what do you call them? loopCountOne? [20:05] lol oh ok [20:05] i,j,k are fine of course. [20:05] it didn't flag those. Only top level names. [20:05] I think python-mode was the vim module that did it. [20:06] oh intersting [20:45] hatch: I can reduce it from three loops to two, but can't reduce it any further than that. Updated code's been pushed. [20:47] kadams54 got it up somewheres? [20:48] Yeah, the PR's been updated. [20:48] ok looking [20:49] oh I see what you did, ok I had something else in mind [20:49] umm how to esssplain [20:51] ok I think I got it [20:51] will comment on the PR [20:54] kadams54 ok added comment to the PR [20:54] I hope I did a reasonable job explaining it :) [20:59] kadams54 using my suggested approach you'd also need to modify the template so feel free to tell me to shove-it and leave your code as is :) [20:59] the hacks are at least now contained to a single method [20:59] :)