09:00:03 #startmeeting Ansible Community PR Review 09:00:03 Meeting started Thu Nov 29 09:00:03 2018 UTC. 09:00:03 This meeting is logged and archived in a public location. 09:00:03 The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot. 09:00:03 Useful Commands: #action #agreed #halp #info #idea #link #topic. 09:00:03 The meeting name has been set to 'ansible_community_pr_review' 09:00:31 #chair robertdebock felixfontein Xaroth alexsaezm davegarath 09:00:31 Current chairs: Xaroth alexsaezm davegarath felixfontein gundalow robertdebock 09:00:49 I like how zodbot sorts case sensitively. 09:01:20 I never noticed that before 09:03:08 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 #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 Xaroth: yup, that takes a while, thanks for looking into that 09:04:04 #info We will be ignoring `label:new_plugin` instead focusing on improving what's already there 09:04:24 #info https://github.com/ansible/community/issues/407 contains more information. 09:05:10 #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 #info filter is `is:pr is:open -label:backport -label:needs_triage -label:new_module label:small_patch` 09:05:32 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 Would people like to work through from the top of the list?, so starting with 49235 09:06:35 might be better to start oldest first 09:07:28 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 So starting at the top 09:08:15 top it is 09:08:20 docker_swarm_service: ensure idempotency when the user, publish and update_order parameter are not specified #49235 09:09:02 * PR by the module author (as shown by label:owner_pr) 09:09:22 Anyone here use Docker? 09:09:30 Me 09:10:42 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 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 robertdebock: yup, I think that's fair 09:11:40 Use System.Xml.XmlDocument.Load rather than Get-Content #49206 - Looks like it's in hand, skipping 09:12:09 azure: fix keyvault key secret not support cloud environment #49182 09:13:04 change looks sensible, though I don't know anything about Azure, anyone else? 09:13:19 regarding #49235: there's still some discussion going on in the issue which this PR solves as well 09:13:27 could be tested, I don't have an Azure account. 09:14:12 Will leave that for #ansible-azure 09:15:10 #chair cybette 09:15:10 Current chairs: Xaroth alexsaezm cybette davegarath felixfontein gundalow robertdebock 09:15:13 Morning 09:15:24 k8s: Fix error where pod has error and no containers #49154 09:15:50 So we can see that one of the maintainers have given this a `shipit` 09:16:34 Change looks sensible, I'll merge (unless anyone says otherwise) 09:17:55 lgtm 09:19:18 Remove possible parenthesis for media type in OpenBSD #49128 09:19:30 Going to mark ^ as WIP as they want to fix another bug 09:20:30 rabbitmq_user: Specify key to use while sorting permissions #49126 09:20:38 No one has looked at this yet 09:21:50 Is ^ a good use of Python? 09:23:25 I've added some comments 09:23:31 ACME: add basic unit tests #49103 09:23:53 looks like sane Python to me (re: #49126) 09:23:53 felixfontein: I think this is good, shall I merge? 09:24:11 Xaroth: Thanks, could you please put that on the PR 09:25:28 Comments on the PR make are useful for the next person that looks at it 09:25:55 Extended cloudfront_facts module to have a predictable return value… #49061 09:27:19 Looks good, but not tested, nor automated tests. 09:27:32 robertdebock: which PR 09:28:02 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 Extended cloudfront_facts module to have a predictable return value… #49061 09:29:19 That's a plan, if comments relate to a PR, prepend with #pr_number. 09:29:42 #49061: Looks good, but not tested, nor automated tests. 09:30:52 49061: Missing from `RETURN`, to be consistent I think it should be `distribution_id`, comments added 09:31:27 49061: good feedback. 09:31:49 gundalow: yes, you can merge the ACME unit tests if you want :) 09:32:05 #info documenting RETURNS is something that is often overlooked, important to check that 09:32:40 49103 ACME unit tests, merged. Thanks felixfontein 09:32:57 IAM module: Also return user_name on create, to be more consistent. #49059 09:33:11 49059: No review comments so far 09:34:25 #chair ganeshrn 09:34:25 Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow robertdebock 09:34:29 ganeshrn: Afternoon :) 09:34:42 Morning :) 09:34:51 49059 I haven't worked out what this is doing yet 09:35:53 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 49059 ah, so it's closer to line 762, 770, 774 09:37:15 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 +1 09:39:19 49059: The username should always be returned, I'll request why only on create/ 09:39:32 robertdebock: +1 09:39:48 49059: If it's only returned on create, it's difficult to loop over the results later. 09:40:09 Good spot on usability there :) 09:40:53 49059: commented in github. 09:41:20 morning 09:41:29 #chair shaps 09:41:29 Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow robertdebock shaps 09:41:33 Morning :) 09:42:00 ovirt_storage_connection: comparing passwords breaks idempotency in update_check #48933 09:42:03 No comments so far 09:44:19 don't see anything wrong with it though 09:44:22 and it makes sense 09:45:22 48933: looks good to me, but I have no way of testing it. 09:47:08 Can't test it either, it's just a 1 line in a list of equal(...) though, should be fine 09:47:55 @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 #action gundalow merge 48933 in a day unless negative reviews come through 09:48:29 48933: well done. 09:48:33 48933: request for other views added 09:49:09 ec2_vpc_nacl: Don't default name into Name unless able #48889 09:49:22 felixfontein: a bit late, but raw-string literals look odd @ 49103 09:49:34 #chair webknjaz 09:49:34 Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow robertdebock shaps webknjaz 09:49:38 webknjaz: Morning :) 09:50:00 Morning 09:50:44 48889: In 48832 johnboy2 has given clear steps to reproduce and shown his working, so I think this is good 09:51:35 48889: agreed, looks good. 09:51:36 webknjaz: is there a difference between r and R? 09:53:28 webknjaz: bah, already hit merged on 48889. I guess code is good, though it's useful to know 09:53:29 hum 09:54:05 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 I'm not sure whether uppercase R prefix works, never seen it uppercase. But probably fine 09:54:29 Thoughts? 09:54:32 #chair jborean93 09:54:32 Current chairs: Xaroth alexsaezm cybette davegarath felixfontein ganeshrn gundalow jborean93 robertdebock shaps webknjaz 09:54:37 Sounds good 09:55:13 49059: Feed back from contributor: https://github.com/ansible/ansible/pull/49059 09:55:15 #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 robertdebock: nice work :) 09:56:15 os_server: server.security_groups is a list of dicts #48798 09:56:24 webknjaz: I think I've used both extensively, and both work 09:57:05 48798 emonty and gtema have given this a shipit, so I think this is good 09:57:16 webknjaz: just tested with Python 2.7 and 3.6, both accept both cases 09:57:16 #action merge 48798 09:58:07 #action gundalow 48798 - should gtema be a maintainer for `cloud/openstack/`? 09:58:44 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 templar: ensure that exceptions are handled, fix 'AttributeError' #48792 10:00:01 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 48792: bcoca already approved the pr. 10:01:16 #action merge 48792 10:01:35 #action AWS to review and merge 49061 10:01:54 #action Network to review 41107 10:02:06 ^ Just some notes I'd made before 10:02:29 Fix win_nssm credentials quoting #48761 10:02:37 Powershell 10:03:27 * gundalow isn't going near that 10:03:27 It's not really the right fix, it will help in this case but there's a better way 10:03:36 Hi 10:03:38 ah, theres a man that knows 10:03:42 #chair felix__ 10:03:42 Current chairs: Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 robertdebock shaps webknjaz 10:03:59 I'll do a review tonight/tomorrow to try and nudge the person in the right direction 10:04:01 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 action jborean93 48761 10:04:14 #action jborean93 48761 10:04:16 Thanks 10:04:20 nw 10:04:41 I'll be there 10:04:50 win_reboot: Use a less error-prone command to get uptime #48666 10:05:23 we don't want to do that 10:05:26 jborean93: 48666, given last comment, should this PR be rejected (or maybe discussed in Windows Working Group) 10:05:40 48666: Why would you want to check uptime on a reboot? 10:06:07 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 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 Thansk @alexsaezm! 10:06:47 48666 is an issue because Packer uses their own connection plugin for Windows and it breaks this scenario 10:07:22 alexsaezm: Thank you for joining us :) 10:07:26 gundalow: so a thanks but no thanks for 48666 10:08:33 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 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 Packer has https://github.com/hashicorp/packer/issues/6453 10:09:16 I'll close 48666 10:09:51 Thanks 10:10:05 #action jborean93 to add words & close 48666 10:10:09 rajeevarakkal: Hi :) 10:10:58 hi 10:11:36 rajeevarakkal: You joining in the Bug review? 10:11:47 rabbitmq_binding: Add support for state=absent #48599 10:11:58 48599: Feature request, no comments added so far 10:12:17 #info For future Bug Reviews should we focus on bug fixes? 10:12:41 This my firt time over here.. i would like to discuss about my PR 10:12:58 Am i in right channel? 10:13:24 @rajeevarakkal, what PR? 10:13:35 https://github.com/ansible/ansible/pull/46675 10:13:49 #chair rajeevarakkal 10:13:49 Current chairs: Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz 10:14:03 Contributing DellEMC Install Firmware Modules to Ansible #46675 10:15:13 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 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 It's just my internal compass. 10:15:25 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 Thanks John :) 10:16:14 #48599 looks sensible 10:16:43 shaps: Can you put that on the PR please 10:16:47 sure 10:17:08 (good to put it here as well, though GitHub is the permanent record) 10:17:35 yep yep, just still a bit low on caffeine, will get there :) 10:17:50 about #48599, should not rabbitmq integration tests be updated ? 10:17:55 shaps: the quiet patch was me running to make more tea :) 10:18:19 Pilou: oh, I didn't even look to see if there were some. That would be good 10:18:21 #chair Pilou 10:18:21 Current chairs: Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz 10:18:24 Pilou: Morning :) 10:18:42 46675: It looks like all review items are done and the tests are passing. 10:19:10 hi :) 10:19:23 rajeevarakkal: Are you from Dell EMC? Your module code looks good. I'll trust that you know OpenManage. 10:19:47 yes, you are right robert, so far all tests passed. 10:20:07 #action 46675 merge 10:20:07 Yes we are from DellEMC .. 10:20:25 rajeevarakkal: Looks good, I'll merge later (that gives anyone else here time to add comments if they spot anything) 10:20:36 Thanks a lot John :) 10:20:41 Pilou, I don't see tests for rabbitmq_binding 10:20:44 there is a setup_rabbitmq target which is used by other rabbitmq_ targets but there isn't a rabbitmq_binding target 10:21:47 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 OK, lets look back to rabbitmq 10:22:19 rajeevarakkal: anything else? 10:22:50 Nope.. I think we are good :) 10:24:41 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 rajeevarakkal: I've posted a few concerns about the public API (return value) in your PR 10:25:31 webknjaz: Thanks 10:25:55 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 Pilou: shaps what's your thoughts for rabbitmq 10:26:12 robertdebock: oh, yes. i totally missed that 10:26:31 setup of rabbitmq is already handled by "setup_rabbitmq" target (which is the boring/complicated bit) 10:26:32 I'll add it to the github pull request for others to read. 10:26:33 This is why many eyes is good 10:28:05 also setup_rabbitmq seems to only setup ubuntu? 10:28:22 for example #44718 which added a new rabbitmq_publish reuse setup_rabbitmq 10:29:30 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 You both OK to look into that, and I'll continue (keep chating here)) 10:30:31 Pilou, Cool so you working on non-ubuntu setup? :) 10:30:52 rabbitmq_binding: Add support for state=absent #48599 10:31:22 48599 This looks way too simple, which makes me think something is missing 10:31:27 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 shaps: agreed 10:32:12 gundalow, the actual bidning removal was already there, it looks like just the check he's added was missing 10:32:34 shaps: ah, yes on line 209 10:32:45 correct 10:32:52 48599 Just stange that it wasn't in the original version 10:33:06 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 gundalow, not sure why, the docs say 'only present implemented' though 10:33:45 shaps: that line has been removed 10:34:09 yes, was referring to current module :) 10:34:30 Pilou, yes I agree, although that seems a fairly trivial change? 10:34:55 shaps: I think because without the PR line 205 doesn't run (as change_required = false) 10:35:14 yep 10:35:27 #action 48599 merge 10:35:33 It's shown in the output, before/after 10:35:50 I believe that should actually be considered a bug, since the module accepted state=absent 10:36:02 but did nothing about it 10:36:22 Also, I'll add comment to PR to ask for tests, see what happens 10:36:28 shaps: +1 10:37:03 Avoid duplicated recursive calls in inventory CLI #48598 10:37:23 48598 Looks sensible bcoca has approved 10:37:31 48598: Everything is approved, tests pass. 10:38:15 webknjaz: 48598 look good to you? It's support:core, so extra set of eyes would be good 10:38:28 #action Core to review & merge 48598 10:38:48 rabbitmq_binding: Fix using empty routing key #48597 10:38:55 48597 No reviews yet 10:42:27 48597 Think need someone that knows rabbitmq to review that 10:42:54 Changed single file zip write parameter for arcname to use os.path.ba… #48285 10:43:50 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 48397: `self.routing_key != ''` 10:45:05 48285: is for `archive` I've requested the tests be extended 10:45:54 is there any simple or documentation RP that needs fixing? i can try something easy :) 10:46:17 #chair AndyG 10:46:17 Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz 10:46:33 AndyG: Hi. Would you like something so you can understand the GitHub work flow? 10:46:43 sure 10:48:14 robertdebock, I guess you meant 48597? 10:49:04 @shaps, correct. sorry. 10:50:42 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 48598: just one nitpick 10:52:21 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 ``` 10:52:21 .. contents:: 10:52:21 :local: 10:52:21 ``` 10:52:22 That is the correct form 10:52:22 The old (incorrect form is) `.. contents:: Topics` (and perhaps without `:local:`) 10:52:23 Fixing that in all the rst files under `docs/docsite/rst/dev_guide/` would be really useful 10:53:37 AndyG: in a single PR 10:54:36 Hope that makes sense 10:55:20 thanks, let me take a gander 10:58:26 Add the ability to download a specific version of an S3 object #47867 11:00:44 47867: looks valid and simple. 11:00:52 #47867 akasurde already approved the changes too :) 11:01:15 Added readability suggestion 11:02:01 Thanks 11:02:08 47867: good, that PR is alive again. 11:02:31 47867: I want to Feature, then back to bug as it should have worked 11:02:43 robertdebock: alive again? 11:02:58 alive, not dead. (?) 11:03:09 robertdebock: was the `again` that confused me 11:03:22 ;-) 11:03:57 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 Good one @gundalow. For 90% of the contributors IRC is the same as the dark-web. ;-) 11:05:32 If it's not on GitHub it doesn't count 11:06:12 true 11:06:51 I'll be back in a bit, lunch time for me. 11:07:01 robertdebock: Thanks for everything so far :) 11:08:15 Update ``make`` module honor the return code of the ``make`` command #47861 11:08:21 I see shaps has already commented 11:09:18 yep, and got no reply :( 11:13:22 * gundalow has gently poked 11:14:08 module_utils/mysql: Fixing unexpected keyword argument 'cursorclass' error after migratio… #47809 11:17:27 #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 shaps: oh, yes, much more readable, could you add that 11:17:50 yep adding 11:18:19 Add default nolog param for PreSharedKey argument in ec2_vpc_vpn.py module #47788 11:18:57 #action merge, + backport PRs for 47788 11:23:02 #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 webknjaz: Seeing lots of PRs where maintainer isn't being pinged :( 11:25:16 OK, let's do another 15 minutes then regroup 11:25:58 Fix GCE and scaleway VM detection #47193 11:26:50 winem_: Hi, you here for the PR review session? 11:27:05 47193: Looks straight forward 11:27:37 Hi gundalow, yes. winem at github 11:27:43 #chair winem_ 11:27:43 Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz winem_ 11:27:45 Welcome :( 11:27:46 :) 11:27:53 urgh, typing is difficult :( 11:28:20 that's why we automate everything, right? :) thanks ;) 11:28:21 #info unless we enforce changelogs via CI we aren't going to get them 11:29:06 winem_: Exactly, we are currently going through `label:small_patch` PRs. This is all very informal, so feel free to chat :) 11:29:31 I found adding changelog fragments to be the easier part of my PR, but I understand how it's often overlooked. 11:29:47 Xaroth: Yup, I think that's a fair summary 11:30:51 47193 I think that's good to merge, though needs changelog 11:32:09 #action merge (changelog) and backport 47193 11:33:26 #action merge (needs changelog) 47134 11:35:35 hostname.py: Fix openSUSE distribution name #47020 11:35:41 Anyone use SUSE? 11:35:48 Yes, 11:36:07 I'm also annoyed by Leap and Tumbleweed as ansible_distibution 11:36:09 robertdebock: Wondering if we need to check for both names? 11:36:59 Would you be able to take a look at 47020 and put some comments on the PR? 11:37:06 I guess the ansible_distribution is openSUSE, and the ansible_distribution_major_version is Leap or Tumbleweed. 11:37:16 #action review 47020 11:37:28 #action robertdebock review 47020 11:37:33 Thanks :) 11:37:51 #chair 11:37:51 Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal robertdebock shaps webknjaz winem_ 11:38:05 So, we've been through ~60 PRs, great work 11:38:12 * davegarath lunch break 11:38:32 Lets take a bit of time to think how it's been and if we want to change anything before continuing 11:39:05 1) Missing changelogs is going to stop a lot of these PRs from being merged 11:39:58 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 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 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 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 I mean we can do the changelog for those PRs 11:44:23 if we are ready to merge and all that's needed is a changelog and it's an older PR 11:44:32 Sounds good 11:45:17 More generally, are people finding what we've done over the last couple of hours useful? What suggestions do people have 11:46:40 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 Some modules require specific hardware to run against for example. How would you test that in CI? 11:48:48 a) We don't 11:48:58 b) We `mock` out the API 11:49:12 gundalow: maintainer not pinged as in module autor? 11:49:13 c) we use simulators, like we do for foreman, VMware, etc 11:49:19 author* 11:49:41 webknjaz: yup https://github.com/ansible/ansible/issues/48785 11:50:08 well.. I could shift my focus to that issue 11:51:47 webknjaz: That would be really useful 11:52:30 Thank you :) 11:52:33 :) 11:53:10 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 lol 11:55:36 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 webknjaz: Starting next week I plan to shift more of my focus on to Bot/Workflow stuff. We can chat then 11:56:51 I don't want this to be a part of ansibullbot repo. But sure, let's chat. 11:57:25 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 I recall we wanted to talk about it a while back 11:57:58 Yup, other stuff got in the way 11:58:01 11:58:01 11:58:01 Upload a fileChoose Files 11:58:03 bah 11:58:08 Support hostvars in openstack.py dynamic inventory #46683 11:58:10 heh 12:00:18 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 would be in `module_utils/basic.py` if there was, though I'm not aware of anything 12:08:46 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 hi 12:10:25 winem_: Sure :) 12:10:29 #chair resmo 12:10:29 Current chairs: AndyG Pilou Xaroth alexsaezm cybette davegarath felix__ felixfontein ganeshrn gundalow jborean93 rajeevarakkal resmo robertdebock shaps webknjaz winem_ 12:10:32 resmo: Afternoon :) 12:12:21 Fix Unicode-objects must be encoded before hashing #46608 12:12:53 jborean93: 46608 looks good 12:13:47 part 12:14:39 #action Azure to review& merge 46608 12:16:46 sets admin_password to no_log in na_ontap_cifs_server #46604 12:17:03 #action merge & backport 46604 12:17:42 46604: lgtm 12:18:34 [aws] Retry Autoscaling Group delete calls when scaling activity is in progress #46124 12:19:27 winem_: just added public API improvement notes/suggestions to your 48729 12:21:21 #action AWS to review and merge 46124 12:21:29 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 webknjaz: thanks. I have mixed feelings about that. on one hand it sounds like a beautiful pitfall, yes 12:21:48 Make description as optional parameter in jira module #46023 12:21:51 gundalow: aye 12:22:16 gundalow, need add changelog and description in #46023 12:22:34 I will comment in PR 12:22:40 Thanks 12:24:35 46023: Looks good to me, except the changelog. 12:24:48 Fix incorrect processor information in Setup #45870 12:24:54 ^ Windows 12:25:08 * gundalow updates description 12:25:50 * akasurde no power in Windows section 12:26:10 ditto 12:26:46 #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 Makefile: build rpm git parameters #45718 12:29:33 * gundalow assigns to people that should know 12:30:22 sensu_silence: Set expire to int #45706 12:30:39 hehe, https://github.com/ansible/ansible/pull/45706/files == https://github.com/ansible/ansible/pull/33239/files 12:30:43 yes 12:30:52 duplicates 12:31:04