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