09:01:34 <gundalow> #startmeeting Ansible Community PR Review 09:01:34 <zodbot> Meeting started Wed Dec 19 09:01:34 2018 UTC. 09:01:34 <zodbot> This meeting is logged and archived in a public location. 09:01:34 <zodbot> The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot. 09:01:34 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic. 09:01:34 <zodbot> The meeting name has been set to 'ansible_community_pr_review' 09:01:40 <gundalow> Who's here for PR review? 09:02:05 * akasurde waves 09:02:38 <bob_cheesey> i'm here, currently doing work stuff but i'll follow along and join in when i'm available 09:03:02 <akasurde> #chair bob_cheesey 09:03:14 <robertgwilliam> im here - first timer, not sure how useful... 09:03:23 <akasurde> robertgwilliam, welcome 09:03:29 <akasurde> #chair robertgwilliam 09:03:42 <robertgwilliam> thanks 09:04:06 <gundalow> #chair akasurde bob_cheesey robertgwilliam xenlo 09:04:06 <zodbot> Current chairs: akasurde bob_cheesey gundalow robertgwilliam xenlo 09:05:09 <gundalow> bartmon: Hi, you here for the PR review? 09:05:11 <akasurde> gundalow, Can paste GitHub PR list link which are going to refer for this session ? 09:05:15 <gundalow> Sure 09:06:20 <gundalow> #info For the general community PR review days we will be ignoring anything that has a Working Group (https://docs.ansible.com/ansible/devel/community/communication.html#working-groups) as through 2019 we will have dedicated Working Group review sessions 09:06:21 <bartmon> gundalow, just curious, really. not planning on doing reviews since i'm at work. 09:06:53 <gundalow> bartmon: Sure. People are always welcome to idle. Please do shout out if you want any more info on anything 09:06:55 <gundalow> #chair bartmon 09:06:55 <zodbot> Current chairs: akasurde bartmon bob_cheesey gundalow robertgwilliam xenlo 09:07:16 <akasurde> gundalow++ 09:07:16 <zodbot> akasurde: Karma for gundalow changed to 1 (for the current release cycle): https://badges.fedoraproject.org/tags/cookie/any 09:08:20 <gundalow> #info This is all very informal. Please do ask questions, if you are confused by something, chances are others are. We will go through PR by PR and we will use the MeetingBot to track what needs merging. This allows other people to add comments through the day 09:08:34 <gundalow> fridim: Hi, you here for the PR review session? 09:08:45 <fridim> yes :) 09:09:02 <gundalow> #info Thank you all that are here, for our second big PR review days. Today we will be focusing on `bugfix` PRs. 09:09:13 <gundalow> fridim: Welcome :) 09:09:15 <gundalow> #chair fridim 09:09:15 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow robertgwilliam xenlo 09:09:18 <winem_> morning :) 09:09:25 <gundalow> #chair winem_ 09:09:25 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow robertgwilliam winem_ xenlo 09:09:53 <gundalow> #topic pr:bug `is:pr is:open -label:backport -label:support:core -label:support:curated -label:networking -label:new_module -label:aws -label:azure -label:docker -label:linode -label:vmware -label:windows label:bug -label:aws -label:azure -label:docker -label:linode -label:networking -label:vmware -label:windows` 09:10:08 <gundalow> #info Starting with https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3Abackport++-label%3Asupport%3Acore+-label%3Asupport%3Acurated+-label%3Anetworking+-label%3Anew_module+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Avmware++-label%3Awindows++label%3Abug+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Anetworking+-label%3Avmware+-label%3Awindows 09:10:09 * jhawkesworth_ is lurking 09:10:24 <gundalow> influxdb_user: Fixed unhandled exception on invalid login credentials #50134 09:10:27 <gundalow> #chair jhawkesworth_ 09:10:27 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow jhawkesworth_ robertgwilliam winem_ xenlo 09:10:34 <gundalow> jhawkesworth_: Morning :) 09:11:02 <jhawkesworth_> hey. will try to tune in a bit but $dayjob is priority 09:11:07 <akasurde> jhawkesworth_, morning 09:11:09 <gundalow> jhawkesworth_: Sure :) 09:11:35 <gundalow> Would people like me to paste in the URL, or is `influxdb_user: Fixed unhandled exception on invalid login credentials #50134` OK (number at the end is the PR 09:11:40 <gundalow> https://github.com/ansible/ansible/pull/50134 09:12:25 <gundalow> So we can see from the PR description that 50134 fixes 50131, reported and fixed by the same person 09:12:31 <winem_> the headline and PR # is fine for me 09:12:38 <bob_cheesey> same 09:12:43 <gundalow> Thanks, that does make it a bit easier for me, thanks 09:13:34 <winem_> oh, didn't want that. let's use urls ;) 09:14:01 <gundalow> 50134: Thoughts? 09:14:06 <fridim> So it just adds "influx.exceptions.InfluxDBClientError" to the catch ? 09:14:45 <gundalow> fridim: Yup, and if you look at the original bug report, last line of the code block in https://github.com/ansible/ansible/issues/50131 you can see `raise InfluxDBClientError(response.content, response.status_code)` 09:15:42 <gundalow> This is a good example of a module not having being tested with invalid input (negative testing) 09:15:59 <winem_> Let me take a look at the tests for that module 09:16:46 <gundalow> #info Use GitHub to provide feedback so it's tracked. IRC is for general discussion 09:18:03 <gundalow> Update `Unknown error` to specific error message #50129 09:18:21 <fridim> #50134 What if another exception than the 2 listed is thrown ? Shouldn't we have a general 'except' ? 09:18:46 <gundalow> fridim: Good question. We generally avoid general exceptions 09:18:49 <fridim> ok 09:18:50 * gundalow looks to find the docs 09:19:39 <bob_cheesey> it would be nice to have a test for it, but overall that seems a reasonable fix - it'll be obvious to the user what's happened 09:19:53 <akasurde> bob_cheesey, I agree 09:20:20 <gundalow> fridim: https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_best_practices.html#handling-exceptions-bugs-gracefully 09:20:28 <gundalow> > Avoid catchall exceptions, they are not very useful unless the underlying API gives very good error messages pertaining the attempted action. 09:20:56 <gundalow> #action merge and backport 50134 09:21:05 <gundalow> (I'll do the merge and backports tomorrow) 09:21:13 <gundalow> ----- 09:21:24 <gundalow> Update `Unknown error` to specific error message #50129 09:21:46 <gundalow> 50129 fixes 49713, issue and PR by same person 09:22:15 <shaps> Morning all 09:22:35 <gundalow> #chair shaps 09:22:35 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow jhawkesworth_ robertgwilliam shaps winem_ xenlo 09:22:38 <gundalow> Morning :) 09:23:44 <navalkp> Morning all, joining for first time 09:23:49 <gundalow> #chair navalkp 09:23:49 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow jhawkesworth_ navalkp robertgwilliam shaps winem_ xenlo 09:23:52 <shaps> hi navalkp 09:23:53 <gundalow> navalkp: Welcome :) 09:24:43 <gundalow> 50129: Looks like we have active maintainers (see 49713), given it's only been a day I'll leave that to the maintainers. Though if people have comments please add them to the PR 09:24:53 <gundalow> davegarath: Hi, you here for the PR review? 09:25:01 <fridim> #50129 is to_text(e) mandatory here ? Would str(e) work ? 09:25:17 <gundalow> Good question, akasurde ? 09:25:44 <akasurde> fridim, generally we prefer to_text or to_native 09:25:55 <shaps> to_text is supposed to be py2/3 compatible 09:25:58 <akasurde> as it handles unicode related text 09:25:59 <davegarath> Hi gundalow, I'm here just for lurk :P I haven't so much time to be present but I'll try to follow something 09:26:20 <gundalow> davegarath: Cool. Feel free to shout out at anypoint 09:27:12 <davegarath> gundalow, did you already shared link with the list of PR that are object of review ? 09:27:36 <gundalow> davegarath: https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3Abackport++-label%3Asupport%3Acore+-label%3Asupport%3Acurated+-label%3Anetworking+-label%3Anew_module+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Avmware++-label%3Awindows++label%3Abug+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Anetworking+-label%3Avmware+-label%3Awindows 09:27:56 <davegarath> ty 09:28:56 <gundalow> 50129 uses `to_text` 09:29:10 <gundalow> So that a +1? 09:29:27 <shaps> I can never remember if it is to_text or to_native to be used, they have the same end result 09:29:36 <shaps> to_text should be fine in anycase, so +1 from me 09:31:20 <gundalow> Cool, could you add that on the PR 09:31:44 <gundalow> perhaps something like `Not used redfish_facts, though to_text is the right thing` 09:32:38 <shaps> yes sorry, still bit slow 09:32:44 <gundalow> #info Giving details of what you've tested, or what type of review comments you've given allow an informed decision to be made 09:33:02 <gundalow> openssh_keypair: prevent error when path includes no path #50106 09:34:21 <gundalow> 50106 is a good example of a one-lint (7 char) fix that may cause issues 09:35:04 <gundalow> For example, does this change backwards compatibility? 09:36:25 <gundalow> Wonder if it would be worth updating the docs for `path` to make this clearer. 09:36:35 * gundalow adds that as a comment 09:36:46 <akasurde> same here 09:36:48 <shaps> it does change the behavior a bit, although I don't think it's a bad thing 09:36:52 <winem_> I also miss an update on the documentation for the path attribute 09:36:58 <shaps> +1 to the docs update 09:37:11 <winem_> oh, gundalow said that already, sry :) 09:37:42 <gundalow> winem_: It's cool, means we are thinking similar things :) 09:38:10 <gundalow> VMware: Don't think all files are valid cause libs present #50103 09:38:28 <navalkp> path is required isn't it sufficient 09:38:49 <gundalow> #action gundalow 50103 update BOTMETA for inventory 09:39:06 <gundalow> navalkp: how so? 09:39:27 <akasurde> +1 for 50103, Ok if someone take another look 09:39:28 <shaps> navalkp, with the change path is actually not required anymore 09:39:53 <navalkp> got it 09:40:05 <akasurde> gundalow, I will add backport for 50103 once merged 09:41:02 <shaps> +1 50103 09:41:33 <gundalow> akasurde: Thanks. We will do the merging tomorrow. Last review day I merged stuff then people smarter than me pointed out things that need fixing 09:41:59 <gundalow> #action merge 50103, request backport 09:42:16 <gundalow> os_security_group_rule proper module exit when deleting rule #50076 09:42:54 <gundalow> 50076 fixes 50057 (reported by different user) 09:44:52 <gundalow> 50076 added a comment asking for a changelog 09:45:03 <gundalow> #action 50076 merge once it's got a changelog 09:45:41 <gundalow> Update os_client_config to use openstacksdk #50046 09:46:10 <gundalow> 50046: Thoughts? 09:48:18 <bob_cheesey> just having a nosey at 50046 09:48:32 <bob_cheesey> spotted a couple of things - i'll comment on them 09:48:38 <gundalow> Thanks :) 09:50:54 <gundalow> The change of Python lib makes me wonder if this isn't suitable for backport 09:52:30 <bob_cheesey> it's fairly breaking behaviour to introduce outside of a major release (when it can be mentioned in the porting guide) 09:52:58 <bob_cheesey> from memory, the two are independent and so you may not have the newer lib 09:53:14 <bob_cheesey> (i could be wrong in that though, it's been a while) 09:54:18 <gundalow> nod 09:54:29 * gundalow doesn't know all the openstack libs 09:55:18 <gundalow> #info Being mindful of user experience (backwards compatibility) is an import thing in reviews 09:55:23 <gundalow> Shall we move to the next PR? 09:55:24 <bob_cheesey> Debian appears to package os-client-config as depending on python-openstacksdk, but not sure about other distros 09:56:19 <bob_cheesey> but Ubuntu doesn't - i think that's possibly messy depending on where you install the package from (pip, repos etc) 09:56:44 <gundalow> oh, that's good info. Could you add it as a comment, perhaps as a reply to mine? 09:56:55 <bob_cheesey> yep sure 09:58:56 <gundalow> Put in documented default for gcp_compute filters #50025 10:00:35 <gundalow> Needs changelog 10:03:24 <gundalow> #action 50025 merge + backport once changelog added 10:04:19 <gundalow> shell & multiline YAML free form: add integration test #49993 10:04:29 <gundalow> Something different, an integration test 10:07:10 <gundalow> So, what do people think this is doing/ 10:09:41 <gundalow> Integration tests generally playbooks that do something then use the `assert` module to check certain conditions have been met 10:10:04 <gundalow> Filter DNSimple request by record name. #49981 10:12:44 <gundalow> 49981 What they are doing looks sensible though I've never used that module 10:14:35 <gundalow> Do not load user kube config if path specified #49952 10:14:52 <gundalow> 49952 has +1 from maintainers 10:15:02 <gundalow> #action merge 49952, ask for backport inc changelog 10:15:20 <gundalow> Fix netbox url with string concat #49943 10:16:17 <gundalow> So some concerns that `self.api_endpoint + "/api/dcim/platforms/?limit=0"` might cause `//` 10:19:03 <bob_cheesey> just taking a look at the integration test (49993) 10:19:12 <winem_> just verified that netbox will fail if // are used - so that's a valid concern (imho) 10:19:41 <gundalow> winem_: oh, nice test, would be worth adding that as a comment 10:23:51 <gundalow> #action gundalow inventory scripts (and plugins) are not getting labels, or pinging authors 10:24:25 <gundalow> Added organization in the scm_credential get #49884 10:24:28 <gundalow> oh, one from Shaps 10:25:04 <winem_> done 10:26:42 <gundalow> Thanks :) 10:27:37 <gundalow> shaps: Could you please add a changelog fragment to your 49884 10:28:42 <gundalow> #action 49884 merge once it's got a changelog (and possibly backport) 10:29:12 <gundalow> Fix broken urpmi module #49881 10:31:22 <gundalow> the `label:owner_pr` tells us that the PR has been raised by the maintainer 10:33:16 <gundalow> icinga2_host: A lot of fixes after extensive use in production #49789 10:35:10 <gundalow> hum, this PR isn't that readable due to the DOCUMENTATION block being reordered 10:37:09 <akasurde> gundalow, I will comment to add testcase 10:37:16 <gundalow> :) 10:37:25 <winem_> I have a question to the icinga2 PR 10:37:27 <akasurde> gundalow, I already did - https://github.com/ansible/ansible/pull/49789#issuecomment-446468146 10:37:38 <gundalow> winem_: sure 10:37:44 <winem_> what about the commit messages? I was checking them because I hoped to get an idea about the changes from the messages 10:38:25 <gundalow> https://xkcd.com/1296/ 10:38:33 <winem_> but we have 4 add and delete service for example and a message like "lot of fixes". how much do we / does the ansible core team care about it? 10:38:54 <winem_> hahaha, gundalow 10:39:12 <gundalow> We squash+merge 10:39:52 <akasurde> gundalow, 49789, will wait till author adds testcases 10:39:57 <winem_> ok :) 10:41:09 <gundalow> akasurde: Thanks, could you please add a negative review comment 10:41:32 <akasurde> ok 10:41:39 <gundalow> Katello: Added product to the dict choices #49776 10:42:53 <akasurde> Katello moved to foreman repo / it is deprecated from ansible repo 10:43:22 <gundalow> nod, closed 10:43:24 <akasurde> #info https://github.com/theforeman/foreman-ansible-modules 10:43:50 <gundalow> List correctly current PV in "lvg" module: fix lvg reduce #49731 10:45:33 * gundalow has to step out for a bit and will leave you in akasurde's care :) 10:45:57 <akasurde> gundalow, thanks 10:47:24 <akasurde> not sure about LVG functionality, anyone want to take a stab ? 10:51:56 <akasurde> any views on 49731 ? 10:52:14 <bob_cheesey> i've used LVM a bit - will need to get my head around the module as a whole though first 10:53:14 <akasurde> ok then we can move on to next then 10:53:28 <akasurde> #action 49731 need to look code as whole and then merge 10:53:47 <akasurde> Handle 'latest' version when installing plugin for first time #49723 10:57:12 <akasurde> Code LGTM, added request for unit test 10:58:02 <akasurde> #action 49723 requested for unit test, code LGTM 10:59:27 <akasurde> Update os_image.py 10:59:34 <akasurde> #49705 11:04:18 <shaps> gundalow, sorry had to go out for a bit, will add changelog and backport 11:04:43 <akasurde> shaps, gundalow is away 11:04:46 <akasurde> shaps, ok 11:08:26 <akasurde> #action 49705 added a note for updating documentation fragement 11:08:43 <akasurde> Added npm ci command 49664 11:13:25 <akasurde> any opinion about 49664 11:15:00 <akasurde> Read above as 49665 instead of 49664 11:18:30 <michaelkaye> hi - i've not got involved before, but thought i'd put my nose in. I worry a little about that because it seems that you'd turn "ci" on for "use package-lock" but unexpectedly I think state="absent" ci="true" would stop it uninstalling 11:19:34 <akasurde> michaelkaye, Thanks for stepping-in, could you please add your comment on PR itself so that author can address it 11:22:05 <akasurde> #action 49665 added comments for PR author to address 11:22:47 <akasurde> next one - Provide both service state and status when possible in service_facts #49618 11:25:54 <akasurde> 49618 looks good to me 11:27:20 <akasurde> any views for 49618 ? 11:27:54 * gundalow returns, thanks for keeping it moving akasurde :) 11:28:46 <akasurde> gundalow, no problem 11:29:15 <gundalow> michaelkaye: All are welcome, thanks for joining in. If during today's session you you just spot one things and add a comment that's a success in my books. Could you please add that as a comment on the PR? 11:30:11 <gundalow> gcp_compute: Add vars_prefix to prefix host_vars #49601 11:32:03 <gundalow> Hum, another inventory PR where the bot doesn't know about labels or to ping maintainers 11:33:39 <akasurde> gundalow, I think this will break old things as now 'gcp_' is added as prefix 11:35:23 <akasurde> gundalow, added my comment on PR 11:38:45 <gundalow> terbolous: rbarlik Hello, are you here for the Community PR review day? 11:39:21 <rbarlik> @gundalow yes, indeed :) 11:39:36 <gundalow> rbarlik: Excellent, welcome. How did you hear about this? 11:39:49 <gundalow> We are goin through https://github.com/ansible/ansible/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3Abackport++-label%3Asupport%3Acore+-label%3Asupport%3Acurated+-label%3Anetworking+-label%3Anew_module+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Avmware++-label%3Awindows++label%3Abug+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Anetworking+-label%3Avmware+-label%3Awindows 11:40:09 <gundalow> oh, just on Page 2 now, looking at: 11:40:23 <gundalow> IBM_Storage: fix host_add_ports #49420 11:41:10 <terbolous> I'm just trying to get a glimpse of how this works :-) 11:41:19 <gundalow> akasurde: I see some comments have been added to 49420 11:41:29 <terbolous> I'll see if any of the PRs is something I'm able to review 11:41:31 <gundalow> #chair rbarlik terbolous 11:41:31 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow jhawkesworth_ navalkp rbarlik robertgwilliam shaps terbolous winem_ xenlo 11:42:28 <gundalow> terbolous: Welcome. We are working through the above GitHub query. It's all very informal so feel free to ask questions at anypoint. Part of today is about reviewing PRs, part of it is about educating *how* to review 11:43:26 <akasurde> gundalow, fix seems to be very odd to me 11:45:07 <rbarlik> gundalow: do you test the code or is it just a visual review? 11:45:44 <gundalow> rbarlik: a mix 11:46:36 <gundalow> Someone may find a PR they are interested and test it in the background, in this Channel we may have go forward 10 PRs. Then someone says "PRx does/doesn't work due to X, I've added comments on the PR" 11:47:14 <rbarlik> okay, thanks 11:47:52 <akasurde> rbarlik, general documentation may not require testing as well 11:49:16 <gundalow> rbarlik: https://github.com/ansible/community/issues/407 have a look at `Before you start` for a list of hopefully helpful guides on how to review 11:49:47 <gundalow> Feedback on any of those docs is welcome, especially from people that are newer to this as no doubt we've missed things out, or could improve them 11:50:07 <gundalow> akasurde: you OK to add some comments on 49420 11:50:07 <rbarlik> gundalow: just reading it :) 11:50:44 <gundalow> :) 11:50:52 <gundalow> ovirt add sync_networks #49298 11:51:36 <gundalow> 49298 comments already added, doesn't look close to merge yet 11:51:51 <gundalow> add conditional checks for mysql8 compatibility #49251 11:52:07 <bob_cheesey> just to return to #49731 (the LVG module) - I'm pretty confident the filter list building is now correct - i've tested it on multiple systems and it always returns all the devices now (whereas it may not before) 11:52:16 <bob_cheesey> sorry, got sidetracked by work 11:53:09 <gundalow> bob_cheesey: No need to apologize for that. Thanks for adding comments to the PR 11:55:53 <gundalow> 49251 (MySQL) lots changed here 11:56:31 <gundalow> Needs someone that uses MySQL8 to test this 11:58:02 <shaps> gundalow, i have added the changelog, I'll backport once merged 11:58:28 <shaps> sorry, that was re- #49884 11:59:55 <gundalow> 49251 I've pinged some other people that have worked on that module 12:00:02 <akasurde> bob_cheesey, could you add your findings on 49731 12:00:02 <gundalow> #info MySQL needs some maintianers 12:01:18 <gundalow> shaps: I've merged 49884 thanks 12:01:41 <gundalow> pam_limits.py: Fixes #49130 Allow negative number on value parameter #49137 12:01:43 <shaps> great, thanks 12:03:26 <gundalow> 49130 looks like we are waiting on apenney re-review 12:04:09 <gundalow> mongodb_user: Add new API for user management #49074 12:04:13 <gundalow> Any mongo users here? 12:11:00 <gundalow> [WIP] digital_ocean_tag_facts - Fix KeyError problems #49037 12:11:04 <gundalow> 49037 seems to be in hand 12:11:25 <gundalow> nmcli: fix NetworkManager-glib package obsoleted #48870 12:13:19 <gundalow> Lunch time 12:21:29 <akasurde> 48870 is waiting on contributor I am taking care of that 12:21:58 <bob_cheesey> lunchtime here too 12:45:30 * gundalow waves, who's around? 12:49:50 <navalkp> join ansible-devel 12:53:51 <gundalow> Fix lastpass lookup error #48804 12:54:05 <gundalow> hum, need Toshio to rereview that 13:07:30 <resmo> re 13:08:10 * resmo waves 13:16:02 <gundalow> #chair resmo 13:16:02 <zodbot> Current chairs: akasurde bartmon bob_cheesey fridim gundalow jhawkesworth_ navalkp rbarlik resmo robertgwilliam shaps terbolous winem_ xenlo 13:16:05 <gundalow> afternoon :) 13:16:39 <gundalow> rabbitmq_binding: Fix using empty routing key #48597 13:17:05 <gundalow> shaps: I think you're comments have been addressed in 48597 13:17:18 <gundalow> resmo: going through https://github.com/ansible/ansible/pulls?page=2&q=is%3Apr+is%3Aopen+-label%3Abackport++-label%3Asupport%3Acore+-label%3Asupport%3Acurated+-label%3Anetworking+-label%3Anew_module+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Avmware++-label%3Awindows++label%3Abug+-label%3Aaws+-label%3Aazure+-label%3Adocker+-label%3Alinode+-label%3Anetworking+-label%3Avmware+-label%3Awindows&utf8=%E2%9C%93 13:18:04 <shaps> gundalow, yes looks good, adding comment 13:18:20 <gundalow> #action review and maybe merge 48597 13:20:18 <resmo> I'll give it a +1, even though I would have preferred a check for falsy not for emtpy string. but good enough 13:24:51 <gundalow> #action gundalow remove afternburn https://github.com/ansible/ansible/pull/48558#issuecomment-446642255 13:25:19 <gundalow> Fix Pacman regex for unmatched Arch package name #48558 13:25:26 <gundalow> Anyone here use `Arch Linux`? 13:31:46 * resmo <- nope 13:34:15 * bob_cheesey waves 13:34:41 <gundalow> xfconf: "get": don't set changed flag, don't require value #48331 13:34:49 <bob_cheesey> i'll take a nosey at the Pacman PR 13:34:59 <gundalow> bob_cheesey: Thanks :) 13:35:25 <resmo> gundalow: https://github.com/ansible/ansible/pull/50134 merge? 13:36:34 <gundalow> #action merge 50134 13:36:36 <gundalow> resmo: Thanks 13:37:07 <gundalow> resmo: FYI, I'm not merging anything today as during the previous PR review I was merging then few minutes/hours later people were adding comments 13:37:24 <resmo> gundalow: roger that 13:38:08 <gundalow> 48331 confused why `.lower` is being used rather than bool 13:39:11 <gundalow> oh, are they using a general str that could be anything or a bool 13:40:26 <resmo> value could be anything of int bool, float and string it seems 13:40:33 <gundalow> nod 13:41:44 <resmo> hmm if we don't default the value to be type="str" wouldn't ansible handle this smartly? 13:42:41 <bob_cheesey> gundalow: #48558 lgtm 13:43:20 <resmo> in other words, wouldn't ansible cast it into the "right" type? 13:46:00 <jtanner> i don't think anyone should expect ansible to guess at the type, reliably 13:46:33 <resmo> jtanner: yeah, good point ;) 13:48:16 <jtanner> the argspec in that module imples the arg can be int/bool/float/string and the user must supply the type. if this module was accepted with that design, we need to either tell the author to deprecate the interface and redesign it, or we accept whatever patches they send to fix up the after-the-fact casting mess that they have created 13:55:40 <gundalow> Update openshift version pin to latest working release #48311 13:56:28 <gundalow> needs updating 13:56:57 <gundalow> fix grafana_dashboard KeyError when create new dashboard #47119 #48299 13:59:12 * gundalow adds comments in the hope of getting it unstuck 13:59:41 <gundalow> 48285 waiting on author 14:00:05 <gundalow> Skip non-management Packet IP addresses #48168 14:03:07 <gundalow> Not used packet.net, though change looks sensible 14:05:08 <gundalow> thedoubl3j: hey :) 14:06:04 <gundalow> #action merge 48168 14:08:18 <bob_cheesey> re 48168 - i looked through the api docs but they're a bit bare in places :( 14:08:25 <bob_cheesey> seems a sensible change though! 14:08:40 <gundalow> Update proxmox_kvm.py #47979 14:08:44 <gundalow> I'm confused by this 14:10:54 <bob_cheesey> state: running isn't an a listed choice - that is odd 14:11:26 <gundalow> yup 14:11:40 <bob_cheesey> running is exposed as a status _from_ proxmox, maybe that's where the confusion has arisen? 14:11:41 <gundalow> And it's not in their example playbook in last reply 14:12:16 <bob_cheesey> https://github.com/ansible/ansible/blob/013f69477a37a2b353a4680deaa118fa8ecd3d35/lib/ansible/modules/cloud/misc/proxmox_kvm.py#L575 14:12:38 <bob_cheesey> yeah that makes no sense at all really 14:13:57 * gundalow composes a reply 14:14:22 <gundalow> module_utils/mysql: Fixing unexpected keyword argument 'cursorclass' error after migratio… #47809 14:20:19 <gundalow> Comment added on https://github.com/ansible/ansible/pull/47979#issuecomment-448611780 I hope that's clear 14:21:40 <bob_cheesey> yeah i think that spells it out nicely 14:22:20 <gundalow> Thanks :) 14:25:02 <gundalow> pam_limits: properly support nice and priority limits #47680 14:29:37 <felixfontein> about mysql: I somehow vaguely remember that there was a difference between pymysql and the other (forgot its name) by which way to connect was used by default. I think the old connector tried to connect via unix sockets by default, while the new one tries to connect via tcp/ip by default. I don't remember in which issue/PR that discussion went (or was it IRC?), but maybe that's related to 14:29:43 <felixfontein> the problem 14:30:41 <felixfontein> if the default user (root) is configured that connection via socket is ok, but via tcp/ip not, then the command line mysql tool will connect, the old connector will connect, but pymysql not (IIRC) 14:31:03 <bcoca> felixfontein: the other 'big' difference is one uses libmysql (C lib) and the other is pure python implementation (sloooooooooooooow) 14:32:34 <gundalow> #chair felixfontein bcoca 14:32:34 <zodbot> Current chairs: akasurde bartmon bcoca bob_cheesey felixfontein fridim gundalow jhawkesworth_ navalkp rbarlik resmo robertgwilliam shaps terbolous winem_ xenlo 14:32:39 <gundalow> bcoca: Morning :) 14:32:41 <felixfontein> bcoca: yes, that too. installing PyMySQL is much easier though (especially when your mysql runs in a docker container and you don't have libmysql installed) 14:33:30 <bcoca> most of my hardware servers dont have mysql installed ... 14:34:19 <bcoca> but if you are using mysql, it is a lot more likely you have the C client installed, including the lib, using the pure python for production is not something i've seen (nor recommend) 14:38:25 <felixfontein> we're running MySQL in a docker container, so we'd like to avoid having libmysql installed (the CLI tools can be run via docker as well, so Ansible is the only thing which requires something installed for this on the host machine) 14:38:53 <felixfontein> and for making sure a user or DB exists, the python connector should be fine :) 14:42:40 <gundalow> #action merge 48558 14:46:28 <gundalow> icinga2_host: fix use_proxy option #47671 14:46:46 <gundalow> > icinga2_host uses standard set of url parameters including use_proxy, which is documented as an paramter to this module. However, parameter is not passed to fetch_url resulting in proxy beeing used all the time (when defined in environment) 14:50:45 <felixfontein> hmm, so every module using url_argument_spec() must take care to pass all these parameters manually to fetch_url()? 14:51:47 <felixfontein> I guess that the icinga2_host author assumed (incorrectly) that fetch_url() fetches default values from the module parameters if they are defined in url_argument_spec() 15:01:51 <gundalow> Who's still around? 15:02:10 <gundalow> I might take a break for this and update BOTMETA for inventory scripts 15:03:23 <felixfontein> I'm only partially here; I'll be more active later 15:03:30 <jhawkesworth_> sorry slightly distracted today due to company being bought 15:03:59 <bob_cheesey> gundalow - more or less still here, i finish at 1600 though 15:05:43 <bcoca> felixfontein: at that point you can execute module on any of the clients .. also, iirc most packaging tools include the client as dependency when installing server 15:05:49 <bcoca> since admin tools require it 15:09:58 <bcoca> jhawkesworth_: i know that feeling 15:17:30 <winem_> gundalow, sorry, a customer accidentally restored a old vm snapshot instead of deleting them and that fucket up the 16 node prod cluster... slightly distracted due to that :/ 15:20:18 <bcoca> winem_: customers! 15:22:14 <winem_> yep.. and we have redis running as db and our own stuff for the cluster sync thingy which is actually quite awesome (sorry to say that). unless you restore a node with an old format where we stored values in strings and switched to a binary format later.. so the worst possible-fuckup here. stuff on the snapshot was even too old to be identified as "obsolete" by the security mechanisms in place here 15:22:17 <winem_> meh, back to ansible :) 15:25:45 <bcoca> replication works great ! ... oh wait .. we added crap data! 15:25:51 <bcoca> its replicated perfectly! 15:26:00 <bcoca> ^ old 'RAID 1/5 joke' 15:26:20 <bcoca> aka, start of presentation titled 'replication is not backup' 15:30:40 <bob_cheesey> bcoca can you tell some of my customers that please? i've gotten bored of saying it 15:35:04 <winem_> a reply in one of the recent calls on this topic a guy said something like "why do we have to enable backups? don't they just happen?" were talking about vm backups 15:35:15 <bcoca> bob_cheesey: i've been saying that for 20yrs ... very few actually listen 15:35:16 <winem_> looks like they all need a bcoca :) 15:35:43 <bcoca> and i only know cause i had a grey beard tell me 20yrs ago 15:36:04 <bcoca> also, backups don't exist/work unless you have restore (and test it) 15:36:23 <bcoca> winem_: at least you know restore works! 15:36:53 <felixfontein> test it is the crucial point. just having backup set up (but forgot to enable it) doesn't help either ;) 15:37:16 <winem_> oh, that's actually my fave. the saying that I don't care about any backups as long as they have proven me that the restore works.. some do and some people don't care / get it.. 15:37:28 <bcoca> had a coworker, she opened my eyes to that, you don't really have backups until you verify restoration 15:37:32 <bob_cheesey> winem_: yes of course, we back your stuff up for free 15:37:51 <bob_cheesey> tested out my backups last night when i blitzed my vscode dir by mistake :/ 15:38:02 <winem_> ouch 15:39:04 <winem_> hmm... what about the PRs? if it's ok to stay a bit off topic I would like to share a question that I have in my head for a while and I would like to hear others opinions before I put the effort into a PR... 15:42:31 <gundalow> winem_: go for it :) 15:42:42 <gundalow> I'm in another meeting 15:43:22 <winem_> hmm, I will need some minutes to take al ook at the code. am a bit unprepared and forgot the stuff I found out already. thx :) 15:44:49 <winem_> ah got it :) 15:46:48 <winem_> so, when we use the copy or template modue for example with backup: true we find a backup of the file next to the new one with the date extension. this is nice, but can also become a bit messy. so I thought "ha, wouldn't it be cool to have a backup_store attribute which is a folder where you store the backups?". sounded quite smart in the first place, but... what if the folder does not exist? do we create it or not? what if we do not have to 15:46:48 <winem_> backup a file but directoy instead? could we limit this feature to files? 15:47:53 <winem_> I think this is not "undoable". it's basically the backup_local in module_utils/basic.py that would be extended here... but I see a whole bunch of pitfalls here.. first one is that there are a multiple modules using the backup_local function 15:48:26 <winem_> my current workaround would be to register the task output and get the backup file from that one 15:49:05 <winem_> but this leads to duplicate code or requires a module / helper / whatever.. what do you guys think about that? 15:52:15 <winem_> -workaround 15:52:47 <bob_cheesey> right i'm out, thanks for running today gundalow and akasurde :) 15:57:34 <gundalow> bob_cheesey: Thank you for your time 16:00:52 <acozine> o/ 16:08:57 <gundalow> #chair acozine 16:08:57 <zodbot> Current chairs: acozine akasurde bartmon bcoca bob_cheesey felixfontein fridim gundalow jhawkesworth_ navalkp rbarlik resmo robertgwilliam shaps terbolous winem_ xenlo 16:08:58 <gundalow> Hi 16:09:42 <gundalow> Add support to urls util for special characters in proxy username and password #47346 16:18:37 <gundalow> #topic Docs PRs 16:18:39 <gundalow> Remove misleading statement passwords must be same #49798 16:23:33 <acozine> re: ^^^ are folks using multiple passwords with vault? anybody have a good example from a playbook/role where you use one password for multiple files or multiple password? 16:24:01 * gundalow personally has only used a single vault pw 16:24:03 <acozine> I think the vault docs could use more examples to flesh out the descriptions 16:24:47 * acozine rarely uses vault because all my playbooks run on Vagrant on my laptop 16:26:09 <acozine> I'd also appreciate use cases for vault - what problems are folks solving with multiple passwords? 16:26:10 <gundalow> acozine: minor comment added though I think it's OK 16:31:07 <acozine> thanks gundalow 16:31:18 <gundalow> acozine: perhaps since repo of roles & secrets though different permissions. ie vault file/pw for dev. Different one for production? 16:38:45 <gundalow> acozine: what's next on your list? 16:39:53 <acozine> I see a few new ones I haven't looked at yet 16:40:30 <acozine> not sure why this is marked docs . . . have you already looked at https://github.com/ansible/ansible/pull/50109? 16:40:42 <acozine> (collective "you") 16:42:14 <felixfontein> I marked it "docs" because all other labes fit even less :) 16:42:30 <felixfontein> *labels 16:43:34 <acozine> heh, makes sense felixfontein 16:43:51 <acozine> additional perspectives welcome on https://github.com/ansible/ansible/pull/48587 16:44:38 <acozine> do folks find that overrides need a warning? 16:46:09 <rbarlik> acozine: is there a way to disable the warning? 16:46:21 <acozine> rbarlik: I don't know, actually 16:46:24 <felixfontein> hmm, I guess a warning is a good idea, but a) how expensive is this w.r.t. runtime (see bcoca's comment), and b) aren't the galaxy people working on role namespaces? (or was it module/plugin namespaces?) this might conflict with what they are working on 16:49:54 <bcoca> all namespaces 16:50:01 <bcoca> but 'traditional roles' would still work 16:50:45 <bcoca> i would look more to a new 'include_role/import_role' option that triggers warning about 'role already used' include_role: name=x warn_dupe=yes 16:51:43 <bcoca> or just do 'when: x not in role_names' .. since you already have the roles for the play listed 16:52:23 <acozine> here's another one - is anybody using `hashi_vault`? I'd appreciate some testing and review on https://github.com/ansible/ansible/pull/46423/files. 16:53:01 <acozine> without "before and after" output, it's tough to evaluate this change 16:56:37 <felixfontein> I don't think that change is any good. "secret=secret/hello:value" is no valid Jinja2 syntax. maybe it should be "secret='secret/hello:value'"? 16:59:22 <felixfontein> (just added a comment) 17:01:48 <acozine> thanks felixfontein for adding a suggestion to the PR 17:03:46 <acozine> here's one that's failing CI, so we can't merge it as is - but I'd like feedback on how useful it would be if fixed and merged 17:03:55 <acozine> https://github.com/ansible/ansible/pull/45396 17:04:06 <acozine> would folks use a `duplicate` filter? 17:05:32 <bcoca> isnt that the same as intersection? 17:06:03 <bcoca> ah, no single list 17:06:22 <bcoca> well ... (list|unique)|difference(list) 17:06:54 <bcoca> though that might not be exaclty same ... 17:15:52 <acozine> this one looks like a quick test to confirm would let us merge it: https://github.com/ansible/ansible/pull/40073/files 17:23:54 <felixfontein> hmm, that PR looks strange 17:24:14 <felixfontein> first, it talks about /g<number>, but uses \g<number> 17:25:30 <felixfontein> and I think the comment is pretty misleading; I think the point is that the replacement text which is added behind the backreference starts with digits 17:26:19 <acozine> ah, okay, not so simple then 17:26:27 <acozine> how about this one: https://github.com/ansible/ansible/pull/49953? 17:27:14 <felixfontein> maybe we should consider https://github.com/ansible/ansible/pull/31452 first (which seems to supersede https://github.com/ansible/ansible/pull/40073), after all gundalow promised that it will get reviewed 17:28:10 <acozine> felixfontein: excellent 17:28:56 <felixfontein> it's a pretty long PR, though 17:29:49 <felixfontein> and contains some logic changes (besides tests) 17:32:19 <felixfontein> it changes the meanings of the before and after options, I think 17:32:36 <rbarlik> #49953 contains some logic errors 17:34:27 <felixfontein> hmm, the replace situation seems like quite some mess 17:37:37 <felixfontein> gundalow: did you ask someone to review #31452 ? 17:43:28 <felixfontein> bbl 17:48:57 <acozine> felixfontein: I see gundalow's note that he would find someone to review 31452, but no review on the PR yet 17:50:36 * acozine => lunch 17:50:59 <acozine> I don't know how late the PR-a-palooza goes today 17:51:09 * gundalow is nearly done for the day 17:51:20 <gundalow> though happy for people to continue using this channel 17:51:27 <gundalow> and just do `#action merged 1234` 18:05:49 <acozine> cool 18:05:55 <acozine> well, I need to grab some lunch 18:31:07 <gundalow> Anyone got anything they've like to get reviewed? 18:32:51 <bcoca> my cough, its getting worse ... 18:35:32 <shaps> do you have a PR for that? 18:35:48 <gundalow> `state: absent` 18:36:00 * gundalow is heading off in ~30 minutes 18:55:32 <gundalow> #info Thanks everybody! 18:55:34 <gundalow> #endmeeting