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