[00:36] <wallyworld> vino: PR looks pretty good so far. left a comment about the AllBindings() method
[00:41] <vino> Sure thanks wallyworld
[00:42] <vino> i will get back to u soon.
[02:06] <kelvinliu> hi wallyworld, the mock Stdin pipe has been solved.
[02:22] <wallyworld> kelvinliu: that's great! i'll look at PR
[02:23] <kelvinliu> thx, wallyworld
[02:44] <wallyworld> kelvinliu: looking good, see my comments. happy to clarify if needed
[03:19] <kelvinliu> wallyworld, can i have ur 2mins ?
[03:19] <wallyworld> sure
[03:34] <veebers> wallyworld: you have a couple of moments, would like some pointers for the model tear down issue. I had a good destroy and a bad destroy and both exihibited the same logging (that I had tacked in there) So I'm missing something and/or looking in the wrong spot
[03:35] <wallyworld> veebers: just talking to kelvin, give me a sec
[03:35] <veebers> wallyworld: sure thing
=== jhebden-afk is now known as jhebden
[03:43] <anastasiamac> can i get a review for https://github.com/juju/juju/pull/8724, please? more status tests - this time for filtering on machine number :D
[03:43] <wallyworld> veebers: free now but i fear i won't have a good answer for you
[03:43] <wallyworld> anastasiamac: can look soon
[03:44] <anastasiamac> wallyworld: just as i was going to say "could u plz have a good answer for me" :D
[03:44] <veebers> wallyworld: hah, all good, even just pointing in maybe the right direction would be good
[03:44] <wallyworld> veebers: am inhangout
[03:44] <wallyworld> stabdup
[03:44] <veebers> omw
[04:02] <wallyworld> anastasiamac: a couple of small things
[04:07] <babbageclunk> so I've got a fix for that test hang but writing the explanation in the commit message has got me worried about a bug in the raft implementation
[04:07] <anastasiamac> best type of tests/comments :D
[04:07] <veebers> babbageclunk: remove the comment and voila bug is gone
[04:12] <babbageclunk> I'm gonna push the one-line fix with a shruggie in the commit message
[04:14] <kelvinliu> wallyworld, cluster-name and context-name in ~/.kube/config file are different, and we are using context-name to find related context from ClientConfig.Contexts. Do u think we should change the `context-name` flag to `cluster-name` and then we find the related context-name according to the cluster-name?
[04:15] <anastasiamac> wallyworld: ta \o/
[04:15] <wallyworld> kelvinliu: if it works. from a user perspective, i think they look at a named k8s setup as a cluster?
[04:17] <wallyworld> kelvinliu: are cluster names unique within a given config file?
[04:17] <wallyworld> or can different contexts have the same cluster name
[04:17] <wallyworld> if cluster names are not unique then we need to stick with context name
[04:20] <kelvinliu> i think context-name <- 1: 1 -> cluster-name, and they should be all uniq
[04:31] <wallyworld> ok, then i reckon cluster-name is better
[04:40] <kelvinliu> wallyworld, yeah, changing it to use cluster-name now
[04:40] <wallyworld> great, ty
[04:55] <babbageclunk> easy review anyone? https://github.com/juju/juju/pull/8725
[04:56] <babbageclunk> anastasiamac, veebers, thumper, wallyworld: ^
[04:57] <veebers> babbageclunk: looks
[04:57] <veebers> looking even
[04:57] <babbageclunk> ta
[04:58] <veebers> babbageclunk: I've asked a question for clarification. Im never sure to mark that as a 'Comment' or a 'Request changes'
[04:59] <babbageclunk> veebers: I'd say a comment unless you've specifically asked for a change.
[05:00] <veebers> ack, makes sense.
[05:00] <kelvinliu> wallyworld, done. would u mind take a look on the change?
[05:01] <wallyworld> kelvinliu: looking
[05:01] <kelvinliu> wallyworld, thx
[05:01] <wallyworld> anastasiamac: maybe if you have a chance, here's a CLI command to remove a k8s cloud (builds on remove cloud stuff er discussed) https://github.com/juju/juju/pull/8726
[05:01] <veebers> wallyworld: if I understood correctly, Application destroyOps should be called many times if any of the asserts fail, right?
[05:02] <wallyworld> veebers: the build loop will get called 3 times. depending on what happens in the txns, potentially new cleanup jobs will be queued which will call destroy again
[05:02] <anastasiamac> wallyworld: sure thing.. as soon as i finish this error msg m working on \o/
[05:03] <anastasiamac> like "on the tip of my tongue" kind of moment...
[05:03] <wallyworld> no rush need to land kelvin's one first and then rebase as there will be conflicts
[05:04] <babbageclunk> veebers: that's a raft library function - I guess we could have our own local DefaultConfig func somewhere...
[05:05] <veebers> babbageclunk: ah right, makes sense. Not sure it's worth it just to set False for that arg (for real and tests)
[05:05] <babbageclunk> yeah, I was hoping you'd agree :)
[05:06] <veebers> :-)
[05:06] <wallyworld> kelvinliu: done, just a couple of minor typos, let's fix and land :-)
[05:07] <veebers> wallyworld: ok, so I see the logging I put in (a *Application) destroyOps output only once, however I see the application fail to be destroyed. I'm continuing to add more debugging everywhere (well, maybe not everywhere)
[05:09] <kelvinliu> wallyworld, fixed
[05:11] <wallyworld> kelvinliu: go ahead and $$merge$$
[05:11] <kelvinliu> yup thx wallyworld
[05:11] <wallyworld> veebers: destroyOps will only get called again if it results in another cleanup job gettin queued which calls it
[05:12] <wallyworld> debugging should help us figure it out (eventually)
[05:12] <veebers> wallyworld: ah right. Ok, will continue w/ debugging logs :-)
[05:12] <wallyworld> no easy answer sadly
[05:13] <wallyworld> will also help to print what txns are being queued
[05:13] <wallyworld> potentially
[05:13] <veebers> wallyworld: ack, that's a good way to fill up a log file :-)
[05:32] <anastasiamac> wallyworld: if u have a chance, PTAL https://github.com/juju/juju/pull/8727?
[05:32]  * anastasiamac lloking at 8726 now
[05:32] <wallyworld> righto
[06:09] <wallyworld> anastasiamac: all of our state code returns jujutxn.ErrNoOperations if attempt > 0 and the entity is gone
[06:10] <wallyworld> the general pattern is to do a Refresh() and check for not found
[06:10] <wallyworld> cloud don't have refresh() so we just use Cloud(name)
[06:11] <wallyworld> eg see line 186 of state/application.go
[06:13] <anastasiamac> wallyworld: plz send a link, i dont know which version of application.go u r looking at :)
[06:14] <anastasiamac> wallyworld: most of the code does not fuss about and just returns NotFound... However, I do believe u want to cater for something specific, hence,u've coded for it but I do not understnad the reasoning... maybe it deserves a descriptive comment?
[06:15] <anastasiamac> wallyworld: specifically, why would it matter if another call deleted the entity? ur code above still should cater for both NOtFound and NoOp.. right?
[06:17] <anastasiamac> wallyworld: i guess what I'd like to see is a test that if you got a no-op in state, under the circumstances u've described, then ur api call will not err and hence local store will still be cleaned up... even if someone else removed controller-side reference
[06:17] <wallyworld> anastasiamac: i added a remove race test. the application file is the one in state /home/ian/juju/go/src/github.com/juju/juju/state/application.go
[06:18] <wallyworld> tremove cloud behaves consistently as for removing app, machine etc
[06:18] <anastasiamac> wallyworld: :) i was hoping for a link on githu :) i understnad which ile u r talking about... i cannot guess its version, nor can navigate to what u've supplied ;)
[06:18] <wallyworld> this is on develop
[06:19] <wallyworld> https://github.com/juju/juju/blob/develop/state/application.go#L186
[06:20] <anastasiamac> yep
[06:23] <anastasiamac> awesome ! Tahnk you the race test :D
[06:23] <anastasiamac> thank*
=== frankban|afk is now known as frankban
=== disposable3 is now known as disposable2
=== salmankhan1 is now known as salmankhan
=== rogpeppe1 is now known as rogpeppe
[14:36] <manadart> externalreality: If you are inclined to review: https://github.com/juju/juju/pull/8730
=== frankban is now known as frankban|afk