19:02:26 <bcoca> #startmeeting ansible core irc public meeting
19:02:26 <zodbot> Meeting started Tue Jun 25 19:02:26 2019 UTC.
19:02:26 <zodbot> This meeting is logged and archived in a public location.
19:02:26 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:02:26 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:02:26 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
19:02:30 <nitzmahone> o/
19:02:36 <shertel> hello
19:02:36 <jillr> o/
19:02:37 <sdoran> \o
19:02:48 <bcoca> #topic to fact or not to _info
19:02:58 <bcoca> https://github.com/ansible/community/issues/474#issuecomment-504782354
19:03:10 * cyberpear lurks
19:03:24 <felixfontein> hi!
19:03:39 <felixfontein> about topic: I
19:04:03 <felixfontein> I'm wondering what's the best way to proceed with _facts modules which return ansible_facts, but should not be _facts modules
19:04:50 <felixfontein> there are two options: return both ansible_facts and info in another way, and deprecate ansible_facts (will get nicer if return values can effectively deprecated, that's WIP for 2.9, but the output will double)
19:05:02 <cyberpear> rename them and phase out returning of ansible_facts on the same timeline as the old name goes away
19:05:06 <bcoca> i would deprecate module in favor of new copy with 'non facts'
19:05:09 <felixfontein> the other is to deprecate the _facts module, and add a new _info module which will return the information not as ansible_facts
19:05:25 <bcoca> cyberpear: rename either chagnes existing behaviour or perpetuates it
19:05:34 <cyberpear> fair enough
19:06:08 <cyberpear> just put a comment in the top of the file that says "this was copied from _facts" so folks trawling git history can find it
19:06:30 <bcoca> felixfontein: we have a feature we want (but still vaporware) that would allow to deprecate return values, but i think we should not wait for it and go with B option
19:06:32 <felixfontein> I'd add something like that to the module's documentation, so people reading the docs (as opposed the code) will also know
19:06:50 <bcoca> +10K to 'docs telling teh full story'
19:07:12 <nitzmahone> The other issues with duplicate/modify is that "full test" overhead now increases significantly
19:07:20 <bcoca> nitzmahone: also maintenance
19:07:33 <felixfontein> I added something like that to all _facts -> _info renames I made, something like "The module was called xxx_facts until 2.9. The usage didn't change.", but that of course only applies to modules which never returned ansible_facts in the first place
19:07:45 <nitzmahone> Azure folks just went for rename + alias + deprecate `ansible_facts`
19:08:01 <bcoca> nitzmahone: w/o deprecation msg?
19:08:22 <nitzmahone> On a related note: what are we doing at 2.9 release to things still named _facts?
19:08:33 <felixfontein> testing is a good point. we could simply change the tests to only use the new modules, and keep the old ones untested. but I'm not really happy about that
19:08:36 <nitzmahone> (eg "abandoned" modules)
19:08:39 <bcoca> ^ felixfontein has been renameing those, that is why he brought up this point
19:08:56 <nitzmahone> Ah, wasn't sure if it was in relation to a small subset
19:09:05 <felixfontein> I only renamed ones which didn't return ansible_facts so far
19:09:07 <bcoca> he has taken point and done a lot of work already
19:09:10 <bcoca> felixfontein++
19:09:23 <nitzmahone> If maintainers *want* to use it as a chance to reset the UI, new module makes sense, but otherwise it seems like a lot more work to duplicate
19:09:26 <felixfontein> (or tried to push the maintainers to do it, in case of google and azure modules)
19:09:41 <bcoca> how many modules are we talking about?
19:10:01 <felixfontein> several hundred I think
19:10:08 <sdoran> Ugh.
19:10:12 <bcoca> *cringe*
19:10:29 <felixfontein> everything in cloud/ which is still called _facts (and not part of vmware, google and azure)
19:10:37 <felixfontein> and some more
19:10:37 <bcoca> i was going to say 'case by case' ... but we'll be here till sun goes nova
19:10:57 <felixfontein> yes. already "just" renaming them all is a lot of work
19:11:09 <bcoca> im still partial to B .. but at this point 'anything that gets us there'
19:12:38 <mattclay> I agree we should avoid unnecessary copying, to avoid having either untested modules or an unnecessary increase in test overhead (and maintenance in general).
19:12:39 <cyberpear> if you rename, can you have the module detect whether it was called w/ `_facts` name and only return facts in that case?
19:12:58 <nitzmahone> If we go with the bulk rename, I'll take the task to go add the deprecation warnings on all "ansible_facts"-returning things once I merge the dep warnings on retvals support
19:13:07 <felixfontein> cyberpear: yes you can detect the name. I'm using that to issue a warning that the module has been renamed
19:13:15 <bcoca> nitzmahone: how far are we from that ?
19:13:41 <bcoca> cyberpear: you get module_name parameter
19:13:50 <nitzmahone> I have the dep warnings stuff working, just some performance enhancements to dedupe the metadata in ser/deser and figuring out WTH to do with `AnsibleVaultUnicodeText`
19:14:02 <bcoca> which is really 'action called', so ti does not always match either old/new name i.e copy when used from template
19:14:11 <felixfontein> nitzmahone: do you already have a PR?
19:14:23 <nitzmahone> Nah, still just local right now
19:14:54 <bcoca> nitzmahone:  push wip, i might be able to help you with vault stuff
19:14:55 <nitzmahone> We could also enforce the filtering in the default action
19:15:07 <nitzmahone> (in addition, I mean)
19:15:29 <bcoca> nitzmahone: execute_module might be better place, since you can bypass the normal action in many cases
19:15:34 <bcoca> e.g gather_facts
19:15:50 <nitzmahone> Yeah, I meant in ActionBase
19:17:15 <bcoca> so how long?
19:17:26 <bcoca> cause i dont think A is feasable w/o that code
19:17:38 <bcoca> well, its feasable, but really bad user experience
19:18:15 <nitzmahone> I can probably have an e2e working thing ready this week or next
19:18:32 <nitzmahone> Just gotta ignore some other things :)
19:18:36 <bcoca> felixfontein: that work fo ryou?
19:18:48 <felixfontein> sure, fine for me
19:18:58 <felixfontein> I'm somewhat busy this week anyway
19:19:15 <nitzmahone> You want to wait for that to be merged before you proceed with the renames then?
19:19:17 <bcoca> k, so i think we are going option A for now (if nitz can get feature in)
19:19:23 <bcoca> we might want to revisit next month?
19:19:42 <nitzmahone> It's basically the only 2.9 feature I have slated, everything else is playing with future release fun
19:19:46 <bcoca> nitzmahone: w/o your feature, a new module is a better option
19:20:38 <nitzmahone> Yep, one of the reasons I put it on the roadmap
19:20:56 <bcoca> another is for me tore move 'ansible facts injection!'
19:21:01 <nitzmahone> People can still do new modules if they *want* to, but I think the default should be rename + deprecate
19:21:02 <bcoca> s/remove/change the default/
19:21:09 <nitzmahone> yep
19:21:26 <bcoca> k, so tabling this decision contingent on nitzmahone's feature being merged.
19:21:38 <bcoca> #topic ansible_facts in loops
19:21:46 <bcoca> https://github.com/ansible/community/issues/474#issuecomment-505235184
19:22:04 <bcoca> nitzmahone: that would change behaviour of  include_vars: '{{item}}'
19:22:35 <bcoca> also, afaik the loop facts are always merged
19:22:37 <nitzmahone> Not necessarily- that and 'set_fact' have their own conditional separate from "everything else"
19:22:41 <nitzmahone> They aren't
19:23:05 <nitzmahone> They're merged after the task completes, but each iteration of the loop replaces the local value set by the last one
19:23:13 <bcoca> hmm  only for include_vars/set_fact (both come up as examaples that they do)
19:23:16 <bcoca> ?
19:23:32 <bcoca> ah, yes, interloop they are not, they are merged at final result
19:23:35 <nitzmahone> Right
19:23:38 <nitzmahone> That's the problem
19:24:04 <nitzmahone> So either I have to treat discovery result "special" or we need to merge inside the loop
19:24:08 <bcoca> do we really need to merge all facts or just discovery?
19:24:33 <bcoca> for now i would do just discovery, as the other change has many implications we might not see right now (but im sure they'll break playbooks)
19:24:37 <nitzmahone> Doesn't really matter- it's only merging with its own results, not the full var space
19:24:55 <bcoca> but now it affects behaviour of loop if loop depends on them
19:25:01 <nitzmahone> This is a *really* corner case
19:25:09 <bcoca> ansible_os_distribution ....
19:25:35 <nitzmahone> This wouldn't affect individual loop result accessibility, only the internal storage of the merged result (which isn't visible to playbooks)
19:25:59 <bcoca> how not? that is used to template each loop iteration
19:26:13 <bcoca> and passed to each task (action/module)
19:26:41 <nitzmahone> https://github.com/ansible/ansible/blob/devel/lib/ansible/executor/task_executor.py#L686
19:27:16 <bcoca> yes, that is done so EACH loop iteraction can do 'post result processing' failed_when/changed_when/notify/etc
19:27:24 <nitzmahone> This is for the "outer" copy- conditionals etc can still see the individual results per iteration, we're not changing that. This is basically what gets merged with the rest of vars when the task is done
19:27:49 <bcoca> b;ut the 'outer' is what is then entered back into loop to retemplate task/conneciotn/shell/become/etc
19:27:56 <nitzmahone> The only time you'd see a difference is if you called a facts module in a loop and it returned different keys
19:28:13 <bcoca> or you had facts and loop changes + updates them
19:28:16 <nitzmahone> (across iterations)
19:28:19 <bcoca> e.g hostname
19:28:52 <bcoca> though i imagine only time this would actually bite us is doing package/dist upgrades
19:29:07 <nitzmahone> Basically this would make the behavior consistent if you called the module N times manually and in a loop
19:29:37 <nitzmahone> Right now, loop only "keeps" the last value of ansible_facts returned, where calling it N times manually, it merges
19:29:41 <bcoca> but they are not meant to be the same
19:29:55 <bcoca> loops have more data than 'successive tasks'
19:30:37 <bcoca> also that line affects not only with_/loop, but retry/until
19:30:39 <nitzmahone> Intra-loop, the observable behavior would be exactly the same, unless different keys were returned, in which case you'd "see" the old keys (where today you wouldn't, unless you're accessing the current result)
19:30:56 <bcoca> i expect diff keys can happen in retry/until
19:31:21 <bcoca> even in normal loop, for momdules that both make changes and return the results as facts
19:31:21 <nitzmahone> Which I still can't see a problem with...
19:31:24 <felixfontein> (or because of modules returning strange stuff in ansible_facts, like docker_service)
19:31:46 <bcoca> hostname is best example i have, but i know others do same, they make a change and return 'updated facts'
19:31:57 <nitzmahone> Right, but still the same keys, right?
19:32:17 <nitzmahone> Same keys will not result in any observable change
19:32:27 <bcoca> diff values, which if conditional on value, changes behaviour of iteration (hostname is bad example here. but something like parted/mount might be more important)
19:33:00 <nitzmahone> Different keys will still show the "newest" values associated, it's just that the previous keys that don't appear in the new result will *also* be present (as if you'd run the module by manually N times)
19:33:15 <bcoca> e.g if ansible_mounts['x'] == 'efs2', now loop runs on contion being same since start, change would make it variable per iteration
19:33:17 <nitzmahone> The value behavior will be indistinguishable for the same keys
19:33:50 <bcoca> keys  only matter in that their value changes
19:34:23 <nitzmahone> Today, it replaces the entire `ansible_facts` dict on each run, I just want to merge it
19:34:31 <bcoca> no, it wont, same key=value would stay same, diff vlaues for same key could change behaviour current is 'value is same as before task started', change, value depends on last iteration
19:34:31 <nitzmahone> And the values will still change
19:34:52 <bcoca> they change on result side
19:35:01 <bcoca> for post,not for pre, you wantto make it reentrant
19:35:36 <nitzmahone> They will be exactly the same on the result side as they are today, except that the set of visible keys will increase if the keys change during iteration
19:35:56 <bcoca> again, keys really dont matter, its the values
19:36:08 <bcoca> also now you need to handle merge_behaviour?
19:36:21 <nitzmahone> How else can I say: "the values will be exactly the same as they are today?"
19:36:42 <nitzmahone> I'd make it behave the same way as the current ansible_facts merge that occurs
19:36:52 <bcoca> then i have no clue what you are saying, since key/val needs to change to make it reentrant
19:37:08 <nitzmahone> There's nothing about concurrency or reentrancy here
19:37:22 <bcoca> you want to persist facts over iterations
19:37:30 <bcoca> which currently they are not
19:37:33 <nitzmahone> I'll just PR it
19:37:43 <nitzmahone> Feel free to poke holes in it there
19:37:50 <bcoca> ok, seeing code might clarify what you mean, cause right now im lost to yoru meaning
19:39:29 <bcoca> #topic open floor
19:40:58 <bcoca> k if nothing new closing in 1min
19:42:00 <bcoca> #endmeeting