19:00:02 #startmeeting ansible core 19:00:02 Meeting started Tue Jan 23 19:00:02 2018 UTC. The chair is thaumos. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:02 Useful Commands: #action #agreed #halp #info #idea #link #topic. 19:00:02 The meeting name has been set to 'ansible_core' 19:01:29 blorp 19:01:33 #chair alikins 19:01:33 Current chairs: alikins thaumos 19:03:28 .hello2 19:03:29 maxamillion: maxamillion 'Adam Miller' 19:03:33 #chair maxamillion 19:03:33 Current chairs: alikins maxamillion thaumos 19:03:41 hey, sorry, was running late 19:03:45 no worries 19:03:47 #chair sivel 19:03:47 Current chairs: alikins maxamillion sivel thaumos 19:04:05 just three people right now, not including me 19:04:09 I think we should start today with the "generated modules" discussion that I believe gundalow placed on the agenda 19:04:17 we didn't get that finished last meeting 19:04:18 I thought that happened last week? 19:04:26 ah, is that why it was added? 19:04:32 We didn't come to a decision, too many people wandered away 19:04:41 and if we have enough people to discuss sounds good to continue it. 19:04:54 right 19:05:04 I'm here 19:05:11 (that whole lunch/nap thing didn't happen) 19:05:31 #chair nitzmahone 19:05:31 Current chairs: alikins maxamillion nitzmahone sivel thaumos 19:05:37 lunch, now? 19:05:43 wake up early nitzmahone? 19:06:08 heya 19:06:08 Pre-freeze scramble until midnight-ish yesterday 19:06:18 I saw that, @nitzmahone 19:06:23 both you and @abadger1999 19:06:30 #chair ryansb 19:06:30 Current chairs: alikins maxamillion nitzmahone ryansb sivel thaumos 19:06:33 Those 16h days are painful 19:06:39 * thaumos nods 19:06:57 yeah, I don't recommend them :) 19:07:19 Olá 19:07:29 I usually only have a few of those a year these days, so life's improving from startup-land where it was a few a month. 19:07:58 #chair abadger1999 19:07:58 Current chairs: abadger1999 alikins maxamillion nitzmahone ryansb sivel thaumos 19:08:05 nitzmahone: that's good 19:08:14 I do find I like coding better in the dead of night. Unfortunately, that doesn't correspond with the rest of the world. 19:08:33 well, @abadger1999, you could claim to be on Aussie time. 19:08:40 @abadger1999 exactly; I feel like I start to hit my stride at 6-7pm 19:08:42 I think we have enough to being. 19:08:44 hard to order pizza from Oz, though ;-) 19:08:46 nitzmahone: I remember when I was on the OpenShift Team they were *really* frequent in my first 6 months, then things settled and it was only every couple weeks ... but I have zero plans to ever do that kind of thing again with that level of frequency 19:08:51 thaumos++ 19:08:53 #topic generated modules discussion continuation 19:09:06 @thaumos++ 19:09:16 #link https://github.com/ansible/ansible/pull/35014 19:09:26 hrmmm... maybe it's fedbot that handles the karma 19:09:26 We tabled the vote on how we handle generated modules last meeting, because we didn't have enough people at the time 19:09:28 can someone who was present last week catch us up really quick? 19:09:30 thanks sivel 19:09:46 Basically, the new gce modules are to be generated using code in a different repo 19:09:47 the problem that made sense to me from last meeting was how to make sure bugs get merged in modules and not regressed. 19:10:01 We discussed how we would handle that, and came up with 2 options to vote on 19:10:02 abadger1999: +1 19:10:04 alrighty - so we were talking about GCE modules that they've written a code generator to create based on API stuff 19:10:08 OPTION 1) We have the bot and ANSIBLE_METADATA dictate auto closing issues or PRs for these modules. (requires a proposal) 19:10:18 OPTION 2) 100% ignoring that they are auto generated (no exceptions), no pointing people elsewhere, just forgetting where they came from, treated like any other module 19:10:18 * ryansb shuts up, sivel's got it 19:10:20 Remember to consider how this applies to existing auto generated modules, such as network/avi 19:10:22 there needs to be a clear way of saying "these modules come from $NOT_ANSIBLE_PLACE, please report bugs there" 19:10:27 sorry :) 19:10:29 Can I propose Option 3? 19:10:35 nitzmahone: of course 19:11:12 We were talking about the possibility of pip-distribution of modules/module_utils... Look into that and say that generated modules should use that (assuming we can make it work reliably) 19:11:14 Of those two, Option 2 made sense to me. 19:11:27 nitzmahone: +1 if we can swing it 19:11:35 Keep it out of our repo 19:11:38 nitzmahone: I think we have kinda decided, to stay away from pip-distribution in lieu of ansible-installer for now iirc 19:11:40 ..... 19:11:45 and drop then in galaxy 19:11:53 them* 19:12:09 But that's 2.7+ at this point, yeah? 19:12:25 2.5 should have basic install functionality, maxamillion is heading that 19:12:26 I feel like option 3 is not sufficiently soon for "actually helping our GCE users" 19:12:27 I'd fully support option #3 and would prefer that path. however, timing is questionable at this time. 19:12:48 sivel: that got moved to a topic branch 19:12:55 or maybe we decided that ansible-installer would be shipped around 2.5 but in a topic branch.. 19:12:59 nitzmahone: installer is going to be in its own branch for now and the people who are testing it will be able to use from there. But of course, that excludes general users. 19:12:59 sorry, brain was catching up 19:13:04 sivel: no worries 19:13:22 I'm hoping to have something "first draft" done tomorrow or Thursday to start getting feedback from others 19:13:40 maxamillion +1 19:14:00 Yeah, timing's definitely the issue; I'm just loathe to open the door at all 19:14:03 So I do like #2, #3 would be nice, but that can always be the future goal 19:14:29 keeping #3 in mind, #2 is the closest to status quo. 19:14:43 #1 would cause unnecessary workflows 19:14:50 @thaumos agreed 19:14:57 I think #2 is the best as well here - keeps module maintainership standard - doesn't make a new class of modules/code review/etc 19:15:27 I think #2 works if we have a good relation with the module devs. I have worked with some that are very good about integrating fixes in the modules here into their generator so for all intents and purposes I don't know that they are using a generator. OTOH, I've worked with some where they argue that they can't make a change because their generator can't do that. 19:15:59 I see. That'd be an expectation-setting exercise with the GCP folks that I can discuss with them 19:16:01 regarding the latter, @abadger1999, then that's something they have to think about it not being merged. 19:16:02 And that is a terrible experience becaue then you feel you have to closely review any of their changes to be sure they aren't regressing things that were previously talked about 19:16:04 Yep; the maintainer team that gets pinged on those can manually close any PRs; I think especially if the code has lots of GENERATED DON'T DORK WITH THIS "caution tape" on it, we'll be fine. 19:16:22 so #2 assumes that the module maintainers will field github issues as per the standard development workflow right now, but will simply do the fix in their repos for the generator code and then merge in newly generatedmodules? 19:16:29 yes, that's right 19:16:37 cool 19:16:48 yeah, then I'm +1 to #2 19:16:51 keep this simple folks, because it makes things a lot easier for when #3 is the main option. 19:16:53 @abadger1999: for generated modules, I'd be inclined to: a) REQUIRE a test suite before they're merged b) only review UI in general for bugfixes 19:17:09 Anyone against #2? 19:17:19 nitzmahone: test as in integration test suite? 19:17:21 I am +1 for #2 19:17:22 yes 19:17:26 or units, or all of aboe 19:17:29 I don't think anyone has expressed against it. 19:17:30 *above 19:17:35 I think tests for modules is a different topic, but I've been led to believe that tests will be required for new modules in 2.6+ 19:17:46 for avi, I stressed very hard that they have internal testing on their code. 19:17:47 We have the infra now, the GCE folks should implement the integration glue required for it 19:17:52 so I think it's fair to ask of anyone else as well. 19:17:53 from what I've been told, no new module will be accepted 2.6+ without tests 19:18:03 that is fair too. 19:18:20 I am 100% for that if code is to be included at all with the engine delivered package. 19:18:27 but I haven;t gotten clarification on that 19:18:32 okay, so I think this is settled. 19:18:35 so that's reasonable 19:18:39 yay 19:18:48 can we have a vote for: #2, and require integration tests/glue? 19:18:57 +1 19:18:57 Yes, good for me 19:18:57 ryansb: note that ui often becomes a point of contention with auto generated modules 19:18:59 +1 19:19:05 +1 19:19:18 Yes, that's true and why I'm having them generate one module and learn 19:19:23 Although I will say, we of course still need to review new submissions 19:19:30 not generate all of them and then make us do 500 reviews 19:19:31 Absolutely 19:19:38 excellent 19:19:40 "The API I'm autogenerating from uses parameters thusly!" "But users of ansible modules will expect parameters to be done thisly" 19:19:41 @sivel, which is what was done for all the avi modules 19:19:45 +1 from me for the vote on issue 19:19:45 so this is not new. 19:19:55 +1 for 2 19:19:59 abadger1999: yes, already discussed that contention with them 19:20:05 I think we are agreed then 19:20:06 +1 for 2 19:20:08 they're lucky in that the GCE API is actually very sensible 19:20:10 #agreed 100% ignoring that they are auto generated (no exceptions), no pointing people elsewhere, just forgetting where they came from, treated like any other module 19:20:22 so the mismatch from API/ansible UI isn't onerous 19:20:32 ryansb: yeah, I'm for this... just want to make sure you know what you're signing up for ;-) 19:20:40 #agreed absolute integration test requirement for generated modules 19:20:40 @sivel, was your inventory paths comment discussed? 19:20:44 thaumos: no 19:20:47 well, what I'm signing *them* up for ;) 19:20:49 k 19:20:56 anything else for the current topic? 19:20:58 hehe 19:21:05 Reminder: if folks would like to review the concrete version of the PR it is here: https://github.com/ansible/ansible/pull/35014 19:21:06 ryansb: the burden for you is in reviewing 19:21:11 yes, it is 19:21:29 .... 19:21:30 ....... 19:21:32 ......... 19:21:34 ......... 19:21:49 #topic inventory paths with commas 19:21:50 aaaand topic closed 19:21:54 If an inventory path has a comma right now, that path is thrown away, simply because it has a comma, used in shorthand explicit hosts like `-i localhost,` 19:21:55 and the generator is open-source'y enough? 19:22:00 #link https://github.com/ansible/ansible/pull/35002 19:22:06 @alikins topic closed 19:22:14 alikins: will follow up with you offline 19:22:33 So if something has a path like /path/to/my,cool,playbook/whatever/hosts then we don't look for host_vars/group_vars relative to the inventory file 19:23:05 i would argue that is not a good practice. 19:23:07 My suggestion was to only throw it away if it had a comma, and was not a valid path 19:23:24 thaumos: sure, I suppose, that argument is valid 19:23:26 runtime warning? 19:23:46 sivel: Could we add does not have a "/" in it? 19:23:48 A runtime warning could work just like my logic change 19:24:18 abadger1999: I'm not sure, I'd have to validate whether the path gets expanded first, or expand it in the check 19:24:19 silent skip could be confusing 19:24:27 right now, it is a silent skip 19:24:31 sivel: in the bug report, it was expanded first. 19:24:42 but I don't know about other uses that get to that code. 19:25:26 this is the first time anyone has complained about this. it seems like a very uncommon approach to directory naming. 19:25:29 Expandd is actually what shifted me to think some improved heuristic is needed. Because the user couldn't even specify a relative path to workaround the issue. 19:25:34 thaumos: windows users ;) 19:26:36 So, 1) I could make it a warning, or I could ensure the path is expanded, and if it has a `/` and the path exists, then don't throw it away 19:26:42 forgot a 2) in there 19:26:56 2) ensure the path is expanded, and if it has a `/` and the path exists, then don't throw it away 19:26:59 and of course, user could specify ./file,with,commas if they needed to circumvent the "/" in heuristic (which matches with the standard way that PATH variables are configured too) 19:27:23 abadger1999: which is why I would ensure to expand before the check 19:28:13 Anyone have an opinion on the approach? 19:28:24 Or just forget the issue was ever filed and wontfix 19:28:39 I'm +1 (2), okay with (1), wishy-washy on the PR as is (probably makes the situation better but feels like it could overshoot the other way slightly) 19:29:16 I'm not super concerned about having to change my impl, that's what we are here for 19:29:29 nitzmahone: ? 19:29:32 I like forgetting about it until there is a way to explicit indicate a arg is a name and not a path 19:29:34 I'm on the fence about it, paths with commas just seem silly, but if it's a valid path on the system I hate to just say, "no we don't support that" 19:30:12 I think the problem is that all other args use @ for a file, but -i would be opposite of that 19:30:26 since the @ is inferred there 19:30:49 This was the way it was in 2.4, right? 19:31:00 (or is this new behavior for 2.5?) 19:31:03 krh 19:31:06 or could just add an escape for the 'somehost,' mini-lang 19:31:06 oops 19:31:11 the code was changed in some manner in 2.4 through the inventory re-write 19:31:26 I'm guessing it has been this way for a while though, but without concrete proof 19:31:50 so afaik it is not a regression 19:31:52 If it was new for 2.5, I'd be more inclined toward the escape/warning path, but I'm +1 for sivel's fix as-is since it's been this way for awhile 19:32:31 And apparently *somebody* hit it... 19:32:59 could combine and use optional '@file,with,commas' as a disambiguator 19:33:27 actually... 2.3 worked 19:33:33 the 2.4 inventory rewrite seems to have broken it 19:33:40 ah 19:33:57 so throw another log on the fire for the patch as-is, then 19:34:11 alright, so the question then becomes "Was this a bug before that got fixed, or a bug now that was introduced?" 19:34:35 both behaviours are bugs ;-) 19:34:53 It's unspecified and both behaviours negatively impact someone. 19:35:16 with sivel's fix in place, who's negatively impacted? 19:35:16 I'm with nitzmahone, though, when both choices are wrong, previous behaviour wins. 19:35:39 nitzmahone: +1 19:35:42 abadger1999: +1 19:36:08 I'm of the opinion that the current fix has a lower chance of negatively impacting someone 19:36:15 either way having -i supprt 'some_string_to_use_as_hostname' and 'some/path' is ambiguous so I think a new cli args would be right fix 19:36:19 nitzmahone: someone who has a file or directory that consists of hostnames and commas which is not their intended inventory. 19:36:23 but I coudl extend it to check for '/' 19:36:39 nitzmahone: or more exactly "not the intended inventory for this run" 19:36:45 so someone has a file called `localhost,` 19:36:59 I'd think that was less likely than the current situation though 19:37:16 and expanding it to check for `/` likely wouldn't solve anything 19:37:18 or hostnames that look like file paths 19:37:34 alikins: it would have to include a `,` to satisfy that 19:37:54 yeah, definitely a corner case, and since it used to work... 19:37:55 https://memegenerator.net/img/instances/250x250/74052180/tradition.jpg 19:37:57 wouldn't "/" (asuming paths are alwways expanded by then) be better? 19:38:16 but yeah, if the patch restores previous behaviour, warts and all, I'm fine with that too. 19:38:26 abadger1999: if you used `-i localhost,` and had a file called `localhost,` the expanded path would have / and still match 19:38:28 long term pref would be a cli arg for inventory paths and another for hostname string 19:38:50 sivel: Ah I see what you're saying... previous logic is broken. 19:39:03 previous == further up the call stack 19:39:13 execution stack. whatevs. 19:39:16 the `-i localhost,` format was always said to be "internal", but too many people use it at this point 19:39:24 ... and this is really only applicable for adhoc anyway, right (the host/file/dir confusion) 19:39:35 nitzmahone: it applied to playbooks 19:39:37 @nitzmahone no, it works for ansible-playbook too 19:40:02 Hm, never tried that 19:40:09 I often end up using ansible-playbook -i 'host1,host2' test.yml when testing things for issues. 19:40:45 anyhow... I think we now all agree sivel is doing the right thing. 19:40:46 Ok, am I good to proceed with the current implementation in that PR, or does anyone object? 19:40:58 cool 19:41:27 I see no further objections 19:41:36 nitzmahone: are you ok for this to merge to 2.5? 19:42:00 +1 19:42:06 sivel: you can backpotr to temp-staging-post-2.4.3 as well. 19:42:11 thaumos: I think we are ready to move on 19:42:23 since it's correcting a change in behaviour 19:42:25 sivel++ nitzmahone++ abadger1999++ 19:42:25 maxamillion: Karma for toshio changed to 1 (for the f27 release cycle): https://badges.fedoraproject.org/tags/cookie/any 19:42:31 #action sivel to merge 35002 to devel for 2.5 and backport to temp-staging-post-2.4.3 19:42:32 oh, it is zodbot 19:42:40 but toshio is the only one with a fas account :) 19:42:49 Yay, I win! 19:43:16 #topic new label to identify wip of issue 19:43:43 @akasurde is curious if we can do ^^ something like has_pr or working 19:44:12 context? 19:44:21 When triaging or looking through open issues 19:44:30 What's wrong with [WIP] + WIP label? 19:44:34 sometimes you have to read in depth for context if someone has submitted a PR for the issue 19:44:40 [WIP] is for PRs 19:44:54 this would indicate that an open issue has a PR 19:44:58 Ah I see 19:45:08 I use `has_pr` personally on some projects 19:45:21 Isn't that why you would link the PR in the issue? 19:45:31 jborean93: sure, but you still have to go look for that 19:45:43 yeah but still not easy to see from the issue list, which is what I think akasurde is eluding to 19:45:44 there could be 8 issues, and a single PR, you would have to go looking 19:45:55 ah yep, understand that 19:46:00 a label would be much easier to spot 19:46:07 a bot could apply if there was a link 19:46:12 yeah, I agree with sivel ... having the label there would be nice, maybe it's even something the bot could populate if there's a PR linked (though I could see that being error prone in some scenarios) 19:46:14 +1 has_pr 19:46:18 or we could label manually too 19:46:33 I'd be +1 for the label, but probably manual labeling 19:46:46 People link PRs to all sorts of crap that may or may not be accurate 19:46:46 a manually added label is better than nothing 19:46:52 "resolved_by_pr: nnn" could trigger the new label being added 19:47:07 even if the bot could add it via a !has_pr, there are a lot of options 19:47:17 gundalow++ 19:47:18 bonus +1 for has_pr_with_tests_that_pass 19:47:18 I don't like finding references to PRs to cause the label to be added, as nitzmahone said, people link crap 19:47:27 I wonder if you could get the bot to search for `Fixes: ` in a PR and retroactively but the has PR label on the issue 19:47:28 or vice versa "fixes: xxx" on the PR could add the label 19:47:33 mental jinx 19:47:49 What will people do with this label? 19:47:50 But I don't think we want to add it on just a random PR link 19:48:08 Will someone actually review all label:has_pr and do something with them? 19:48:10 I could see it being useful when looking for issues to close 19:48:16 The trouble is I don't see many people using the bot commands so it will most likely be us doing it 19:48:24 jborean93: yup 19:48:37 We could do it during traige 19:48:43 jborean93: +1 19:48:59 we have to triage new PRs anyway 19:49:05 Also for calling attention to the PR if we happen to trip over an issue with that label 19:49:48 true, I still think it would be a burden to maintain it and there will be a lot of hit and misses 19:50:02 Even if were to automate it there's going to be a lot of false positives 19:50:04 yup, that's why I'm wondering on cost vs benefit 19:50:05 nitzmahone: yes, it could help us get through some of the backlog faster if we could easily search that 19:50:27 I think something is better than nothing 19:50:30 We've got much dumber labels than that; if someone says it'll make their triage better, I'm +1 19:50:39 But maybe minimal-to-no automation to start 19:50:45 I am fine with that 19:50:47 issues containing resolved_by_pr 44 open, 284 closed 19:50:59 even if you just say sivel is the only one who does it, and it only happens on Monda :) 19:51:04 Monday 19:51:19 WFM 19:51:29 I'm not against it, just think it probably won't be used too much unless it is automated 19:51:46 jborean93: agreed, but we can get there, have to start somewhere 19:51:53 sivel: +1 19:51:55 I like it 19:52:56 who wants to start moving this forward? I can, but not sure if anyone cares specifically about label color and other fun topics :) 19:53:13 label created 19:53:21 If you don't like the color, well... tough 19:53:26 :D 19:53:28 thanks! 19:53:41 I can talk with tanner later about potential bot stuff 19:54:03 thaumos: next? 19:54:08 #done has_pr label created, potential automation to follow 19:54:14 #topic Open floor 19:54:23 create a bot called thaumos that can just queue up issues. 19:55:00 2.4.3 rc3 is scheduled for tomorrow. 19:55:09 We have a blocker bug to fix. 19:55:24 Is there any update on the progress of the blocker fix? 19:55:28 (actually a blocker bug whose fix causes a regression and thus we have to fix the fix) 19:55:33 sivel: not yet. 19:56:01 Going to get an update from jimi tonigh but don't wnat to interrupt his concentration until then. 19:56:20 thanks 19:56:24 just curious 19:56:26 #info 2.5 is in core/curated freeze 19:56:37 community plugin/module freeze is Feb 7 19:57:10 if it drags on, 2.4.3 will be the release that makes us re-evaluate how we decide to block a minor release. 19:58:06 two things to truthfully consider. 19:58:48 1. Changing Thursday's time 19:58:48 2. Me to stop being the leader of this because I don't feel like I add much value (saying it this way to troll jctanner) 19:59:20 * nitzmahone ducks out to WWG 19:59:22 * nitzmahone lurks 20:00:37 * thaumos hears crickets. 20:00:46 alright if nothing more, ending meeting 20:00:56 #endmeeting