14:00:54 <gundalow> #startmeeting Ansible PR review day
14:00:54 <zodbot> Meeting started Thu Nov 14 14:00:54 2019 UTC.
14:00:54 <zodbot> This meeting is logged and archived in a public location.
14:00:54 <zodbot> The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:00:54 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
14:00:54 <zodbot> The meeting name has been set to 'ansible_pr_review_day'
14:01:03 <gundalow> Who's around?
14:03:06 <jtanner> nobody it seems ... no PRs merged today i guess
14:03:09 <jtanner> =)
14:03:45 <gundalow> I know others are joining later
14:04:22 <gundalow> #topic Reviewing needs_repo
14:04:59 <gundalow> #info pr_day open: 18, pr_day closed: 126
14:05:40 <xuxiaowei0512_> Hi, gundalow jtanner.
14:05:43 <gundalow> #info https://github.com/ansible/ansible/labels/needs_repo these are PRs where the branch (or repo) no longer exists in the PR authors repo. Possibly an indicator that they are no longer around
14:05:47 <gundalow> #chair xuxiaowei0512_
14:05:47 <zodbot> Current chairs: gundalow xuxiaowei0512_
14:06:41 <gundalow> Added sudo user if using become method sudo for synchronize https://github.com/ansible/ansible/pull/21531
14:08:47 <gundalow> 21531: from Feb 2017, no updates since July 2017. Funzo asked for a toggle and to not change default behavior
14:08:58 <gundalow> I'll close (just finding my default close message)
14:12:04 <gundalow> found it, closed 21531
14:12:34 <gundalow> iptables: chain creation and deletion https://github.com/ansible/ansible/pull/32158
14:15:11 <gundalow> 32158 author has come back
14:15:53 <gundalow> Windows Facts: add WinSystemLocale  https://github.com/ansible/ansible/pull/32930
14:16:42 <gundalow> 32930 is progressing
14:16:59 <gundalow> Support for actions available in ALB https://github.com/ansible/ansible/pull/46369
14:23:05 <gundalow> 46369 no updates in over a year
14:25:26 <agaffney> 47535: no comment from author since January and last CI run failed
14:25:44 <gundalow> adds possible fix to allow additional search filters #47535
14:25:47 <gundalow> #chair agaffney
14:25:47 <zodbot> Current chairs: agaffney gundalow xuxiaowei0512_
14:25:52 <gundalow> Morning :)
14:26:49 <agaffney> 58768: simple module docs PR that is probably fine to merge after CI re-runs
14:27:23 <gundalow> 47535 closed
14:28:13 <gundalow> yup, did you trigger CI again?
14:28:21 <gundalow> Add advice how to prevent jinja2 warning #60594
14:28:47 <agaffney> 60594: simple docs PR with pending suggested wording change
14:29:23 <gundalow> though source branch doesn't exist so can't apply it. I (or someone else) can create a fresh PR for that
14:29:37 <gundalow> fix(mysql): fix underscored privileges #63086
14:30:18 <gundalow> 63086 new contributor, pr created and branch deleted in same day. I'll close
14:31:01 <gundalow> Added production flag to npm ci install command #63296
14:31:18 <agaffney> I did trigger CI again
14:31:41 <gundalow> agaffney: excellent, thanks :)
14:31:53 <agaffney> is it not possible to merge a PR with a missing source branch? I'd think it would still be mergeable with the code still available in the PR
14:32:05 <gundalow> agaffney: ah, yes, we can merge
14:32:17 <gundalow> just can't apply suggestions, though that could always be follow up PR
14:32:22 <gundalow> which is probs easiest
14:33:15 <gundalow> Added production flag to npm ci install command #63296
14:33:22 <gundalow> (first time contributor)
14:33:30 <gundalow> One line change
14:35:49 <gundalow> agaffney: I see Add advice how to prevent jinja2 warning #60594 has passed CI, I'll merge
14:37:05 <gundalow> #topic small_patch
14:37:11 <gundalow> #info https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Asmall_patch+-label%3Abackport
14:38:12 <gundalow> (from the top)
14:38:35 * gundalow ignores PRs by Core Team or modular-magician (GCP) as they have commit powers
14:38:45 <gundalow> consul_acl: allow setting rules for *_prefix where supported #64809
14:39:34 <gundalow> Looks sensible, though only open for 16  hours so maintainers haven't had a chance to look yet
14:39:46 <gundalow> ssh: Properly close stdout and stderr. #64785
14:40:37 <gundalow> 64785 hum, connection/ssh.py
14:42:57 <gundalow> Improved message for unarchive. #64594
14:43:29 <gundalow> 64594 CI restarted
14:43:50 <gundalow> oh, it's just a error message fix
14:44:55 <agaffney> 235 small patches...
14:45:02 <gundalow> yup
14:45:24 <gundalow> hum, I guess we could remove needs_revision and see how many of those could be merged, what do you think agaffney ?
14:46:08 <agaffney> that's down to 143
14:47:46 <gundalow> cool, lets do that
14:48:01 <gundalow> #info https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Asmall_patch+-label%3Abackport+-label%3Aneeds_revision (ignoring needs_revision)
14:51:11 <gundalow> Add default value to 0 for disk in nova_flavor module #64749
14:51:25 <gundalow> 64749  comment added, though needs review from openstack people
14:54:02 <gundalow> rabbitmq_policy support version 3.8.0 #64512
14:55:59 <agaffney> 30467: comment added
14:56:21 <gundalow> 64512 asked for reviews on related PR
14:57:05 <gundalow> agaffney: oh, forgot to say earlier, could you please add `pr_day` label on anything you look at. Helps us track
14:58:21 <agaffney> noted
14:58:52 <gundalow> :)
15:00:35 <gundalow> jillr: Do you think https://github.com/ansible/ansible/pull/64283 could be merged (Just looking at random PRs)
15:04:11 <jtanner> it's early for her
15:04:20 <jtanner> phx time
15:04:21 <gundalow> nod, for later
15:06:20 <gundalow> Update RDS snapshot info to use DBClusterIdentifier #64253
15:09:10 <gundalow> non-contributor has confirmed the fix
15:10:46 <gundalow> pinged in #ansible-aws
15:11:01 <gundalow> Idempotency for yarn #64061
15:14:06 <gundalow> mysql_user: make sure current_pass_hash is a string before using it in comparison #64059
15:16:54 <gundalow> (half in another meeting, so going a little slower)
15:17:52 <gundalow> 64059 looks good, just needs changelog so it can be backported
15:22:59 <gundalow> Adjust conditional for disabling alert email #63951
15:24:01 <gundalow> 63951 has shipit from maintainer, will merge
15:32:51 <gundalow> Updated rhn_register module to use the provided transport in server_url #63587
15:32:59 <gundalow> Looks sensible
15:33:07 <resmo> hi
15:33:31 <gundalow> hey resmo, currently doing PR review day
15:33:49 <resmo> gundalow: +1
15:38:26 <gundalow> oh, one for resmo (as you previously commented on this) https://github.com/ansible/ansible/pull/63573/files did you do full review, or just spot typo?
15:40:22 <gundalow> postgresql_privs module now allows SCHEMAS for valid values of objs pr… #63275
15:41:39 <gundalow> hum, first time contributor, so might not know how to add how to do that. I'll ask maintainer if they are happy to take as is
15:45:36 <gundalow> Add timeout to jenkins_script CSFR requests #62850
15:45:40 <gundalow> shipit from maintainer
15:50:38 <resmo> gundalow: just a typo for now, but I would give it a -1 (even if implemented as an option)
15:51:22 <gundalow> resmo: ah, fair enough, could you be able to add that & your reasoning to that
15:51:30 <gundalow> on that PR
15:52:15 <resmo> gundalow: ack.
15:57:16 <gundalow> Update redhat_subscription.py #62846
15:57:16 <gundalow> I *think* this is right
15:59:04 <gundalow> Update mongodb.py #62802
16:12:51 <gundalow> passwordstore: Improve userpass splitting #62590
16:16:37 * gundalow is going to take a break for a bit
16:17:02 <gundalow> at the top of https://github.com/ansible/ansible/issues?page=3&q=is%3Aopen+label%3Asmall_patch+-label%3Abackport+-label%3Aneeds_revision&utf8=%E2%9C%93 if anyone is still following at home
17:30:21 <acozine> o/
17:39:06 <acozine> I'm going to grab some food, then will look at the docs-related PRs from that list
17:39:08 <acozine> https://github.com/ansible/ansible/pull/60781/files
17:39:35 <gundalow> #chair acozine
17:39:35 <zodbot> Current chairs: acozine agaffney gundalow xuxiaowei0512_
17:39:50 <acozine> is the first one . . . if folks have suggestions for improving the comment per sivel's request, post them here
17:40:07 <gundalow> #topic Docs PRs
17:40:09 <gundalow> acozine: all yours :)
17:40:29 <acozine> gundalow: I need some food in my system before I can drive for realz
17:40:51 <acozine> be back in 20-30 minutes
17:40:58 <gundalow> cool
17:40:58 <acozine> and I'll be looking at https://github.com/ansible/ansible/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3Asmall_patch+-label%3Abackport+-label%3Aneeds_revision+label%3Adocs
18:19:30 <acozine> okay . . . so of the 6 PRs in the list ^^^
18:19:34 <acozine> 1 is still WIP
18:19:43 <acozine> the next one is 60781
18:41:04 <acozine> okay, I've pushed an update to that PR - take a look, folks
18:41:05 <acozine> https://github.com/ansible/ansible/pull/60781/files
18:44:45 <acozine> looking at https://github.com/ansible/ansible/pull/62314/files next
18:45:57 <acozine> what do folks use for testing environments?
18:46:10 <acozine> venv? docker?
18:46:52 <agaffney> ansible-test --docker ...
18:49:19 <acozine> agaffney: cool, so for "You must first setup your testing environment" you'd do `$ pip3 install --docker --requirements`?
18:49:35 <acozine> that looks wrong to me . . . but I'm not sure what it should be instead
18:49:53 <acozine> this would be for https://github.com/ansible/ansible/pull/62314/files#diff-603488f66953449a982f74ed952014e6R269
18:49:58 <agaffney> no, I mean use the --docker flag with ansible-test so that I don't need to setup any local environment
18:51:15 <acozine> oh . . . so if you run ansible-test with `--docker` you don't need to set up your testing environment
18:51:31 <agaffney> I'm not sure what's meant by that comment in that PR
18:51:38 <acozine> me neither
18:52:22 <agaffney> it depends on what's meant by "testing environment". I don't use a particular venv or anything to run ansible out of the git repo. I just 'source hacking/env-setup' and run ansible, and I use the --docker flag with ansible-test to run unit/integration tests locally
18:53:26 <acozine> does running `ansible-test` in docker ensure that all dependencies are installed?
18:53:33 <acozine> for the tests you need to run?
18:53:59 <gundalow> ansible-test will *start* a docker instance and install dependencies when you call `ansible-test WHAT --docker`
18:55:43 <felixfontein> hey! sorry, I'm a bit late and a lot afk right now...
18:55:45 <felixfontein> something got in the way
18:58:13 <gundalow> felixfontein: It's all good :)
18:59:41 <acozine> okay, so is this close?
19:00:08 <acozine> https://www.irccloud.com/pastebin/MueicMmg/draft%20replacement%20for%20Unit%20Tests%20section%2C%20see%20PR%2062314
19:01:05 <acozine> or does the `ansible-test --docker`` command take a path to particular tests instead of just the module name?
19:01:12 <felixfontein> acozine: it's better to write `--docker default` because otherwise the next argument will be interpreted as the name of a container
19:01:25 <acozine> felixfontein: excellent, thanks
19:01:27 <felixfontein> i.e. `ansible-test --docker default <...everything else...>`
19:01:57 <acozine> so what all goes in `< . . . everything_else . . . >`?
19:02:21 <acozine> is it the name of the module? a path to a directory full of tests? something else entirely?
19:02:35 <gundalow> yes yes yes
19:02:57 <felixfontein> I always have to check that... I think it's the module's name, but I'm never sure
19:03:11 <felixfontein> I usually use CI for that ;)
19:03:12 <acozine> heh, then we definitely need to document it better ;-)
19:03:22 <felixfontein> yep
19:03:34 <gundalow> ansible-test integration yum
19:03:34 <gundalow> ansible-test integration lib/ansible/modules
19:03:34 <gundalow> ansible-test units lib/ansible/modules
19:03:34 <gundalow> ansible-test units test/units/playbook/
19:04:49 <acozine> ah, great . . . so since this is the section about unit tests within the page about developing a module, we probably want `ansible-test --docker default units path/to/MY_MODULE`
19:04:51 <gundalow> are all valid
19:05:06 <gundalow> or just `ansible-test --docker default units MY_MODULE`
19:05:11 <felixfontein> there's also `units` missing in the command line
19:05:18 <gundalow> oh, yes, good spot felixfontein
19:06:12 <gundalow> #chair felixfontein
19:06:12 <zodbot> Current chairs: acozine agaffney felixfontein gundalow xuxiaowei0512_
19:06:21 <gundalow> I need to drop off for 30 minutes or so
19:06:38 * acozine waves
19:09:13 <acozine> so what does the `--requirements` flag do? is it necessary with `--docker`? with `--venv`? only when running outside of a container or virtual machine?
19:09:15 <felixfontein> `bin/ansible-test units --docker default luks_device` works
19:09:30 <acozine> excellent!
19:09:32 <felixfontein> (I should try a module which actually has unit tests the next time when trying to figure the command out... that will help a lot :D )
19:09:38 <acozine> heh
19:10:01 <felixfontein> I always got stuff like `ERROR: Target pattern not matched: <module-name>`
19:10:07 <felixfontein> that was because there was no unit tests for that module...
19:10:36 <acozine> I guess that's better than a bogus "all tests are passing" message if there are no tests to run
19:11:20 <felixfontein> definitely!
19:11:43 <acozine> okay . . . so now I have this for the "Unit Tests" section:
19:12:25 <acozine> https://www.irccloud.com/pastebin/0UbWC4dd/Second%20revised%20edition%20for%20PR%2062314
19:12:49 <acozine> is the `venv` example correct and complete?
19:13:04 <acozine> or does it have extra args too?
19:13:33 <acozine> also, just to confirm, both of those commands will install any requirements in the container before running, right?
19:14:01 <felixfontein> `--docker default` needs to be after `units`
19:14:17 <felixfontein> (and probably same for --venv)
19:14:29 <felixfontein> must things should already be installed, but it should install everything that's missing
19:15:16 <acozine> hmmm, looking at the example in the sanity tests section further up the page, it includes `--python 2.7`
19:15:50 <acozine> do the unit test examples need to specify the Python version? Is 3.5 the default for `ansible-test`?
19:15:51 <felixfontein> that restricts the amount of tests run (to the python 2.7 tests)
19:15:57 <acozine> ohhh, I see
19:16:03 <felixfontein> if you don't specify --python, it will just run tests with all Python versoins
19:16:18 <acozine> that's not about "which python do I use to run the utility", it's about "which tests should I run"
19:16:20 <acozine> gotcha
19:16:21 <acozine> thanks!
19:16:27 <felixfontein> it is definitely faster if you specify a specific version though
19:17:53 <felixfontein> I can't get `ansible-test units` to work for non-modules
19:22:18 <acozine> felixfontein: that makes sense to me
19:22:23 <acozine> what other unit tests would we have?
19:22:39 <acozine> do we have unit tests for module utils?
19:22:50 <felixfontein> module_utils, plugins, ...
19:24:51 <acozine> huh, so we have those but they won't run with `ansible-test units`
19:24:55 <acozine> that's weird
19:25:45 <felixfontein> they run somehow, I just always forget how....
19:28:06 <acozine> updated with what i know so far: https://github.com/ansible/ansible/pull/62314/files
19:28:28 <acozine> reviews, comments, additions, corrections welcome!
19:28:56 <felixfontein> I'll take a close look, but that'll have to wait a bit, I'm currently busy with other stuff, sorry...
19:36:40 <acozine> np
19:37:11 <acozine> heh, oops, the link I used for the ansible-test docs is wrong
19:37:39 * acozine scolds self for letting a PR about testing fail our test suite
19:39:27 <acozine> huh, we don't actually have a page for the `ansible-test` utility
19:42:37 * acozine removes that bit
19:44:33 <acozine> hm, and when I try these commands, I get errors
19:48:16 <acozine> I pushed one fix and will wait for comments on the updated PR for 62314
19:48:22 <acozine> next on the list is https://github.com/ansible/ansible/pull/62802
19:48:34 <acozine> who has mongodb experience?
19:49:52 <jtanner> i do, but not with that lookup
19:50:17 <jtanner> does the lookup return a single string? ... if so, that new example would make sense
19:50:45 <jtanner> or with_mongodb: "{{ [ mongodb_parameters ] }}"
19:50:54 <acozine> I don't know . . . it looks like the old example just passed the parameters directly in the `vars` section
19:50:59 <acozine> so I'm not sure why it was failing
19:51:28 <acozine> oh, does it need the square brackets?
19:51:38 <jtanner> well, that may not work either
19:51:46 <jtanner> someone probably just needs to try it out
19:52:06 <acozine> yep . . . someone who has an actual mongodb, preferably
19:58:06 <acozine> #63725 is addressing a similar concern to #62314, with the same blocker - the old instructions for running tests shows installing requirements from a path that no longer exists, but instead of just updating the path, we should update the instructions to use venv or docker, as that is the current preferred method
19:59:53 <acozine> last Big PR Day docs PR is https://github.com/ansible/ansible/pull/64041/files
20:01:49 <acozine> this change would convert any dashes that exist in an IPv4 IP address with underscores
20:03:10 <felixfontein> it's not dashes in IPv4 addresses (there are none), but in interface names
20:03:11 <acozine> so that an IP address like `tap2f186e31-b8` would work
20:03:21 <acozine> felixfontein: ah, right
20:03:50 <acozine> that isn't an IP address, but the interface name associated with an IP address
20:04:52 <felixfontein> I don't know how common interface names with dashes are
20:05:33 <acozine> I don't know either, but this change would let interface names without dashes work as before, right?
20:05:43 <felixfontein> yes
20:05:51 <acozine> so . . . mostly harmless?
20:06:14 <felixfontein> I wonder whether it's better to add a note that for interface names with dashes this should be used (i.e. keep the old example and add this somewhere below), or whether this should be the default version
20:06:22 <felixfontein> yes, except that it looks more complicated now ;)
20:06:49 <acozine> hm, yeah
20:06:59 <acozine> it's part of the FAQ entry `How do I access a variable name programmatically?`
20:08:36 <acozine> which is a more generic question than the answer we've given there
20:09:10 <acozine> however, I think the more detailed discussion belongs in the User Guide
20:09:31 <acozine> I've been thinking about adding a section about "working with data"
20:09:52 <felixfontein> that would be good.
20:10:13 <acozine> we could do better at explaining how to use return values generally
20:11:42 <felixfontein> in 64041 I'd probably prefer to keep the old example, but add the extended example as a note
20:11:52 <acozine> sounds reasonable
20:12:50 <felixfontein> I've added a comment
20:18:25 <acozine> felixfontein: https://github.com/ansible/ansible/pull/64041/files - does that do what you had in mind?
20:19:19 <felixfontein> acozine: I'm wondering whether it's better to move that after the next paragraph, because that's a big part of the answer
20:19:35 <felixfontein> when you're at it: `'inventory_hostname'` two lines below should have double backticks and not single quotes
20:19:53 <acozine> heh, the floodgates open . . .
20:20:01 <felixfontein> :)
20:21:59 <acozine> final offer ;-): https://github.com/ansible/ansible/pull/64041/files
20:23:06 <felixfontein> added a suggestion
20:23:14 <felixfontein> WDYT?
20:23:58 <acozine> looks good, but I can't commit to this other person's repo
20:24:04 <acozine> so I'll recreate it on my branch and push
20:24:27 <felixfontein> but how did you added commits then?
20:25:23 <acozine> pulled the branch, added my commits on top, pushed back
20:25:32 <felixfontein> ah ok
20:25:36 <acozine> I have no idea why the one approach works and the other one doesn't
20:26:48 <acozine> heh, now Shippable is refusing to run - I can't blame it, I've kicked it off 3-4 times in 5 minutes
20:26:51 <felixfontein> no idea...
20:27:01 <felixfontein> hmm
20:27:08 <felixfontein> sometimes it takes a minute
20:27:30 <acozine> there it goes
20:27:31 <acozine> phew
20:27:41 <acozine> and we're at time for the Big PR Review Day
20:28:05 <acozine> thanks felixfontein agaffney gundalow and everyone else who participated earlier!
20:29:59 <felixfontein> thanks gundalow and acozine and everyone else for doing this!
20:30:10 <felixfontein> (and sorry again for really not being around...)
20:30:22 <acozine> hey, you were definitely around felix!
20:30:34 <felixfontein> I promised to be around earlier ;)
20:30:36 <felixfontein> and more actively
20:30:46 <gundalow> #info pr_day 36/135 (open/close)
20:30:49 <acozine> well, we're going to do this pretty often now, I think
20:30:55 <acozine> so you'll get more chances!
20:31:14 <felixfontein> ah, that's cool!
20:31:15 <gundalow> I previously said I'd run this every two weeks
20:31:26 <felixfontein> great!
20:31:27 <gundalow> That hasn't happened
20:31:35 <felixfontein> not yet :D
20:31:39 <gundalow> #info Thank you everybody
20:31:43 <gundalow> #endmeeting