| wallyworld | veebers: i made some comments on the PR, we probably need to chat about them. let me know if you have a minute at some stage | 01:18 |
|---|---|---|
| veebers | wallyworld: ack, in process of removing the suggested deps. | 01:18 |
| veebers | wallyworld: I have time now if that suits | 01:18 |
| wallyworld | ok, let's | 01:19 |
| wallyworld | babbageclunk: the tracker workers need to offer the full Environ or CAAS Broker as these are used in several places and the downstream workers need the full interface | 01:57 |
| babbageclunk | wallyworld: oh, sure, but they could have another clause in their type-switches for CloudDestroyer | 01:57 |
| wallyworld | babbageclunk: i'm not sure I follow. Those tracker workers (originally just the one - EnvironTracker) are to provide the full Environ to downstream, and the guarantee is that any config changes will be reflected in the supplied Environ. Narrowing the type at the Tracker output is the wrong thing to do IMO. Bt maybe I'm missing something. If it's easier to talk 1:1 we can do that? | 02:00 |
| babbageclunk | yeah, lets | 02:01 |
| wallyworld | 1:1 | 02:01 |
| thumper | OMG... | 02:47 |
| thumper | found it | 02:47 |
| thumper | was digging through the code looking for how we get ports assigned for tests. | 02:48 |
| thumper | a lot of digging | 02:48 |
| thumper | hmm... | 02:56 |
| thumper | while part of me thinks we can wrap this... another part says I'm just going to be introducing a race condition | 02:57 |
| thumper | not one we care about, but one that will trigger the race detector | 02:57 |
| thumper | ffs | 02:57 |
| wallyworld | babbageclunk: i pushed the change we discussed but sadly testing fails.... juju.worker.dependency "undertaker" manifold worker returned unexpected error: expected *environs.Environ, got *undertaker.cloudDestroyer | 03:12 |
| wallyworld | so it seems the Get() does strict type checking | 03:13 |
| wallyworld | so i think we need to use the original approach | 03:13 |
| babbageclunk | wallyworld: No, you just need to add a clause to the manifold output funcs for CloudDestroyer | 03:15 |
| babbageclunk | in the environ tracker and broker | 03:15 |
| babbageclunk | wallyworld: here https://github.com/juju/juju/blob/develop/worker/environ/manifold.go#L58 | 03:16 |
| babbageclunk | and here https://github.com/juju/juju/blob/develop/worker/caasbroker/manifold.go#L58 | 03:17 |
| wallyworld | ah i see, so support both clouddestroyer and environ | 03:17 |
| babbageclunk | yup | 03:18 |
| thumper | wallyworld: the test port change is going to have to be a change in the agent itself | 03:25 |
| thumper | otherwise there are races setting the config and getting other workers to notice | 03:25 |
| thumper | so won't be doing it today... | 03:25 |
| wallyworld | ok | 03:25 |
| thumper | it is possible, just icky | 03:25 |
| wallyworld | babbageclunk: changes pushed. manually tested on both caas and iaas models | 03:39 |
| babbageclunk | cool - looking now | 03:40 |
| babbageclunk | wallyworld: approved | 03:43 |
| wallyworld | yay | 03:43 |
| wallyworld | ty | 03:43 |
| === akhavr1 is now known as akhavr | ||
| === akhavr1 is now known as akhavr | ||
| jam | while reviewing other code, I found a bunch of panic() calls in normal code paths. Anyone care to review https://github.com/juju/juju/pull/8510 | 08:03 |
| jam | veebers: balloons: ^^ somehow we have both go vet and go fmt issues on that branch. Did we stop calling scripts/verify.bash as part of the pre-commit check ? | 08:04 |
| === frankban|afk is now known as frankban | ||
| manadart | jam: Looking at #8510 now. Just pushed to #8501 with sanitised address scenarios. | 09:20 |
| === akhavr1 is now known as akhavr | ||
| === salmankhan1 is now known as salmankhan | ||
| === akhavr1 is now known as akhavr | ||
| balloons | jam, Chris mentioned some needed changes to the check; but it was to ensure things didn't pass that shouldn't | 12:26 |
| balloons | Did you figure or | 12:26 |
| jam | balloons: I just wanted to note that I ran into it, I didn't dig in at all | 12:26 |
| jam | balloons: why is it that everytime I check ci-run, I see red, I click on it, and I see *lots* of green, but the one that matters is red.. :( | 12:29 |
| jam | :) | 12:29 |
| balloons | ;) | 12:35 |
| balloons | I like clicking the view tabs at the top. Unit tests are tantalizingly close | 12:35 |
| jam | balloons: we need a similar cleanup of $TEMP on Windows, thoughts? | 12:38 |
| jam | balloons: I'm guessing we don't have cygwin/msys there, right? | 12:38 |
| === akhavr1 is now known as akhavr | ||
| jam | balloons: i finally worked out: ssh developer-win-unit-tester powershell.exe 'Set-Location "$Env:TEMP"; Dir' | vim -R - | 12:50 |
| jam | and we have ~11k files in TEMP | 12:50 |
| balloons | jam, wow | 12:57 |
| balloons | jam, the intent was always to spawn a new instance, but never could get the experience right; where we could ssh in on a fresh instance | 12:58 |
| jam | balloons: well, we have a weird symlink or something in %TEMP%, but I've deleted 10k or so files | 13:00 |
| balloons | jam, windows unit tests are now having trouble it seems after running brilliantly for a long time | 13:18 |
| cmars | good morning. could someone review this metrics-related PR, https://github.com/juju/juju/pull/8484 ? | 14:22 |
| cmars | would be much appreciated :) | 14:23 |
| externalreality | manadart, really small https://github.com/juju/juju/pull/8512 | 14:29 |
| manadart | externalreality: Ack. Got 1:1 with balloons. Shouldn't be long. | 14:30 |
| externalreality | manadart, someone seems to have been making a effort to migrate all these retry attempts to juju/retry. I wonder if that is still a thing | 14:30 |
| balloons | jam, we use https://github.com/PowerShell/Win32-OpenSSH | 15:08 |
| balloons | no cygwin nonsense needed that way | 15:08 |
| manadart | externalreality: Approved. | 15:21 |
| cmars | hi manadart, are you the on call reviewer today? (Is that still a thing?) | 15:41 |
| externalreality | manadart, as far as testing for the upgrade implicit space names in 2.3 -> 2.4 - would you be able to provide more context around that. | 15:42 |
| externalreality | manadart, the upgrade step is pretty clear, but I can't tell what is missing from the trello card description. | 15:42 |
| balloons | cmars, there really isn't an on-call reviewer anymore. The Pr looks pretty straightforward though, just adding labels to the struct | 15:49 |
| manadart | externalreality: Want to jump on a hangout? | 15:51 |
| cmars | balloons: ok, yeah, that's it. Ive got a followup that adds the labels to 'juju metrics' output | 15:51 |
| balloons | cmars, I don't see a test for checking the error raised in the hook context | 15:51 |
| balloons | follow-up pr ready? | 15:54 |
| cory_fu | Is there a way to tell what feature flags are enabled for a controller / unit? | 15:57 |
| agprado | Question, has application Facade V6 landed? I see V5 as normal, and V6 with a CAAS flag. | 16:15 |
| agprado | I'm implementing `resolv --all` and I will need a new application Facade, but I trully trully don't want to jump from V5 to V7. That just looks wrong. | 16:16 |
| cmars | balloons: updated, is that what you had in mind? | 16:19 |
| balloons | cmars, looking | 16:20 |
| balloons | cmars, thank you. I approved. The PR displaying labels should be of more interest | 16:22 |
| cmars | balloons: thanks for the review. the followup's in progress, i'm basically just adding a labels field/right-most column to the output | 16:24 |
| hml | Is there an OpenStack version if this bug: https://bugs.launchpad.net/juju/+bug/1699930 - duplicate subnets cause spaces fail during bootstrap? | 16:26 |
| mup | Bug #1699930: Subnets are not associated with VPCs when using AWS <juju:Fix Released by wpk> <juju 2.2:Fix Released by wpk> <https://launchpad.net/bugs/1699930> | 16:26 |
| hml | i just can’t find the bug | 16:26 |
| balloons | cory_fu, I think there is -- presumably you've looked at show-controller and model details? | 16:29 |
| hml | foudn it | 16:29 |
| cory_fu | balloons: I have, and nothing | 16:30 |
| cory_fu | balloons: I also checked controller and model config, and didn't see anything there, either | 16:33 |
| cmars | balloons: could you review https://github.com/juju/utils/pull/298 or find someone to take a look? it's a prereq for displaying metric labels. | 16:55 |
| cmars | and sorting on them.. | 16:55 |
| jam | balloons: I realize no need for cygwin for ssh, but then I need to learn Powershell instead of just doing bash | 17:20 |
| balloons | jam, yea, powershell or cmd.exe | 17:33 |
| balloons | cory_fu, i guess perhaps the db, but then that may be more than desired. | 17:33 |
| === frankban is now known as frankban|afk | ||
| balloons | cmars, a slice isn't inherently unique -- what am I missing in your PR? | 20:18 |
| cmars | balloons: the source, a map, will have unique keys | 20:18 |
| cmars | balloons: it's probably academic at this point anyway.. it looks like juju/juju at develop's HEAD doesn't build with juju/utils current master | 20:20 |
| balloons | cmars, ahh sure | 20:21 |
| balloons | cmars, fixing that would be appreciated I'm sure | 20:21 |
| cmars | balloons: that's probably too heavy for me to lift, but i can ping rog about it | 20:22 |
| balloons | cmars, rogpeppe has a couple outstanding PR's I wish he would land already | 20:22 |
| cmars | balloons: meantime i'll use local private functions to join the string maps.. maybe that's safer anyway in the long run.. less chance of keyvalues.Join getting changed and breaking things elsewhere | 20:22 |
| cmars | sometimes a little code duplication isn't such a bad thing, when the intents are different | 20:23 |
| balloons | cmars, ok. But I was kind of reviewing with that in mind.. since you put it into utils, it should have secondary use | 20:23 |
| cmars | sorting vs rendering vs ... | 20:23 |
| cmars | yeah, maybe i should close it | 20:23 |
| balloons | right.. I'm ok with it | 20:23 |
| balloons | veebers, are you going to bring juju/utils up to date? | 20:27 |
| balloons | I'm actually a little surprised it's not | 20:27 |
| veebers | balloons: update juju/juju to use newest juju/utils? yes currently working on it (rolled into the task of moving to charmstore.v5 from -unstable) | 20:31 |
| balloons | cmars, so I guess it is part of the big update ^^ | 20:32 |
| balloons | veebers, ack, I didn't realize utils was caught up in it as well | 20:32 |
| cmars | balloons: yes, i think that's what i uncovered :) | 20:33 |
| veebers | balloons: fyi https://github.com/juju/juju/pull/8498 | 20:34 |
| wallyworld | thumper: did we wan tto chat about intrview? | 21:44 |
| thumper | yep | 21:44 |
| babbageclunk | thumper: hey, the failure we sometimes see where the API starts up is because we try to find a free port but when we try to open the API server listener it's been taken, is that right? | 22:52 |
| babbageclunk | thumper: so if I'm adding code to open a listener (which needs to be able to switch ports because it should be run in tests where 80 can't be bound), I should open the listener in the test and pass it in to avoid another case of that problem? | 22:53 |
| === salmankhan1 is now known as salmankhan | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!