15:04:24 <bcoca> #startmeeting ansible core irc public meeting
15:04:24 <zodbot> Meeting started Thu Jun 13 15:04:24 2019 UTC.
15:04:24 <zodbot> This meeting is logged and archived in a public location.
15:04:24 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:04:24 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
15:04:24 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
15:04:34 <sdoran> o/
15:04:40 <bcoca> #topic https://github.com/ansible/ansible/pull/57623
15:04:47 <bcoca> ^ any new arguments on this?
15:04:52 <nitzmahone> o/
15:05:18 <sdoran> I don't think so.
15:06:16 <bcoca> #topic https://github.com/ansible/ansible/issues/57138
15:06:51 <nitzmahone> -1
15:06:54 <bcoca> ^ user complains that 'end play' is not 'visual enough' , wanted all hosts/tasks failed from that point (i think most of us disagree on it)
15:07:13 <bcoca> but i do think he has a point (see full discussion) that we should have a way to reflect 'play ended badly' in stdout
15:07:29 <bcoca> rc already provides the status, but  ... non cli people ... won't realize that
15:07:31 <nitzmahone> Making fake failed masks the error. I'd rather add a message that execution stopped because of AEF
15:07:48 <bcoca> nitzmahone: ^ that is what my comment (scroll down) proposes
15:07:57 <tchernomax> 57623: no new argument for my part
15:08:32 <bcoca> but i looked at code and we need to change either stats (add rc so callback can decide on message) or add new callback event
15:09:15 <nitzmahone> Maybe some extra context to the play end event? New event seems like overkill...
15:09:36 <nitzmahone> "terminated_abnormally" or something
15:09:55 <bcoca> adding new param to stat event is an issue with existing playbooks, but adding new data to .stats itself should not
15:10:02 <bcoca> s/playbooks/callback plugins/
15:10:59 <nitzmahone> Yeah, seems like we did be able to hang it off one of the things we're already passing
15:11:06 <nitzmahone> *should
15:11:19 <bcoca> set_stats action already allows user to 'hang' arbitrary data to playbook.stats
15:11:31 <bcoca> so though an 'inernal' _ansible_rc would be easiest way
15:12:02 <bcoca> then deal with msg 'playbook failed due to : ' % explain[pb.stats._ansbile_rc]
15:12:57 <bcoca> if that makes sense to all?
15:13:10 <nitzmahone> rc math is not fun (HRESULT anyone?), but I'm sure we can find a place to smuggle it
15:13:34 <bcoca> right now we have 'incorrect' comment in bin/ansible to explain rcs ...
15:13:42 <bcoca> would be nice to have explicit doc of it
15:14:17 <nitzmahone> Anyway, sounds like we're agreed on the big picture approach (error message, no fake stats)
15:14:42 <bcoca> anyone else want to weigh in?
15:15:22 <bcoca> #topic open floor
15:15:33 <nitzmahone> mind the gap
15:16:16 <bcoca> ^ there is disco in ibiza where the floor opens into a pool ... always fun at 4am watching all drunk people fall in
15:16:35 <tchernomax> can we get back to https://github.com/ansible/ansible/pull/57623 ?
15:16:58 <bcoca> tchernomax: we can, but i thnk we lack enough people and new arguments, so was going to leave for tuesday
15:17:19 <tchernomax> bcoca: ok
15:17:41 <shertel> I'd rather a new filter (but share code) because it seems like adding this into the existing filter adds overly specific behavior to a generic, generally used filter.
15:18:10 <bcoca> that is kind of my issue of creating a new 'generically named' filter that does almost same thing
15:18:23 <bcoca> its already hard for people to find 'combine'
15:19:22 <agaffney> how is that different/better than `list1 + list2`?
15:19:30 <bcoca> i find it easier to have options and explain those in a filter than try to judge 'which filter i want'
15:19:54 <bcoca> agaffney: its  { key: [1,2,3]} + { key: [2,4,6] }
15:20:01 <bcoca> combine works on dicts
15:20:21 <agaffney> oh, list values within the dict
15:20:27 <bcoca> but when it finds 'lists' it just overwrites, they want to have control on exact behaviour (overwrite, addition, uniqueness)
15:21:22 <agaffney> adding a param to control that behavior makes sense
15:22:00 <bcoca> options were a) add param, b) create new filter c) reject ( c got 0  votes, the others were tied)
15:22:22 <bcoca> PR current implementation was rejected, but feature idea was generally accepted
15:22:37 <tchernomax> bcoca: I think I will create 2 PR (*if I have the time*) and propose them tuesday, it will be easyer to discuss actual code
15:22:51 <bcoca> works4me
15:22:57 <tchernomax> 1 for a) and 1 for b)
15:23:12 * nitzmahone leans a
15:23:28 <nitzmahone> But doesn't have seeing feelings either way
15:23:37 <nitzmahone> *strong
15:23:50 <bcoca> my feelings only see at night
15:24:57 * nitzmahone needs to stop phone walking
15:25:08 <bcoca> typing?
15:25:23 <nitzmahone> Typing on phone while walking
15:25:26 * bcoca should not throw stones since he 'phone types' on full keyboards ...
15:25:35 <shertel> lol
15:25:54 <bcoca> while sitting down
15:26:17 <sivel> damn, I suck
15:26:35 <sivel> I feel like I just saw the notification a few minutes ago. Not 30 minutes ago
15:26:54 <sivel> I am still (b)
15:27:22 <sivel> adding a new param is going to make the current code more complex, and more difficult to maintain
15:27:42 <sdoran> b, for the same reasons
15:27:44 <bcoca> i would argue complexity is the same, just code organization is different
15:27:53 <sivel> espeically because we have to be aware of the original functionality
15:27:57 <bcoca> and i prefer to have complexity in code than user confused between combine and combine_deep
15:28:14 <tchernomax> (I also still in favor of b)
15:28:16 <shertel> yes, what sivel said
15:28:35 <nitzmahone> If we're going to have N versions of the filter for all the permutations, I'd agree, but if it's just going to be a new one with switches, I'd vote to add the switches to the existing
15:28:35 <sivel> I'm saying added complexity, because we have to ensure that we don't deviate from the original intent, while adding much more complex functionality
15:28:41 <bcoca> adding the feature itself is the increase in code complexity
15:29:29 <bcoca> i dont see how to avoid it, other than code duplication, which we dont want (talk aobut harder maintenance) , so we are left with refactoring the common code into common function
15:29:44 <bcoca> if you are worreid about keeping original itnent ... that creates the most obfuscation
15:30:25 <sivel> If I were doing this myself right now, I'd leave the original untouched, as I don't really like the implementation anyway, and start combine_deep as green field
15:30:30 <bcoca> i dont see how to keep code simplicity when adding this feature, only interface simplicity
15:30:51 <bcoca> sivel: so dupe code to do mostly same thing? .. if you are going to do that, fix original
15:31:10 <sivel> I have no idea how duplicated it would be, as I indicated, the last I looked at combine, I hated the code
15:31:22 <sivel> so I'd just ignore it, and do a better job on the new function
15:31:31 <sivel> as to not have any chance of changing current behaviors
15:31:33 <bcoca> s/duplicate code/duplicate functionality/
15:32:01 <sivel> sure, why not? And we could deprecate the original at some future point
15:32:19 <sivel> I have no affinity towards existing code and functionality
15:32:21 <bcoca> so you really just want new reimplemetnation
15:32:39 <bcoca> which goes a lot farther than original request (which just modified current behaviour)
15:33:03 <sivel> I am just speaking towards what I would do, if I were the one implementing this functionality
15:33:46 <sivel> I personally have no problems with duplicating code/work/functionality to create something better
15:33:59 <bcoca> i do cause it confuses the consumers, i would just replace it
15:34:20 <sivel> in any case, you have my vote, I figured I'd try to justify my reasoning, in case it swayed others
15:34:49 <bcoca> i just dont see your reasoning matching your vote, you are not voting for 'copy + expand' you want 'reimplement'
15:35:09 <sivel> I voted for a new filter
15:35:36 <bcoca> yeah, but the new filter was posed as copy current + expand function, not reimplementation
15:36:01 <bcoca> if you really want to vote for reimplementation, i would say that is a diff case
15:36:25 <bcoca> tchernomax: ^ unless that is what you want to tackle in your PR
15:37:01 <tchernomax> I don't know yet how I will code that
15:37:23 <sivel> but I would imagine, if one were not tied to an existing implementation, it may be easier to achieve a more robust goal
15:37:38 <bcoca> before we had talk about copying code, shertel said to 'refactor into common function', but sivel now brings 'reimpliement from scratch' to the table
15:37:38 <sivel> at least that is how I tend to approach these types of things
15:37:50 <shertel> I think that was just me though
15:38:17 <bcoca> shertel: but if we take that route, i would agree, refactor made more sense, i dont know that anyone had thought 'reimplement from scratch'
15:38:29 <shertel> I don't have a strong opinion, really. Just probably wouldn't overload the current filter if I chose.
15:38:44 <bcoca> sivel: to be fair i also dislike current code, but was not looking at new filter as reimplementation
15:39:30 <bcoca> in my case, im looking it as user perspective 'combine' is how you merge dicts, having options on how to fine tune that merge seems better to me than having X diff filters to combine dicts
15:39:53 <bcoca> how the code underneath looks, i care a lot less
15:41:26 <bcoca> from the experience with nested/toghether/cartesian debacle, seems people are more willing to find the 'general tool' then deal with fined tuned settings in that
15:41:28 <cyberpear> rewrite from scratch then rename once proven seems like a good approach to me, if someone has time to do the rewrite
15:41:30 <bcoca> vs having to compare each tool
15:41:46 <bcoca> cyberpear: at that poitn i would not rename, just substitute
15:42:00 <bcoca> unless we go to merge_dicts .. which is more explicit, then i would deprecate combine
15:42:17 <bcoca> my main issue with 'combine' is that its not clear on what it does
15:42:28 <bcoca> having more 'unclear filters' ....
15:56:03 <shertel> We don't really have good tests for combine. If another option was added to the current filter I'd like to see those improved.
15:57:56 <bcoca> there is a reason python itself avoids having a deep_merge ...
15:58:31 <bcoca> k, going to postpone till tuesday, at least we got some new arguments, though i would say they are for 3rd option more than for existing b one
15:58:41 <bcoca> #endmeeting