19:04:38 <bcoca> #startmeeting ansible core irc public meeting
19:04:38 <zodbot> Meeting started Tue Jun 18 19:04:38 2019 UTC.
19:04:38 <zodbot> This meeting is logged and archived in a public location.
19:04:38 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:04:38 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:04:38 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
19:04:44 <sdoran> o/
19:04:51 <bcoca> #topic to param or to filter 'combine'
19:04:53 <tchernomax> o/
19:05:32 <tchernomax> I implement the b solution (add a param to the existing filter) : https://github.com/ansible/ansible/pull/57894
19:06:02 <tchernomax> I was satisfied with it … so I didn't implement the a) solution
19:06:20 <bcoca> that got rejected
19:06:30 <shertel> o/
19:06:39 <shertel> I thought we just postponed
19:06:45 <tchernomax> > that got rejected
19:06:56 <bcoca> pretty unnanimouslly
19:07:02 <tchernomax> what got rejected ?
19:07:03 <bcoca> do NOT overload recurse
19:07:45 <tchernomax> the a solution was to add another filter
19:07:58 <bcoca> 2 solutions 1 add new option, 2nd add new filter
19:08:02 <nitzmahone> o/
19:08:03 <bcoca> you said you would do both
19:08:12 <bcoca> otherwise we are still tied
19:08:20 <bcoca> k, going to skip topic then
19:08:23 <tchernomax> yes, I know
19:08:34 <bcoca> #topic https://github.com/ansible/ansible/pull/57174
19:08:50 <bcoca> ^ quick look, we have alt fix and this one introduces some things i don think we want, the other was simpler
19:09:16 <tchernomax> ok do you have a link to the other fix ?
19:09:30 <bcoca> looking for it .. but lots of systemd tickets ...
19:12:27 <tchernomax> also, do you have some examples of "things i don think we want" ?
19:13:36 <bcoca> the change to glob detection, you make the error message less informative
19:13:42 <bcoca> not sure why that change is needed at all
19:14:01 <bcoca> 368 +  .. why create soo many vars that are the same thing in the end?
19:14:13 <bcoca> the other fix just altered the commands that supported global/user or not
19:14:37 <bcoca> you are reorganizing a lot of the code for no reason i can see
19:14:43 <tchernomax> glob detection : non, that change isn't need
19:14:52 <bcoca> ^ which will create merge hell for the other tickets
19:15:21 <tchernomax> but I though it was simpler
19:15:29 <bcoca> looks more complicated to me
19:15:56 <tchernomax> I can easily remove it
19:16:09 <tchernomax> > why create soo many vars that are the same thing in the end
19:16:29 <bcoca> ^ and that is just quick scan, not even a review
19:16:29 <tchernomax> because it help keep track of what variable to use
19:16:42 <bcoca> which systemctl handled nicely
19:17:07 <bcoca> you end up with same command building logic, except you now add 1 layer of indirection
19:17:16 <bcoca> its not as if those are reused all over
19:18:18 <bcoca> if they were, that would make more sense, but also to define them when you need to construct them, i find making all those refs to same var pointless at that stage, since you probalby wont use 1/2
19:18:41 <bcoca> im fine with reorganizing code, but this does not seem like an improvement
19:18:52 <tchernomax> ok
19:19:24 <bcoca> i'll put on my queue for actual review (im guessing that is why you put on agenda?)
19:19:33 * bcoca finds its actually already in queue ...
19:20:00 <tchernomax> did you find the other PR that fix the global scope ?
19:20:40 * bcoca goes back to search
19:22:32 <tchernomax> yes, that's why, but that's already a start, I can modify the PR from the comment you already wrote here
19:22:56 <shertel> looks like there are 11 open for that file now, referencing https://ansible.sivel.net/pr/byfile.html
19:23:27 <tchernomax> ok, i will look at it
19:24:52 <tchernomax> (if it was not clear enough : I change my vote for the 'combine' topic)
19:24:58 <bcoca> i cant find it right now, but will link when i do
19:25:13 <bcoca> tchernomax: to keep same filter?
19:25:24 <tchernomax> yes
19:25:38 <tchernomax> > i cant find it right now, but will link when i do
19:25:41 <bcoca> slim majority ... but ok, see comments i left, you should not reimplement merge_hash
19:25:49 <tchernomax> no pb, I will look the 11 PR
19:25:59 <bcoca> k
19:26:02 <bcoca> #topic open floor
19:26:35 <tchernomax> I would like to discuss  :wait_for_connection: allow test with raw module
19:26:46 <tchernomax> https://github.com/ansible/ansible/pull/57915
19:26:51 <bcoca> that was preptty much rejected in triage
19:26:59 * abadger1999 wonders who's reimplementing merge_hash
19:27:04 <bcoca> no disenting votes
19:27:09 <bcoca> abadger1999: tchernomax in his pr
19:27:14 <abadger1999> what PR?
19:27:34 <abadger1999> I ask because someone else already re-implemented merge_hash and I've been arguing with them about that...
19:27:44 <bcoca> https://github.com/ansible/ansible/pull/57894
19:28:00 <bcoca> abadger1999:  we can update merge_hash if needed, we really dont want N reimplementations
19:28:24 <tchernomax> why I didn't use merge_hash
19:28:31 <sivel> ah yeah, I saw the PR with the local function using obfuscated variable names
19:28:49 <bcoca> tchernomax: but that is the opposite of what we woudl want
19:28:58 <abadger1999> You might be Hmm... that's a superset of merge_hash.
19:29:03 <bcoca> yes, it is
19:29:03 <abadger1999> We probably need two functions.
19:29:09 <bcoca> but not 2 implementations
19:29:12 <abadger1999> But they should live side-by-side.
19:29:21 <tchernomax> because, I need to access list_merge arg
19:29:30 <bcoca> the vote was for adding list_merge option, which can be done at merge hash level
19:29:42 <tchernomax> and reduce need a 2 argument function
19:29:57 <bcoca> adding optional arg can be done in backwards compat way
19:30:13 <bcoca> creating new function and have 'merge_hash' call it with list_merge=overwrite is also a way to do it
19:30:15 <sivel> fwiw, we probably should not be using the python `reduce` function
19:30:40 <bcoca> well, once we have 'single' implementation we can deal with optimizing it
19:30:46 <sivel> I've never looked at code using reduce and thought to myself that it was readable
19:31:07 <tchernomax> > fwiw, we probably should not be using the python `reduce` function
19:31:09 <tchernomax> ok
19:31:13 * agowa338 I'd like to discuss `Fix ansible-vault cipher_whitelist #57272`, does that fit requirements for a bugfix, or does this need to wait for the next big release?
19:31:16 <bcoca> sometimes you sacrafice readability for performace, such function would be one case i woudl favor that
19:31:16 <sdoran> Yeah, I did not like that PR with a local function. I agree it'd be nice to put merge_hash in more accessible location.
19:31:30 <sivel> agowa338: I think we have 1 more topic before you, that we've glossed over so far
19:31:43 <bcoca> agowa338: one topic at time
19:31:48 <abadger1999> The backport PR I've been looking at needs merge_hash to end up in module_utils.
19:31:52 <abadger1999> @sdoran ^
19:31:56 <agowa338> sivel: ok, I'll wait.
19:32:06 <sivel> abadger1999: sdoran: yeah, I think I saw the ping this morning
19:32:13 <sivel> about moving to module_utils
19:32:21 <sdoran> @abadger1999 Yup.
19:33:59 <sdoran> I like moving things around. :)
19:34:23 <sivel> Ok, so unravelling this conversation, are we done with #57894 for now?
19:34:40 <bcoca> #topic backport of #57272
19:34:43 <bcoca> ^ abadger1999?
19:35:34 <sivel> hrm, I feel like we needed to verify #57272
19:36:02 <bcoca> its not merged yet
19:36:40 <sivel> iirc the old cipher was left because we could read, which is why it's not in the WRITE whitelist
19:36:44 <bcoca> so backport is a bit premature, but it does look like bugfix to me
19:37:02 <sivel> but I cannot remember how much of the old code was removed, without digging in further
19:37:03 <bcoca> did we ever confirm we removed 'old format read code'?
19:37:09 <sivel> I don't think so
19:37:17 <agowa338> sivel: The cipher was removed in a former commit, so the functionality is gone. The whitelist stayed...
19:37:35 <bcoca> i'll confirm
19:37:38 <sivel> "we" (the core team) need to verify before we can continue forward
19:38:58 <abadger1999> That looks like it should be okay to backport.  The class VaultAES code was removed like the PR says.
19:39:33 <bcoca> yep, confirmed, only new '1.2' vaults seem to be readable
19:40:24 <bcoca> 617372f8c0103c0f508f640bbb2f9f4a1fc85957  #44320 removed vaultaes
19:41:26 <bcoca> agowa338:  set rebuild_merge, you should create backport once it is in devel
19:41:41 <bcoca> #topic open floor
19:41:46 <sivel> yeah, I thought I was the one who removed it, but I've removed so much...
19:41:58 <bcoca> sivel: yes, and i reviewed your pr
19:42:02 <sivel> :)
19:42:04 <bcoca> both of us need better RAM
19:42:16 <tchernomax> about https://github.com/ansible/ansible/pull/57915 : would you reconsider this PR if I find enough people wanting it ? And if that's the case : how many people ?
19:42:28 <sivel> 1 million
19:42:39 <bcoca> ^ same but ^ 10000
19:43:14 <bcoca> until/raw will give you mostly same
19:43:22 <tchernomax> also, when you talk about complexity : is it code complexity or usability complexity ?
19:43:38 <bcoca> both
19:43:46 <tchernomax> yes, I know until/raw is exactly the same
19:43:47 <sivel> In all seriousness, we believe until/raw is best. If you want, you can always create your own action plugin, that you maintain outside of ansible, potentially shipped via galaxy
19:44:00 <agowa338> bcoca: Thanks ;-)
19:44:05 <tchernomax> what I don't understand is : why this module is here in the first place
19:44:23 <bcoca> tchernomax: if it is any solace, i voted against it
19:44:42 <bcoca> wait_for/delegate_to localhost basically does same
19:44:50 <sivel> tchernomax: in some cases, users make a case for a module, that is on the limit of what we are interested in, but have made it in due to popular demand.  Often we don't agree with the choice
19:45:06 <sivel> later, when someone wants to take such a module even further, we will often decline, and prevent that
19:45:32 <tchernomax> ok, I understand
19:45:35 <bcoca> i woudl point to synchornize as example of 'why'
19:45:39 <tchernomax> thanks sivel !
19:45:59 <sivel> We are moving towards a mechanism of shipping more things outside of ansible, called collections
19:46:13 <sivel> such a feature, might be best maintained in collections
19:48:01 <tchernomax> no pb, I will just use until/raw ; I made those modifications because I thought ssh (wait_for) could respond "OK", without the system being able to launch a shell
19:48:48 <tchernomax> I re-read the logs after this PR have been rejected and discovered it was something else that made my playbook failed
19:49:02 <tchernomax> so this PR isn't need anymore
19:50:47 <tchernomax> I just didn't understand why it was reject (I don't really see the complexity it bring… maybe more line of code) ; but that's clear know → wait_for_connection shouldn't have been there is the first place
19:51:02 <tchernomax> s/know/now
19:51:26 <tchernomax> s/been there is the first/been there in the first
19:56:44 <bcoca> #endmeeting