#ansible-community: Ansible Community PR Review

Meeting started by gundalow at 09:00:03 UTC (full logs).

Meeting summary

    1. 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)
    2. We will be ignoring `label:new_plugin` instead focusing on improving what's already there (gundalow, 09:04:04)
    3. https://github.com/ansible/community/issues/407 contains more information. (gundalow, 09:04:24)
    4. 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)
    5. filter is `is:pr is:open -label:backport -label:needs_triage -label:new_module label:small_patch` (gundalow, 09:05:26)
    6. 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)
    7. documenting RETURNS is something that is often overlooked, important to check that (gundalow, 09:32:05)
    8. ACTION: gundalow merge 48933 in a day unless negative reviews come through (gundalow, 09:48:18)
    9. 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)
    10. ACTION: merge 48798 (gundalow, 09:57:16)
    11. ACTION: gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`? (gundalow, 09:58:07)
    12. ACTION: merge 48792 (gundalow, 10:01:16)
    13. ACTION: AWS to review and merge 49061 (gundalow, 10:01:35)
    14. ACTION: Network to review 41107 (gundalow, 10:01:54)
    15. ACTION: jborean93 48761 (gundalow, 10:04:14)
    16. ACTION: jborean93 to add words & close 48666 (gundalow, 10:10:05)
    17. For future Bug Reviews should we focus on bug fixes? (gundalow, 10:12:17)
    18. https://github.com/ansible/ansible/pull/46675 (rajeevarakkal, 10:13:35)
    19. ACTION: 46675 merge (gundalow, 10:20:07)
    20. ACTION: 48599 merge (gundalow, 10:35:27)
    21. ACTION: Core to review & merge 48598 (gundalow, 10:38:28)
    22. ACTION: merge, + backport PRs for 47788 (gundalow, 11:18:57)
    23. 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)
    24. unless we enforce changelogs via CI we aren't going to get them (gundalow, 11:28:21)
    25. ACTION: merge (changelog) and backport 47193 (gundalow, 11:32:09)
    26. ACTION: merge (needs changelog) 47134 (gundalow, 11:33:26)
    27. ACTION: review 47020 (robertdebock, 11:37:16)
    28. ACTION: robertdebock review 47020 (gundalow, 11:37:28)
    29. ACTION: Azure to review& merge 46608 (gundalow, 12:14:39)
    30. ACTION: merge & backport 46604 (gundalow, 12:17:03)
    31. ACTION: AWS to review and merge 46124 (gundalow, 12:21:21)
    32. 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)
    33. ACTION: merge 33239 (gundalow, 12:32:42)
    34. ACTION: close 45706 (gundalow, 12:32:52)
    35. 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)
    36. ACTION: gundalow to speak to Monty and see what help I can give to get `label:openstack` moving (gundalow, 13:09:50)
    37. ACTION: merge 45198 (gundalow, 13:15:25)
    38. 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)
    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)
    40. https://help.github.com/articles/searching-issues-and-pull-requests/#search-by-commenter not tested it (gundalow, 13:35:33)
    41. https://help.github.com/articles/searching-issues-and-pull-requests/ has *loads* of different ways to search/filter issues & PRs (gundalow, 13:35:56)
    42. ACTION: merged 45117 (gundalow, 13:40:07)
    43. ACTION: merge 44005 (gundalow, 13:53:46)
    44. ACTION: merge 43722 (gundalow, 13:54:58)
    45. ACTION: backport 43722 (gundalow, 13:55:20)
    46. ACTION: Ricky to review & merge 43151 (gundalow, 14:00:48)
    47. ACTION: 42239 merge (gregdek, 14:14:22)
    48. HELP: (gundalow, 14:36:11)
    49. HELP: (gundalow, 14:36:15)
    50. HELP: (gundalow, 14:37:12)

  2. Review! (gundalow, 14:37:27)
    1. 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)
    2. ACTION: jborean93 to look at 41105 - (gundalow, 14:56:45)
    3. ACTION: backport https://github.com/ansible/ansible/pull/40680 (gundalow, 14:58:27)

  3. Big PR closing session (gundalow, 15:10:05)
    1. 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)
    2. https://github.com/ansible/ansible/pull/16182 <= this we can just close (bcoca, 15:17:05)
    3. 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)
    4. No more `label/needs_shippable` (gundalow, 15:29:00)
    5. ACTION: gundalow to look at Bot code and see if we can remove `needs_shippable` label (gundalow, 15:30:28)
    6. https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Adeprecated+label%3Afeature (gundalow, 15:32:06)
    7. 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)
    8. 5 deprecated PRs closed (gundalow, 15:42:51)
    9. ACTION: botbug deprecated for issues looks wrong. May need to recalculate (gundalow, 15:43:16)
    10. 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)
    11. ACTION: close 12489 (gregdek, 15:49:38)
    12. ACTION: close 13907 (gregdek, 15:50:27)
    13. 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)
    14. 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)
    15. ACTION: gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment (gundalow, 16:48:53)
    16. ACTION: work out guide to reopen a closed PR, perhaps by pushing to the branch again (gundalow, 16:58:09)

  4. new_contirbutor (gundalow, 17:07:30)
    1. 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)
    2. http://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ (gundalow, 17:12:00)
    3. I'm really impressed with what was achived. Thanks everyone (gundalow, 17:14:21)
    4. Having clear GitHub searches defined in advance is a must (gundalow, 17:14:40)
    5. We need to agree the criteria for closing old feature PRs, and not be afraid of doing so (gundalow, 17:15:06)
    6. I wonder if ignoring any PR where there is a working group would also be useful (gundalow, 17:15:39)
    7. 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)
    8. ACTION: gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close (gundalow, 17:19:45)
    9. 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)
    10. Any ideas/feedback in here or feel free to ping me directly (gundalow, 17:25:31)


Meeting ended at 17:25:33 UTC (full logs).

Action items

  1. gundalow merge 48933 in a day unless negative reviews come through
  2. merge 48798
  3. gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`?
  4. merge 48792
  5. AWS to review and merge 49061
  6. Network to review 41107
  7. jborean93 48761
  8. jborean93 to add words & close 48666
  9. 46675 merge
  10. 48599 merge
  11. Core to review & merge 48598
  12. merge, + backport PRs for 47788
  13. merge (changelog) and backport 47193
  14. merge (needs changelog) 47134
  15. review 47020
  16. robertdebock review 47020
  17. Azure to review& merge 46608
  18. merge & backport 46604
  19. AWS to review and merge 46124
  20. merge 33239
  21. close 45706
  22. gundalow to speak to Monty and see what help I can give to get `label:openstack` moving
  23. merge 45198
  24. merged 45117
  25. merge 44005
  26. merge 43722
  27. backport 43722
  28. Ricky to review & merge 43151
  29. 42239 merge
  30. gundalow consider raising adding to Bot plan: command to raise backport after merge (if it contains changelog) `rebase_merge_create_backport`)
  31. jborean93 to look at 41105 -
  32. backport https://github.com/ansible/ansible/pull/40680
  33. 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
  34. gundalow to look at Bot code and see if we can remove `needs_shippable` label
  35. botbug deprecated for issues looks wrong. May need to recalculate
  36. close 12489
  37. close 13907
  38. 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
  39. gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment
  40. work out guide to reopen a closed PR, perhaps by pushing to the branch again
  41. gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close


Action items, by person

  1. gundalow
    1. gundalow merge 48933 in a day unless negative reviews come through
    2. gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`?
    3. gundalow to speak to Monty and see what help I can give to get `label:openstack` moving
    4. gundalow consider raising adding to Bot plan: command to raise backport after merge (if it contains changelog) `rebase_merge_create_backport`)
    5. 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
    6. gundalow to look at Bot code and see if we can remove `needs_shippable` label
    7. 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
    8. gundalow turn https://github.com/ansible/ansible/pull/24032#issuecomment-442906978 into a GitHub fragment
    9. gundalow get core to review https://github.com/ansible/ansible/labels/candidate_to_close
  2. jborean93
    1. jborean93 48761
    2. jborean93 to add words & close 48666
    3. jborean93 to look at 41105 -
  3. robertdebock
    1. robertdebock review 47020
  4. UNASSIGNED
    1. merge 48798
    2. merge 48792
    3. AWS to review and merge 49061
    4. Network to review 41107
    5. 46675 merge
    6. 48599 merge
    7. Core to review & merge 48598
    8. merge, + backport PRs for 47788
    9. merge (changelog) and backport 47193
    10. merge (needs changelog) 47134
    11. review 47020
    12. Azure to review& merge 46608
    13. merge & backport 46604
    14. AWS to review and merge 46124
    15. merge 33239
    16. close 45706
    17. merge 45198
    18. merged 45117
    19. merge 44005
    20. merge 43722
    21. backport 43722
    22. Ricky to review & merge 43151
    23. 42239 merge
    24. backport https://github.com/ansible/ansible/pull/40680
    25. botbug deprecated for issues looks wrong. May need to recalculate
    26. close 12489
    27. close 13907
    28. work out guide to reopen a closed PR, perhaps by pushing to the branch again


People present (lines said)

  1. gundalow (548)
  2. gregdek (91)
  3. robertdebock (70)
  4. bcoca (65)
  5. shaps (31)
  6. zodbot (31)
  7. webknjaz (24)
  8. Xaroth (13)
  9. jborean93 (12)
  10. evrardjp (12)
  11. mrproper (12)
  12. winem_ (11)
  13. acozine (11)
  14. akasurde (10)
  15. rajeevarakkal (9)
  16. Pilou (7)
  17. jtanner (7)
  18. mnaser (6)
  19. felixfontein (6)
  20. sivel (4)
  21. samccann (4)
  22. resmo (4)
  23. chopraaa (4)
  24. AndyG (4)
  25. misc (2)
  26. felix__ (2)
  27. gwmngilfen (2)
  28. karstensrage (2)
  29. alexsaezm (2)
  30. ganeshrn (2)
  31. dericcrago (2)
  32. davegarath (1)
  33. abadger1999 (1)
  34. bizonk (1)
  35. rbarlik (1)
  36. apple4ever (1)
  37. cybette (0)


Generated by MeetBot 0.1.4.