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