=============================================== #ansible-community: Ansible Community PR Review =============================================== Meeting started by gundalow at 09:00:03 UTC. The full logs are available at https://meetbot.fedoraproject.org/ansible-community/2018-11-29/ansible_community_pr_review.2018-11-29-09.00.log.html . Meeting summary --------------- * 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. (gundalow, 09:03:37) * We will be ignoring `label:new_plugin` instead focusing on improving what's already there (gundalow, 09:04:04) * https://github.com/ansible/community/issues/407 contains more information. (gundalow, 09:04:24) * 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` (gundalow, 09:05:10) * filter is `is:pr is:open -label:backport -label:needs_triage -label:new_module label:small_patch` (gundalow, 09:05:26) * LINK: 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 (gundalow, 09:05:32) * documenting RETURNS is something that is often overlooked, important to check that (gundalow, 09:32:05) * ACTION: gundalow merge 48933 in a day unless negative reviews come through (gundalow, 09:48:18) * 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 (gundalow, 09:55:15) * ACTION: merge 48798 (gundalow, 09:57:16) * ACTION: gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`? (gundalow, 09:58:07) * ACTION: merge 48792 (gundalow, 10:01:16) * ACTION: AWS to review and merge 49061 (gundalow, 10:01:35) * ACTION: Network to review 41107 (gundalow, 10:01:54) * ACTION: jborean93 48761 (gundalow, 10:04:14) * ACTION: jborean93 to add words & close 48666 (gundalow, 10:10:05) * For future Bug Reviews should we focus on bug fixes? (gundalow, 10:12:17) * LINK: https://github.com/ansible/ansible/pull/46675 (rajeevarakkal, 10:13:35) * ACTION: 46675 merge (gundalow, 10:20:07) * ACTION: 48599 merge (gundalow, 10:35:27) * ACTION: Core to review & merge 48598 (gundalow, 10:38:28) * ACTION: merge, + backport PRs for 47788 (gundalow, 11:18:57) * 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 (gundalow, 11:23:02) * unless we enforce changelogs via CI we aren't going to get them (gundalow, 11:28:21) * ACTION: merge (changelog) and backport 47193 (gundalow, 11:32:09) * ACTION: merge (needs changelog) 47134 (gundalow, 11:33:26) * ACTION: review 47020 (robertdebock, 11:37:16) * ACTION: robertdebock review 47020 (gundalow, 11:37:28) * ACTION: Azure to review& merge 46608 (gundalow, 12:14:39) * ACTION: merge & backport 46604 (gundalow, 12:17:03) * ACTION: AWS to review and merge 46124 (gundalow, 12:21:21) * 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 (gundalow, 12:26:46) * ACTION: merge 33239 (gundalow, 12:32:42) * ACTION: close 45706 (gundalow, 12:32:52) * http://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ is an interesting read on how to give review feedback (gundalow, 13:01:43) * ACTION: gundalow to speak to Monty and see what help I can give to get `label:openstack` moving (gundalow, 13:09:50) * ACTION: merge 45198 (gundalow, 13:15:25) * negative testing can be done in integration tests, include `ignore_errors: yes` with `register: response` and check `response.failed` and `responsed.message` contains something (gundalow, 13:22:39) * 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] (gundalow, 13:26:12) * LINK: https://help.github.com/articles/searching-issues-and-pull-requests/#search-by-commenter not tested it (gundalow, 13:35:33) * https://help.github.com/articles/searching-issues-and-pull-requests/ has *loads* of different ways to search/filter issues & PRs (gundalow, 13:35:56) * ACTION: merged 45117 (gundalow, 13:40:07) * ACTION: merge 44005 (gundalow, 13:53:46) * ACTION: merge 43722 (gundalow, 13:54:58) * ACTION: backport 43722 (gundalow, 13:55:20) * ACTION: Ricky to review & merge 43151 (gundalow, 14:00:48) * ACTION: 42239 merge (gregdek, 14:14:22) * HELP: (gundalow, 14:36:11) * HELP: (gundalow, 14:36:15) * HELP: (gundalow, 14:37:12) * Review! (gundalow, 14:37:27) * ACTION: gundalow consider raising adding to Bot plan: command to raise backport after merge (if it contains changelog) `rebase_merge_create_backport`) (gundalow, 14:42:39) * ACTION: jborean93 to look at 41105 - (gundalow, 14:56:45) * ACTION: backport https://github.com/ansible/ansible/pull/40680 (gundalow, 14:58:27) * Big PR closing session (gundalow, 15:10:05) * LINK: 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++ (gundalow, 15:13:11) * LINK: https://github.com/ansible/ansible/pull/16182 <= this we can just close (bcoca, 15:17:05) * 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 (gundalow, 15:24:01) * No more `label/needs_shippable` (gundalow, 15:29:00) * ACTION: gundalow to look at Bot code and see if we can remove `needs_shippable` label (gundalow, 15:30:28) * LINK: https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Adeprecated+label%3Afeature (gundalow, 15:32:06) * LINK: 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 (gundalow, 15:40:35) * 5 deprecated PRs closed (gundalow, 15:42:51) * ACTION: botbug deprecated for issues looks wrong. May need to recalculate (gundalow, 15:43:16) * LINK: 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 (gundalow, 15:46:56) * ACTION: close 12489 (gregdek, 15:49:38) * ACTION: close 13907 (gregdek, 15:50:27) * 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 (gundalow, 16:11:53) * LINK: 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+++ (gundalow, 16:14:21) * ACTION: gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment (gundalow, 16:48:53) * ACTION: work out guide to reopen a closed PR, perhaps by pushing to the branch again (gundalow, 16:58:09) * new_contirbutor (gundalow, 17:07:30) * LINK: 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 (gundalow, 17:08:02) * LINK: http://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ (gundalow, 17:12:00) * I'm really impressed with what was achived. Thanks everyone (gundalow, 17:14:21) * Having clear GitHub searches defined in advance is a must (gundalow, 17:14:40) * We need to agree the criteria for closing old feature PRs, and not be afraid of doing so (gundalow, 17:15:06) * I wonder if ignoring any PR where there is a working group would also be useful (gundalow, 17:15:39) * 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 (gundalow, 17:16:25) * ACTION: gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close (gundalow, 17:19:45) * 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) (gundalow, 17:23:27) * Any ideas/feedback in here or feel free to ping me directly (gundalow, 17:25:31) Meeting ended at 17:25:33 UTC. Action Items ------------ * gundalow merge 48933 in a day unless negative reviews come through * merge 48798 * gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`? * merge 48792 * AWS to review and merge 49061 * Network to review 41107 * jborean93 48761 * jborean93 to add words & close 48666 * 46675 merge * 48599 merge * Core to review & merge 48598 * merge, + backport PRs for 47788 * merge (changelog) and backport 47193 * merge (needs changelog) 47134 * review 47020 * robertdebock review 47020 * Azure to review& merge 46608 * merge & backport 46604 * AWS to review and merge 46124 * merge 33239 * close 45706 * gundalow to speak to Monty and see what help I can give to get `label:openstack` moving * merge 45198 * merged 45117 * merge 44005 * merge 43722 * backport 43722 * Ricky to review & merge 43151 * 42239 merge * gundalow consider raising adding to Bot plan: command to raise backport after merge (if it contains changelog) `rebase_merge_create_backport`) * jborean93 to look at 41105 - * backport https://github.com/ansible/ansible/pull/40680 * 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 * gundalow to look at Bot code and see if we can remove `needs_shippable` label * botbug deprecated for issues looks wrong. May need to recalculate * close 12489 * close 13907 * 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 * gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment * work out guide to reopen a closed PR, perhaps by pushing to the branch again * gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close Action Items, by person ----------------------- * gundalow * gundalow merge 48933 in a day unless negative reviews come through * gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`? * gundalow to speak to Monty and see what help I can give to get `label:openstack` moving * gundalow consider raising adding to Bot plan: command to raise backport after merge (if it contains changelog) `rebase_merge_create_backport`) * 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 * gundalow to look at Bot code and see if we can remove `needs_shippable` label * 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 * gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment * gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close * jborean93 * jborean93 48761 * jborean93 to add words & close 48666 * jborean93 to look at 41105 - * robertdebock * robertdebock review 47020 * **UNASSIGNED** * merge 48798 * merge 48792 * AWS to review and merge 49061 * Network to review 41107 * 46675 merge * 48599 merge * Core to review & merge 48598 * merge, + backport PRs for 47788 * merge (changelog) and backport 47193 * merge (needs changelog) 47134 * review 47020 * Azure to review& merge 46608 * merge & backport 46604 * AWS to review and merge 46124 * merge 33239 * close 45706 * merge 45198 * merged 45117 * merge 44005 * merge 43722 * backport 43722 * Ricky to review & merge 43151 * 42239 merge * backport https://github.com/ansible/ansible/pull/40680 * botbug deprecated for issues looks wrong. May need to recalculate * close 12489 * close 13907 * work out guide to reopen a closed PR, perhaps by pushing to the branch again People Present (lines said) --------------------------- * gundalow (548) * gregdek (91) * robertdebock (70) * bcoca (65) * shaps (31) * zodbot (31) * webknjaz (24) * Xaroth (13) * jborean93 (12) * evrardjp (12) * mrproper (12) * winem_ (11) * acozine (11) * akasurde (10) * rajeevarakkal (9) * Pilou (7) * jtanner (7) * mnaser (6) * felixfontein (6) * sivel (4) * samccann (4) * resmo (4) * chopraaa (4) * AndyG (4) * misc (2) * felix__ (2) * gwmngilfen (2) * karstensrage (2) * alexsaezm (2) * ganeshrn (2) * dericcrago (2) * davegarath (1) * abadger1999 (1) * bizonk (1) * rbarlik (1) * apple4ever (1) * cybette (0) Generated by `MeetBot`_ 0.1.4 .. _`MeetBot`: http://wiki.debian.org/MeetBot