00:02:25 <Kylie_> #startmeeting azure_working_group
00:02:25 <zodbot> Meeting started Thu Jan 25 00:02:25 2018 UTC.  The chair is Kylie_. Information about MeetBot at http://wiki.debian.org/MeetBot.
00:02:25 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
00:02:25 <zodbot> The meeting name has been set to 'azure_working_group'
00:02:49 <Kylie_> Two topics from me today.
00:02:54 <Kylie_> #topic bug triage
00:03:03 <Kylie_> https://github.com/ansible/ansible/issues/17925
00:03:03 <Kylie_> https://github.com/ansible/ansible/issues/33448
00:03:03 <Kylie_> https://github.com/ansible/ansible/issues/28392
00:03:03 <Kylie_> https://github.com/ansible/ansible/issues/26195
00:03:03 <Kylie_> https://github.com/ansible/ansible/issues/34047
00:03:03 <Kylie_> https://github.com/ansible/ansible/issues/31105
00:03:04 <Kylie_> Any objection to close it?
00:03:04 <Kylie_> https://github.com/ansible/ansible/issues/29580
00:03:05 <Kylie_> https://github.com/ansible/ansible/issues/29585
00:03:17 <nitzmahone> which- all of them?
00:04:39 <Kylie_> yes:) We could start from 29580 and 29585.
00:05:27 <Kylie_> I think these 2 are already in 2.4. Could you please take a look? If yes,could we close them?
00:08:06 <nitzmahone> I don't think 29585 has been implemented
00:08:50 <nitzmahone> I assume he's talking about an arg that would allow for a storage account to be specified to receive VM metrics and/or boot diagnostics.
00:08:57 <nitzmahone> You quoted data disks
00:09:28 <Kylie_> O, ok. I thought he means metrics of data disks.
00:09:49 <Kylie_> ok. keep it open.
00:10:38 <Kylie_> Hi all, could you please take a look https://github.com/ansible/ansible/issues/28392?
00:10:39 <nitzmahone> Closed 29580 though, that was implemented a LONG time ago
00:11:19 <Kylie_> What do you suggest for the behavior? If fail to execute a playbook, should Ansible clean up others which already created?
00:11:27 <Kylie_> https://github.com/ansible/ansible/issues/28392
00:11:40 <nitzmahone> I debated that when we first built it
00:12:13 <Kylie_> I saw some different behaviors.
00:12:14 <jborean93> I personally believe it should cleanup errors
00:12:47 <jborean93> especially when this could end up costing the user money for running in the background
00:13:08 <nitzmahone> We could definitely add "best effort" cleanup- it's just unfortunate when you get all the way to VM and have an issue, then have to create/teardown everything in between. But I guess you could always not use the auto-creation stuff anyway in that case if you want maximum performance.
00:13:44 <nitzmahone> (eg explicitly create dependent resources)
00:13:46 <zikalino> i have the same idea, it should clean up.
00:14:07 <zikalino> i think vm module is just too large though.
00:14:53 <nitzmahone> Yeah- I was hoping to clean up a bunch of things by removing support for automatic storage account creation in favor of managed disks, but until Azure Stack supports managed disks...
00:15:23 <nitzmahone> I guess we could still rip all that stuff out and just say if you're on stack, we won't create the storage account for you
00:15:42 <nitzmahone> Then someday when stack supports managed disks, it works the same everywhere
00:15:43 <zikalino> maybe if it cleans up everything, somethimes it my clean up too much. i believe some sometimes some resources may be created beforehand...
00:16:01 <nitzmahone> We would only clean up things we'd explcitly created in that case
00:16:30 <nitzmahone> (ie, things that the module had auto-created)
00:18:29 <nitzmahone> I'm fine with changing the behavior to clean up better- just need to make sure we don't accidentally delete resources that we didn't create.
00:18:48 <nitzmahone> (so track internally which ones were created by us and only delete those)
00:19:42 <nitzmahone> Anything more to discuss on that one?
00:19:57 <Kylie_> ok. I think there is an agreement - clean up errors. As for how, seems need to be reviewed further.
00:20:02 <Kylie_> Zim, can you take this one?
00:20:10 <zikalino> yes, i can
00:20:15 <Kylie_> Thank you.
00:20:23 <zikalino> could we switch to prs
00:20:25 <zikalino> ?
00:20:32 <Kylie_> Yes, please.
00:20:59 <zikalino> i have this keyvault pr: https://github.com/ansible/ansible/pull/35150
00:20:59 <Kylie_> And another topic for Yuwei and Matt is to see any question about API versioning. And schedule.
00:21:29 <zikalino> reviewed by jborean93 already but needs nitzmahone to check
00:21:49 <zikalino> nitzmahone could you check and possibly merge?
00:22:24 <nitzmahone> yes; might be Monday next week though (I'm off tomorrow and Friday)
00:22:48 <nitzmahone> If I have some time  waiting for tests on something else I might be able to do today
00:23:38 <nitzmahone> I'll also update the PR for API versioning on Monday next week
00:23:44 <zikalino> well, it's a kind of critical, so if you could briefly look and perhaps jborean93 could merge when i fix your comments
00:23:58 <zikalino> i have 2 more modules depending on this.
00:24:03 <yuwei> I have two prs without any comment
00:24:24 <zikalino> i am fixing https://github.com/ansible/ansible/pull/33607
00:24:49 <yuwei> #35037 #35038
00:24:50 <zikalino> and this https://github.com/ansible/ansible/pull/33606
00:25:28 <zikalino> these 2 modules in addition need more variables from CI
00:25:53 <jborean93> yuwei: I'll look at your PR's soon
00:25:54 <zikalino> azure_client_id and azure_<object id related to client id>
00:26:13 <nitzmahone> zikalino: The immediate thing that made me want to look further at keyvault stuff is this statement: "All identities in the array must use the same tenant ID as the key tenant ID."
00:26:19 <zikalino> so i think mattclay is right person to make such changes
00:26:30 <nitzmahone> If that's the case, why is tenant ID even allowed on the rules?
00:27:51 <zikalino> indeed, i will hide this param
00:28:23 <zikalino> i couldn't find any circumstance when this should be different
00:28:59 <nitzmahone> Also, hardcoding tenant ID on the vault itself in the Ansible content is probably not the way most people would want to use it, so we should probably default to the current connection's tenant ID. Good to be able to override for more complex scenarios, but otherwise things like CLI auth are problematic as a user would have to dynamically get tenant ID somehow.
00:29:57 <zikalino> ok, i can add this too
00:30:10 <nitzmahone> Maybe as a special non-GUID value like "current" or something
00:30:26 <nitzmahone> (so it can also be selected dynamically)
00:30:38 <zikalino> i think i will just default to current
00:31:20 <nitzmahone> Anyway, just saw those things on first glance and thought I should spend some time playing with it to see if there are other usability problems
00:31:32 <nitzmahone> (as I've not used a keyvault before)
00:32:17 <nitzmahone> It's not just a matter of code review
00:32:46 <zikalino> then major problem people encounter with keyvault is using proper object_id
00:33:05 <zikalino> (often confused with application_id)
00:33:35 <Kylie_> guys, I need to leave for a customer meeting. I don't have other topics. Thank Matt to take care of API versioning. I will ask Yuwei check your PR next Monday. Thanks.
00:33:55 <nitzmahone> (well, her Tuesday since you're many hours ahead of us)
00:34:30 <nitzmahone> Usability-wise, it'd be nice if we could look up the user object ID by some other selector, but I don't think we want to hold it up for that
00:34:33 <Kylie_> yes:)
00:35:10 <nitzmahone> Could be added later (eg, provide a dictionary with values that could be looked up and translated to an object id)
00:35:56 <nitzmahone> As-is, I don't think we have a facts module or anything that could do that translation, so it'd be very difficult to actually use this module in a real-world scenario
00:35:58 <zikalino> that could be done only via sth like "az ad sp show --id <application id>"
00:36:52 <zikalino> if we had ansible module to do this, user could first query object_id from and then use it in this module.
00:37:06 <jborean93> you want to make this easy for users, not everyone who uses Ansible is a programmer
00:38:26 <nitzmahone> Yeah, something like azure_rm_serviceprincipal_facts and azure_rm_aduserfacts
00:38:36 <zikalino> i think object id should somehow come from common implementation. we already have credentials there, we could obtain object_id or we could just require object_id to be provided along with credentials.
00:39:48 <zikalino> this is not only ansible problem, i run into that when testing keys and secrets modules, and i found our several people made the same mistake (when using Azure API from difefrent SDKs)
00:40:19 <nitzmahone> That's why we try to make the modules more than just a thin wrapper over the SDK
00:40:51 <nitzmahone> (idempotence, linked resource lookup, etc)
00:40:59 <nitzmahone> "what it" scenarios
00:41:03 <nitzmahone> *if
00:42:24 <nitzmahone> Anyway, we don't necessarily have to solve that problem right away, but the module is not very usable in the real world without it IMO
00:42:55 <nitzmahone> Nobody should be hardcoding AD GUIDs into their playbooks
00:43:14 <nitzmahone> (nor required to manually look up and pass a whole bunch in as vars to execute something)
00:45:27 <zikalino> true
00:46:20 <zikalino> but in this case you have to pass some guids somehow whether it's application_id or object_id
00:46:44 <nitzmahone> yep, though application_id could be looked up the same way
00:47:01 <nitzmahone> (eg, hypothetical AD facts module or something)
00:47:11 <zikalino> well, that's not necessarily the same application_id as your environment has
00:47:34 <nitzmahone> right, but if you were creating something reusable, there'd be a name associated with it
00:47:37 <zikalino> you may need to pass 5 different application_ids or (object_ids)
00:48:10 <zikalino> because you may be giving access rights to 5 different applications / people
00:48:30 <nitzmahone> right- so you'd want a way to look those up programmatically by name
00:49:39 <zikalino> yes, that should be possible, but single app may have several service principals
00:49:43 <nitzmahone> The whole intent of Ansible is to build reusable content that can be used across many environments, so hardcoding GUIDs anywhere in the content is a problem. Those kinds of things always need to be discoverable dynamiclly
00:50:44 <nitzmahone> Right, so you'd look them up by name/tag/whatever in a previous task, then pass them into this module (or allow for a selector of some sort in the module)
00:51:19 <zikalino> yes, something like that, or even previous module might create service principal
00:51:27 <nitzmahone> Yep
00:52:05 <nitzmahone> Like I said, we don't have to solve that part now, but something to think about
00:52:10 <zikalino> well, key vault actually is not a good example of module that can be used for "ordinary users".
00:53:20 <zikalino> ok, so right now i think we should:
00:53:42 <zikalino> 1) fix tenant_id thing so it's populated by default if not specified
00:54:00 <zikalino> 2) document object_id to make sure people don't make mistakes
00:54:30 <nitzmahone> sounds good
00:54:57 <nitzmahone> If the underlying SDK accepts tenant_id for the rules, I'd still make it overridable, but also default to "current"
00:55:04 <zikalino> afterwards i will look into creating active directory module.
00:55:35 <nitzmahone> (there are several things like that I've run into where it seems like maybe the SDK is set up for future capability that doesn't exist today, eg, cross-tenant keyvault access)
00:57:03 <nitzmahone> hmm, Kylie didn't chair anyone- I don't think we can add #action or even end the meeting
00:57:17 <nitzmahone> @Kylie_ can you chair someone?
00:57:47 <nitzmahone> #chair jborean93
00:57:52 <nitzmahone> nope
00:57:53 <nitzmahone> :(
00:58:52 <nitzmahone> OK, any others to discuss? We're about at time. @yuwei - you mentioned a couple PRs?
00:59:19 <yuwei> oh i just mentioned 2 which has no comment
00:59:33 <jborean93> just merged 35037 after testing it
00:59:41 <jborean93> going onto 35038 now
00:59:48 <nitzmahone> ah ok
00:59:48 <yuwei> Thanks
00:59:54 <nitzmahone> Anything else for today, then?
01:00:17 <yuwei> No from my side
01:00:53 <nitzmahone> OK.
01:01:30 <nitzmahone> @Kylie_ - can you please #endmeeting if you see this (since nobody else got #chair ed, otherwise the meeting will run on for a long time)
01:01:43 <nitzmahone> Thanks all
01:04:15 <zikalino> thanks!
09:03:38 <dvb> hey! i am currently trying to create a nic with azure_rm_networkinterface without a public ip. documentation says: "public_ip_address_name: Name of the public ip address. None for disable ip address." but when running my playbook i get "ValidationError: Parameter 'public_ip_address_name' can not be None". anyone there for any help, please?
00:00:52 <Kylie_> #endmeeting