10:00:30 #startmeeting Ansible PR Review Day 10:00:30 Meeting started Tue Sep 3 10:00:30 2019 UTC. 10:00:30 This meeting is logged and archived in a public location. 10:00:30 The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot. 10:00:30 Useful Commands: #action #agreed #halp #info #idea #link #topic. 10:00:30 The meeting name has been set to 'ansible_pr_review_day' 10:01:03 * gundalow waves, who's around? 10:01:08 hi 10:01:32 #chair shaps 10:01:32 Current chairs: gundalow shaps 10:02:31 #info So we've been crunching the numbers, if we can close/merge just an extra 25 PRs/week then we will level out the backlog. Anything over that and the PR backlog will start to drop 10:02:36 gundalow: do we have a list or an order for PRs we're looking at? 10:03:11 I do! 10:03:25 #topic label/needs_repo 10:04:09 #info https://github.com/ansible/ansible/labels/needs_repo The source repo (branch) has been deleted. This means that any changes can't be made. Often this indicates that the other isn't around. Idea here is to close 10:04:12 So starting at the top 10:04:47 https://github.com/ansible/ansible/pull/59872 New Windows module to chanage CDROM drive letter and unmount virt. cdroms 10:06:09 hey halberom :) 10:06:17 Given https://github.com/ansible/ansible/pull/59872#issuecomment-521866289 I wonder if this is candidate_to_close? 10:06:38 * halberom waves 10:07:01 halberom mrproper: Are you joining for PR review today? 10:07:29 I will later. Can’t sleep though so just stopping in to see topic and all. 10:07:36 I'm in the curious camp 10:07:48 halberom: cool, feel free to shout out any any point 10:08:00 halberom: So we've been crunching the numbers, if we can close/merge just an extra 25 PRs/week then we will level out the backlog. Anything over that and the PR backlog will start to drop 10:08:25 #action see if 59872 can be closed 10:08:50 https://github.com/ansible/ansible/pull/58170 VMware: vsphere_copy IndexError: tuple index out of range 10:09:55 looks like lijok has deleted all their repos 10:09:59 and this isn't the right fix 10:10:29 https://github.com/ansible/ansible/pull/24111 feels abandoned to me. 10:12:06 58170 candidate to close 10:12:14 #action close 58170 10:12:40 https://github.com/ansible/ansible/pull/24111 New module: Add Uptrends module. (monitoring/uptrends) 10:13:06 Created: Apr 28, 2017. Last update 2017 10:13:12 mrproper: yup, I think close as well 10:13:57 Also seems like only a thin wrapper around URI 10:14:05 #action close 24111 10:14:29 https://github.com/ansible/ansible/pull/57042 make env setup faster 10:14:36 mrproper: Thanks for 24111 10:15:03 https://github.com/ansible/ansible/pull/12090 has an author who shows up once a year. But there’s been no movement in a full year again. How do you feel about this situation 10:16:39 #action close 57042 10:16:59 https://github.com/ansible/ansible/pull/12090 Update cobbler.py inventory script to more fully leverage Cobbler management fields 10:17:49 mrproper: Have you seen the author around? 10:18:11 Not particularly. I’m judging his frequency based on comments. 10:18:23 ack 10:18:46 The Bot doesn't always notify maintainers of inventory scripts 10:19:32 #action BOTMETA add maintainer for inventory/cobbler - Update 12090 10:19:50 Sounds like people would find this useful 10:20:24 I’m stepping away for a few hours but will be on later. Good luck. 10:20:30 mrproper: Thanks :) 10:21:12 https://github.com/ansible/ansible/pull/56953 Update win_domain_group_membership.ps1 10:22:24 #action close 56953 (It's less of an issue in 2.8) 10:22:40 https://github.com/ansible/ansible/pull/55436 PR try 3 10:25:10 #action check with Security Team, most likely close 55436 P 10:25:32 https://github.com/ansible/ansible/pull/54912 Create django_models.py 10:27:10 54912 confuses me. 10:28:35 #action ask others about 54912, most likely close it 10:28:58 gundalow: which bit confuses you? 10:29:13 shaps: why it's got specific things for Networking in there 10:30:01 lack of understanding on my part 10:30:18 https://github.com/ansible/ansible/pull/54167 Return MAC address from Supermicro and HP systems 10:32:50 https://github.com/ansible/ansible/pull/53796 New module: Kibana saved object 10:34:09 53796: Asked if they will work on it, if not close 10:34:52 https://github.com/ansible/ansible/pull/51668 Added OS families 10:36:13 https://github.com/ansible/ansible/pull/51371 flatpak: Open subprocess in text stream mode 10:38:31 51371: Added `needs_info` so will autoclose if no more comments are added 10:39:01 https://github.com/ansible/ansible/pull/50454 Issue #41447 - support AWS Aurora S3 privileges in mysql_user module 10:40:48 50454 (no branch, merge conflict) has been replaced by 59663. So closing the first one 10:43:01 https://github.com/ansible/ansible/pull/47980 scap facts module 10:45:42 47980 I've pinged the author to see if they will still work on it 10:45:57 .... 10:45:57 Anyone still out here? 10:46:10 https://github.com/ansible/ansible/pull/44981 VMware: Add error checking for networking 10:48:07 44981 closed as it breaks backwards compatability 10:49:27 https://github.com/ansible/ansible/pull/42106 Update lambda.py 10:49:27 42106 - I'll ask in #ansible-aws 10:51:04 https://github.com/ansible/ansible/pull/41653 add master_ssl_verify_identity to mysql_replication 10:51:38 41653 looks useful 10:53:04 I’m kind of back. 10:54:21 We don't have any MySQL maintainers so ^ has just sat around :( 10:54:37 https://github.com/ansible/ansible/pull/40411 Add support for GitHub Enterprise for github_deploy_key 10:55:26 How many are you set to close? 10:56:05 So far we've looked at 14 (assuming I've remembered to add the label) https://github.com/ansible/ansible/labels/pr_day 10:56:05 I've added `label/candidate_to_close` to 8 of those 10:56:20 oh, and already closed 4 10:56:43 So clos(ing/ed) 12/14 in an hour 10:57:06 Though I've started with a GitHub query where I expect most can be closed 10:57:15 Are you staying with needs_repo all session? 10:57:17 ie, get a few closes in the bag before moving on to harder things 10:57:59 `label/needs_repo` has PRs in. I was planning to go through all of them. 10:57:59 If anyone has specific PRs I'm happy to go though them 10:58:15 Then I thought I'd look at small_patch/owner_pr 10:58:34 Let me see if I have open PRs I am good with merge on. 10:59:11 Nothing I have tested lately. So not now. 11:00:35 https://github.com/ansible/ansible/pull/40003 Update cron.py 11:04:19 I guess it's because he needed it for networking stuff and then adapted it to add 'normal' hosts? 11:04:23 https://github.com/ansible/ansible/pull/36096 Same bugfix as #30121 11:06:32 36096 I'll ask someone else 11:07:40 https://github.com/ansible/ansible/pull/32930 Windows Facts: add WinSystemLocale 11:11:07 https://github.com/ansible/ansible/pull/32571 Add aci_bd_dhcp_label.py 11:11:07 32571 they look to be active still 11:11:25 https://github.com/ansible/ansible/pull/32549 refactored gen_candidate_chars out to common; hashi_vault generating passwords 11:12:40 richardcs doesn't seem to be active at all on GitHub anymore so closing 11:13:57 https://github.com/ansible/ansible/pull/32158 iptables: chain creation and deletion 11:17:35 32158 poked commenters, set to autoclose if no updates are added 11:17:55 https://github.com/ansible/ansible/pull/30653 Changed 'plugins/json_query.py' to use jebabin's solution to issue #27299 11:18:25 (closed) 11:20:17 https://github.com/ansible/ansible/pull/28474 Add sql_log_bin option to mysql_db module 11:21:08 No one has looked at this since August 2017, so closing 11:22:50 right, gona get some lunch. Thanks everybody 11:22:50 So in the past 80 minutes we've 11:22:50 30 PRs looked at 11:22:51 17 Closed 11:22:51 I think as we continue (back in time) with `label/needs_repo` we will be closing at a higher rate 11:22:51 @gwmngilfen ^ 11:23:05 good stuff! 11:55:16 awesome! 12:21:12 * gundalow waves 12:26:49 https://github.com/ansible/ansible/pull/27431 closed. I found a new PR that doing the same thing 12:32:52 right, I'm going through and closing PRs needs_repo PRs where there have been zero updates in over a year 12:44:36 bcoca: Morning :) Any ideas why https://github.com/ansible/ansible/pull/12272 has `roadmap_candidate` I was going to close it given 1) needs_repo 2) token expirartion & reauthentication requests 12:47:11 24 PRs closed 12:47:25 * resmo likes this progress! 12:47:55 I'm going to skim the remaining 24 needs_repo PRs and lean towards closing (as resmo suggested) 12:48:49 * resmo .oO(we are closing this PR because resmo suggested it... if you feel this is wrong, blame resmo) 12:49:10 There was a PR from rosmo which confused my brain for a while 12:49:16 lol 12:49:59 bcoca: should I close https://github.com/ansible/ansible/pull/28469 12:58:16 oh, win. 4 PRs have updates already so merged/closed a few more based on feedback 13:00:38 #info needs_repo has been reviewed. We've closed/merged 25 13:01:13 #topic owner_pr & small_patch 13:01:24 #info https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Asmall_patch+label%3Aowner_pr+-label%3Abackport 13:01:33 I *assume* these might be easy to review & merge 13:05:42 https://github.com/ansible/ansible/pull/58816 bitbucket_access_key: Fix Python 2.7 compat 13:05:46 58816: Going to assume this is good 13:06:20 https://github.com/ansible/ansible/pull/58089 win_package - Test to solve quotes issues 13:09:57 58089 hum, not sure, asking in #ansible-windows 13:11:44 https://github.com/ansible/ansible/pull/55253 jenkins_plugin: Read update file as UTF-8 (fixes #55250) 13:11:44 55253 needs reviews 13:12:57 https://github.com/ansible/ansible/pull/52017 elb_application_lb: Fix required_together statement - access_logs_s3_prefix is not requir… 13:13:13 52017 looks sensible, will merge 13:14:40 https://github.com/ansible/ansible/pull/39175 Support new mongodb versions in check_compat function 13:16:49 ^ needs revision 13:21:19 https://github.com/ansible/ansible/pull/32912 Fix missing pass of IPXe params to API client call, fixes #32910 13:21:19 ^ looks sensible 13:22:37 #topic small_patch NOT owner_pr 13:22:43 #info https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Asmall_patch+-label%3Aowner_pr+-label%3Abackport+ 13:24:18 I just want to say that I'm quiet but reading PR names to see if there's anything I'd be competent to help with :p 13:25:15 dmsimard: ace, that's part of why I talk to myself :) 13:25:42 * gundalow takes a break to go to another meeting 13:26:13 If anyone has any PRs they are interested in, feel free to list the URLs here 13:35:04 gundalow: Still churning through? 13:38:11 mrproper: yup, though just got another meeting now] 13:39:40 mrproper: next was going to look at `label:small_patch -label:owner_pr -label:backport` 13:41:14 Wow, lots of these. 13:43:08 gundalow: https://github.com/ansible/ansible/pull/18583 has been blessed and is waiting for rebase for 2 years. 14:11:17 There are two PRs for adding a tower_schedule module -- I don't have time to pursue this right now. I had sent https://github.com/ansible/ansible/pull/43437 and more recently someone else sent https://github.com/ansible/ansible/pull/60040 14:11:26 Maybe someone knowledgeable about these waters could look, it would be a good addition 14:20:33 dmsimard: related does https://github.com/ansible/tower-cli/pull/592 need your feedback? 14:21:13 Well, 2.9 has been branched. so we have all of Ansible 2.10 to get those PRs sorted 14:22:52 mrproper: Do you use VMware. I'd say just create a fresh PR based on https://github.com/ansible/ansible/pull/18583 14:23:31 gundalow: I'd trust Alan with that PR, it was to fix the create_on_missing idempotency issue 14:24:21 gundalow: I don't. I'm just going through PRs. 14:25:48 mrproper: ah, cool 14:30:19 gundalow: https://github.com/ansible/ansible/pull/56691 Looks to be possibly an easy one. It's a licensing question. 14:32:13 * gundalow looks 14:33:19 mrproper: hum, needs response from tbouvet, anyone know if they are still around? 14:36:40 * gundalow has added a comment 14:36:48 OK, so lets work through https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Asmall_patch+-label%3Aowner_pr+-label%3Abackport+ 14:37:12 https://github.com/ansible/ansible/pull/61686 Fix examples for bigip_file_copy 14:37:36 Given caphrim007's comment I'll merge 14:39:55 https://github.com/ansible/ansible/pull/61668 github_deploy_key: Clarity on mutual exclusion of Username and Token 14:40:23 * acozine waves 14:40:32 * gundalow waves back 14:40:46 today's Docs WG has been redirected here 14:40:54 * samccann waves 14:40:59 whaddawe got? 14:41:07 https://github.com/ansible/ansible/pull/61668/files looks good though I'm not going to bother backporting it 14:42:03 +1 14:42:10 +1 14:42:38 +1 14:42:40 samccann and I might backport it to 2.9 as part of our docs backportapalooza 14:42:53 acozine: samccann welcome. I'm going through a list. 14:42:53 This time I'm adding `pr_day` label if we look at the PR, that helps us track to see if PRs we process today have better odds at success than the general backlog 14:42:59 backportapalooza, I like that term 14:43:12 heh 14:43:27 Also if we spot any PRs that look close, though need more review we should add the `ansiblefest` label so we can review during Community Central 14:43:38 ah, good idea 14:43:42 https://github.com/ansible/ansible/pull/61660 YAML parser - set source file position for vault encrypted strings 14:43:58 61660 No idea about that 14:44:43 it's failing CI 14:45:03 In an unrelated note to self, do not launch a t2.micro instance to test an application that needs 10 minutes on a t2.large .... 14:45:05 * Xaroth facepalms 14:45:35 61660 skipping 14:45:52 Xaroth: didn't you need an excuse to go get coffee? 14:45:52 https://github.com/ansible/ansible/pull/61657 Remove unnecessary information about amount of actions in the meta module 14:46:12 acozine: I need an excuse to head home early, seeing it's 16:45... 14:46:13 ^ docs PR looks OK to me 14:46:29 failure on 61660 looks legit 14:47:11 Xaroth: ah, that's def. the wrong time to start longrunning tests on slender infrastructure 14:47:17 61657 merged 14:47:41 gundalow: you're too fast for me! 14:47:52 https://github.com/ansible/ansible/pull/61656 Update unit tests requirements/units.txt path 14:48:11 acozine: oh, was there an issue? maybe I should go back to `#action merge 1234` and do them tomorrow 14:48:14 never knew there was more than one meta handler :P 14:48:28 @gundalow nope, the PR was fine 14:48:32 grrr 14:48:38 gundalow: the PR was fine 14:48:52 I was trying to tag-team it for you 14:49:02 61656 merged, could you add this to your backport list please https://github.com/ansible/ansible/pull/61656 14:49:40 https://github.com/ansible/ansible/pull/61655 Fix creation of DigitalOcean droplets using digital_ocean_droplet module 14:50:17 gundalow: thanks, typically we run through any PR that's marked `docs` that's merged between when we branch a new stable branch and when we release 14:50:26 trying to do it weekly this time 14:50:53 they can't be mixed with code changes, though 14:53:03 samccann: this one's in your wheelhouse: https://github.com/ansible/ansible/pull/61651/files 14:53:39 ok I'll review in the background here and merge if appropriate 14:54:17 61655 (digital ocean) Looks OK, though needs someone that understands the code. I've added a minor comment 14:55:37 61599 duplicate: closed 14:56:23 https://github.com/ansible/ansible/pull/61561 resmo Thank you for all you've done and continue to do! (merged with sadness) 14:57:13 https://github.com/ansible/ansible/pull/61556 net_user documentation typo configured_password 14:57:23 61556 I'm just checking the actual code 14:58:09 I've got it half-merged 14:58:30 acozine: 61556? 14:58:35 yep 14:58:53 cool, please ensure you add `pr_day` label to anything you look at 14:58:56 for the idly curious... what's half a merge? :-) 14:58:59 acozine: merge and backport at will 14:59:07 samccann: stuck in the "confirm merge" stage ;) 14:59:12 AAHAHA gotcha 14:59:41 that one is really annoying 14:59:43 github is having a rough day 15:00:07 felixfontein: yep, especially on one-line changes 15:00:27 phew - merged! 15:00:33 https://github.com/ansible/ansible/pull/61552 docs: Update apt_key.py add requirements of gpg 15:00:33 61552 I'm updating the docs (to fix CI) 15:01:20 bah, ENOPOWERSTOPUSHTO61552 15:02:23 looks like someone actually unchecked the checkbox 15:02:33 see is occasionally 15:03:05 wow 15:03:10 urgh, words 15:03:19 I see that occasionally 15:03:22 well, they'll have to wait for their PR to get merged, then 15:04:06 https://github.com/ansible/ansible/pull/61493 Fix bug preventing wait_condition from being respected when using apply 15:04:06 61493 Will asked for tests, though I see they are in a follow up PR. So I'm tempted to merge 15:04:18 #action merge 61493 15:05:16 https://github.com/ansible/ansible/pull/61430 Add mount_point option to approle login 15:05:32 https://github.com/ansible/ansible/pull/61651/files merged (with `pr_day` label added) 15:05:40 samccann: Thanks :) 15:05:46 I have no idea why https://github.com/ansible/ansible/pull/61595/files is marked `docs` 15:06:47 acozine: it has a 'patch-1' branch - do we mark anything edited via the docsite as `docs` even if it's code? 15:06:57 oh, i bet we do 15:07:04 that's a sneaky trick 15:07:24 do we remove the `docs` label when we see these that are code only? 15:07:25 Look at history for description. Originally it had `+label: docsite_pr Docs Pull rquest 15:07:31 yup, update labels 15:07:57 k done 15:08:59 this one fixes indentation in a block in an example: https://github.com/ansible/ansible/pull/61622/files 15:09:01 looks okay to me 15:09:51 +1 15:09:54 61622 merged 15:09:57 oops, forgot the label 15:10:41 `pr_day` label added 15:12:39 what about https://github.com/ansible/ansible/pull/61667 ? 15:13:01 one from felixfontein :) 15:13:36 `module.deprecate("The option 'acme_version' will be required from Ansible 2.14 on", version='2.14')` 15:13:43 is the word `required` right? 15:14:08 oh, I see what Felix is doing, nice 15:15:12 Looks sensible 15:15:13 gundalow, kind of sad how my cloudstack journey ended... 15:15:33 felixfontein's woks looks always sensible 15:15:40 resmo: It seemed like a fun & productive journey 15:15:41 s/woks/works 15:15:45 resmo: truth :) 15:16:23 the code part is a bit opaque to me - let me read it again 15:17:02 gundalow: yes, `required` is the correct word there 15:17:20 gundalow, I wonder if the "import locale" and locale part is really needed 15:17:29 felixfontein, ^^ 15:18:13 https://github.com/ansible/ansible/pull/61667/commits/1fa6f99f044256fc1c52104a55388843f3a13dc5#diff-f304419db157dd40f7accc8a752da63bR979 15:18:40 should I put comments here, or on the PR, or both? 15:18:57 acozine: at least on the PR, can be useful here as education to others 15:19:39 seems he ported it from one module, so historically it was there before 15:19:50 I'd suggest leaving the `default` line in the docs - that's a standard segment and should be documented in a standard way 15:19:51 never mind 15:19:52 https://github.com/ansible/ansible/pull/61667/files#diff-d4f6bc58692a5a4858ccfeae3b7c458eL88 15:21:53 61262 merged 15:22:13 agreed acozine 15:26:02 resmo: I also wondered that; I don't have any machine set up with strange locales, or a localized version of openssl, so I never needed it myself, but it could be that this actually is a problem for others. I remember that we had issues for that (like a year ago or so, from someone from France I think) 15:26:41 gundalow: 61262 shows as still open for me . . . 15:27:22 acozine: if I don't remove "default", I have to add an ignore entry into tests/sanity/ignore.txt as it will be different from argument_spec 15:27:38 acozine: oh, sorry, I'm doing `rebuild_merge` 15:27:52 this one looks good to me - https://github.com/ansible/ansible/pull/61626#pullrequestreview-283078792 15:28:04 acozine: using C(...) for emphasis is even worse I think. at least I(...) looks as expected, and I've seen a lot of people do that :) maybe we need a new code for that? 15:28:29 (I originally thought that `I` stands for 'italics') 15:28:51 I is italics 15:28:56 * gundalow checks 15:29:03 I is module option 15:29:19 which happens to be italics :) 15:29:20 https://docs.ansible.com/ansible/2.6/dev_guide/developing_modules_documenting.html#formatting-functions 15:30:01 gundalow: https://github.com/ansible/ansible/pull/61667/#discussion_r320335121 15:30:20 +1 15:31:33 how about using *...*? that's ReST syntax, but also looks good in ASCII (i.e. with ansible-docs) 15:32:13 I would prefer not to use I just for italics. 15:32:36 the other option is add it to the Notes section as well, and leave it where it is w/o trying to make it italics. 15:32:59 samccann: https://github.com/ansible/ansible/pull/61626/files (adding a collection locat try section) seems OK to me 15:33:15 ok merging.. thanks gundalow 15:34:27 felixfontein: ah, interesting discussion 15:34:46 https://github.com/ansible/ansible/pull/61060 Add caution about handlers & import to Pitfalls (docs) 15:35:04 I guess using `I(...)` for options/parameters is purely a convention? 15:35:34 I'm confused about the default line though 15:35:46 why isn't the default listed in the argspec? 15:36:39 because if it is listed there, the code cannot distinguish the cases "the user didn't specify anyhing" (in this case, a warning has to be emitted) and "the user explicitly specified the default" (in this case, no warning must be emitted) 15:37:44 added a nit comment to https://github.com/ansible/ansible/pull/61060 to remove the Latin (via) 15:37:51 it's definitely ugly, but the way AnsibleModule works 15:37:59 so it's more a suggested default? 15:38:08 samccann: Thanks 15:38:15 TIL `by` not `via` 15:38:23 it is the default currently used by the module, but I want to get rid of it (without breaking backwards compatibility) 15:38:37 just say no to Latin in docs! :-) 15:38:57 I generally think of a default as "the value we use when you don't provide a value at all" 15:38:59 'Closing because it uses Latin' 15:39:16 I like via :) 15:39:42 it is also a German word (probably taken from Latin) 15:40:09 yeah likely 15:40:30 it has a nice, pan-European sound to it, but it trips up translation to Japanese and other languages 15:40:55 does it have some weird meaning in those languages? :D 15:40:56 ah. I didn't knew that... 15:41:33 it can trip up non-English speakers (tho I should extend that to non English speakers who don't speak a romance language) 15:42:11 acozine: That's what the default value is, I guess what felixfontein is trying to do is to actually remove the default value completely 15:42:22 35 PRs closed/merged so far 15:42:27 But as of now it cannot be removed, because it would break backwards compatibility 15:42:29 woot! 15:42:36 gundalow: Nice 15:42:37 Aim is to get to 50 15:42:46 shaps: ah, so the first step to removing the old default is to warn folks who don't pass a value at all 15:43:26 acozine: exactly. if they already provide the default value explicitly, they're fine 15:43:28 that would be my guess 15:44:06 so it isn't really a default any more - it's more of a suggested value? 15:44:41 It's still a default, until it will disappear, and won't be anything 15:44:44 acozine: yes. it still acts like a default, but you get a warning if you don't specify it 15:44:53 If I don't provide a value, I get a warning . . . after the warning, does Ansible still supply that value? 15:44:54 and then the option will be required and you get an error 15:45:04 gotcha 15:45:10 thanks for explaining that 15:45:26 acozine: yes, it supplies the value at the same time as issuing the warning, so that existing playbooks/roles won't break 15:46:09 (fun fact: the default was introduced when the option was added to not break backwards compatibility ;) ) 15:46:29 heh 15:47:07 Pretty sure 90% of the defaults exist for that reason 15:47:40 yep. and I think it happens more often that people want to get rid of default values than you would think 15:47:57 maybe we should add support for it to AnsibleModule 15:48:40 (or at least something which allows querying where an option's value comes from) 15:49:29 O 15:49:29 Probably worth creating a proposal :) 15:49:39 * acozine apparently can't type today 15:49:46 I'd appreciate some input on https://github.com/ansible/ansible/pull/61232 15:50:28 shaps: yep :) 15:50:40 especially https://github.com/ansible/ansible/pull/61232#issuecomment-525816207 15:50:47 added some copy-edits to https://github.com/ansible/ansible/pull/61221/files - otherwise looks good to me, but can someone take a look as it's vault module 15:51:13 that reminds me of dag's PR on adding a mode type 15:53:13 ok, I have to run, see you a lot later or tomorrow :) 15:53:28 felixfontein: thanks and bye! 15:54:01 thanks a lot for reviewing! 15:54:37 felixfontein: Thanks ! 15:56:52 hum, should we carry on with `is:open label:small_patch -label:owner_pr -label:backport ` or move to something different? 15:57:18 gundalow: I've been tossing random docs PRs into the hopper 15:57:38 acozine: yup, andthat's cool 15:57:41 Got any more? 15:57:56 hey, my meeting at the top of the hour just disappeared from my calendar 15:57:59 * acozine does a little dance 15:58:10 lemme look for some more random PRs 15:59:27 this one has some accompanying code that needs some review - https://github.com/ansible/ansible/pull/61178/files 16:00:00 gundalow: I can only see another 55 Prs for that search, maybe worth keep looking at this? 16:00:26 here's another docs-and-code one: https://github.com/ansible/ansible/pull/61072/files 16:01:13 I would probably also go backwards, so from page 3, likely easier to close off lots more 16:02:23 erm, there are 219, so 9 pages in https://github.com/ansible/ansible/issues?page=3&q=is%3Aopen+label%3Asmall_patch+-label%3Aowner_pr+-label%3Abackport&utf8=%E2%9C%93 16:03:24 added comment to -https://github.com/ansible/ansible/pull/60594/files but otherwise looks good. 16:03:36 yeah, got the wrong filter, I put `is: pr` and it only came back with 55 results meh 16:03:52 we are only looking at PRs. did I mess something up 16:03:56 I think my backwards comments still applies though 16:04:24 oh, if we want to close more 16:05:04 yep 16:05:42 ( what I meant earlier is, if you put a space between 'is:' and 'pr' it will come back with 55 open PRs, for some reason ) 16:06:07 with the correct filter I got 219 too 16:06:09 ah 16:06:19 contains word `is:` `pr` 16:06:43 yeah 16:06:50 `is:pr is:open label:small_patch -label:owner_pr -label:backport sort:created-asc ` this works 16:07:33 meaningful whitespace trips us again! 16:07:38 He gets everywhere! 16:08:47 acozine: - does this look ready to merge? https://github.com/ansible/ansible/pull/60600/files 16:09:01 https://github.com/ansible/ansible/pull/18583 been asked for rebas 2017, 2018. I think I'll close 16:09:48 ^ yes 16:10:08 samccann: yes 16:10:16 merging 16:12:17 https://github.com/ansible/ansible/pull/22765 closing 16:12:56 gundalow: w00t! 16:13:01 how about https://github.com/ansible/ansible/pull/60112? 16:14:09 the PR owner didn't actually update the `valueFrom` field 16:14:11 https://github.com/ansible/ansible/pull/60112#pullrequestreview-271678361 16:14:35 oh, I'm wrong 16:14:37 he did update it 16:14:46 he/she/they did 16:15:17 acozine: looks good 16:16:28 https://github.com/ansible/ansible/pull/30467 adding the `ansiblefest` label, need testing though I think should be good 16:16:54 acozine: +1 16:17:36 oh crap 16:17:43 it was marked `stale_ci` and I missed it 16:17:59 probs best to use `rebuild_merge` 16:18:03 * acozine gets a little excitable when merging PRs 16:18:22 gundalow: yes, thanks for the reminder 16:18:42 Don't worry, I've done it lots of times as well :( 16:19:09 if it fails on backport to 2.9 I will make sure the fix gets into `devel` as well 16:20:03 this one looks good to merge (or `rebuild_merge`) do y'all agree? - https://github.com/ansible/ansible/pull/59144/files 16:20:36 gundalow: #30467 looks reasonable 16:21:20 shaps: thanks 16:21:57 going offline for a bit, see you later 16:22:07 shaps: Thanks as always! 16:24:16 https://github.com/ansible/ansible/pull/39557/files looks sensible, I've retriggered CI 16:24:27 I'm going to head offline for an hour or two 16:27:56 If you want to continue feel free 16:28:54 any network folks around? 16:29:21 to look at the PR samccann mentioned above? 16:29:28 https://github.com/ansible/ansible/pull/59144/files 16:30:35 it's got tests 16:32:06 it seems a bit hand-wavy to me, but maybe there are hard rules about determining peer IP addresses in /3[01] netblocks that make it make sense 16:35:38 agaffney: can you think of a better way to implement the feature? 16:35:56 I don't completely understand the use case, so not really 16:36:15 heh, well, you understand it better than I do :-) 16:36:17 it *seems* like it's making assumptions about how peer addresses are configured, but they may be valid assumptions 16:36:31 but I'm not well versed enough on the network side of things to say either way 16:43:51 What does "File was not added to sdist" mean as a test error? 16:45:12 tremble: are you seeing that locally or on Shippable? 16:45:24 acozine: https://github.com/ansible/ansible/pull/61284 16:45:53 acozine: Should I just try a rebase to force retesting? 16:46:42 tremble: the PR is marked `ci_verified` which means the failure is an issue with the code, not an issue with our CI service 16:46:54 so re-testing won't fix it 16:47:27 if you follow the `explain` link here: https://github.com/ansible/ansible/pull/61284#issuecomment-526329438 16:48:10 you will find this page with documentation about the failing test: https://docs.ansible.com/ansible/devel/dev_guide/testing/sanity/package-data.html 16:48:34 is that [explain] link relatively new? 16:48:54 I've never noticed it before 16:49:39 agaffney: it's been there at least a year 16:49:53 I guess I just don't pay much attention :) 16:49:55 heh 16:49:58 Sorry, I still don't get what it's complaining about... 16:50:08 changelogs/61284-ec2_asg-idempotency.yml:0:0: File was not added to sdist 16:50:14 what's weird about this PR, though, is it's complaining about the changelog fragment 16:50:35 tremble: yep, I'm not sure how that test is failing on that file 16:50:40 in general, it's complaining about the fact that setuptools is a PITA 16:50:51 well, it's not wrong about that 16:51:10 I've been down this road before with other projects :) 16:51:37 oh, I think the trouble is that the changelog entry is not in the `fragments` directory 16:51:48 so it would be missed when building the changelog 16:52:11 tremble: see https://github.com/ansible/ansible/tree/95915a83397312a6d88c3cfc2ca0f2377d8481dc/changelogs 16:52:32 Oh, it should have gone into fragments 16:52:35 doh! 16:52:39 yep 17:02:01 any google cloud users here? 17:02:09 I'm looking at https://github.com/ansible/ansible/pull/61178/files 17:04:00 ooh, how about https://github.com/ansible/ansible/pull/61111? from a docs perspective, I like this approach 17:04:22 needs a rebase, so we can't merge it right away 17:20:26 I rebased 61111 17:20:35 running through CI now 17:20:45 * acozine heads out for food 17:22:46 reviews/comments on any PRs from https://github.com/ansible/ansible/issues?page=3&q=is%3Aopen+label%3Asmall_patch+-label%3Aowner_pr+-label%3Abackport&utf8=%E2%9C%93 welcome - our fearless leader gundalow will be back in an hour or two 17:46:07 looking for help on this one - https://github.com/ansible/ansible/pull/58768/files 17:47:42 it replaces `with_items` with `loop`. But I can't tell if that's correct. 17:49:28 this is an example of what it iterates over - https://github.com/ansible/ansible/blob/2d001227699ea2df31ad2230f8fa408a1b0132ab/lib/ansible/modules/net_tools/nmcli.py#L280 (not a simple list I don't think). I think acozine told me before it doesn't need `flatten` but I can't tell if it's a nested list or a dict (both of which need more help than just `loop`) 18:06:54 * gundalow returns 18:13:34 acozine: samccann Are you going through a specific query at the moment? 18:13:46 Or does anyone have anything specific to review? 18:14:36 gundalow: I'm just trolling the backlog of docs PRs looking for things that are docs only, simple, and just waiting on someone smarter than me to approve :-) hard to fit that into a github query 18:17:40 samccann: does https://ansible.sivel.net/pr/byfile.html help you? 18:19:23 gundalow: erm... not sure what I'm looking at there? 18:19:58 oh I see, prs by file (kinda like it sez) 18:20:39 I have a query for open docs prs that's pretty straightforward. Then it's just reading to see what i've looked at before but couldn't move along on my own (with comments or merging). 18:23:17 samccann: You could always create a "docs_team_have_looked_at_this" type label 18:25:20 more like `docs_needs_help` or something to say acozine and I can't resolve it ourselves. (that said, she might know the answer to that latest one on the `loop` 18:33:34 samccann: https://ansible.sivel.net/pr/byfile.html Ctrl+F `docs/docsite/` does that give us anything you'd like us to look at? 18:35:44 this one has sat stagnant for a time - not sure if the debate resolved itself or not - https://github.com/ansible/ansible/pull/53595 18:40:05 I mentioned this one earlier 18:40:40 https://github.com/ansible/ansible/pull/59144 18:44:14 samccann: I'll put that on Core agenda 18:46:13 acozine: I've asked Network team to review that 18:46:31 awesome 18:48:31 this one's passing and I think we can merge: https://github.com/ansible/ansible/pull/61552 18:52:29 thanks! 18:52:48 this one should probably go on the Core agenda too https://github.com/ansible/ansible/pull/59605 18:53:59 I merged 61552 18:53:59 +1 for 61552 18:54:08 oh, good, thanks samccann 18:55:09 here's one I've been working on fora while: https://github.com/ansible/ansible/pull/58102 18:56:10 the OP seems to have disappeared 18:57:00 I can do an end-run by pushing commits to his branch instead of the suggestions 18:58:35 text if all suggestions are applied https://www.irccloud.com/pastebin/iJrbehQf/PR%2058102 18:59:23 anybody more experienced than I am with complicated untar commands? 19:02:54 gundalow: did we already look at https://github.com/ansible/ansible/pull/61072? 19:03:37 hmmm, is that still a `_facts` module not an `_info` module? 19:05:07 acozine: nop. though partner engineering should be keeping an eye on netapp 19:05:38 ah, okay 19:13:05 * gundalow wonders what other queries may give some quick merge/close? 19:13:24 how about this one? 19:13:25 https://github.com/ansible/ansible/pull/60881/files 19:14:16 I've never used Ansible with macOS 19:14:22 I mean, to manage macOS 19:15:23 it looks reasonable to me, but some feedback from a more experienced user would be great 19:15:25 acozine: I've added rebuild_merge 19:15:32 gundalow: cool! 19:16:16 restarted shippable on https://github.com/ansible/ansible/pull/61111 cuz it's having a bad hair day 19:17:29 uh-oh, did it fail on some "can't find the package, too much internet traffic" thing? 19:17:57 dunno... took ages and then failed as 'unstable' which seems like... you know... and unkind thing to say really 19:17:59 :-) 19:18:09 heh 19:18:44 I think "unstable" means "it passed this time but it has failed so often that I can't really believe it's passing" 19:21:02 afaik, "unstable" means it failed once and then succeeded when running it a second time in verbose mode 19:21:23 thanks... let's see how the next attempt goes 19:21:49 agaffney: ah, thanks for the clarification 19:22:00 does anyone understand the intent of this PR? https://github.com/ansible/ansible/pull/61080/files 19:23:16 it adds a note about the 'dnsName' key in the dict, but I don't know much about what that module is managing 19:23:19 I find the suggested text hard to read, but I'd be happy to push commits to the branch and get the docs clarified once I understand what needs clarification 19:24:13 yeah, the modified docs text doesn't read very clearly, and the PR is also failing tests and has a merge conflict 19:31:22 **49** 19:31:56 we'll be at 50 once this passes shippable - https://github.com/ansible/ansible/pull/61111 19:32:50 and we'll be down to 75 open docs PRs (3 pages on GitHub) 19:32:59 go, Shippable, go! 19:34:43 ooh, this one too: https://github.com/ansible/ansible/pull/61060/commits 19:34:54 fixed, and mergeable once Shippable is happy 19:35:07 heh meanwhile reading on 61080 - the existing example uses the register approach. There isn't an example of the alternate approach the PR is correcting. 19:36:50 to be fair, the original docs aren't readable eithr 19:36:56 #either 19:37:07 oops, meant that for a * 19:37:13 *either 19:37:45 I know what the addition in #61060 is trying to say, but I don't think it says it very clearly for people that don't already know how that works 19:38:00 **50** 19:38:36 oh, oops - in 61060 the commit message is problematic 19:38:41 gundalow: w00t! 19:38:52 goal achieved, congratulations! 19:39:33 yep. So thinking out loud here - either you create a managed zone first and register it (that's what the example does with the gcp_dns_managed_zone module) - then you can update the dns resource record with this command `gcp_dns_resource_record_set` by using the registered managed zone and the name of the dns record. 19:40:09 i shoudl rewrite that 19:40:19 option 2 seems to be - you use the name of the managed zone, and the name of the dns record... to update the resource record for that dns resource on that managed zone 19:40:28 'imports are not handlers, they create handlres from the imported tasks, hence you need to reference the imported tasks names' 19:40:56 I'm signing off - need to prep for a meeting at the top of the hour 19:41:07 it's been a good Big PR Day! 19:41:13 \o/ 19:47:21 bcoca: did you see the couple of PRs I pinged you about above ^^^ 19:47:48 no 19:48:07 been on migration, just took a break and checked out htis channel 19:54:29 cool 19:55:00 #info 51 PRs closed/merged. I'm sure a few more will click through in the next day or so. Great work everybody 19:55:06 #endmeeting