09:00:03 <gundalow> #startmeeting Ansible Community PR Review 09:00:03 <zodbot> Meeting started Thu Nov 29 09:00:03 2018 UTC. 09:00:03 <zodbot> This meeting is logged and archived in a public location. 09:00:03 <zodbot> The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot. 09:00:03 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic. 09:00:03 <zodbot> The meeting name has been set to 'ansible_community_pr_review' 09:00:31 <gundalow> #chair robertdebock felixfontein Xaroth alexsaezm davegarath 09:00:31 <zodbot> Current chairs: Xaroth alexsaezm davegarath felixfontein gundalow robertdebock 09:00:49 <Xaroth> I like how zodbot sorts case sensitively. 09:01:20 <gundalow> I never noticed that before 09:03:08 <Xaroth> side note; I'm working on acozine's request of nuking out the srtd theme from the docs, but since `make webdocs` takes quite a while, I'll jump in and help out as much as I can. 09:03:37 <gundalow> #info Thank you all that are here, for out first (of many?) big PR review days. The aim is to 1) give new community members a place to learn and understand Ansible's review (and merge) process 2) Improve the docs and process 3) Give some PR love 4) Identify PRs we think we can merge or close. 09:03:46 <gundalow> Xaroth: yup, that takes a while, thanks for looking into that 09:04:04 <gundalow> #info We will be ignoring `label:new_plugin` instead focusing on improving what's already there 09:04:24 <gundalow> #info https://github.com/ansible/community/issues/407 contains more information. 09:05:10 <gundalow> #info During this if anyone identifies any "If we did X could help", just add that here. Or if there are specific things you can do `#action Document X on page Y` 09:05:26 <gundalow> #info filter is `is:pr is:open -label:backport -label:needs_triage -label:new_module label:small_patch` 09:05:32 <gundalow> https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch 09:06:08 <gundalow> Would people like to work through from the top of the list?, so starting with 49235 09:06:35 <Xaroth> might be better to start oldest first 09:07:28 <gundalow> That's a good point, and one we've gone back and forth a fair bit. Newest first is good if we've like it merged (as the PR author is likely still around), oldest first is good for closing 09:08:09 <gundalow> So starting at the top 09:08:15 <Xaroth> top it is 09:08:20 <gundalow> docker_swarm_service: ensure idempotency when the user, publish and update_order parameter are not specified #49235 09:09:02 <gundalow> * PR by the module author (as shown by label:owner_pr) 09:09:22 <gundalow> Anyone here use Docker? 09:09:30 <alexsaezm> Me 09:10:42 <gundalow> alexsaezm: Are you aware of https://github.com/ansible/community/issues/408 felixfontein and others have been making great progress on progressing the various Docker PRs and adding tests 09:10:59 <robertdebock> felixfontein has recently requested feedback for the author, it's not a dead pull request, so maybe it does not require further attention right now? 09:11:12 <gundalow> robertdebock: yup, I think that's fair 09:11:40 <gundalow> Use System.Xml.XmlDocument.Load rather than Get-Content #49206 - Looks like it's in hand, skipping 09:12:09 <gundalow> azure: fix keyvault key secret not support cloud environment #49182 09:13:04 <gundalow> change looks sensible, though I don't know anything about Azure, anyone else? 09:13:19 <felixfontein> regarding #49235: there's still some discussion going on in the issue which this PR solves as well 09:13:27 <robertdebock> could be tested, I don't have an Azure account. 09:14:12 <gundalow> Will leave that for #ansible-azure 09:15:10 <gundalow> #chair cybette 09:15:10 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felixfontein gundalow robertdebock 09:15:13 <gundalow> Morning 09:15:24 <gundalow> k8s: Fix error where pod has error and no containers #49154 09:15:50 <gundalow> So we can see that one of the maintainers have given this a `shipit` 09:16:34 <gundalow> Change looks sensible, I'll merge (unless anyone says otherwise) 09:17:55 <robertdebock> lgtm 09:19:18 <gundalow> Remove possible parenthesis for media type in OpenBSD #49128 09:19:30 <gundalow> Going to mark ^ as WIP as they want to fix another bug 09:20:30 <gundalow> rabbitmq_user: Specify key to use while sorting permissions #49126 09:20:38 <gundalow> No one has looked at this yet 09:21:50 <gundalow> Is ^ a good use of Python? 09:23:25 <gundalow> I've added some comments 09:23:31 <gundalow> ACME: add basic unit tests #49103 09:23:53 <Xaroth> looks like sane Python to me (re: #49126) 09:23:53 <gundalow> felixfontein: I think this is good, shall I merge? 09:24:11 <gundalow> Xaroth: Thanks, could you please put that on the PR 09:25:28 <gundalow> Comments on the PR make are useful for the next person that looks at it 09:25:55 <gundalow> Extended cloudfront_facts module to have a predictable return value… #49061 09:27:19 <robertdebock> Looks good, but not tested, nor automated tests. 09:27:32 <gundalow> robertdebock: which PR 09:28:02 <gundalow> oh, I wonder if we should prefix everything with PR number, this may get confusing as people find a PR of interest and start reviewing it 09:28:04 <robertdebock> Extended cloudfront_facts module to have a predictable return value… #49061 09:29:19 <robertdebock> That's a plan, if comments relate to a PR, prepend with #pr_number. 09:29:42 <robertdebock> #49061: Looks good, but not tested, nor automated tests. 09:30:52 <gundalow> 49061: Missing from `RETURN`, to be consistent I think it should be `distribution_id`, comments added 09:31:27 <robertdebock> 49061: good feedback. 09:31:49 <felixfontein> gundalow: yes, you can merge the ACME unit tests if you want :) 09:32:05 <gundalow> #info documenting RETURNS is something that is often overlooked, important to check that 09:32:40 <gundalow> 49103 ACME unit tests, merged. Thanks felixfontein 09:32:57 <gundalow> IAM module: Also return user_name on create, to be more consistent. #49059 09:33:11 <gundalow> 49059: No review comments so far 09:34:25 <gundalow> #chair ganeshrn 09:34:25 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow robertdebock 09:34:29 <gundalow> ganeshrn: Afternoon :) 09:34:42 <ganeshrn> Morning :) 09:34:51 <gundalow> 49059 I haven't worked out what this is doing yet 09:35:53 <robertdebock> 49059: I looks like the returned data includes user_name after this pull request, sounds like a small improvment if you want to loop over results of creates users. 09:36:09 <gundalow> 49059 ah, so it's closer to line 762, 770, 774 09:37:15 <gundalow> 49059: Only adding new, not changing existing (ie not breaking backwords compatibility). Knowing nothing about iam. I think this is good to merge 09:39:03 <ganeshrn> +1 09:39:19 <robertdebock> 49059: The username should always be returned, I'll request why only on create/ 09:39:32 <gundalow> robertdebock: +1 09:39:48 <robertdebock> 49059: If it's only returned on create, it's difficult to loop over the results later. 09:40:09 <gundalow> Good spot on usability there :) 09:40:53 <robertdebock> 49059: commented in github. 09:41:20 <shaps> morning 09:41:29 <gundalow> #chair shaps 09:41:29 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow robertdebock shaps 09:41:33 <gundalow> Morning :) 09:42:00 <gundalow> ovirt_storage_connection: comparing passwords breaks idempotency in update_check #48933 09:42:03 <gundalow> No comments so far 09:44:19 <shaps> don't see anything wrong with it though 09:44:22 <shaps> and it makes sense 09:45:22 <robertdebock> 48933: looks good to me, but I have no way of testing it. 09:47:08 <shaps> Can't test it either, it's just a 1 line in a list of equal(...) though, should be fine 09:47:55 <robertdebock> @shaps, please add the pr-number in front of your remarks, so it's easy to relate what you are referring to. 09:48:18 <gundalow> #action gundalow merge 48933 in a day unless negative reviews come through 09:48:29 <robertdebock> 48933: well done. 09:48:33 <gundalow> 48933: request for other views added 09:49:09 <gundalow> ec2_vpc_nacl: Don't default name into Name unless able #48889 09:49:22 <webknjaz> felixfontein: a bit late, but raw-string literals look odd @ 49103 09:49:34 <gundalow> #chair webknjaz 09:49:34 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow robertdebock shaps webknjaz 09:49:38 <gundalow> webknjaz: Morning :) 09:50:00 <webknjaz> Morning 09:50:44 <gundalow> 48889: In 48832 johnboy2 has given clear steps to reproduce and shown his working, so I think this is good 09:51:35 <robertdebock> 48889: agreed, looks good. 09:51:36 <felixfontein> webknjaz: is there a difference between r and R? 09:53:28 <gundalow> webknjaz: bah, already hit merged on 48889. I guess code is good, though it's useful to know 09:53:29 <gundalow> hum 09:54:05 <gundalow> I'm wondering if we should just do `#action merge 1234` which means if others look at stuff we will merge during review 09:54:15 <webknjaz> I'm not sure whether uppercase R prefix works, never seen it uppercase. But probably fine 09:54:29 <gundalow> Thoughts? 09:54:32 <gundalow> #chair jborean93 09:54:32 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow jborean93 robertdebock shaps webknjaz 09:54:37 <webknjaz> Sounds good 09:55:13 <robertdebock> 49059: Feed back from contributor: https://github.com/ansible/ansible/pull/49059 09:55:15 <gundalow> #agreed Rather than merging during this session we will do `#action merge pr:1234`, this avoids merging a PR that someone else maybe busy studying 09:55:38 <gundalow> robertdebock: nice work :) 09:56:15 <gundalow> os_server: server.security_groups is a list of dicts #48798 09:56:24 <felixfontein> webknjaz: I think I've used both extensively, and both work 09:57:05 <gundalow> 48798 emonty and gtema have given this a shipit, so I think this is good 09:57:16 <felixfontein> webknjaz: just tested with Python 2.7 and 3.6, both accept both cases 09:57:16 <gundalow> #action merge 48798 09:58:07 <gundalow> #action gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`? 09:58:44 <gundalow> Since we've had a few people, join we are working throughhttps://github.com/ansible/ansible/pulls?q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc though skipping over PRs that look like they are in hand 09:59:01 <gundalow> templar: ensure that exceptions are handled, fix 'AttributeError' #48792 10:00:01 <webknjaz> felixfontein: what about 3.7? Anyway, it's not our style: having consistent code style improves code readability. Code is read way more than it's written. 10:00:51 <robertdebock> 48792: bcoca already approved the pr. 10:01:16 <gundalow> #action merge 48792 10:01:35 <gundalow> #action AWS to review and merge 49061 10:01:54 <gundalow> #action Network to review 41107 10:02:06 <gundalow> ^ Just some notes I'd made before 10:02:29 <gundalow> Fix win_nssm credentials quoting #48761 10:02:37 <gundalow> Powershell 10:03:27 * gundalow isn't going near that 10:03:27 <jborean93> It's not really the right fix, it will help in this case but there's a better way 10:03:36 <felix__> Hi 10:03:38 <gundalow> ah, theres a man that knows 10:03:42 <gundalow> #chair felix__ 10:03:42 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 robertdebock shaps webknjaz 10:03:59 <jborean93> I'll do a review tonight/tomorrow to try and nudge the person in the right direction 10:04:01 <gundalow> felix__: Welcome, we are working throughhttps://github.com/ansible/ansible/pulls?q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc though skipping over PRs that look like they are in hand 10:04:11 <gundalow> action jborean93 48761 10:04:14 <gundalow> #action jborean93 48761 10:04:16 <gundalow> Thanks 10:04:20 <jborean93> nw 10:04:41 <felix__> I'll be there 10:04:50 <gundalow> win_reboot: Use a less error-prone command to get uptime #48666 10:05:23 <jborean93> we don't want to do that 10:05:26 <gundalow> jborean93: 48666, given last comment, should this PR be rejected (or maybe discussed in Windows Working Group) 10:05:40 <robertdebock> 48666: Why would you want to check uptime on a reboot? 10:06:07 <jborean93> we can't check the port in case port forwarding in use so we compare the uptime before and after issuing the reboot to validate it actually rebooted 10:06:29 <alexsaezm> Duty calls, knowing that there are an AWS and a Docker community group, I'll try to participate. Thanks for the initiative! 10:06:45 <robertdebock> Thansk @alexsaezm! 10:06:47 <jborean93> 48666 is an issue because Packer uses their own connection plugin for Windows and it breaks this scenario 10:07:22 <gundalow> alexsaezm: Thank you for joining us :) 10:07:26 <jborean93> gundalow: so a thanks but no thanks for 48666 10:08:33 <felixfontein> webknjaz: I don't have 3.7 on this machine, but why should it stop working there? that would be a huge backwards compatibility break. anyway, I wasn't aware of that; is there a code style guide somewhere? 10:08:43 <gundalow> jborean93: ack, you happy to put some words on and close? Does a bug need raising against https://github.com/hashicorp/packer 10:09:10 <jborean93> Packer has https://github.com/hashicorp/packer/issues/6453 10:09:16 <jborean93> I'll close 48666 10:09:51 <gundalow> Thanks 10:10:05 <gundalow> #action jborean93 to add words & close 48666 10:10:09 <gundalow> rajeevarakkal: Hi :) 10:10:58 <rajeevarakkal> hi 10:11:36 <gundalow> rajeevarakkal: You joining in the Bug review? 10:11:47 <gundalow> rabbitmq_binding: Add support for state=absent #48599 10:11:58 <gundalow> 48599: Feature request, no comments added so far 10:12:17 <gundalow> #info For future Bug Reviews should we focus on bug fixes? 10:12:41 <rajeevarakkal> This my firt time over here.. i would like to discuss about my PR 10:12:58 <rajeevarakkal> Am i in right channel? 10:13:24 <robertdebock> @rajeevarakkal, what PR? 10:13:35 <rajeevarakkal> https://github.com/ansible/ansible/pull/46675 10:13:49 <gundalow> #chair rajeevarakkal 10:13:49 <zodbot> Current chairs: Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz 10:14:03 <gundalow> Contributing DellEMC Install Firmware Modules to Ansible #46675 10:15:13 <gundalow> rajeevarakkal: ah, thanks for addressing my comments. I'll review that again now. I get a lot of GitHub email, so I personally only get through it (at most) once a week 10:15:25 <webknjaz> felixfontein: well, u-prefix got deprecated over time combined with another prefix, for example. It's just that I never ever saw uppercase prefixes used. And I have quite a bit of Python experience. If in doubt, I always recommend using PEP8 and PEP257. 10:15:25 <webknjaz> It's just my internal compass. 10:15:25 <webknjaz> For multipython env I use pyenv, it gets you lots of pythons in userspace, you can try it out to have py37 :) 10:15:44 <rajeevarakkal> Thanks John :) 10:16:14 <shaps> #48599 looks sensible 10:16:43 <gundalow> shaps: Can you put that on the PR please 10:16:47 <shaps> sure 10:17:08 <gundalow> (good to put it here as well, though GitHub is the permanent record) 10:17:35 <shaps> yep yep, just still a bit low on caffeine, will get there :) 10:17:50 <Pilou> about #48599, should not rabbitmq integration tests be updated ? 10:17:55 <gundalow> shaps: the quiet patch was me running to make more tea :) 10:18:19 <gundalow> Pilou: oh, I didn't even look to see if there were some. That would be good 10:18:21 <gundalow> #chair Pilou 10:18:21 <zodbot> Current chairs: Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz 10:18:24 <gundalow> Pilou: Morning :) 10:18:42 <robertdebock> 46675: It looks like all review items are done and the tests are passing. 10:19:10 <Pilou> hi :) 10:19:23 <gundalow> rajeevarakkal: Are you from Dell EMC? Your module code looks good. I'll trust that you know OpenManage. 10:19:47 <rajeevarakkal> yes, you are right robert, so far all tests passed. 10:20:07 <gundalow> #action 46675 merge 10:20:07 <rajeevarakkal> Yes we are from DellEMC .. 10:20:25 <gundalow> rajeevarakkal: Looks good, I'll merge later (that gives anyone else here time to add comments if they spot anything) 10:20:36 <rajeevarakkal> Thanks a lot John :) 10:20:41 <shaps> Pilou, I don't see tests for rabbitmq_binding 10:20:44 <Pilou> there is a setup_rabbitmq target which is used by other rabbitmq_ targets but there isn't a rabbitmq_binding target 10:21:47 <gundalow> rajeevarakkal: If you plan on add other modules, you may wish to set yourself as a `maintainer` of `module_utils/remote_management/dellemc/` and `lib/ansible/modules/remote_management/dellemc/` in https://github.com/ansible/ansible/blob/devel/.github/BOTMETA.yml 10:22:16 <gundalow> OK, lets look back to rabbitmq 10:22:19 <gundalow> rajeevarakkal: anything else? 10:22:50 <rajeevarakkal> Nope.. I think we are good :) 10:24:41 <gundalow> rajeevarakkal: Excellent. Please do shout out in #ansible-devel if there is anything else https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/roadmap/ROADMAP_2_8.rst has the Ansible 2.8 dates on. Any new modules MUST be merged before 2019-03-21 to be included in Ansible 2.8 10:25:08 <webknjaz> rajeevarakkal: I've posted a few concerns about the public API (return value) in your PR 10:25:31 <gundalow> webknjaz: Thanks 10:25:55 <robertdebock> 46675: semantics here, your module is called "dellemc_install_firmware", that indicates an action, not a state. Ansible typically expresses a state. I would expects a module name like dellemc_firware... 10:26:06 <gundalow> Pilou: shaps what's your thoughts for rabbitmq 10:26:12 <gundalow> robertdebock: oh, yes. i totally missed that 10:26:31 <Pilou> setup of rabbitmq is already handled by "setup_rabbitmq" target (which is the boring/complicated bit) 10:26:32 <robertdebock> I'll add it to the github pull request for others to read. 10:26:33 <gundalow> This is why many eyes is good 10:28:05 <shaps> also setup_rabbitmq seems to only setup ubuntu? 10:28:22 <Pilou> for example #44718 which added a new rabbitmq_publish reuse setup_rabbitmq 10:29:30 <Pilou> shaps: there is no technical reason for that, i was able to run the tests on other distro (but i didn't made a PR for that yet) 10:29:37 <gundalow> You both OK to look into that, and I'll continue (keep chating here)) 10:30:31 <shaps> Pilou, Cool so you working on non-ubuntu setup? :) 10:30:52 <gundalow> rabbitmq_binding: Add support for state=absent #48599 10:31:22 <gundalow> 48599 This looks way too simple, which makes me think something is missing 10:31:27 <shaps> In any case re- #48599 we can ask contributor if he wants to add tests, although should not hang the PR if he doesn't want to, tests take some time 10:31:43 <gundalow> shaps: agreed 10:32:12 <shaps> gundalow, the actual bidning removal was already there, it looks like just the check he's added was missing 10:32:34 <gundalow> shaps: ah, yes on line 209 10:32:45 <shaps> correct 10:32:52 <gundalow> 48599 Just stange that it wasn't in the original version 10:33:06 <Pilou> shaps: ok (but then should not a reviewer perform a manual test ?) 10:33:06 * gundalow looks at the PR that added the code 10:33:27 <shaps> gundalow, not sure why, the docs say 'only present implemented' though 10:33:45 <gundalow> shaps: that line has been removed 10:34:09 <shaps> yes, was referring to current module :) 10:34:30 <shaps> Pilou, yes I agree, although that seems a fairly trivial change? 10:34:55 <gundalow> shaps: I think because without the PR line 205 doesn't run (as change_required = false) 10:35:14 <shaps> yep 10:35:27 <gundalow> #action 48599 merge 10:35:33 <shaps> It's shown in the output, before/after 10:35:50 <shaps> I believe that should actually be considered a bug, since the module accepted state=absent 10:36:02 <shaps> but did nothing about it 10:36:22 <shaps> Also, I'll add comment to PR to ask for tests, see what happens 10:36:28 <gundalow> shaps: +1 10:37:03 <gundalow> Avoid duplicated recursive calls in inventory CLI #48598 10:37:23 <gundalow> 48598 Looks sensible bcoca has approved 10:37:31 <robertdebock> 48598: Everything is approved, tests pass. 10:38:15 <gundalow> webknjaz: 48598 look good to you? It's support:core, so extra set of eyes would be good 10:38:28 <gundalow> #action Core to review & merge 48598 10:38:48 <gundalow> rabbitmq_binding: Fix using empty routing key #48597 10:38:55 <gundalow> 48597 No reviews yet 10:42:27 <gundalow> 48597 Think need someone that knows rabbitmq to review that 10:42:54 <gundalow> Changed single file zip write parameter for arcname to use os.path.ba… #48285 10:43:50 <robertdebock> 48397: I have no way to test reading the authors remarks, looks realistic. Is there no better python way to test for an empty string? 10:44:49 <robertdebock> 48397: `self.routing_key != ''` 10:45:05 <gundalow> 48285: is for `archive` I've requested the tests be extended 10:45:54 <AndyG> is there any simple or documentation RP that needs fixing? i can try something easy :) 10:46:17 <gundalow> #chair AndyG 10:46:17 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz 10:46:33 <gundalow> AndyG: Hi. Would you like something so you can understand the GitHub work flow? 10:46:43 <AndyG> sure 10:48:14 <shaps> robertdebock, I guess you meant 48597? 10:49:04 <robertdebock> @shaps, correct. sorry. 10:50:42 <shaps> 48597 there's no need for that if...else... the routing_key has a default value defined ( # ), if the value needs to be ~ then I'd rather change the default 10:50:51 * shaps is adding a comment/review to the PR 10:51:53 <webknjaz> 48598: just one nitpick 10:52:21 <gundalow> AndyG: https://raw.githubusercontent.com/ansible/ansible/devel/docs/docsite/rst/dev_guide/developing_modules_general_windows.rst See how it has 10:52:21 <gundalow> ``` 10:52:21 <gundalow> .. contents:: 10:52:21 <gundalow> :local: 10:52:21 <gundalow> ``` 10:52:22 <gundalow> That is the correct form 10:52:22 <gundalow> The old (incorrect form is) `.. contents:: Topics` (and perhaps without `:local:`) 10:52:23 <gundalow> Fixing that in all the rst files under `docs/docsite/rst/dev_guide/` would be really useful 10:53:37 <gundalow> AndyG: in a single PR 10:54:36 <gundalow> Hope that makes sense 10:55:20 <AndyG> thanks, let me take a gander 10:58:26 <gundalow> Add the ability to download a specific version of an S3 object #47867 11:00:44 <robertdebock> 47867: looks valid and simple. 11:00:52 <shaps> #47867 akasurde already approved the changes too :) 11:01:15 <webknjaz> Added readability suggestion 11:02:01 <gundalow> Thanks 11:02:08 <robertdebock> 47867: good, that PR is alive again. 11:02:31 <gundalow> 47867: I want to Feature, then back to bug as it should have worked 11:02:43 <gundalow> robertdebock: alive again? 11:02:58 <robertdebock> alive, not dead. (?) 11:03:09 <gundalow> robertdebock: was the `again` that confused me 11:03:22 <robertdebock> ;-) 11:03:57 <gundalow> DING DING DING If you review a PR can you put "a comment, if it's `I've looked at the code and this seems sensible, though i've not tested it`" - Yes, I need to do the same 11:05:20 <robertdebock> Good one @gundalow. For 90% of the contributors IRC is the same as the dark-web. ;-) 11:05:32 <gundalow> If it's not on GitHub it doesn't count 11:06:12 <webknjaz> true 11:06:51 <robertdebock> I'll be back in a bit, lunch time for me. 11:07:01 <gundalow> robertdebock: Thanks for everything so far :) 11:08:15 <gundalow> Update ``make`` module honor the return code of the ``make`` command #47861 11:08:21 <gundalow> I see shaps has already commented 11:09:18 <shaps> yep, and got no reply :( 11:13:22 * gundalow has gently poked 11:14:08 <gundalow> module_utils/mysql: Fixing unexpected keyword argument 'cursorclass' error after migratio… #47809 11:17:27 <shaps> #47809 to keep consistency, I would add a HAS_PYMYSQL in the try/except then check for that, instead of having 'if mysql_driver is sys.modules...blah' 11:17:45 <gundalow> shaps: oh, yes, much more readable, could you add that 11:17:50 <shaps> yep adding 11:18:19 <gundalow> Add default nolog param for PreSharedKey argument in ec2_vpc_vpn.py module #47788 11:18:57 <gundalow> #action merge, + backport PRs for 47788 11:23:02 <gundalow> #info When people put `alternative fix #1234` should we get better at closing the other PR, or making it clearer that possibly the PR author should do that - example 47733 11:23:42 <gundalow> webknjaz: Seeing lots of PRs where maintainer isn't being pinged :( 11:25:16 <gundalow> OK, let's do another 15 minutes then regroup 11:25:58 <gundalow> Fix GCE and scaleway VM detection #47193 11:26:50 <gundalow> winem_: Hi, you here for the PR review session? 11:27:05 <gundalow> 47193: Looks straight forward 11:27:37 <winem_> Hi gundalow, yes. winem at github 11:27:43 <gundalow> #chair winem_ 11:27:43 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz winem_ 11:27:45 <gundalow> Welcome :( 11:27:46 <gundalow> :) 11:27:53 <gundalow> urgh, typing is difficult :( 11:28:20 <winem_> that's why we automate everything, right? :) thanks ;) 11:28:21 <gundalow> #info unless we enforce changelogs via CI we aren't going to get them 11:29:06 <gundalow> winem_: Exactly, we are currently going through `label:small_patch` PRs. This is all very informal, so feel free to chat :) 11:29:31 <Xaroth> I found adding changelog fragments to be the easier part of my PR, but I understand how it's often overlooked. 11:29:47 <gundalow> Xaroth: Yup, I think that's a fair summary 11:30:51 <gundalow> 47193 I think that's good to merge, though needs changelog 11:32:09 <gundalow> #action merge (changelog) and backport 47193 11:33:26 <gundalow> #action merge (needs changelog) 47134 11:35:35 <gundalow> hostname.py: Fix openSUSE distribution name #47020 11:35:41 <gundalow> Anyone use SUSE? 11:35:48 <robertdebock> Yes, 11:36:07 <robertdebock> I'm also annoyed by Leap and Tumbleweed as ansible_distibution 11:36:09 <gundalow> robertdebock: Wondering if we need to check for both names? 11:36:59 <gundalow> Would you be able to take a look at 47020 and put some comments on the PR? 11:37:06 <robertdebock> I guess the ansible_distribution is openSUSE, and the ansible_distribution_major_version is Leap or Tumbleweed. 11:37:16 <robertdebock> #action review 47020 11:37:28 <gundalow> #action robertdebock review 47020 11:37:33 <gundalow> Thanks :) 11:37:51 <gundalow> #chair 11:37:51 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz winem_ 11:38:05 <gundalow> So, we've been through ~60 PRs, great work 11:38:12 * davegarath lunch break 11:38:32 <gundalow> Lets take a bit of time to think how it's been and if we want to change anything before continuing 11:39:05 <gundalow> 1) Missing changelogs is going to stop a lot of these PRs from being merged 11:39:58 <gundalow> 2) I wonder if we should ignore `label:needs_revision`. This label is applied by the Ansibulbot if CI is failing OR rebase_needed OR someone left a review comment 11:40:56 <jborean93> It's pretty trivial to create a changelog ourselves if that's the only thing stopping a merge. The question probably is whether we want the original author to create the changelog 11:41:59 <gundalow> jborean93: I guess my concern is it's an extra loop around. If the PR will get `label:shipit` it's OK. though for some of the examples we've been though I'm not sure when the PR would be seen again 11:43:34 <gundalow> If the PR is < n-days old I think it's fine to ask the PR author, though if it's month(s) old going "hey this PR we've ignored till now is good apart from you need to do this simple task" might not be the best thing 11:44:06 <jborean93> I mean we can do the changelog for those PRs 11:44:23 <jborean93> if we are ready to merge and all that's needed is a changelog and it's an older PR 11:44:32 <gundalow> Sounds good 11:45:17 <gundalow> More generally, are people finding what we've done over the last couple of hours useful? What suggestions do people have 11:46:40 <robertdebock> I think it's useful. I do notice some modules are very specific and it's not easy to determine if the PR is working or not. 11:46:58 <robertdebock> Some modules require specific hardware to run against for example. How would you test that in CI? 11:48:48 <gundalow> a) We don't 11:48:58 <gundalow> b) We `mock` out the API 11:49:12 <webknjaz> gundalow: maintainer not pinged as in module autor? 11:49:13 <gundalow> c) we use simulators, like we do for foreman, VMware, etc 11:49:19 <webknjaz> author* 11:49:41 <gundalow> webknjaz: yup https://github.com/ansible/ansible/issues/48785 11:50:08 <webknjaz> well.. I could shift my focus to that issue 11:51:47 <gundalow> webknjaz: That would be really useful 11:52:30 <gundalow> Thank you :) 11:52:33 <webknjaz> :) 11:53:10 <gundalow> As you are one of the few people than can fix that. And by fixing it you get maintainers working. That's a community force multiplier right there :) 11:53:43 <robertdebock> lol 11:55:36 <webknjaz> gundalow: changelog note: PyPA crew has a bot (GitHub) for this check and it's very easy to copy that thing. I could tackle that as well. I believe that this kind of check should live outside of Shippable but still report to GH via Checks API. 11:56:17 <gundalow> webknjaz: Starting next week I plan to shift more of my focus on to Bot/Workflow stuff. We can chat then 11:56:51 <webknjaz> I don't want this to be a part of ansibullbot repo. But sure, let's chat. 11:57:25 <gundalow> webknjaz: Sure. I'm happy to use existing tooling if it works, or we update that tooling so it works for us 11:57:27 <webknjaz> I recall we wanted to talk about it a while back 11:57:58 <gundalow> Yup, other stuff got in the way 11:58:01 <gundalow>  11:58:01 <gundalow> 11:58:01 <gundalow> Upload a fileChoose Files 11:58:03 <gundalow> bah 11:58:08 <gundalow> Support hostvars in openstack.py dynamic inventory #46683 11:58:10 <webknjaz> heh 12:00:18 <shaps> sorry just a quick going back to #48597, the guy raises a good point, if you pass an empty string as routing_key, you'd end up with the issue he's fixing, wasn't there a way to disallow/set-to-None empty string args directly from argument_spec? 12:04:30 <gundalow> would be in `module_utils/basic.py` if there was, though I'm not aware of anything 12:08:46 <winem_> sorry guys, I have to handle a big boooooom in an customer environment with a really shitty network so I can not pay as much attention as I would like to. please ping me directly when it comes to #48729 if I miss it. that would be great! 12:09:02 <resmo> hi 12:10:25 <gundalow> winem_: Sure :) 12:10:29 <gundalow> #chair resmo 12:10:29 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal resmo robertdebock shaps webknjaz winem_ 12:10:32 <gundalow> resmo: Afternoon :) 12:12:21 <gundalow> Fix Unicode-objects must be encoded before hashing #46608 12:12:53 <gundalow> jborean93: 46608 looks good 12:13:47 <rbarlik> part 12:14:39 <gundalow> #action Azure to review& merge 46608 12:16:46 <gundalow> sets admin_password to no_log in na_ontap_cifs_server #46604 12:17:03 <gundalow> #action merge & backport 46604 12:17:42 <robertdebock> 46604: lgtm 12:18:34 <gundalow> [aws] Retry Autoscaling Group delete calls when scaling activity is in progress #46124 12:19:27 <webknjaz> winem_: just added public API improvement notes/suggestions to your 48729 12:21:21 <gundalow> #action AWS to review and merge 46124 12:21:29 <gundalow> resmo: we are going through https://github.com/ansible/ansible/pulls?page=3&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc 12:21:39 <winem_> webknjaz: thanks. I have mixed feelings about that. on one hand it sounds like a beautiful pitfall, yes 12:21:48 <gundalow> Make description as optional parameter in jira module #46023 12:21:51 <resmo> gundalow: aye 12:22:16 <akasurde> gundalow, need add changelog and description in #46023 12:22:34 <akasurde> I will comment in PR 12:22:40 <gundalow> Thanks 12:24:35 <robertdebock> 46023: Looks good to me, except the changelog. 12:24:48 <gundalow> Fix incorrect processor information in Setup #45870 12:24:54 <gundalow> ^ Windows 12:25:08 * gundalow updates description 12:25:50 * akasurde no power in Windows section 12:26:10 <gundalow> ditto 12:26:46 <gundalow> #info I may look at doing Working Group specific triage days if we can find a time that we can get a good set of people together 12:27:28 <gundalow> Makefile: build rpm git parameters #45718 12:29:33 * gundalow assigns to people that should know 12:30:22 <gundalow> sensu_silence: Set expire to int #45706 12:30:39 <gundalow> hehe, https://github.com/ansible/ansible/pull/45706/files == https://github.com/ansible/ansible/pull/33239/files 12:30:43 <akasurde> yes 12:30:52 <shaps> duplicates 12:31:04 <akasurde> LGTM 12:31:07 <shaps> #33239 is from last year though, so probably should have precedence :) 12:31:51 <webknjaz> 45718: lgtm actually 12:32:42 <gundalow> #action merge 33239 12:32:52 <gundalow> #action close 45706 12:34:09 <gundalow> Add check for existence of key in the old config. #45438 12:34:42 <gundalow> I see webknjaz has added some comments already. I wonder if we want this fix even without tests? 12:35:23 <webknjaz> gundalow: 33239/45706 note: we should credit that work to both guys via `Co-Authored-By` trailer in such cases 12:35:32 * gundalow needs to afk to sort lunch out, would someone be interested in running it https://github.com/ansible/ansible/pulls?page=3&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc we are upto 45438 12:35:59 <robertdebock> 45438: Can the title be updated to better reflect lxd? 12:36:02 <gundalow> akasurde: you OK to run? 12:36:13 <akasurde> gundalow, I am head to home 12:36:17 <gundalow> akasurde: OK 12:36:23 <akasurde> heading* 12:36:40 <akasurde> webknjaz, Can you help us out here ? I will be back in 20 min or so 12:36:56 <webknjaz> 45438: it was just a high-level look-through during triage, I don't really know how that module works 12:36:59 <gundalow> robertdebock: You OK to run for 20 minutes? 12:38:06 <robertdebock> Sure. 12:38:13 <gundalow> robertdebock: Thanks :) 12:38:22 <evrardjp> o/ 12:38:28 <gundalow> #chair evrardjp 12:38:28 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath evrardjp felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal resmo robertdebock shaps webknjaz winem_ 12:38:44 * resmo 5min coffee break 12:54:27 <robertdebock> So, next up: 12:54:28 <robertdebock> clarify registered vs facts #45432 12:56:09 <robertdebock> I agree to bcoca's feedback; the original text is not good, but the suggested text is not very strong either. 12:57:35 * gundalow returns 12:59:43 <gundalow> robertdebock: Thanks :) 13:00:08 <evrardjp> agreed with bcoca too 13:00:39 <evrardjp> I am fine with the adaptation myself, it's already an improvement. 13:00:45 <robertdebock> 45432 If we close this merge request, the author will be demotivated, but the merge is not good enough in my opinion. 13:00:46 <gundalow> 45432 assigning to acozine needs wording improving. If anyone has any ideas on how better to phrase this please add to the PR 13:00:56 <robertdebock> Let's ask the author to rewrite in GitHub. 13:01:07 <gundalow> robertdebock: That's one of the issues in reviewing 13:01:43 <gundalow> #info http://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ is an interesting read on how to give review feedback 13:02:09 * robertdebock reading 13:02:34 * resmo re 13:03:40 <gundalow> Shall we move on? 13:04:12 <gundalow> fix swallowing line no in template syntax errors #45397 13:06:16 <gundalow> I've poked 13:07:35 <robertdebock> Ooh, I like 45397... 13:08:45 <gundalow> os_keystone_endpoint: endpoint_interface, not interface #45216 13:08:45 <robertdebock> Cool, next? 13:08:59 <gundalow> `new_contributor`, I've politly checked in and seen if they need help 13:09:50 <gundalow> #action gundalow to speak to Monty and see what help I can give to get `label:openstack` moving 13:09:57 <gundalow> monty = emonty on github 13:11:00 <evrardjp> gundalow: I am supposed to help there. 13:11:15 <evrardjp> well me and others. 13:12:28 <gundalow> evrardjp: We can maybe find some other people to help out :) 13:12:57 <evrardjp> ofc, let me invite a few other ppl in those meetings, I see other maintainers are not here :p 13:13:21 <gundalow> evrardjp: Ace, thanks 13:13:56 <gundalow> AnsibleModule.set_mode_if_different: handle symlink is in a sticky directory #45198 13:14:01 <gundalow> One from Pilou :) 13:15:07 <gundalow> 4 line change, two of which are comments. +100 lines of integration test, + unit test 13:15:08 <gundalow> MERGE 13:15:25 <gundalow> #action merge 45198 13:15:51 <robertdebock> 45198 is a very good contribution! 13:16:14 <gundalow> Pilou is a great contirbutor 13:16:22 <gundalow> chandan_kumar: Hi, you here for the PR review? 13:16:47 <Xaroth> 45198 : +1 from me. 13:17:03 <gundalow> Xaroth: Thank,s please add that to GitHub 13:17:13 <gundalow> fix archive module can not compress a single file in a tar file #45117 13:17:21 <gundalow> 45117 Has +1 from maintainer 13:17:22 <Xaroth> already done :) 13:17:29 <gundalow> Xaroth: :) 13:17:41 <gundalow> You may notice I'm on auto pilot with some responses :P 13:17:59 <Xaroth> I noticed 13:18:19 <gundalow> mnaser: Hi :) 13:18:29 <robertdebock> 45117 Ran into this too, lgtm 13:18:58 <mnaser> Hey gundalow o/ 13:19:10 <evrardjp> thanks for joining mnaser 13:19:11 <gundalow> mnaser: You here for PR review? 13:19:24 <gundalow> ah, OpenStack, though I reconised the name :) 13:19:29 <evrardjp> gundalow: I asked him to join, due to PR 45216 13:19:34 <gundalow> #chair mnaser 13:19:34 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath evrardjp felix__ felixfontein ganeshrn gundalow jborean93 mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 13:19:46 <evrardjp> gundalow: :) 13:20:08 <evrardjp> gundalow: we are spread thin, but with many of us, it helps. 13:21:11 <gundalow> evrardjp: every little bit helps. Happy to arrange something on IRC with various people to go through just the openstack issues & PRs. Even if that's 2 minutes per item and decude keep/close 13:21:16 <gundalow> Whatever works for you 13:21:21 <robertdebock> Just a question in between, inspired by pilou's merge request (45198): Is there a place to add 'negative tests'? Tests that should fail? 13:21:42 <gundalow> robertdebock: Excellent question. 13:22:15 <winem_> robertdebock: good question, asked me the same yesterday :) 13:22:19 <mnaser> Is there a way to find the ones I haven’t triaged? 13:22:39 <gundalow> #info negative testing can be done in integration tests, include `ignore_errors: yes` with `register: response` and check `response.failed` and `responsed.message` contains something 13:23:02 <robertdebock> Ah, yes. That's a valid use case for ignore_errors. 13:23:21 <winem_> gundalow: hmm isn't that possible already ? 13:23:26 <gundalow> mnaser: https://github.com/ansible/ansible/labels/openstack contains everything OpenStack related. 13:23:33 <gundalow> winem_: that? 13:24:04 <winem_> have a test case with ignore_errors, register the result and assert: that: my_test is failed? 13:24:14 <gundalow> winem_: yup, works today 13:24:15 <winem_> or do I miss anything here? 13:24:36 <winem_> ah sorry.. missed that it wasn't a question or todo, it was a statement 13:24:42 <winem_> sorry. go ahead :) 13:26:12 <gundalow> #info Example of testing negative case https://github.com/ansible/ansible/blob/devel/test/integration/targets/vyos_smoke/tests/cli/common_utils.yaml#L32-L43 [ADD TO DOCS] 13:26:25 <gundalow> robertdebock: winem_ ^ 13:27:08 <robertdebock> @gundalow, thanks, cool! I've achieved my goal; learned something today! 13:27:15 <gundalow> \o/ 13:28:24 <gundalow> 45117 `archive` does that need an integration test? 13:28:44 <gundalow> robertdebock: did you say you've hit 45117 before, does that fix work for you? 13:29:09 <gundalow> archive & unarchive are `support:community` though they do a lot 13:29:19 <robertdebock> I'll try it out. Give me a moment, I'll report in the GitHub PR. 13:30:27 <gundalow> Thanks 13:31:04 <gundalow> Handle !!binary data from YAML in JSON conversion #45099 13:31:13 <Xaroth> 45099: lacks tests, possibly documentation (and changelog fragment) 13:31:55 <gundalow> Yup, it's `support:core` so really should have those things, you OK to update the PR? 13:32:24 <Xaroth> I'll try to ask him kindly to add test cases for this 13:32:32 <gundalow> Thanks :) 13:32:55 <gundalow> DNS Made Easy: Fixed TXT record handling #44678 13:33:02 <gundalow> I see resmo has looked at this already 13:34:25 <mnaser> gundalow: know of a github filter to see if i havent commented on any of them because i know i did a whole bunchw hen i was added 13:34:26 <gundalow> 44678 author to add unit tests 13:35:12 <gundalow> mnaser: `-commenter:gundalow` should work 13:35:33 <gundalow> https://help.github.com/articles/searching-issues-and-pull-requests/#search-by-commenter not tested it 13:35:56 <gundalow> #info https://help.github.com/articles/searching-issues-and-pull-requests/ has *loads* of different ways to search/filter issues & PRs 13:37:48 <gundalow> 44678 - updated to be WIP 13:39:00 <robertdebock> 44968: Updated the issue and PR, lgtm, shipit, merge etc. 13:39:36 <gundalow> robertdebock: hehe, thanks. 13:39:40 <gundalow> #action merge 44968 13:39:50 <gundalow> #undo 13:39:50 <zodbot> Removing item from minutes: ACTION by gundalow at 13:39:40 : merge 44968 13:40:07 <gundalow> #action merged 45117 13:40:51 <gundalow> Corrects the problem of having the special "+" character in the URL #44055 13:40:55 <mnaser> gundalow: thanks for that. there's 26 open ones which i havent commented so ill get onto those now :) 13:41:17 <gundalow> mnaser: Excellent, if their are any you'd like me to close just let me know 13:41:45 <gundalow> 44055: `support:core` Needs tests 13:42:09 <mnaser> can we remove openstack tag from 48012? 13:42:53 <robertdebock> 44055: Indeed, needs tests. 13:43:38 <robertdebock> 48012: Indeed openstack seems irrelevant. 13:43:56 <robertdebock> I'm back in 15 minutes or so. 13:44:28 <gundalow> mnaser: removed from 48012, it's because they didn't use the template (so component matching got confused) 13:44:46 <gregdek> Morning gundalow, where's the stack, and what needs doing? 13:44:54 <gundalow> #chair gregdek 13:44:54 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 13:45:14 <gundalow> gregdek: We are going through https://github.com/ansible/ansible/pulls?page=3&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc currently on 44055 13:45:37 <mnaser> can we close 20314 because 29031 already merged the work 13:46:16 <gundalow> We are not merging anything, instead we are doing `#action merge #1234` as we've had a few cases where I've got "LGTM <merge>", then someone smarter has said "what about X" 13:47:14 <gundalow> For things we think could be closed, though we'd like core to review we can add `label:candidate_to_close`, or use `#action` 13:49:31 <gundalow> 44055; -1, marked as needing tests 13:50:28 <gregdek> 44005 looks like a simple PR that's low risk and has lots of details in the PR. Is it suitable for merge? 13:53:35 <gundalow> gregdek: I think so, I've pinged the maintainers (bot issue so that isn't always happenign) 13:53:46 <gundalow> #action merge 44005 13:54:16 <gundalow> ignore empty lines in rabbitmqctl output #43722 13:54:58 <gundalow> #action merge 43722 13:55:20 <gundalow> #action backport 43722 13:55:24 <gundalow> (as I've just hit merge) 13:55:52 <gundalow> Bug in grafana basic auth handling #43717 13:55:52 <gundalow> 43717 closing as fixed elsewhere 13:57:20 <gundalow> mnaser: I've closed 20314 13:57:44 <gregdek> 43151 looks like another simple one that just changes a regex. 13:58:39 <gregdek> with lots of explanation about why it's needed. 13:59:01 <gregdek> Do we have particular criteria for recommending merge other than "we're sensible and this looks sane?" :) 14:00:07 <gundalow> gregdek: not really. Though another advantage of using `#action` is that others can review before hitting merge 14:00:25 <gundalow> Lot of this is because don't have (enough) maintianrs typing `shipit` 14:00:48 <gundalow> #action Ricky to review & merge 43151 14:01:38 <gundalow> [git] Better filtering of unknown files. #43359 14:01:38 <gundalow> * 43359, I agree with commentm, not sure how portable `--untracked-files=no` is 14:02:21 <gundalow> 43359 -1 to prevent merge 14:03:25 <gundalow> Fix potential bad entry #43148 14:05:36 <gundalow> 43148L: updating title, marking as WIP 14:06:00 <gundalow> Cast scaling factor to int #42862 14:06:57 * robertdebock is back. 14:08:41 <robertdebock> 45198 merge conflicts. 14:09:11 <gundalow> Fix list k8s objects #42558 14:09:20 <gundalow> 42558 merge conflicts 14:09:44 <gundalow> #chair bcoca 14:09:44 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm bcoca cybette davegarath evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 14:09:51 <gundalow> Morning Brian :) 14:11:14 <bcoca> mrnin 14:12:09 <gundalow> 42558 comments added, not sure if it's still needed 14:12:37 <gundalow> ========================== 14:12:53 <gundalow> I'm wondering if we should filter out `needs_rebase`? 14:13:18 <gundalow> unless we want to review for closing PRs. Though we have only closed ~2 PRs so far 14:13:48 <robertdebock> Can ansibot update the author? 14:14:13 <robertdebock> on merge conflicts. 14:14:22 <gregdek> #action 42239 merge 14:14:36 <gregdek> Because it's basically just an update of static metadata. 14:14:39 <gregdek> Am I doing it right? :) 14:14:56 <gundalow> robertdebock: it *should* be doing that 14:15:30 <robertdebock> You are - once again - correct. 14:15:55 <gundalow> gregdek: Yup, FYI, only `42239 merge` will end up in the action log 14:16:24 <gregdek> Oh, I guess that's backwards, yes. 14:16:38 <gregdek> How is the action log being processed at the end? Is it just you hitting merge on all of them? :) 14:16:44 <bcoca> merged 14:16:58 <bcoca> dont know why that was not done during triage 14:17:25 <gregdek> That's a good, and separate, question. Is our triage process missing stuff? 14:17:33 <gundalow> Will review action log tomorrow/next week and go through and merge and backport as needed 14:18:08 <gregdek> There have been a lot of 2- and 3-liners here that would have looked like simple merges, but I don't know how the triage process works these days and what its goals are... 14:18:09 <bcoca> gregdek: it just changed, in this case it was the 'old process' and i would say, this should have been merged on the spot 14:18:15 <gregdek> Ahhhh ok 14:19:44 <gundalow> Allow fuse type mounts without :/ in device string #41502 14:20:07 <gundalow> dericcrago: Hi, you here for the PR review session? 14:20:54 <bcoca> tempted to not allow this, we alrady have enough problems since we allowed nfs/network systems .. setup keeps hanging on those .. adding fuse to the mix .... *shudder* 14:21:19 <jtanner> 42239 was mislabeled community, and therefore wouldn't have been looked at during tirage 14:21:23 <dericcrago> yes, I thought I'd check it out, even if that means, just standing in the back of the room :) 14:21:34 <gundalow> #chair dericcrago jtanner 14:21:34 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm bcoca cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 14:21:46 <bcoca> jtanner: not current triage, but it was old triage, which should also have caught the mislabeling 14:23:14 <robertdebock> So, 41502 needs tests to prevent hanging setups? 14:23:16 <gundalow> dericcrago: Welcome, we are going though https://github.com/ansible/ansible/pulls?page=4&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc currently on 41502. Don't let the fact that we are moving through fairly fast stop you from looking at things. When we talk we generally make sure we have the PR number in the sentance, as we end up looking at many things at once. 14:23:16 <bcoca> ^ which is one worry i have with 'current', mislabled PRs 14:23:44 <gundalow> bcoca: there were also some bot issues so inheritence of `support:` wasn't working 14:23:49 <gundalow> labels 14:24:11 <gundalow> 41502 added `candiate_to_close` 14:24:18 <dericcrago> thanks gundalow 14:25:23 <gundalow> dericcrago: also, just shout out at any point, this is all very informal. As we put on https://github.com/ansible/community/issues/407 one of the thigns we are after is input from people that are new(er) to this process 14:25:24 <bcoca> robertdebock: not sure how, this is somethign we have not been able to solve for nfs/network cases as the process goest into 'B' state in which signals/timers are ignored 14:25:58 <gundalow> Passing comment, I see achinthagunasekara add lots of +1, I not personally count that vote 14:26:10 <bcoca> gundalow: i would also 'not close' but put it penging a redesign on how fact gathering works 14:26:47 <gundalow> bcoca: OK, I was going to feed `candiate_to_close` into Core's backlog meeting 14:26:59 <gundalow> Adding note on async+check mode incompatibility #41427 14:27:21 <gundalow> bcoca: 41427 is there some confusion in the last two comments that needs resolving? 14:28:27 <bcoca> 'fail' the task would also be wrong, proper way to do it is skip tasks when they dont support check mode 14:29:18 <bcoca> clarified in ticket 14:30:19 <samccann> it's unclear to me whether the user has to 'do' something to make sure async is skipped in check mode, or if this is just a note to say async WILL BE skipped in check mode 14:30:34 <bcoca> its a docs pr 14:30:35 * samccann waves 14:30:43 <bcoca> shoudl reflect what happens 14:30:52 <samccann> yep, and I'm a docs person, unclear which way to go :-) 14:31:37 <bcoca> also, its not as 'as ansible 2.3', afaik its never worked with async 14:32:26 <bcoca> damn, he is right, its actually an error ... 14:32:38 <bcoca> unsure if i should change that at this point 14:33:32 <gregdek> jtanner this makes me wonder if we're essentially doing the community equivalent of the core triage here. 14:33:34 <bcoca> k, +1 to merging, might want to edit out 2.3 version afterwards 14:33:50 <gregdek> Is the process for core triage documented to the point that we could basically lift it? 14:33:57 <AndyG> can you remove me from chair please, i just don't have the time to contribute right now (sorry) 14:34:10 * robertdebock is going to focus on something else, it's been a pleasure, John, thanks for organizing! 14:34:11 <bcoca> gregdek: old one is, new one is 'in progress' 14:34:55 <jtanner> people have different definitions of "triage" 14:35:17 <gregdek> Is this true within the core team? 14:35:26 <bcoca> gregdek: this seems more of a resolution process (analog to our also new backlog review) than triage 14:35:42 <jtanner> it was true within the core team, and new process+meeting aims to solve that 14:36:11 <gundalow> #help 14:36:15 <gundalow> #halp 14:36:22 * gundalow stabs zodbot 14:36:24 <gregdek> Does it make sense to have an analogous community driven process for community PRs? 14:36:44 <gregdek> Is zodbot down? 14:37:12 <gundalow> #halp 14:37:21 <bcoca> gregdek: the main difference between the 2 is 'which list are we looking at', triage focuses on newest issues, while backlog starts from oldest 14:37:27 <gundalow> #topic Review! 14:37:35 <gundalow> nah, just can't remember the comand 14:38:24 <bcoca> the old triage was mostly 'check labels are correct, identify high priority and route those, deal with low hanging fruit' 14:38:45 <gundalow> AndyG: robertdebock Thank you so much for your time! 14:39:18 <gundalow> gregdek: as of ~last week there isn't any triage of non `support:core` 14:39:42 <gregdek> Right. 14:40:04 <gundalow> I'm not sure yet what (if anything) the replacement will be 14:40:05 <gregdek> That's what I'm saying. There should be, even if it isn't done by the core team. The process itself is sound if we can recruit people to do it. 14:40:20 <gregdek> Like, maybe that's in essence what we're doing now. 14:41:09 <gregdek> It may not have the same precise goals, but "deal with low hanging fruit" is certainly a key thing to do. 14:41:11 <bcoca> gregdek: in most cases the support:community tickets will have notified the 'known maintainers/authors' so unless you are just checking for correctness/orphans im not sure how much you can do in normal triage 14:41:21 <gundalow> Possibly, that's one of the side things to find out from todays session 14:42:03 <gregdek> I've looked at 3 PRs. Two were obvious simple merges, one was a simple closure because it was fixed elsewhere. Small sample size, but still. :) 14:42:05 <bcoca> just having a 'is this an orphan' process would be very nice 14:42:15 <gregdek> Yep. 14:42:32 <mrproper> gundalow: We were 14:42:35 <bcoca> yeah, a lot of these seem to have gotten through due to the problems with old triage not having a good detail on 'common rules' of what to do with tickets 14:42:39 <gundalow> #action gundalow consider raising adding to Bot plan: command to raise backport after merge (if it contains changelog) `rebase_merge_create_backport`) 14:42:43 <mrproper> Whoops. We were going through https://github.com/ansible/ansible/pull/48999 and I wanted to talk to you about it a little more. 14:43:13 <jtanner> you want the bot to rebase also? 14:43:17 <gundalow> Meraki scenario guide - describe how to merge new data with old data #48999 14:43:40 <mrproper> gundalow: I just commented. I thought I commented yesterday, but apparently it didn't go through. 14:43:52 <gundalow> jtanner: not sure, I'm going to start thinking about workflow, CI and Bot work startingnext week 14:44:23 <gundalow> #chair bizonk 14:44:23 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm bcoca bizonk cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 14:44:28 <gundalow> morning bizonk :) 14:44:42 <bizonk> Good morning! 14:45:12 <gundalow> mrproper: 48999: Did you see my comment about extra `-`? 14:45:54 <mrproper> gundalow: Yes I did. I just replied. Basically, I need to merge all the data and I can't merge a list and a dict so I wrapped the dict in a list. However, if there's a more elegant way to do this, I'd be happy to use that instead. 14:46:22 <gundalow> uuuuuurm, not sure 14:46:54 <mrproper> In the example I have two lists that I need to merge with the dict. That's why the extra - is there. Yes, it's a little unintuitive. 14:47:11 <gundalow> mrproper: ah, OK, maybe add a comment to make that clearer? 14:47:14 <chopraaa> Hey 14:47:23 <chopraaa> What pr are we on? 14:47:25 <mrproper> That's exactly what I was going to propose. I'll do that now and let you know. 14:47:37 <gundalow> #chair chopraaa 14:47:37 <zodbot> Current chairs: AndyG Pilou Xaroth alexsaezm bcoca bizonk chopraaa cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 14:47:50 <akasurde> gundalow, I am back 14:48:08 * chopraaa takes a seat 14:48:29 <bcoca> gundalow: you might want to exclude backports from your lists, since only RMs will deal with those 14:48:31 <gundalow> chopraaa: We are working through https://github.com/ansible/ansible/pulls?page=4&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Aneeds_triage+-label%3Anew_module+label%3Asmall_patch+sort%3Acreated-desc and currently on 14:48:46 <gundalow> bcoca: I think I have 14:48:53 <mrproper> gundalow: Updated. Would you take a look at it? 14:49:01 <bcoca> ah, seems i had older link 14:49:14 <gundalow> Bug fix for #24365: Do not disable SSH connection sharing #41332 14:49:34 <gundalow> synchronize :S 14:49:50 <bcoca> gundalow: this is support:core, shoudl we be looking at these here or more in the core backlog meeting? 14:50:00 <gundalow> ah, point 14:50:11 <gundalow> bcoca: I'm currently going through `small_patch` 14:50:30 <bcoca> i know, but just as you exclude backport .. does it make sense to exclude support:core? 14:50:34 <gundalow> Which I think is good for thse people 14:50:34 <gregdek> Is small_patch auto-identified by the bot? 14:50:38 <gundalow> gregdek: yes 14:50:44 <gregdek> Neat. 14:51:10 <gundalow> dnstxt: Non-existent RR returns empty string #40680 14:51:41 <bcoca> gundalow: probably same with 'networking' since they have their own process also 14:52:09 <gundalow> Networking should be skipped, that's a good point, much clearer in that case 14:53:38 <gregdek> 41105 I wonder if this is basically a case of "write a new PR because what you've described is completely different than what this one does" 14:53:58 <gundalow> 40680 I've poked jpmens on Twitter 14:54:48 <gundalow> gregdek: Fix logic in win_service for updating password. #41105? 14:55:02 <gregdek> Yes. 14:55:32 <gregdek> The PR itself is a one line fix that is a genuine misunderstanding of what's going on, and they sort out in the PR that they need to solve a different problem. 14:56:03 <gregdek> Changing "equal" to "not equal" in a comparison is usually not a proper fix :) 14:56:45 <gundalow> #action jborean93 to look at 41105 - 14:56:53 <gundalow> gregdek: could you add comments on the PR please 14:57:00 <gregdek> will do 14:57:06 <gundalow> OK. I'm about six hours in now 14:57:35 <gundalow> And Core Meeting is in #ansible-meeting in 3 minutes 14:57:41 <winem_> so, you have my full attention for a while now :) especially looking for stuff where testing or documentation is needed 14:58:05 <gundalow> woot, poking jpmens via twitter works 14:58:27 <gundalow> #action backport https://github.com/ansible/ansible/pull/40680 14:58:43 <gundalow> So lets recap where we've got to 14:59:59 <gregdek> gundalow is a machine! 15:00:08 <akasurde> I totally agree 15:00:09 <gundalow> * We've reached 41105 which is 4th July. so ~5 months back. I think (with zero research) hitting diminishing returns on original PR author being active 15:00:29 <gundalow> Advantage of IRC meetings is I can keep on running and making more tea while people look at PRs 15:00:48 <gundalow> I think this is the end of `label:small_patch` 15:01:24 <gundalow> * We've identified that merging PRs during this isn't a good idea, as you hit <merge> then someone adds a comment 15:01:57 <gundalow> So I think we change the query now 15:02:26 <gregdek> I could see many different kinds of queries we could do. 15:02:30 <gregdek> So many choices! :) 15:02:49 <gregdek> A simple "oldest PR wut" pass might be useful 15:03:07 <gundalow> a) We start again at most recent PR (-network, -small_patch -new_module -needs_revision) - Should identify things to merge 15:03:15 <bcoca> -support:core 15:03:18 <gundalow> b) We look for things to close 15:03:21 <gundalow> bcoca: ah, yes, thansk 15:03:23 <gundalow> hadsoifudsa;oif ds 15:03:30 <gregdek> o_O 15:03:38 <bcoca> and when looking backlog sort: oldest 15:03:44 <gundalow> Yup 15:05:03 <gundalow> hum 15:05:06 <gundalow> oh, 15:05:30 <gundalow> c) label:new_contributor - Make new people happy 15:05:48 <gundalow> I like the ideas of closing a load of old PRs 15:06:40 <gregdek> I used to have a super nice "this PR looks really old so we're closing, feel free to reopen" message that was suitable for most old PRs. 15:07:06 <gregdek> It's helpful to know what state it's in, though. You don't really want to close PRs if they're blocking on a reviewer, only if they're blocking on the original contributor. 15:08:18 <gregdek> Old PRs that are blocking on maintainer are prime candidates for "look for a new/add'l maintainer and maybe invite the PR submitter to be that maintainer." 15:08:24 <gregdek> Bye resmo :) 15:09:17 <gwmngilfen> foreman uses a "waiting_on_contributor" flag for blocking on the original contributor - i see you have that labe, but I have no idea if it's correctly enforced / reliable to query on :) 15:10:05 <gundalow> #topic Big PR closing session 15:10:16 <gundalow> How's this look: `is:pr is:open -label:backport -label:support:core -label:network -label:new_module sort:created-asc ` 15:11:11 <gundalow> For finding things to close 15:12:13 <gregdek> A good a place as any :) 15:12:37 <gundalow> DING DING DING This is about Closing PRs now 15:12:40 <bcoca> -label:new_plugin 15:13:11 <gundalow> https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=+is%3Aopen+-label%3Abackport+-label%3Asupport%3Acore+-label%3Anetwork+-label%3Anew_module+-label%3Anew_plugin++sort%3Acreated-asc++ 15:13:12 <gundalow> bcoca: thanks 15:13:34 <gundalow> oh, wait 15:13:50 <gundalow> can we close all of https://github.com/ansible/ansible/issues?q=is%3Aopen+sort%3Acreated-asc+label%3Aneeds_repo (that's older than x-days) as the source repo has been deleted? 15:14:10 <gundalow> oh, confused, I can still merge 15:14:12 <bcoca> i would check if we actually still want the change 15:14:24 <acozine> good morning from the snowy midwest 15:14:30 <bcoca> gundalow: githyb does 'magic branches' so the PR is actually a pseudo branch of OUR repo 15:14:34 <gundalow> nm, I thought `needs_repo` meant sourc ebranch was deleted 15:14:38 <gundalow> #chair acozine 15:14:38 <zodbot> Current chairs: AndyG Pilou Xaroth acozine alexsaezm bcoca bizonk chopraaa cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner mnaser rajeevarakkal resmo robertdebock shaps webknjaz winem_ 15:14:41 <gundalow> acozine: Morning :) 15:14:46 <bcoca> gundalow: yes, source is deleted, but 'copy' still persists 15:14:50 <Xaroth> Morning acozine. 15:14:54 <bcoca> so we can still merge those 15:15:47 <gundalow> ah, so these 6 are so old they don't have shippable, lets start here https://github.com/ansible/ansible/labels/needs_shippable (no filtering) 15:15:58 <gundalow> pacemaker_cluster: Fail ansible if the cluster state is not the expected state #19856 15:16:56 <gundalow> Close with a "If you are interested please rebase and reopen"? 15:17:02 <Xaroth> +1 for that 15:17:05 <bcoca> https://github.com/ansible/ansible/pull/16182 <= this we can just close 15:17:08 <chopraaa> +1 15:19:24 <gundalow> Permit to query a proxmox with invalid certificate #17247 15:20:11 <gregdek> So how are we doing this? Are we closing with standard text? Are we doing "action close xxx"? 15:20:12 <gundalow> closing 15:20:22 <gundalow> gregdek: look at 17247 15:20:34 <gundalow> AFAIK nothing else should end up in `label/needs_shippable` 15:20:49 <gundalow> adding use_floatingip arg to openstack inventory script to allow sett… #17052 15:21:22 <gundalow> marques: openstack is https://github.com/ansible/ansible/pull/17052 useful? 15:21:34 <sivel> we aren't accepting features to contrib inventory scripts any longer 15:21:42 <bcoca> we can close that, os inventory script is really dead at this point, in favor of the inventory plugin 15:21:49 <gundalow> #chair apple4ever sivel 15:21:49 <zodbot> Current chairs: AndyG Pilou Xaroth acozine alexsaezm apple4ever bcoca bizonk chopraaa cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner mnaser rajeevarakkal resmo robertdebock shaps sivel webknjaz winem_ 15:21:51 <gundalow> Good point 15:22:16 <gundalow> clsoed 15:22:24 <gundalow> (or some letters like that) 15:22:37 <acozine> spelling is overrated 15:22:41 <gundalow> Refresh TREE_DIR #16182 15:22:41 <gundalow> bcoca said close 15:22:47 <apple4ever> Hello 15:22:47 <sivel> already closed it 15:22:52 <gundalow> \o/ 15:22:59 <gregdek> hi apple4ever :) 15:23:06 <gundalow> You can create private instances in SoftLayer without a primaryIpAddress #12644 15:23:19 <gundalow> plugins/inventory/softlayer.py 15:23:58 <sivel> gundalow: that is a contrib script as well, but pre-move to contrib 15:24:01 <gundalow> #action gundalow to 1) Manually review all open inventory PRs (byfile) and work out what can be closed/directed to inventory plugins 2) Add to bot logic 15:25:41 <gundalow> close? 15:25:54 <sivel> close 12644 15:27:54 <gundalow> Tests for amazon ecs modules #11819 15:28:48 <gundalow> clsoed 15:29:00 <gundalow> #info No more `label/needs_shippable` 15:29:12 <acozine> w00t! 15:29:56 <abadger1999> Olá 15:29:58 <acozine> can we remove the `needs_shippable` label, then? 15:30:28 <gundalow> #action gundalow to look at Bot code and see if we can remove `needs_shippable` label 15:30:30 <gundalow> Good idea 15:30:32 <bcoca> probably 15:30:33 <gundalow> #chair abadger1999 15:30:33 <zodbot> Current chairs: AndyG Pilou Xaroth abadger1999 acozine alexsaezm apple4ever bcoca bizonk chopraaa cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner mnaser rajeevarakkal resmo robertdebock shaps sivel webknjaz winem_ 15:30:56 <gundalow> I'm not sure what the Bot does if it tries to apply a label if it doesn't exist 15:31:05 <gundalow> in the case of someone reopening one of the ~30 PRs 15:31:09 <acozine> oh, good point 15:31:16 <gundalow> hum, though chances are low 15:31:21 <gundalow> Good idea anyways 15:31:56 <gundalow> `label:deprecated label:feature ` 15:31:58 <acozine> we could set the bot to auto-close any PR that doesn't have a `shippable.yml 15:32:06 <gundalow> https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Adeprecated+label%3Afeature 15:32:32 <gundalow> Features for things that are deprecated 15:32:43 <karstensrage> wow that started early 15:32:50 <gundalow> karstensrage: ? 15:33:12 <karstensrage> i got an email at 5am 15:33:14 <gundalow> pjc42: Hi, you here for the PR review? 15:33:33 <gundalow> karstensrage: some of us started at 0900UTC 15:34:04 <gundalow> OK, so for deprecated we need to point them to the new location 15:34:18 <gundalow> Add support for applying Kubernetes deployments from URL #48402 15:36:11 <gundalow> module: k8 - deprecated 15:36:23 <gundalow> `k8s` 15:37:17 <gundalow> oh, no 15:38:53 <gundalow> Add to the ansible module "ovirt_snapshots" support to download/upload of snapshots #44268 15:39:35 <gundalow> hum, though ovirt_snapshots isn't deprecated 15:39:36 <gundalow> meh 15:40:35 <gundalow> https://github.com/ansible/ansible/issues/44268#issuecomment-415853558 confuses mehttps://github.com/ansible/ansible/blob/commits/lib/ansible/modules/cloud/ovirt/_ovirt_snapshots.py never existed 15:41:27 <gundalow> OK, looking at PRs 15:42:51 <gundalow> #info 5 deprecated PRs closed 15:43:16 <gundalow> #action botbug deprecated for issues looks wrong. May need to recalculate 15:43:56 <gundalow> #chair karstensrage 15:43:56 <zodbot> Current chairs: AndyG Pilou Xaroth abadger1999 acozine alexsaezm apple4ever bcoca bizonk chopraaa cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner karstensrage mnaser rajeevarakkal resmo robertdebock shaps sivel webknjaz winem_ 15:44:24 <gundalow> So for next, shall we use `is:open -label:backport -label:support:core -label:network -label:new_module -label:new_plugin sort:created-asc ` 15:45:00 <gundalow> wondering if addng `needs_revision`drops that from 600 to 300. Wondering how we can filter it more 15:45:23 <gundalow> + needs_revision + feature drops to 191 15:45:56 <bcoca> but needs_revision is a bit sticky, it isn't always removed after user fixes the issue that caused it 15:46:27 <gundalow> ah, that's a fair point, as people aren't always doing `ready_for_review`, etc 15:46:52 <gundalow> Let's use ` is:open -label:backport -label:support:core -label:network -label:new_module -label:new_plugin sort:created-asc label:feature ` 15:46:56 <gundalow> https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=+is%3Aopen+-label%3Abackport+-label%3Asupport%3Acore+-label%3Anetwork+-label%3Anew_module+-label%3Anew_plugin++sort%3Acreated-asc+label%3Afeature 15:47:01 * gregdek clicky 15:47:16 <gundalow> Update cobbler.py inventory script to more fully leverage Cobbler management fields #12090 15:48:03 <gregdek> OK, what's the process here? 99% of these are going to be "close with nice message" I suspect. Should we just #action that for later, or...? 15:48:06 <gundalow> * Looks like people are interested, though original branch has been deleted, so no rebase 15:48:40 <gundalow> gregdek: yup, though I assum it's not just "Close all 342 PRs" 15:48:43 <gundalow> oooooooh 15:48:56 <gundalow> needs_rebase + needs_repo = PR that can't be merged 15:49:00 <bcoca> we can rebase manually 15:49:16 <bcoca> but its for inventory script ... 15:49:38 <gregdek> #action close 12489 15:49:52 <gundalow> bcoca: I'm wondering what GitHub query gives us a list of things we can most likely close. Happy to go through them quickly rather than botclose 15:49:56 <gundalow> What do you think? 15:50:12 <bcoca> if i knew, they would be closed 15:50:27 <gregdek> #action close 13907 15:50:54 <gregdek> 13907 is a clear case of "we've had a green light to close this for ages and no one looked at it who could do so" 15:52:07 <bcoca> well, iirc authors of plugin are same as the 3rd party lib .. so kind of conflict here 15:52:36 <bcoca> its an implementation that bypasses 3rd party lib .. always +1 for me .. though it uses it's own urllib and misses many thigns we fix in our internal open_url functions 15:52:38 <gregdek> I mean, as a matter of policy, we could simply close every PR of over a year that's in "waiting for submitter", assuming we have such a flag that's valid/accurate 15:53:28 <bcoca> this one does not look like such, submitter just chose to use alternate 'secrets vault' 15:54:14 <gundalow> OK, I think we should do quick fire. Aim is to reduce the backlock. It's fine for some stuff to be left open 15:56:39 <gundalow> I'm happy to close old features 15:56:53 <gundalow> I'm less happy to close old bug fixes, as they may still apply 15:57:13 <gundalow> Groups naming convention in vagrant hosts provider #14235 15:58:03 <gregdek> 14109 I'm going to ask the most recent commenter if he'd be willing to start a new PR, but otherwise I'd recommend closure. 15:58:30 <gundalow> +1 15:59:10 <gundalow> I'm wondering if we should go back to most recent PRs that look good to merge 16:00:17 <gregdek> I dunno. I think cleaning up old stuff is useful, even if it's painful. 16:00:37 <gregdek> And it's obvious that we've been continually punting on some of these because we haven't been able to bring ourselves to make decisions. 16:01:01 <gundalow> hum, if we can find anythign that Funzo asked to be rebased we can probs close most of them 16:01:04 <gregdek> The worst case is still "this is useful, but no one is tending to it, so we're closing it, but if you care about it, please reopen and it will go through our Shiny New Process." 16:01:05 <gundalow> as that was ~april 2017 I think 16:01:33 <gregdek> There's no reason to leave old PRs open if it's obvious that no one is tending to them, right? 16:01:40 <gundalow> Yup 16:02:15 <gregdek> So I'd be in favor of soldiering through and closing old PRs, and letting them know that new PRs will go through a (hopefully) improved process. 16:02:23 <gundalow> +1 16:02:26 <gundalow> What's funzo's GitHub 16:02:31 <gregdek> Which we hope is actually approved :) 16:02:34 * gregdek looks 16:02:35 <bcoca> some is 'nice to have' but also needs someone with time to manage it 16:03:05 <bcoca> gregdek: i would not say that until we have a new and improved process 16:03:23 <bcoca> otherwise its like kicking the stabbed guy already on the ground 16:03:46 <gundalow> well if his stabbed and on the ground, I doubt he'll get up again? 16:04:00 <gregdek> I would say that gundalow leading this effort once a month is more process than most of these old community labeled issues have had in quite some time. 16:04:26 <gregdek> But point taken. 16:04:32 <bcoca> agreed, but is this going to be looking at new open ones also? 16:04:42 <gundalow> bcoca: yup, did this morning 16:04:43 <bcoca> cause that is what they will be if we ask them to resubmit 16:04:47 <gregdek> Yes, that's what gundalow did for the first 6 hours :) 16:05:00 <bcoca> ah, sorry, slept through that ... 16:05:02 <gundalow> well, only small_patch, though I think nexttime I'll look at all new community non-plugin 16:05:05 <gundalow> bcoca: :D 16:05:12 <gundalow> erm, non new plugin 16:05:18 <gregdek> To me, it's clear that we need a process that's analogous to the core triage process. 16:05:24 <gregdek> That is community focused. 16:05:28 <bcoca> once we have a core triage process ... 16:05:32 <gregdek> :) 16:06:05 <bcoca> we should ensure both have 'complimentary' lists to look at .. so as to avoid cracks for things to fall through 16:06:49 <gregdek> Well, core_support versus community_support should be that list, right? 16:07:00 <gregdek> Like, every single PR should have one or the other of those tags 16:07:09 <bcoca> i would just want to ensure we dont say 'hey, we ignored this for a year, why don't you resubmit so we can ask you to resubmit after ignoring it for another year' 16:07:29 <gundalow> ozkan: Hi, you here for the PR review? 16:07:38 <bcoca> gregdek: we also have networking .. asn many PRs overlap 2 or 3 of those tags 16:07:59 <gregdek> How can a PR be both community_support and core_support? 16:08:10 <gregdek> If that's ever true, our assignment process is broken. 16:08:15 <bcoca> i would sometims prefer to do the rebase/merge work myself .. as compensation for the delay 16:08:28 <gregdek> But you don't have time to do that. 16:08:32 <gundalow> gregdek: yes, Each file is exactly one, though a PR can have multiple files 16:08:32 <bcoca> gregdek: cause PRs can have files that are core supported and community supported 16:08:48 <gundalow> support:network is a subset of label/networking 16:09:09 <bcoca> label networking != support:network .. but we exclude it as such 16:09:11 <gregdek> Then we should flag those and say "please submit as different PRs" or we should say "core overrides community" or something. 16:09:36 <bcoca> gregdek: then we get into the issue of PRs being 'out of sync' with their dependencies .. whole reason we went badck to single repo 16:09:55 <bcoca> prs might not pass tests, cause they cannot w/o the 'core supported' test file modifications 16:10:03 <gregdek> OK, then. How about "if it touches a core file it's core's responsibility"? 16:10:52 <gregdek> I don't want to step on core's feet, but I also want to make sure that community has an accurate queue of work that isn't dependent on an already overburdened core. 16:10:56 <bcoca> not sure i agree with that, as you can modify 20 modules and 1 core file to do minor 'common' fix for them, does then core become responsible to review all 20 modules? 16:10:58 <gundalow> I'm working through https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=+is%3Aopen+-label%3Abackport+-label%3Asupport%3Acore+-label%3Anetwork+-label%3Anew_module+-label%3Anew_plugin++sort%3Acreated-asc+label%3Afeature+++commenter%3Acalfonso+ and closing as funzo asked them to rebase 19 months ago 16:11:25 <bcoca> gregdek: i dont have a good solution , just making you aware of the problem 16:11:49 <gregdek> No, but core is responsible for making the right decision, which in some cases may be "we'll handle this even if it has a few bits", or "there is no reason to have a PR with 20 modules, please resubmit individually". 16:11:53 <gundalow> #action gundalow to ensure that between Core Triage and Community Triage everything gets looked at - account for if an issue (or more likely a PR) hits core, network, community it's clear who's responsible for 16:11:58 <gundalow> guys' I got this 16:12:00 <gundalow> MOVING ON 16:12:02 <bcoca> i think in those cases both core AND community have to weigh in, each for the part they are responsible for 16:12:03 <gregdek> ok :) 16:12:13 <gundalow> I'll draw some ven diagrams 16:12:27 <gundalow> ================================= 16:12:49 <gundalow> Given funzo chased all of https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=+is%3Aopen+-label%3Abackport+-label%3Asupport%3Acore+-label%3Anetwork+-label%3Anew_module+-label%3Anew_plugin++sort%3Acreated-asc+label%3Afeature+++commenter%3Acalfonso+ ~19 months ago I'm going to go through and close 16:12:55 <gregdek> +1 16:14:16 <gundalow> So while I'm doing that, y'all fancy looking at ` is:open -label:backport -label:support:core -label:network -label:new_module -label:new_plugin sort:created-asc label:feature ` given that *I* am happy that we close older features. I don't want us to close older bugsfixes unless we know they are not an issue 16:14:21 <gundalow> https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=+is%3Aopen+-label%3Abackport+-label%3Asupport%3Acore+-label%3Anetwork+-label%3Anew_module+-label%3Anew_plugin++sort%3Acreated-asc+label%3Afeature+++ 16:15:14 <gundalow> back in ~10 minutes 16:21:10 <acozine> does anyone here use ESXi? I'd really appreciate another voice chiming in on https://github.com/ansible/ansible/pull/38507 16:21:30 <acozine> (VMware) 16:24:57 <misc> gundalow: the community call today is on metrics, but recorded, (since that's on your interest) 16:25:24 <misc> (but robin is here too) 16:33:45 <gundalow> misc: ah, forgot about that. I'lllisten to the recording, thanks 16:35:48 <gwmngilfen> oh darn, i meant to join that too 16:48:43 <gundalow> Closed another 10 PRs, with acozine wording https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 16:48:53 <gundalow> #action gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment 16:51:28 <jtanner> gundalow: iirc, when we close a PR, the submitter can not reopen it 16:51:46 <gundalow> jtanner: hum, I thought they could 16:52:36 <gundalow> jtanner: can you please close https://github.com/jctanner/ansible-tools/pull/4 16:53:00 <gundalow> one test is worth a thousand expert opinions 16:53:12 <jtanner> done 16:53:17 <gundalow> oh, damn 16:53:31 <gundalow> meh, oh well 16:54:01 <jtanner> can't do it? 16:54:04 <gregdek> We can edit comments retroactively if needed 16:54:19 <gundalow> nop, only comment or delete branch 16:55:08 <gundalow> I don't know if I care 16:58:09 <gundalow> #action work out guide to reopen a closed PR, perhaps by pushing to the branch again 16:59:29 <gregdek> I think it's not a big concern. 17:07:30 <gundalow> #topic new_contirbutor 17:07:47 <gundalow> `is:pr is:open -label:backport -label:new_module -label:new_plugin -label:networking -label:support:core -label:needs_revision label:new_contributor` 17:08:02 <gundalow> https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3Abackport+-label%3Anew_module+-label%3Anew_plugin+-label%3Anetworking+-label%3Asupport%3Acore++-label%3Aneeds_revision+label%3Anew_contributor 17:08:29 <gundalow> *new_contirbutor* needs love, not bikesheed 17:10:38 <gregdek> 49286 looks nice. 17:11:39 <gregdek> So when we say "needs love", not to bikeshed... :) 17:11:52 <gregdek> Is part of giving love saying nice things and thank you and so forth? 17:12:00 <gundalow> http://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ 17:13:37 <gundalow> hum, I think I'm done 17:13:45 <gundalow> and more importantly, hungry 17:13:47 <gregdek> :) 17:14:16 <gregdek> Want to call the meeting, then? 17:14:21 <gundalow> #info I'm really impressed with what was achived. Thanks everyone 17:14:40 <gundalow> #info Having clear GitHub searches defined in advance is a must 17:15:06 <gundalow> #info We need to agree the criteria for closing old feature PRs, and not be afraid of doing so 17:15:22 <bcoca> i just use random generator that checks if number matches open issue 17:15:39 <gundalow> #info I wonder if ignoring any PR where there is a working group would also be useful 17:16:25 <gundalow> #info I want to do some dedicated working group triage days. I think this shows it does work, may just need more notice and doodle poll to find a day that works best 17:16:29 <gundalow> #chair 17:16:29 <zodbot> Current chairs: AndyG Pilou Xaroth abadger1999 acozine alexsaezm apple4ever bcoca bizonk chopraaa cybette davegarath dericcrago evrardjp felix__ felixfontein ganeshrn gregdek gundalow jborean93 jtanner karstensrage mnaser rajeevarakkal resmo robertdebock shaps sivel webknjaz winem_ 17:16:35 <gundalow> Anyone else got any closing thoughts? 17:16:41 <gundalow> Thank you all again 17:16:54 <mrproper> gundalow: How often are you thinking of doing this? 17:16:56 <gregdek> Thanks gundalow. Nice work. 17:16:59 <samccann> thanks for driving this all day long gundalow 17:17:06 <acozine> awesome work, folks 17:17:13 <evrardjp> yeah thanks everyone 17:17:22 <acozine> and thanks gundalow for coming up with the idea and making it happen! 17:17:23 <evrardjp> thanks gundalow :) 17:17:26 <gundalow> mrproper: I'm not sure, what do you think 17:17:43 <gregdek> mrproper my question is how frequently can it be done without burning folks out :) 17:17:48 <gundalow> Yup 17:18:00 <gregdek> I think monthly may hit that balance. 17:18:05 <gundalow> I think 8 hours is too long :P 17:18:12 <mrproper> gundalow: It depends on the main goal. If you want to go through active PRs and approve, then probably every 2-3 months makes sense. But if you're looking to do cleanup of old stuff (ex. what's 2 years old and has these tags) then it's probably closer to 6. 17:18:33 <gregdek> Maybe different events on a monthly calendar. 17:18:44 <gundalow> I think monthly (varying day of the week) 17:18:55 <gundalow> any futher apart and stuff will not happen 17:19:00 <gregdek> One month is clear out the oldest, one month is focus on X, one month is focusing on Y. 17:19:12 <acozine> between meetings we could use the `candidate_to_close` label to prepare 17:19:14 <mrproper> gregdek: I kind of like that idea of switching between focus. 17:19:41 <gregdek> I like candidate_to_close. I think that's similar to a label I used to use back in the day. 17:19:45 <gundalow> #action gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close 17:19:51 <mrproper> gundalow: Doing this for individual working groups could be interesting too. 17:19:55 <gregdek> It's good because it gives people notice: "hey, we're thinking of closing this." 17:20:19 <gregdek> Although I think the bot needs to be quite a bit more talky, while some think the opposite :) 17:20:36 <gundalow> Workflow needs to be *clear* 17:20:39 <gregdek> +1 17:21:51 <gundalow> mrproper: yup, I think a lot of this needs individual working groups, set of people with the right knowledge 17:22:14 <gundalow> <blinking>Closing things that will never happen is OK</blinking> 17:22:20 <mrproper> gundalow: There's 180+ AWS PRs. A group of AWS in a channel for a couple hours could be really productive. But a general group isn't going to be as effective with those. 17:22:31 <gundalow> mrproper: exactly 17:22:45 <gregdek> +1 17:23:27 <gundalow> #info I'm doing an internal review of `label/support:network` over the next week or two, followed by `label/networking` (the stuff not maintianed by networking team) 17:25:15 <gundalow> Thanks again y'all 17:25:31 <gundalow> #info Any ideas/feedback in here or feel free to ping me directly 17:25:33 <gundalow> #endmeeting