18:01:19 <gregdek> #startmeeting new-modules-meeting 18:01:19 <zodbot> Meeting started Wed Aug 3 18:01:19 2016 UTC. The chair is gregdek. Information about MeetBot at http://wiki.debian.org/MeetBot. 18:01:19 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic. 18:01:19 <zodbot> The meeting name has been set to 'new-modules-meeting' 18:01:27 <gregdek> #chair rbergeron gundalow 18:01:27 <zodbot> Current chairs: gregdek gundalow rbergeron 18:01:34 <gregdek> #topic Roll Call 18:01:40 <jimi|ansible> yo 18:01:44 <gregdek> (gundalow notes that he'll be 15m late) 18:02:13 <ryansb> hi 18:02:19 * rbergeron waves 18:02:22 <gregdek> Agenda, as always: https://github.com/ansible/community/issues/92 18:02:30 <rbergeron> meetings, so missed 18:03:13 <gregdek> Working the agenda from bottom to top: 18:03:56 <gregdek> #topic Archive Module 18:03:58 <gregdek> https://github.com/ansible/ansible-modules-extras/pull/2323 18:04:02 <abadger1999> óla 18:04:27 <gregdek> Hey abadger1999 :) 18:04:55 <gregdek> So looks like dag gives this one a shipit, and since he's the author of the unarchive module, seems like his shipit carries disproportionate weight here 18:05:16 <alikins> howdy 18:05:34 <chouseknecht> yo 18:06:37 * gregdek waits for folks to have a look 18:07:25 <abadger1999> Concept is reasonable. Don't like that the code is all in the main() function but we don't really patrol code quality in extras. 18:08:00 <rbergeron> gregdek: it looks good to me. Lots of feedback back and forth and seems like everything has been addressed more or less? 18:08:06 <gregdek> Seems like it. 18:08:32 <rbergeron> plus, as you said, dag :) 18:08:36 * gundalow waves 18:08:48 <rbergeron> hey gundalow :) 18:09:59 <gregdek> Any objection to putting this one in shipit for final review? 18:10:17 <abadger1999> +1 18:10:25 <gundalow> LGTM 18:10:31 <gundalow> (looks good to me) 18:11:03 <gundalow> I assume filecmp, zipfile and tarfile are part of default python 18:11:15 <jimi|ansible> that's two +1's, any reason not to just merge it? 18:11:28 <gundalow> Sure, just merge 18:11:30 <gregdek> That's up to y'all. :) 18:11:57 <gundalow> oh, am as I'm core do I get full vote rights? 18:12:25 <gundalow> Do we want to be adding stuff to MAINTAINERS-EXTRAS as we give shipits (I can update that) 18:12:46 <gregdek> Well, when we actually merge, yes. 18:13:14 <jimi|ansible> gundalow: yes you are core and have full voting rights :) 18:13:23 <mattclay> It would be nice if older PRs such as this had tests run on Shippable, but the only way I know to do that is to have the contributor push their branch again. 18:13:59 <mattclay> In this case it won't matter, since it's a new module and has no tests. 18:14:15 <jimi|ansible> shippable doesn't have a retest option in the github ui? 18:14:39 <mattclay> Shippable doesn't know about the PR because we added Shippable after the PR was last updated. 18:14:40 <jimi|ansible> anyway it was tested and passed, since this is modules that shouldn't have changed so not really worried about it 18:14:53 <jimi|ansible> well, tested on travis then 18:14:57 <jtanner> famous last words 18:15:17 <jimi|ansible> and it will be tested on shippable after merging 18:15:26 <gregdek> OK, so we're shippping, then. 18:15:30 <gregdek> Seems like we're agreed. 18:15:33 <gundalow> +1 18:15:36 <gundalow> hey mattclay :) 18:15:43 <gregdek> Who's doing it, and shall we move on? 18:15:49 * mattclay waves 18:15:54 * gundalow will, and I'll update MAINTAINERS 18:15:58 <gregdek> Either we shipit now, or we add the "shipit" label to make sure we do it later... 18:15:59 <gregdek> :) 18:16:03 <gregdek> OK, thx gundalow 18:16:08 <gregdek> Next up: 18:16:29 <gregdek> #topic datadog_downtime module 18:16:30 <gregdek> https://github.com/ansible/ansible-modules-extras/pull/2381 18:17:04 <gregdek> Copious +1s after several attempts. :) 18:17:09 * gregdek will brb 18:17:56 <jeff-logicmonito> hi all. jeff from logicmonitor here. spoke to some of you at the fest last week. i'm late because i didn't realize there was a separate meeting room. just want to make sure i didn't miss discussion of our module. 18:18:10 <ryansb> hi Jeff 18:18:14 <gundalow> hey jeff-logicmonito :) 18:18:39 <jeff-logicmonito> we meet again 18:18:49 <jimi|ansible> jeff-logicmonito: hey, that was me :) 18:19:02 <jimi|ansible> you didn't miss it 18:19:12 * gundalow is vritual greg 18:19:13 <rbergeron> gregdek: well -- aside from the "do we really mind if people are saying works_for_me for their own module submissions 18:19:17 <rbergeron> " 18:19:18 <rbergeron> thing 18:19:44 <jimi|ansible> rbergeron: i don't mind if they do, but it adds no weight for me generally 18:19:47 <rbergeron> ... it seems like others are okay with it that had been waiting on it 18:19:59 <gundalow> alikins: oh, just noticed you added comment son file/archive 18:20:24 <gundalow> oh and abadger1999 18:20:55 <gregdek> well, skornehl and goekhanm both give it shipit 18:21:08 <rbergeron> but otherwise... nobody else seems sad or anything 18:21:10 <abadger1999> gundalow: sorry -- I saw "mark for final review" earlier so started putting comments in. 18:21:17 <rbergeron> gregdek: indeed, i missed that there was a second person 18:21:25 <gundalow> alikins: abadger1999 It's cool. I'm glad you spotted stuff 18:21:28 * abadger1999 can always find something to nitpick about. 18:21:35 <gundalow> I'll ask the submitted to patch it 18:21:40 <gundalow> Not like anyone is using it yet 18:21:45 <abadger1999> <nod> 18:21:58 <gregdek> So then we're shipping it and we'll ask for a subsequent patch, or...? 18:22:08 <gundalow> I'll ask for a patch 18:22:16 <gundalow> It's safe as no one is using it 18:22:25 <gundalow> if it was an update to a module I'd be more tempted to revert 18:22:28 <gregdek> Before or after merge? 18:22:30 <rbergeron> on which? 18:22:44 <gregdek> Are we merging, or waiting for a new patch to merge? 18:22:44 <gundalow> 2323 archive 18:22:50 <rbergeron> ah 18:22:57 <gundalow> sorry, I should have changed topic back 18:23:03 <gregdek> Ohhhhh. 18:23:06 <gundalow> OK, carry on 18:23:11 <gregdek> Well, no, wait. 18:23:11 <gundalow> datadog_downtime 18:23:13 <rbergeron> it's all good. are we kosher with 2381? i think we are 18:23:18 <gregdek> Are you merging 2323 or not? 18:23:47 <gundalow> I've already merged 2323. I'm going to add a follow up comment asking him to raise a new PR to fix the few minor issues 18:23:54 <gregdek> Got it. Thanks! 18:24:10 <gundalow> . 18:24:10 <gundalow> . 18:24:11 <gundalow> . 18:24:11 <gundalow> . 18:24:11 <gundalow> . 18:24:14 <gundalow> . 18:24:16 <gregdek> OK, back to 2381... ok to put into shipit, or merge directly, or neither? 18:24:17 <gundalow> datadog_downtime 18:24:41 <ryansb> :ship: 18:25:15 <rbergeron> also: jeff-logicmonito -- hi and welcome :) (sorry, got distracted by other typing of things) 18:25:42 <rbergeron> gregdek: i am for at least shipit, not able to comment on the merge part of it but that sounds delightful if possible :) 18:26:22 <gregdek> So, put it in shipit for final review? 18:26:27 <gregdek> Sounds like the consensus. 18:26:34 <gundalow> NO 18:26:36 <gundalow> NO 18:26:44 <gundalow> the docs for 2381 are all erong 18:26:58 <gundalow> or some letters like that 18:27:16 <gregdek> Ok! 18:27:23 <gundalow> unless you already spotted that 18:27:41 <gregdek> I'm just asking the questions, LOL 18:28:11 <alikins> do we have a policy on using stdlib modules that are imported via the 'from ansible.module_utils.basic import *' ? stuff like time,sys,re, etc 18:28:30 <gundalow> alikins: ziploaded concerns? 18:28:34 <bcoca> since 2.1, they are not 'autoimported' 18:28:49 <bcoca> modules should specify their own if they use em 18:28:52 <alikins> I'd prefer module to import those itself for clarity, even if the modules do get imported into the local namespace from the splat 18:29:05 <gregdek> gundalow: so what's wrong with the docs? 18:29:21 <bcoca> agreed, but that would go with removal of 'from ansible.module_utils.basic import *' form 18:29:29 <alikins> yup 18:29:30 <bcoca> to new 'actual improt' 18:29:35 <bcoca> import 18:29:37 <gundalow> I(api_key): 18:29:43 <alikins> and general lower level of wtf 18:29:55 <gundalow> (just adding comments 18:30:07 <alikins> 'wheres that from' that is 18:30:11 <bcoca> we should be doing that 'in passing' to modules we touch, not mandatory for 2.2, but proably should be for 2.3 18:30:38 <gregdek> ok, so gundalow is adding comments to the PR directly, we will put back into needs_revision I imagine? 18:30:40 <bcoca> for modules in the repo, not going to remove the facility as custom ones might still use 18:31:54 <rbergeron> gregdek: sounds like it 18:33:07 <gregdek> Is someone also adding comments about expected usage of module_utils import? 18:33:13 <gregdek> (And is that in the module guidelines?) 18:34:50 <bcoca> http://docs.ansible.com/ansible/developing_modules.html#common-module-boilerplate 18:34:53 <bcoca> it was updated 18:35:14 <gregdek> OK, very good. 18:35:19 <bcoca> but most of these PRs predate it 18:35:25 <bcoca> so do most modules .... 18:35:42 <gregdek> Yes, it's a moving target, of course. But at least we can point people to the new guidelines. 18:36:23 <gregdek> I think we got that one covered. 18:36:24 <gregdek> Moving on: 18:36:44 <gregdek> #topic AWS NAT Gateways 18:36:48 <gregdek> https://github.com/ansible/ansible-modules-extras/pull/1882 18:37:34 <gregdek> This is from linuxdynasty, who was at the contributor conference 18:37:52 <gregdek> And there was conflict among two modules, so I picked this one because it had tests and both had multiple advocates 18:38:08 <alikins> wow 18:38:11 <alikins> it has tests! 18:38:29 <gregdek> inorite 18:38:45 <bcoca> its a trap! 18:38:52 <gregdek> linuxdynasty is *very* excited about testing. 18:39:55 <gundalow> +10000000000000000 18:41:30 <gundalow> (though I have a few minor docs things) 18:41:42 <gundalow> If someone can look at the code that would be good :) 18:42:37 <rbergeron> I think the various aws folks have all gone through this with a pretty srs comb haven't they? 18:42:43 <rbergeron> gregdek ^^ ? 18:43:04 <gregdek> Seems like a lot of them have. 18:43:50 <gundalow> My comments are only nitpick. And only there because apparently I'm not allowed to go raise one EPIC PR to fix all formatting for all modules :( 18:43:58 * ryansb hasn't personally tested, but would be pro-shipit 18:44:13 <jtanner> you can't do "EPIC PRs" ? 18:44:37 <mattclay> Once this is merged, lets make sure to ask for a PR to get the tests added. 18:44:43 <gregdek> gundalow: you're core now. You can do whatever you need to do. 18:45:04 <gregdek> Unless the core team says you can't, LOL 18:45:15 <gundalow> jtanner: I was told I shouldn't go round just fixing typos/spelling. In the same way I shouldn't just do PRs that only fix PEP8 18:45:39 <gundalow> (though I have ignored this for modules/networking/*) as there were spelling mistakes & formatting issues 18:45:42 <gregdek> If this is the most feedback we have, I say we mark for shipit. 18:45:45 <jtanner> i've never agreed with that 18:45:48 <rbergeron> okay, so: can we shipit? let's do that. :) 18:46:00 <jtanner> fix whatever you find 18:46:39 <gundalow> jtanner: woot 18:46:44 <gregdek> I vote we mark "shipit" and let submitter fix issues with separate PR. We know he's engaged. 18:46:49 <gregdek> Any objection? 18:46:50 <rbergeron> gregdek: +1 18:46:51 <gundalow> sure :) 18:46:53 <abadger1999> gundalow: typos/spelling +1 to just fix. PEP8, I feel good about it but others do not... Figure networking is fair game since you are one of the primaries for that subcategory. 18:47:30 * gundalow will add to MAINTAINERS 18:47:31 <ryansb> I'm pro docs/typo fixing, but for whitespace I'm ambivalent about fixing just whitespace in a PR 18:47:33 <gundalow> abadger1999: aye 18:48:09 <gundalow> ryansb: oh, I agree. my vimrc screams if there is trailing whitespace. I only fix that if I'm making other changes that justify a PR 18:48:20 <gregdek> OK! 18:48:24 <gregdek> #topic Open Floor 18:48:41 <jtanner> vmware_guest was merged into extras yesterday 18:48:43 <gregdek> jeff-logicmonito: hello! 18:48:46 <jeff-logicmonito> hi! 18:48:50 <gregdek> What's your PR? 18:48:58 * rbergeron suspects it is https://github.com/ansible/ansible-modules-extras/pull/2652 :) 18:49:12 <jeff-logicmonito> correct 18:49:55 <jeff-logicmonito> i appear totally incapable of updating my PRs and have them pass travis if i leave them untouched for awhile. sorry about that. =[ 18:50:00 <rbergeron> and looks like jimi removed the needs_revision on the previous PR 18:50:12 <jtanner> are we picky on the order of DOCUMENTATION, EXAMPLES, RETURNS ? 18:50:16 <gregdek> jeff-logicmonito: more our fault than yours. 18:50:34 <gregdek> #topic Logic Monitor PRs 18:50:38 <rbergeron> which seemed to have all the necessary shipits otherwise 18:51:02 <jeff-logicmonito> correct. the only changes necessitating the new PR was some docs formatting fixes jimi pointed out at the fest 18:51:04 <gregdek> jtanner: good question. It's generally in that order, don't know if it's a killer. 18:51:25 <jtanner> the linter didn't seem to care, so i suspect not .. but i have to wonder 18:51:25 <gregdek> I presume that jimi|ansible can vouch for these changes, since he recommended them personally ;) 18:51:51 <jeff-logicmonito> he told me that docs build threw errors, so i fixed the errors 18:52:39 <jeff-logicmonito> i should point out that the doc generator doesn't actually accept ALL valid YAML for the documentation string. it was freaking out on the line continuations i was using. not a big deal to take those out, but took some digging to figure out what it was complaining about 18:53:08 <gundalow> jeff-logicmonito: Do you list Linux for a specific reason under requirements. Are there none linux versions of logicmonitor? 18:53:09 <jtanner> "Manage your LogicMonitor account through Ansible Playbooks" ... technically modules can be used from ansible adhoc too 18:53:30 <gundalow> s/ Playbooks/ will fix that 18:53:47 <abadger1999> jtanner: I'd like to be picky about the order but no code relies on it. 18:54:53 <jeff-logicmonito> re windows: i think there is some weirdness with our windows collector installer that the decision was made not to support it in ansible at this time 18:55:04 <gundalow> jeff-logicmonito: cool 18:55:34 <gregdek> Would sort of like feedback from jimi|ansible here... 18:56:14 <rbergeron> gregdek: did his removal of needs_revision not provide enough feedback? :) 18:56:15 <jtanner> "import urllib" ... 18:56:25 <jeff-logicmonito> it's just for param encoding 18:56:37 <jtanner> i think we have a 'safe' wrapper for that 18:57:21 <gregdek> Oh, right, we're supposed to use the url stuff in module_utils before urllib, right? 18:57:34 <jtanner> looking for example 18:57:45 <jimi|ansible> we have a parsing method, not sure if we have an encoding method wrapper 18:58:16 <gregdek> "When fetching URLs, please use either fetch_url or open_url from ansible.module_utils.urls rather than urllib2; urllib2 does not natively verify TLS certificates and so is insecure for https." 18:58:26 <gregdek> Refers to urllib2, though, not urllib 18:58:34 <gregdek> And don't know the difference in this case 18:59:18 <abadger1999> jeff-logicmonito, jtanner: If it's just for param encoding it's fine. 18:59:25 <jtanner> k 18:59:34 <jeff-logicmonito> i swear that's all 18:59:35 <jtanner> i see that outbound calls are through our lib 18:59:46 <abadger1999> It really does mean urllib2 -- urllib2 fetches files (urllib does not). 18:59:49 <jtanner> "self.urlopen = open_url" 19:00:15 <jimi|ansible> yeah i checked all that already 19:00:36 <jimi|ansible> as jeff-logicmonito said, it was only a docs issues, so if they compile now i'm ok with the module 19:00:37 <jtanner> "santaba" is that a product codename? 19:00:52 <jeff-logicmonito> correct 19:01:25 <gregdek> Well then. 19:01:37 <gregdek> jimi|ansible gives his thumbs up, so shipit. 19:01:51 <jtanner> SHIP ZE DAMN MODULE 19:01:53 <jeff-logicmonito> awesome. thanks everyone! 19:01:54 * rbergeron is with that 19:02:10 <gundalow> jeff-logicmonito: I've added some docs feedback 19:02:15 <tima> nice work jeff-logicmonito. 19:02:42 <gregdek> And that's our time for today. Thanks everyone! 19:02:46 <gregdek> #endmeeting