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