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