00:00:04 #startmeeting Ansible Azure Working Group 00:00:04 Meeting started Thu May 24 00:00:04 2018 UTC. 00:00:04 This meeting is logged and archived in a public location. 00:00:04 The chair is nitzmahone_. Information about MeetBot at http://wiki.debian.org/MeetBot. 00:00:04 Useful Commands: #action #agreed #halp #info #idea #link #topic. 00:00:04 The meeting name has been set to 'ansible_azure_working_group' 00:00:44 yo 00:00:44 I am here. Anyone else online? 00:00:47 Hello everyone 00:00:56 #topic PR for 2.6 00:01:03 #chair Kylie_ jborean93 yungezz yuwei 00:01:03 Current chairs: Kylie_ jborean93 nitzmahone_ yungezz yuwei 00:01:16 Thank you Matt for merging resource module and ADFS. 00:01:28 yes, thanks! :-) 00:01:37 Thanks! 00:01:57 I will leave time to Matt, Catherine and Zim first for modules - app gateway and web app to see any comment can be addressed here. 00:02:32 For web app module https://github.com/ansible/ansible/pull/40005, I sent an email yesterday to detail on UX 00:02:45 Any comments or feedback? 00:04:06 If I understand correctly, the top level settings are nearly all mutually exclusive, correct? 00:05:02 Xxx_versions and linux_fx_version is mutual exclusive 00:05:09 Like, you can't specify `php_version` and `java_version` together, right? 00:05:21 Yes 00:06:10 So why dedicated top-level named args for all of those instead of: 00:06:10 framework_type: java 00:06:10 framework_version: 1.8 00:06:16 Do you want a check on the mutual exclusive? 00:06:53 It’s aligning az cli and rest api 00:07:05 No, I just think the overall UX is not good with dedicated named args per framework type plus this composite "linux_fx" thing with a name and version too... It's very inconsistent with the rest of Ansible as well as within itself 00:07:15 Just because az cli did it doesn't mean it's a good UX 00:07:44 We're not trying to map the CLI or the API, we're trying to expose a usable interface to build the resource in question 00:07:48 It will give user hints by seeing the name immediately 00:08:20 I think if you poll anyone in the #ansible channels about whether this is a good UX, you'll get a pretty resounding "no" 00:08:21 that's what the description is for 00:08:38 or the choices field 00:09:05 If user do reading some azure docs, it will be easily connected Ansible args and azure args 00:09:44 Agree the args is confusing, I sent lots of time to figure out how to use it 00:09:55 The reason I haven't given more explicit feedback is that I haven't had time to go review the underlying API for webapp to propose a different alternative, but I'm -1 to merging in its current state 00:10:27 I'm pretty confident we can come up with a better UX, and that's the thing that's "set in stone" once we ship it 00:10:52 The docs seem to indicate that the cli doesn 00:11:04 doesn't have a dedicate arg for each runtime but rather just 1 00:11:05 https://docs.microsoft.com/en-us/cli/azure/webapp?view=azure-cli-latest#az-webapp-create 00:11:19 I can refine the Ux 00:11:38 That runtime is linux_fx_version 00:11:51 so going with nitzmahone_'s suggestion of having just `framework_type/framework_name`seems a better option 00:12:04 For windows, it’s still use different args of version 00:12:30 what do you mean by that? 00:12:34 Yes, that’s good idea for windows, 00:13:42 I mean, runtime = linux_fx_version, this arg only works for linux web app, for windows web app , user need specify xxx_version 00:13:56 have the module handle that info 00:13:56 Still seems like we could vastly simplify the UX on this with an internal mapping of some sort, even if the REST API is suboptimal (which I haven't had time to investigate). 00:14:00 This az cli and rest api 00:14:50 "Go read the REST API docs to use this module" is not a good starting point... 00:15:21 Ok, how about keep one framework name and version, and another arg framework_os? 00:15:45 That's probably good 00:15:50 Framework_platform seems a better name 00:15:56 Any suggestion 00:15:59 * nitzmahone_ is ok with either 00:16:21 Ok, I will update the pr today 00:17:05 also DNS registration should probably be a single parameter 00:17:12 Any other comments on this module? 00:17:17 Ok 00:17:23 what's the behavior when not skip and not force? 00:18:20 you probably don't need another arg to specify the OS, seems like you already have `plan: is_linux` 00:18:23 if it's something else, seems like we need dns_registration: (skip/force/auto) or whatever 00:18:28 Force true, I think 00:19:15 so probably just `dns_register: (bool, default true)`, but might want to verify that 00:19:32 Yes 00:19:34 ugggh, the plan stuff is ugly all over- I don't know what to do about that 00:19:47 (not just in this module, just around Azure in general) 00:20:13 You mean the sky? 00:20:18 Sku 00:20:33 yeah 00:20:41 Agree.. 00:20:52 hard to fill it right 00:21:21 So I put a link from docs.azure on allowed values 00:21:55 The SKU is fine, it's the other values. 00:22:32 Does it create a thing in a resource group, use a thing that must already exist in a resource group, or what? 00:23:46 I didn’t get the question. It will create app service plan if not exists, could be in another resource group 00:24:07 Or same resource group as web app 00:24:14 so a plan can exist in 1 resource group but you want the app in another 00:24:21 Yes 00:24:45 So plan has resource group sub option 00:24:59 * nitzmahone_ wonders if webapp_plan should be its own module if it's creating a resource 00:25:15 Yes, that’s the plan 00:25:22 I haven't used webapps since Resource Groups were a thing 00:25:39 Add separate module for app service plan later 00:27:05 sounds like plan should be similar to what is being done with referencing an object in another resource group 00:27:17 OK, so we can retrofit with the URL/name/dict-of-(name/RG) pattern later 00:27:18 yep 00:27:19 e.g. allow a name by default but a dict as well if you wish to specify the resource group 00:27:32 but the other args in that suboptions should move to the top level 00:27:48 e.g. sku, number_of_workers, is_linux 00:27:57 You mean plan”s suboptions 00:27:59 I think those are plan-specific values 00:28:03 To top level 00:28:07 Matt 00:28:10 Is right 00:28:19 It’s plan specific 00:28:20 You create a plan and can have N webapps share it 00:28:25 Yes 00:28:32 (so N webapps might be sharing the workers) 00:28:37 and the plans are OS-specific 00:28:38 Yes 00:28:45 Right 00:28:54 ok fair enoguh 00:29:01 So just for extensibility, perhaps change 'is_linux' to plan type 00:29:14 with choices linux/windows 00:29:32 Maybe I ll add docker there 00:29:45 Linux/win/docker 00:29:46 (assuming they're not compatible, eg, you can't host a Windows framework-y thing on a Linux plan) 00:29:56 Yes 00:30:15 Well, even docker would still be about the host type though, at least for the plan 00:30:50 Make sense 00:31:50 Do you want me to just put other comments in the review? I wanted to discuss the big-picture stuff with the framework types interactively, but there are other suggestions 00:32:06 Sure, thanks 00:32:54 Yes, please key in comments in PR. Thank you all. 00:33:04 I will update pr after 1 hour to see if it can get reviewed again 00:33:14 ok, how about appgw :-) 00:33:29 Wonderful. Thank you Catherine. Reserve Matt and Jordan's time:) 00:33:52 ：） 00:34:01 I had some complexity concerns about appgw when I looked it over briefly last week, but I'm forgetting what they were now 00:34:18 😃 00:34:33 well, appgw is a kind of aggregation of lots of things on it's own 00:34:50 I think it was around the two cert types and how you'd specify them- I was having to go troll around in the Azure docs to figure out which one does what 00:35:54 ok, then perhaps it's documentation issue 00:36:58 i was wondering myself about one thing. appgw has lots of lists of different resources. every item has "name". perhaps we should have dictionaries instead of lists? 00:37:03 but i am not sure about it 00:37:27 i will look into cert types and try to improve it 00:38:06 Was mainly like "am I providing a reference to a cert that somehow has already appeared, or the raw cert data (in format X?), or something else? 00:40:13 and what happens to the certs once used- are they created as resources, stored in a vault somewhere, only used ephemerally to create the object (then how do you verify idempotence and/or change them later), stuff like that 00:41:13 and like backend_http_settings_collection takes a list of authentication_certificates (even the Azure docs for appgw are unclear to me about when I need one vs the other) 00:41:34 But that one is a reference to something that got created by ... ? 00:42:20 So I'm guessing these modules have not been vetted by any real-world usage? 00:43:40 ok, actually now i see what you mean.... 00:44:00 I'm going to push back a lot harder on these after the keyvault thing ;) 00:44:26 Want to make sure that we're shipping things that are really useful, not just a wrapper over the API 00:44:59 Because that's not often sufficient for users to do real work from Ansible, which IMO is *worse* than not having anything at all 00:45:26 Make sense? 00:45:57 especially given the short timeframe for 2.6, I'm going to propose that these get targeted on 2.7 00:45:58 yes, i definitely agree.... 00:46:28 At least if something's not there, people can plan accordingly, but if they have to fight with something for a long time befoer determining it's not useful... 00:46:36 Does none of us any good 00:47:19 hmm... that's why i actually think azure_rm_rest is useful. anything can be prototyped before module is actually created. 00:47:43 Zim, let us see whether we could test a real scenario with app gateway from ARM template examples. 00:47:54 yes 00:48:11 Hi Matt, how about https://github.com/ansible/ansible/pull/39940, https://github.com/ansible/ansible/pull/38643? Bug fixes for permitting use other resource group 00:48:13 (and resource has now been merged ;) ) 00:49:05 and https://github.com/ansible/ansible/pull/37627 00:49:25 if Matt doesn't get onto those I'm planning on testing them out after lunch 00:49:43 Yes, Thank you Yuwei. Should be 38643 and 37627. 00:50:37 @yuwei 38643 is good, other than I'd like to see Catherine's suggestion of including an ID test implemented. 00:50:52 I'll merge that one now if you'll go back and add a test PR for that soon ;) 00:52:12 https://github.com/ansible/ansible/pull/37627 looks good at a glance- @jborean93 if you can play with it a bit before merge, that's cool 00:52:22 will do 00:52:31 (go ahead and merge if you're happy with it) 00:52:39 The virtual network has the id test 00:53:02 At line 240 00:53:20 I failed to copy the line link using phone 00:53:29 @yuwei sorry, security group id 00:53:50 Okay I will add 00:53:59 (this comment): https://github.com/ansible/ansible/pull/38643/files#r189485154 00:54:18 #action Zim, 39940 (app gateway) test with real scenario 00:55:04 I have to take off- Jordan, can you wrap up? 00:55:11 sure thing 00:55:13 #action Jordan, help test 37627 to confirm it can be merged or not 00:55:33 I think the two community-source modules should probably wait for 2.7 also, would also like to see some real-world usage to validate that they're good 00:55:48 Thanks all- have a good weekend! 00:56:09 cya 00:56:25 Thank you Matt. 00:56:28 thanksmatt 00:56:35 https://www.irccloud.com/pastebin/XYFJIDe3/ 00:56:35 Thanks 00:57:51 anything else to discuss? 00:57:53 Yuwei, as for security group ID, any action? 00:58:29 Zim and Jordan, where are we for these two from external contributors? 00:58:30 https://github.com/ansible/ansible/pull/37333 00:58:42 https://github.com/ansible/ansible/pull/38279#discussion_r188149080 00:58:47 one of them we have decided should be a part of another module 00:59:25 37333 is just too small to be a module on it's own 00:59:32 yea the keys facts should just be part of the storagemodule_facts 01:00:14 Then Zim, will you make that happen (move key to stroagemodule_facts) today? 01:00:23 I haven't looked over 38279 recently so not sure where that ended up 01:00:30 yes, i could, it's easy 01:00:48 38279 also 01:00:52 vm facts are a pretty important thing so we want to do it right 01:01:41 yeah, 37333 is pretty easy in comparison.... 01:02:24 so i will start with 38279 today, and see if i can complete 01:03:11 i think the discussion is pretty tough here, as raw/curated is pretty new concept 01:03:33 yea, I think we are cutting a bit too close to the line for the vm facts 01:03:47 it would be better to hold off for 2.7 when we have more time to refine and get it right 01:03:47 i think maybe we can just make sure that options are right, and if something is incomplete (regarding to curated) we could add it later 01:05:03 Understand. Do right thing right. Let us move forward. If it is good enough for 2.6, great. If not, let us ensure it is mature for 2.7. 01:05:10 Ok. I think that's all. 01:05:13 Any other open? 01:05:23 No from me 01:05:23 just an update 01:05:47 i have reenabled and fixed all sanity tests on azure (well 3 still disabled)... 01:05:56 yes, thank you for that, much appreciated 01:06:00 i am going to focus on integration stability afterwards 01:06:49 Nice. 01:06:50 still had some comments from mattclay about version_added (i had to add 2.6 for options that were not documented) so now i have to fix it 01:07:05 Thank you Zim. Thank you all. 01:07:08 ok, so that's all from me :-) 01:07:11 yea, in cases like that the CI will fail in the PR but once merged it is fine 01:07:27 thank you all! 01:07:31 #endmeeting