23:59:57 <nitzmahone> #startmeeting Ansible Azure Working Group 23:59:57 <zodbot> Meeting started Wed Jan 9 23:59:57 2019 UTC. 23:59:57 <zodbot> This meeting is logged and archived in a public location. 23:59:57 <zodbot> The chair is nitzmahone. Information about MeetBot at http://wiki.debian.org/MeetBot. 23:59:57 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic. 23:59:57 <zodbot> The meeting name has been set to 'ansible_azure_working_group' 00:00:03 <yuwei> hi 00:00:04 <nitzmahone> Hey all 00:00:06 <jborean93> hey 00:00:09 <zikalino82> hello 00:00:22 <yuwei> happy new year 00:00:25 <nitzmahone> #chair yuwei jborean93 zikalinio82 yungezz 00:00:25 <zodbot> Current chairs: jborean93 nitzmahone yungezz yuwei zikalinio82 00:00:30 <nitzmahone> Same to you! 00:00:37 <yungezz> Happy new year 00:01:05 <yungezz> I m not sure will Kylie will join 00:01:27 <yungezz> But we have some topic 00:01:38 <nitzmahone> Go for it 00:01:47 <yungezz> First is ansible_facts in facts module 00:02:40 <zikalino82> yes, i think some time ago we discussed that and decided that we shouldn't return facts in "ansible_facts" 00:02:47 <yungezz> We removed the key from facts per previous discussion, but got complain facts are not automatically registered in ansible_facts 00:02:49 <nitzmahone> Yeah, definitely nothing new 00:03:14 <zikalino82> well, the problem is that they like it but it's not right 00:03:45 <nitzmahone> Yeah, it's convenient for a small subset of use cases, but more painful for everyone that's doing it "right" because they have to dot through "ansible_facts" when registering the output 00:03:46 <yungezz> How automatic registered facts output works 00:04:28 <jborean93> plus the facts aren't per hsot so they will overwrite any existing facts if you are looking up another resource 00:04:28 <zikalino82> in case of cloud resources, all facts are stored on local machine 00:04:47 <jborean93> sorry aren't related to the host, they are still stored per host 00:04:49 <nitzmahone> Any keys that appear under the "ansible_facts" dictionary in a return value will set a host fact with that key name in the global namespace. 00:04:51 <zikalino82> so if you request facts for vm1 for instance and then facts for vm2, vm2 facts will overwrite facts for vm1 00:04:59 <nitzmahone> Yep 00:05:26 <jborean93> which is why facts under `ansible_facts` should be facts based on the host, not the resource you are looking up and why we moved away from it 00:06:02 <nitzmahone> It's something I should've pushed back on harder when we were building the original set of modules 00:06:13 <zikalino82> well, i think making facts curated will make it easier to used them and it should make people happier :-) 00:06:26 <yungezz> Ok. So conclusion is we’ll keep current behavior: not add ansible_facts in facts module 00:07:13 <yungezz> Second topic about not updatable argument behavior 00:07:13 <nitzmahone> Yep- anyone that's complaining about that is kind of doing it wrong. At any scale, you have to use `register` anyway. 00:07:25 <yungezz> Ok 00:08:12 <yuwei> i see 00:08:26 <zikalino82> yes, we think that not updatable arguments, if change is detected, it shouldn't be just ignored silently. which sometimes happens 00:08:33 <yungezz> If a playbook updating not updatable argument, currently we will error out, but there’s not consistent behavior, some modules just ignore it 00:09:14 <nitzmahone> Probably the best thing is to just warn in those cases 00:09:27 <zikalino82> i think that was our conslusion 00:09:37 <yungezz> We also talked about warnings 00:09:53 <yungezz> Great that we are aligned 00:10:04 <zikalino82> another option was error 00:10:29 <zikalino82> ok, so let's do warnings then 00:10:31 <nitzmahone> eg, think about a VM playbook that looks up the "latest" version of a given image- you run it today, then tomorrow, a newer image is uploaded. Should that VM task fail? 00:10:42 <nitzmahone> (if you run it tomorrow) 00:11:22 <nitzmahone> I think it's reasonable for it to warn that you can't update the image to a new version without destroying the VM, but anything else (auto-destroy, error out) doesn't seem right. 00:11:26 <zikalino82> i think that can actually be updated, so it won't fail :-) 00:11:30 <nitzmahone> Warning is probably the best balance 00:11:38 <yungezz> Yes 00:12:01 <zikalino82> ok, so i think i have another topic fitting here 00:12:06 <zikalino82> when talking about vms 00:12:14 <nitzmahone> So that's probably another note for the guidelines wiki stuff 00:12:16 <zikalino82> i have this pr: https://github.com/ansible/ansible/pull/50652 00:12:36 <nitzmahone> Oh, excellent 00:13:04 <zikalino82> This PR will improve automatic resource removal in azure_rm_virtualmachine. 00:13:07 <zikalino82> t resources created by VM will be removed. 00:13:18 <zikalino82> i am not sure how to make it obsolete exactly 00:13:35 <zikalino82> current behaviour is "all" option 00:13:48 <zikalino82> to enable new behaviour you need to use "own" option 00:14:08 <zikalino82> but when obsoleted default will be "own" 00:15:31 <nitzmahone> Probably the best way is to make a new "all_default" option as the interim default, so we can distinguish between the user asking for "all" and the user not specifying a value. 00:16:15 <zikalino82> so all_default should become default? 00:16:23 <nitzmahone> Seems like we should be able to ask the question of the argspec, but I think it's destructive, so we can't get at the original values passed (or not passed) to determine the difference between "all" and "user didn't specify" 00:16:35 <nitzmahone> Probably 00:16:57 <nitzmahone> I hate to do it that way, because we'll actually have to document that value to keep the sanity check happy :( 00:17:17 <jborean93> wouldn't be the first time :( 00:17:19 <nitzmahone> Ideally we'd just have that be an implementation detail, but I think the docs sanity check will fail 00:17:49 <nitzmahone> Then just fire a deprecation warning if it's "all_default" 00:18:14 <zikalino82> i think that's the only way... 00:19:44 <zikalino82> hehe, now i became a bit confused :-) 00:19:51 <yungezz> Any other topic? If no, let’s go to prs 00:19:53 <nitzmahone> how so? 00:20:44 <yungezz> Sorry my new messages lagging :( 00:21:01 <zikalino82> "all_default" would be option to delete all resources created by the vm by default 00:21:29 <nitzmahone> Sorry, "all_default" would be the same behavior as "all" today, but be the one set as the default in the argspec 00:21:55 <nitzmahone> (so we can tell the difference between the user explicitly saying "all" and the use not specifying anything) 00:21:56 <zikalino82> oh, so just renaming "all" 00:22:06 <nitzmahone> We'd only fire the warning on "all_default", not "all" 00:22:14 <nitzmahone> (because "all" is still valid) 00:22:36 <nitzmahone> All could still be used explicitly to delete things that had been created explicitly 00:22:50 <nitzmahone> Just would eventually no longer be the default 00:24:00 <zikalino82> ok, so we should change default module behaviour now, and if sth breaks users could still go back and use "all_default" option 00:24:50 <zikalino82> "all" -> all resources created by the module 00:24:54 <nitzmahone> No, that's just how you determine whether to fire the deprecation warning 00:25:05 <zikalino82> "all_default" -> current behaviour 00:25:07 <nitzmahone> The default doesn't become `own` until 2.10 00:25:10 <nitzmahone> right 00:25:17 <zikalino82> ok 00:25:42 <zikalino82> ok, i know what to do 00:25:54 <nitzmahone> Just from a UI bikeshedding standpoint `own` should probably be like `auto_created` or something, too 00:26:04 <nitzmahone> (something a little more descriptive) 00:26:11 <zikalino82> ok 00:26:42 <nitzmahone> So, some PRs you want our feedback on? 00:26:53 <yungezz> https://github.com/ansible/ansible/pull/50006#issuecomment-448230126 00:27:02 <yungezz> Inventory fix 00:27:18 <nitzmahone> oh cool- will review 00:27:46 <zikalino82> and here is vmss extension: https://github.com/ansible/ansible/pull/50709 00:27:57 <zikalino82> should be easy as it's very similar to existing vm extension 00:27:57 <yungezz> One question about the new inventory, if I download the azure_rm.py , will it work with old ansible, such as 2.6? 00:28:55 <nitzmahone> yungezz: maybe, maybe not- I don't recall if there were any changes to the inventory plugins code in 2.7 that might prevent it from working on older Ansible or not 00:29:04 <nitzmahone> Easiest thing to just try it. 00:29:08 <yungezz> Got it 00:29:39 <nitzmahone> zikalino82: cool, I'll add that to my review queue as well 00:30:43 <nitzmahone> Any others? 00:32:22 <yungezz> No from me 00:33:24 <nitzmahone> OK, if nothing else for today, we'll wrap it up in 5... 00:33:29 <abadger1999> I've gotr one 00:33:41 <nitzmahone> go for it 00:34:05 <abadger1999> https://github.com/ansible/ansible/pull/50121 00:34:20 <abadger1999> This is a backport PR but required some extra work to make it work on stable-2.7. 00:34:34 <abadger1999> Would someone be able to review it and just give me a shipit before I merge it? 00:36:23 <nitzmahone> I'll take care of that 00:36:32 <nitzmahone> (looks OK at a glance, but will look a little harder) 00:37:05 <nitzmahone> OK, closing in 5... 00:37:08 <nitzmahone> 4.. 00:37:13 <nitzmahone> 3.. 00:37:17 <nitzmahone> 2.. 00:37:20 <nitzmahone> 1.. 00:37:25 <yungezz> Thanks all 00:37:29 <nitzmahone> Thanks all- until next week. Happy new year! 00:37:32 <nitzmahone> #endmeeting