19:00:27 #startmeeting Ansible Core Team Meeting 19:00:27 Meeting started Tue May 30 19:00:27 2017 UTC. The chair is jtanner. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:27 Useful Commands: #action #agreed #halp #info #idea #link #topic. 19:00:27 The meeting name has been set to 'ansible_core_team_meeting' 19:00:39 #chair jtanner 19:00:39 Current chairs: jtanner 19:00:45 #chair bcoca 19:00:45 Current chairs: bcoca jtanner 19:00:48 since i just saw you 19:01:06 lies! 19:01:23 who else is here? 19:01:27 * akasurde waves 19:01:32 #chair akasurde 19:01:32 Current chairs: akasurde bcoca jtanner 19:02:01 hello 19:02:04 I'm here 19:02:11 ping abadger1999 alikins funzo ganeshrn gundalow jimi|ansible nitzmahone privateip Qalthos ryansb 19:02:18 #chair shertel willthames 19:02:18 Current chairs: akasurde bcoca jtanner shertel willthames 19:03:23 ok, just gonna roll with it 19:03:34 o/ 19:03:40 #chair funzo 19:03:40 Current chairs: akasurde bcoca funzo jtanner shertel willthames 19:04:16 i can't tell if https://github.com/ansible/community/issues/159#issuecomment-301614845 is resolved or not 19:04:25 no strikethrough 19:04:59 #topic using raw strings for documenation 19:05:14 nitzmahone: i believe you were the missing party for this last discussion on this 19:05:22 the answer was 'sometimes' 19:05:28 dharmabumstead you get what you needed? 19:06:07 Awaiting my CT scan results and not really paying attention. Context? 19:06:13 sup team 19:06:21 #chair ryansb 19:06:21 Current chairs: akasurde bcoca funzo jtanner ryansb shertel willthames 19:06:37 dharmabumstead: related to using r''' for raw text in module docstrings 19:07:06 ^ basically, use when you need \X to not be interpreted... mostly with windows examples 19:07:20 Works for me. As long as the text is within shouting distance of grammatically and mechanically correct. :) 19:07:43 so i would add to dev docs as a hint, but not a requirement 19:07:55 * bcoca does not want 300 PRs add r''' .... 19:07:58 I actually am at the doctors office and will have to bail any minute here 19:08:12 #action no requirements, but should add a hint in docs 19:08:21 alright, moving on 19:08:40 #topic Ansible Contributor Summit 4 (Part of AnsibleFest 2017 London) 19:08:41 * abadger1999 get sout of previous meeting 19:09:17 is this something we need to discuss? 19:09:23 looks like an announcement 19:09:29 gundalow: ^ 19:09:38 #chair abadger1999 19:09:38 Current chairs: abadger1999 akasurde bcoca funzo jtanner ryansb shertel willthames 19:10:02 hi 19:10:13 oh, you made it ... working from pub? 19:10:15 =P 19:10:26 #chair gundalow 19:10:26 Current chairs: abadger1999 akasurde bcoca funzo gundalow jtanner ryansb shertel willthames 19:10:30 Yep, I'd concur- use raw string when necessary, but not required 19:10:36 #info Please add topics and final voting (+1/-1) to https://public.etherpad-mozilla.org/p/ansible-summit-june-2017 19:10:37 #chair nitzmahone 19:10:37 Current chairs: abadger1999 akasurde bcoca funzo gundalow jtanner nitzmahone ryansb shertel willthames 19:10:47 jtanner: pfft, I wish 19:11:49 so ...? 19:11:59 anything specific to note about contributors summit? 19:12:01 next topic 19:12:11 [20:10] <@gundalow> #info Please add topics and final voting (+1/-1) to https://public.etherpad-mozilla.org/p/ansible-summit-june-2017 19:12:13 I think gundalow has noted something specific 19:12:23 oh, duh 19:12:52 jtanner: :P 19:13:03 #topic https://github.com/ansible/ansible/pull/24822 19:13:22 akasurde: ^ please lay out it for us 19:13:59 24822 also raises an interesting question around what constitutes authorship 19:14:17 jtanner, I tried it CentOS and RHEL it works with downgrading and upgrading using --security option 19:14:50 does a 14 line PR put you on the authors list, or is it just that akasurde should now be considered a primary maintainer of yum? 19:15:14 jtanner, I will write unit test for this and will remove WIP 19:15:15 #info akasurde tested PR with downgrade + upgrade pattern 19:15:26 willthames, I will love to 19:15:32 #action akasurde to write integration test 19:15:43 jtanner, Thanks 19:15:55 willthames: can you add topic to agenda? 19:16:47 #topic https://github.com/ansible/ansible/pull/24867 19:16:56 bcoca: you wanted to vote on this 19:17:13 dag: you around? 19:17:14 jtanner done 19:17:20 tnx 19:17:41 i'm guessing today's mega merges broke this PR 19:18:27 maybe we should just vote another day? .... 19:18:55 lgtm after conflict resolved 19:19:38 willthames: add shipit to PR. doesn't seem like voting is going to happen today 19:19:58 needs another commit so shipit is a bit pointless no? 19:20:06 didnt we vote against that last meeting? 19:20:06 as my shipit bit gets ignored after commit 19:20:09 it's not gonna get automerged anyway 19:20:34 willthames: normal, you cannot 'shipit' future changes that you have not seen 19:20:39 bcoca: nothing recorded in meeting comment 19:20:45 yep, I understand that bcoca 19:20:49 jtanner: i remember vote 19:21:02 result was "no" ? 19:21:37 afair 19:21:42 if it's a no, there should be better objections in the PR 19:22:17 #note previous vote is thought to have been to reject PR 19:22:22 ^ someone was supposed to update after meeting, there were other objections .. dont remember them 19:22:39 #action decide on action for PR and provide sufficient reasoning 19:23:21 #topic https://github.com/ansible/ansible/pull/24871 19:23:30 "do we allow 'none/null' values on required parameters?" 19:23:34 bcoca: you added this one 19:23:51 thought that was also answered 19:23:58 the previous logs have all the discussions https://meetbot.fedoraproject.org/ansible-meeting/2017-05-25/ansible_core_meeting.2017-05-25-15.00.log.html 19:23:59 ^ seems someone dropped ball 19:24:04 for this one and previous one 19:24:22 answer was 'yes' we allow, each module should then deal with values itself, but not forbidding them in arg_spec 19:24:31 the meeting "issue" is too confusing, so can't blame anyone 19:25:00 also ... supposed to have rotated each month ... and not everyone updates it 19:25:11 19:25:22 #action vote results were "yes" at last meeting 19:25:34 #note vote results were "yes" at last meeting 19:26:05 #topic https://github.com/ansible/ansible/issues/25053 19:26:10 willthames: this one is yours 19:26:13 it is! 19:26:24 There's a PR linked from it now 19:26:31 there is also petition to eliminate the warning 19:26:34 what's the ask? 19:26:37 / discussion? 19:26:43 https://github.com/ansible/ansible/issues/25092 19:26:45 ^ several tickets opened/closed, thread in ansible-devel 19:27:01 i think we should discuss removing/not first, as that might make this PR obsolete 19:27:20 I think the PR fixes the warning in the expected way 19:27:27 #note i think we should discuss removing/not first, as that might make this PR obsolete 19:27:34 I'm not sure anyone wants the warning removed entirely? 19:27:42 willthames: still not needed when you are doing double interpolation on purpose 19:27:45 #note https://github.com/ansible/ansible/pull/25092 19:28:00 when: x == {{value_of_var_in_this_var}} 19:28:17 willthames: some do 19:28:43 i kinda think the example in the PR is a bit contrived and may not show how complicated this can get 19:29:01 you might have a {{}} 3+ levels deep 19:29:05 jtanner it's supposed to be a trivial example showing that the current warning is way over the top 19:29:12 if you want to remove the warning, that's fine 19:29:23 but currently the warning is stupid as anything 19:29:45 bcoca, you added the warning didn't you? can't you just remove it if you don't like it? 19:29:55 nope 19:29:57 i di dnot 19:30:10 https://github.com/ansible/ansible/issues/22397 19:30:38 https://groups.google.com/d/msg/ansible-project/W_zIjyhCFPg/eLrTWO4XCgAJ 19:30:52 so the primary issue here is that we munge a new template out out of the conditional and we don't want to have {{ }} in the source? 19:30:57 ^ many more 19:31:37 jtanner: the issue is that {{}} in when means 'interpolate', which is not always what user wants as the when already interpolates, so its only for x2 indirection of var names 19:31:43 ^ confusing 19:31:57 when: "{{a}} == b 19:32:19 ^ if user meant 'var a' == b, its wrong, ifi user meant 'var that has its name stored in a', then its right 19:33:20 this is one of those things that always frustrates me 19:33:35 i have to look at code to understand what's going on behind the scenes in when: 19:33:38 bcoca: does it hurt for the user to do when: {{a == b }} ? 19:34:03 If it can never hurt, I lean towards warning should jus go away. 19:34:04 bcoca, you're right in that you didn't add the warning but one of your commits did make it trigger when I didn't expect it to 19:34:09 abadger1999: no ... unless you have var named True/False 19:34:35 willthames: when i fixed template detection? 19:34:54 https://github.com/ansible/ansible/commit/4594bee65ae made it far worse 19:35:40 well, in my defense it DOES properly detect that templating will work on that string 19:36:38 #note issue relates to https://github.com/ansible/ansible/commit/4594bee65ae 19:38:06 I'm happy with sivel's fix (the one I put in my PR) - it'll fix 99% of cases and if you're doing when: "{{ a }} 19:38:08 " == b 19:38:18 then you probably know what you're doing! 19:38:20 bcoca: any concerns with that? 19:38:49 shifts the warning message right above ... 19:38:54 probably better than reverting the relevant bit of 4594bee65ae 19:38:56 if conditional in all_vars and VALID_VAR_REGEX.match(conditional): 19:38:56 conditional = all_vars[conditional] 19:38:56 Yeah, I think we should accept the PR if we don't want to remove the warning. 19:38:58 i thogyht the whole reason for the warnings was for when: {{a}} == b 19:39:09 and the false postive was a: {{c}} when: a == b 19:39:18 we have tests for those scenarios? 19:39:36 bcoca, isn't that what the PR provides? 19:39:45 im confused now 19:39:50 you would get a warning for when: {{ a }} == b 19:39:55 but not for the false positive 19:40:03 ah, that is what i thought you just said would NOT happen 19:40:19 the PR probably explains it better than I do (I hope) 19:40:23 i think we dont need warning either way, which would make the PR obsolete 19:41:36 sivel are you about? 19:41:44 so just remove the warning block altogether? 19:42:05 that is what the PRs and ansible-devel thread were about 19:42:12 ^ why i said we need to discuss that first 19:42:19 No, not really. Driving home from doctors appointment 19:42:28 original PR: https://github.com/ansible/ansible/pull/20312 19:42:30 table this for another day? 19:42:53 can do I suppose 19:42:58 bcoca: ? 19:42:59 needs sivel here really 19:43:02 Using jinja in a when is the cause for a number of problems we handle on list and in irc 19:43:21 don't want to be encouraging driving and ircing 19:43:22 I'm +1 to removing the warning altogether. 19:43:22 This helps with that, but of course seems to cause confusion 19:43:29 sivel: understood, but now we handle problem of 'warnings when using when:' 19:43:31 sivel: okay, we'll wait for a meeting where you can chat more aobout it 19:43:38 plz no crashing 19:43:47 +1 19:43:50 19:43:53 if we table that +1 for merging willthames' for now 19:43:59 as tabling == keeping for now 19:44:07 #action discussion tabled for the next meeting sivel is able to attend 19:44:35 https://github.com/ansible/ansible/pull/24974 19:44:46 https://github.com/ansible/ansible/issues/22397 19:45:09 what are those? 19:45:11 bcoca: +1 19:45:27 https://groups.google.com/d/msg/ansible-devel/tjIpsz8VDtY/KEp1qwR6AAAJ 19:45:33 ^ people complaingin about the warning 19:45:48 oh ... so merge https://github.com/ansible/ansible/pull/25092 now? 19:46:04 im on fence, but most cannot seem to make meetings, so told them i would express their view 19:46:23 ^ i get as many bothering me in list/irc for warning as i did for when: {{a}} ==b misunderstandings .... 19:46:55 possibly because the warning doesn't provide enough context 19:47:05 or way to disable it 19:47:09 #info bcoca proposed merging 25092 (do not warn when a variable itself contains a template) now and discussing removal of warning at later meeting. 19:47:13 +1 to that proposal 19:47:36 +1 to merge now, +1 revisit removal in later meeting 19:47:41 +2 also 19:48:15 so ... should i press the green button then? 19:48:23 * dag likes to discuss https://github.com/ansible/ansible/pull/24867 before the end of this meeting 19:48:25 Do it. seems uncontroversial 19:48:39 yep, if we cannot discuss warning itself, this will help make it less noisy 19:48:58 merged, thanks willthames + sivel 19:49:03 #action https://github.com/ansible/ansible/pull/25092 merged 19:49:37 dag: trying to move as quick as i can 19:49:43 #chair dag 19:49:43 Current chairs: abadger1999 akasurde bcoca dag funzo gundalow jtanner nitzmahone ryansb shertel willthames 19:49:43 jtanner: sure 19:50:29 dag: we actually already went over 24867 19:50:44 #note previous vote is thought to have been to reject PR 19:50:44 #action decide on action for PR and provide sufficient reasoning 19:50:46 jtanner: yes, but the resulting feedback makes no sense :) 19:51:15 and I wasn't involved in the discussion 19:51:36 mikedlr: you around? 19:51:52 if no mikedlr, i think we can go back to 24867 19:52:14 mikedlr is in the UK, so probs logged off already 19:52:44 #topic https://github.com/ansible/ansible/pull/24846 19:52:56 #info mikedlr wants PR feedback 19:53:24 hi; 19:53:44 speak the name call the person. 19:53:47 mikedlr: seems like discussion is happening on the PR 19:54:18 anything specific you wanted to talk about? 19:54:33 I think the thing is that having looked at that module it should really have someone, probably from core team, want to do some serious fixing. 19:55:04 it has some serious inefficiencies; e.g. it makes three calls to dpkg as an external program for every package it examines. 19:55:37 the current structure has become a bit complex and difficult to deal with 19:55:41 Someone who knows the dpkg/apt python bindings would be even better. 19:55:45 I'm not too worried about module inefficiencies 19:55:54 we probably need to get some maintainers on the module too 19:55:56 packaging/os/apt.py: ansible 19:55:58 I wrote a unit test for it but I don't feel it's really reasonable to merge. 19:56:01 anyone who is seriously worried will fix them, right? :) 19:56:06 mikedlr: are you interested in maintainership? 19:56:29 not maintainership for me. I don't reliably enough use dpkg and know rpm much better. 19:56:37 same here 19:56:45 i make due with apt+dpkg when i have to 19:57:12 but if a new (even temporary) maintainer came along I'd work with refactoring to get unit tests merged. 19:57:20 #info apt module has no maintainers outside of the core team 19:57:58 unless there is someone from core who is going to take it on, perhaps consider making it curated/community ? (this meshes nicely with my next topic) 19:58:02 gundalow strongly suggested bringing it up here to see if someone from the core team would be able to take an interst.. 19:58:20 I looked over that Pr in an earlier incarnation and thought it would be okay to merge. 19:58:29 mikedlr: so the apt module needs refactoring first (before adding new unit tests) ? 19:58:37 abadger1999: the PR IMHO is fine. but it's very limmited 19:58:37 i can "fix" anything, but deciding on rearchitecture/refactor on apt is not my deal 19:58:41 can't speak to the larger underlying architecture issues because I'm in the same boat... know rpm better than apt 19:58:42 Pilou: exactly; 19:58:45 #chair mikedlr Pilou 19:58:46 Current chairs: Pilou abadger1999 akasurde bcoca dag funzo gundalow jtanner mikedlr nitzmahone ryansb shertel willthames 19:59:11 sorry, got called away 19:59:15 i'll look at apt 19:59:26 the unit test I wrote works and seems correct but because the function is so large it will be horribly brittle. 19:59:33 mikedlr: are you mandating arch? wouldnt that screw with noarch/any packages? 19:59:39 or multiarch installs 19:59:53 ok, so in the context of https://github.com/ansible/ansible/pull/24846, the patch is really small 19:59:57 and includes new tests 20:00:15 in the PR I have made a set of proposed changes https://github.com/ansible/ansible/pull/24846#pullrequestreview-39463773 20:00:18 I didn't crash, and I made it home safe 20:00:26 sivel: nice 20:00:31 evgeni /will not be able to 20:00:36 do those. 20:00:52 sivel: any drive you walk away from... ;-) 20:01:14 mikedlr: dont know api that well, will it still find packages with no arch/any? 20:01:15 I was at a stop light when I messaged, so not too dangerous 20:01:21 jtanner: the new integration tests should alwaus perform cleaning tasks but this isn't the case 20:01:24 mikedlr: how much work do you think the refactoring is going to be? 20:01:47 without a maintainer, i think a refactor is going to have to be handled like any other PR 20:01:54 dag is familiar with that sort of thing 20:01:55 not too much for the stuff I know about ; day or two. I think that once the person started it they would want to do more. 20:03:20 there are integration tests already which makes it easier to do reliably 20:03:31 ok, well i don't think we're going to get anything in terms of heavy lifint from the core side beyond code review / merging 20:03:34 ah, nvmd this only affects deb files, which should have arch in name if followin standard, 20:03:36 +1 20:03:48 to patch, have not looked at tests 20:04:11 mikedlr: might need to add those to 'destructive' 20:04:45 mikedlr: you work with evgeni? 20:04:50 Evgeni says current tests are marked destructive; I think that's going to be true for most of them. 20:04:59 20:05:02 that should be ok then 20:05:26 * bcoca does not know where those are marked anymore .. used to be destructive.yml play .... 20:05:51 somewhere buried in ansible-test 20:05:53 should be good to merge afaict 20:06:13 mikedlr: does this fix whatever you came across it for? 20:06:42 jtanner: did some work with him and would support him. He made a comment that he's probably not able to take on the refactoring 20:06:54 understood 20:07:01 in terms of this small patch, it works for you? 20:07:13 jtanner: should not change install, but should change detection .. were we getting false negatives when using .deb? 20:07:16 jtanner: don't understand the comment. I just came across this on Evgeni's request for a review and tried to write a unit test as part of reviewing. 20:07:44 ah, i thought you might have run into the problem is was fixing 20:07:58 s/is/it 20:08:26 #info related issue https://github.com/ansible/ansible/issues/24673 20:08:37 anyone oposed to merge? 20:09:06 #info related PR https://github.com/ansible/ansible/pull/24703 20:09:26 bcoca: +1 20:09:59 #action bcoca merged PR 20:10:03 I support merge as is without unit test (but with integration test) 20:11:10 mikedlr: since we don't really have committment from anyone on refactor, can you open bugs on what you think is wrong? 20:11:20 we can then mark needs_contributor 20:11:28 jtanner: sure; will do. 20:11:31 thanks 20:11:47 it will also give me a place to put a link to the commits with my unit test. 20:11:49 #action mikedlr to open bugs on refactoring possibilities so they can be marked needs_contributor 20:13:11 dag: you still here? 20:13:36 * mikedlr wonders away but will read log later. 20:13:43 jtanner: yes 20:13:49 #topic https://github.com/ansible/ansible/pull/24867 20:13:54 jtanner: I updated the PR with the rationale 20:14:03 #info cycling back to topic for dag 20:14:23 why a simple issue became a fix touching a few core modules + integration tests 20:15:00 (and why splitting the PR makes no sense, because it is all tied together) 20:15:23 sivel: you around still? 20:15:27 bcoca: ^ 20:15:34 I am 20:15:42 i don't remember the previous vote / discussion 20:16:13 It was brief. I mentioned it looked like it hit too much code at once 20:16:29 As we looked, we believed a targeted change for win_chocolatey could be made independent of the other changes 20:16:37 https://meetbot.fedoraproject.org/ansible-meeting/2017-05-25/ansible_core_meeting.2017-05-25-15.00.log.html from 15:16:00 onwards 20:16:46 jtanner ^^ 20:17:01 There are also formatting changes in the PR, that aren't really necessary iirc 20:18:02 sivel: The fix for win_chocolatey is: https://github.com/ansible/ansible/pull/24867/files#diff-76ffc0551f8cf3d6255500316568e60bL575 20:18:18 that change will cause command/shell/script etc to behave differently from before 20:18:27 so we need to add that logic in the module 20:18:45 and as a result, the integration tests fail because they were testing things that they shouldn't 20:18:52 Is the `rc` of win_chocolately helpful in any way? 20:19:07 sivel: yes, it provides feedback to the user 20:19:39 but don't question the one module, question the default behavior 20:20:20 if a module explicitly returns failed=False, ansible shouldn't be changing that 20:20:39 my opinion is, that things should continue to work, without making larger changes to things outside of the most minimal possible 20:20:52 (the reason it does, is because it may not know if the module returned it explicitly, or it was added implicitly) 20:21:01 right 20:21:05 and that's what we did 20:21:45 I think a change is fesable that only impacts win_choclately, and maybe few lines elsewhere 20:22:04 allow things to continue working as is, except supporting rc=!0 and failed=False 20:22:10 15:36:41 #action thaumos to update #24687 asking dag to make this pr for only changed/failed presence. 20:22:10 15:37:12 #action thaumos to also ask dag to create individual prs for rc issue on win_* and everything else. 20:22:16 which shouldn't impact any other modules 20:22:17 did that ever get relayed to you dag? 20:22:31 it did not 20:22:42 sivel: it's not possible, unless you want to add something like if module.name == win_chocolatey inside ansible 20:22:50 Okay, I've reviewed the non-test changes and they look good. If this is just a matter of breaking up or not, I think is good to merge. 20:23:04 jtanner: it did, and that's why I asked to reconsider 20:23:10 +1 on merge ... if tests pass. I can't think why not 20:23:43 sivel: the win_chocalatey fix seem to be the part requireing removal of - if result.get('rc', 0) != 0: 20:23:43 - result['failed'] = True 20:23:50 from task-executor. 20:23:59 Can't do that without the other changes going in as well. 20:24:05 crud, tests failed 20:24:28 I guess if the first PR was the task-executor and all the module and test changes, then the win_chocolatey fix could come after that. 20:24:31 I can remove the cosmetic changes 20:24:44 they are non-PEP8 compliant anyway 20:24:58 dag do you want to followup on #ansible-devel after you are done? 20:25:07 sure 20:25:15 s/was the task-executor and all the modules/was all the modules/ 20:25:30 #action dag to remove cosmetic changes in PR and follow up on ansible-devel 20:25:43 5 more min left? 20:26:42 willthames: you still here? 20:26:46 yep 20:26:51 it's like dawn here 20:27:12 #topic supported_by field in aws modules 20:27:27 seems there are a lot of curated aws modules 20:27:29 shertel: ryansb this one is probably for you guys to discuss 20:27:42 not sure, would like core input too 20:28:12 I believe ryansb is in another meeting 20:28:15 most of the core team is in favor of marking almost everything community 20:28:25 do we even have enough committers contributing to aws modules 20:28:34 willthames no 20:28:38 I'm focussing on aws, but same might be true of loads of other subsections 20:28:47 there is a way to improve the numbers... 20:28:56 which numbers? 20:29:04 but would still like to make most things community reviewed anyway 20:29:13 jtanner the number of committers working on ansible 20:29:16 aws sorry 20:29:18 #info willthames would like aws modules to be "community" supported 20:29:24 willthames: there is dedicated 'cloud' group, mostly they are responsible for those 20:29:28 or at least the majority 20:29:51 bcoca what is the cloud group? 20:29:57 you! 20:30:01 heh 20:30:06 shertel and ryansb alone 20:30:07 technically, curated vs community is only differentiated by bot automerge 20:30:25 it only means that 'human with commit' does final push 20:30:28 shipit process should still apply 20:30:34 getting the shipit tag is a bar 20:30:50 if it relies on committer shipit 20:30:52 a high bar? 20:30:53 willthames: that same bar applies to community 20:31:07 I can add shipit to community and have that count 20:31:11 willthames: no, not commiter (they can) shipits works same as with community, only diff is automerge or not 20:31:11