19:16:24 <maxamillion> #startmeeting Ansible Core Community Meeting 2018-06-12 19:16:24 <zodbot> Meeting started Tue Jun 12 19:16:24 2018 UTC. 19:16:24 <zodbot> This meeting is logged and archived in a public location. 19:16:24 <zodbot> The chair is maxamillion. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:16:24 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic. 19:16:24 <zodbot> The meeting name has been set to 'ansible_core_community_meeting_2018-06-12' 19:16:31 <maxamillion> alright, now we're cooking with gas 19:16:44 <maxamillion> #topic https://github.com/ansible/ansible/pull/41297 19:16:48 <maxamillion> jdelaros1: the floor is yours 19:16:57 <jdelaros1> Thanks. 19:17:56 <jdelaros1> This is a request for a new module for HW management via Redfish. This is a long time coming, presented at RH Summit last month and have engaged with several Ansible Prod Mgrs, not that it matters too much :) 19:18:42 <jdelaros1> First contribution, read all submission instructions, fixed CI failures, so hopefully not missing anything. 19:19:20 <jdelaros1> Ansible was written to be vendor-neutral, and the DMTF is picking up development of module once upstream. 19:19:47 <maxamillion> "the DMTF" ? 19:20:00 <jdelaros1> Requests are not done via ssh but via URIs to Out-Of-Band controllers (i.e. iDRAC, iLO, etc) 19:20:21 <jdelaros1> That's the gist of it. Questions? 19:21:03 <maxamillion> jdelaros1: do you just need someone to review the PR? 19:21:43 <jdelaros1> If that is needed for inclusion, then yes. 19:21:56 <sivel> The new Request class in 2.7 could simplify some of the logic, but not a requirement. 19:22:26 <sivel> since it acts somewhat similarly to the Session class from requests, could consolidate some functionality 19:23:19 <maxamillion> sivel: +1 19:23:28 <jdelaros1> @maxamillion, Distributed Management Task Force 19:23:34 <sivel> https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py#L819 19:24:00 <jdelaros1> Organization that defines the Redfish specs 19:24:05 <maxamillion> jdelaros1: what is that and how do we get them roped into Ansible maintenance/development? 19:24:14 <maxamillion> ah alright 19:24:59 <jdelaros1> The DMTF board chair sits on my floor, we've had many discussions, and they are waiting for this module to be upstream so they can extend functionality 19:25:10 <maxamillion> jdelaros1: alright 19:25:28 <maxamillion> alright, we'll get some review done 19:25:32 <maxamillion> moving on ... 19:25:46 <jdelaros1> ok thanks 19:25:50 <maxamillion> #topic k8s facts 19:26:00 <maxamillion> #undo 19:26:00 <zodbot> Removing item from minutes: <MeetBot.items.Topic object at 0x7f541d538d90> 19:26:14 <maxamillion> #info we need a review for https://github.com/ansible/ansible/pull/41297 19:26:18 <maxamillion> #topic k8s facts 19:26:50 <maxamillion> alright, so there's two PRs .... one adds facts functionality to the k8s module and the other adds a new k8s_facts module 19:26:51 <maxamillion> https://github.com/ansible/ansible/pull/41297/files 19:26:55 <maxamillion> errrr 19:27:03 <maxamillion> https://github.com/ansible/ansible/pull/41128 19:27:15 <maxamillion> https://github.com/ansible/ansible/pull/41088 19:27:34 <maxamillion> I'm curious about a vote from the group on preferred method of handling this 19:28:12 <maxamillion> I'm really sorry, but I'll brb in like 10 minutes 19:28:51 <ryansb> I'd prefer a _facts module if the lookup isn't doing the job 19:29:04 <willthames> 41088 is just a bug fix imho - making sure that the k8s module actually works as documented 19:29:26 <willthames> lookup plugin is doing it's job, to be fair, but it wouldn't if I wasn't using connection=local 19:29:56 <ryansb> in AWS-land we're trying to get rid of/not add `state: list`, but I agree that the k8s case does seem more bugfixy 19:29:59 <willthames> but where do we stand on lookup plugins vs facts 19:30:37 <willthames> 41128 is really to allow the deprecation of the `state: list` functionality and move it to k8s_facts (or lookup plugin) 19:30:48 <ryansb> different purposes, basically. Lookups are host-executed, and in some cases don't work (as you mentioned) 19:30:59 <ryansb> I don't think there's any work to move all the way to one or the other 19:31:12 <willthames> We have `query` for one of the cases I mentioned 19:31:52 <ryansb> yeah, I'm +1 on a separate facts module, and since the https://github.com/ansible/ansible/pull/41088/files one is fixing to match docs I don't have an issue with it 19:32:16 <willthames> I'm used to writing facts modules, I saw that facts were being implemented in a weird way in the k8s module so wrote k8s_facts, not really remembering there was a k8s lookup plugin too 19:32:53 <maxamillion> I'm +1 (driving, irc on my phone while at stop light) ... brb 19:33:35 <willthames> fabianvf did you ever find the original pushback on k8s_facts? 19:35:03 <fabianvf> willthames: unfortunately no, think it may have been an ephemeral communication. I think the gist of it was that facts modules aren't the Ansible way and that the lookup plugin was preferred, I don't think I really pushed on it 19:35:27 <willthames> in the general case would we always want lookup plugin and facts module? do we need dozens of AWS lookup plugins? 19:35:45 <fabianvf> I personally have no objection though, +1 for me 19:35:48 <willthames> fabianvf: I'm used to the AWS approach where *everything* has a facts module :) 19:35:54 <ryansb> heheh :) 19:36:08 <willthames> apart from aws_eks_cluster_facts, I do mean to get around to that at some point :) 19:37:17 <willthames> ok, sounds like there are no dissenting voices 19:37:22 <ryansb> ok, so let's call that closed since there are no other objections, and if there are they can go to the issue 19:37:26 <willthames> I'll work on tidying up the k8s_facts modules 19:37:28 <ryansb> hehe, jinx 19:37:49 <willthames> I have no idea why tests are failing, but that's one for #ansible-devel 19:38:02 <ryansb> #topic handling extra error information on modules https://github.com/ansible/community/issues/317#issuecomment-396299814 19:38:16 <ryansb> #link https://github.com/ansible/ansible/pull/40723/files 19:39:23 <ryansb> I agree with bcoca's comment that if it's an error it should be a `fail_json` but that adding extra keys beyond a message on the failure makes sense 19:39:30 <ryansb> like having a list of failed packages, etc 19:39:41 <ryansb> otherwise warnings are supported via module.warn() 19:40:35 <ryansb> thoughts? 19:41:15 <maxamillion> alright, I'm back 19:41:38 <sivel> In the `go` module I am writing, failed packages are places in the msg 19:41:59 <sivel> just an FYI: https://github.com/ansible/ansible/pull/39752/files#diff-2195b9bd50d313da12caf83b1cc100d7R195 19:42:13 <ryansb> is there a case for people using the extra list in followup rescue/on-fail tasks? 19:42:26 <maxamillion> yeah, I still think the information is best handled in the `msg` field just since that's basically what that's designated for 19:42:37 <maxamillion> ryansb: which is a fair point 19:42:40 <sivel> honestly I kinda doubt people are trying to directly act on the failed packages 19:42:43 <maxamillion> meh, I'm on the fence 19:42:52 <sivel> the task shoudl fail, and the msg properly indicates whey 19:43:05 <sivel> why* 19:43:07 <maxamillion> sivel: I could see some auto remediation happening in certain scenarios with blocks 19:43:12 <ryansb> yeah, for this case maybe they wouldn't 19:43:21 <ryansb> +0 from me: up to maintainer 19:43:44 <jborean93> I like more specific errors and helpful hints to be part of the msg 19:44:04 <maxamillion> willthames: I'll gladly review or we can con fabianvf into it 19:44:12 <maxamillion> willthames: just to follow up since I was afk 19:44:33 <maxamillion> jborean93: I can see how that could be difficult if someone's trying to parse or handle things specifically 19:44:55 <maxamillion> I agree with ryansb, up to the maintainer 19:45:13 <sivel> I mean modules can return whatever, but at minimum it should be duplicated into the message 19:45:24 <willthames> maxamillion: main issues are failing docs tests but I made no docs changes and the errors don't *seem* to be related to my changes 19:45:26 <jborean93> that's fair enough, I think parsing the msg is a bit too brittle but that doesn't stop people from doing it 19:46:11 <maxamillion> willthames: it's possible you didn't make a docs change but a new change went in to enforce a docs guideline that the CI system didn't previously enforce (which is not uncommon) 19:46:15 <sivel> I'm not against including a list of failed packages in the response, but without including it in the message, it might not be clear enough 19:47:04 <maxamillion> sivel: I'm +1 for ensuring all the info is in `msg` but can also be provided under a different key in order to take action if someone wants to 19:47:06 <sivel> (also, I haven't looked at the PR in question, just speaking theoretially) 19:47:23 <sivel> and I am having a really hard time typing today ;) 19:49:31 <maxamillion> alright, there doesn't seem to be any more discussion on this 19:50:50 <maxamillion> #info modules should provide all relevant output/status/error information via `msg` but can also provide other information (duplicate or otherwise) in a way that can have action taken on it automatically, at the maintainers discretion 19:51:36 <maxamillion> #topic should inventory.any_unparsed_is_failed configuration setting be backported to 2.6? 19:51:46 <maxamillion> #link https://github.com/ansible/ansible/pull/41171 19:52:22 <maxamillion> mattclay: anything specific you want to discuss here or just get +/-1 s on "backport?" 19:52:36 <mattclay> Yeah, just looking for a vote on whether or not we should backport that. 19:52:44 <sivel> -1 19:53:01 <maxamillion> sivel: why so? 19:53:14 <sivel> I'm classifying it as a feature 19:53:26 <maxamillion> ah ok 19:53:38 <maxamillion> from that perspective, that's fair 19:53:42 <sivel> people had an expecation, it was a wrong expectation, users thought it was a bug, but they were really missing a feature 19:53:55 <maxamillion> sivel: agreed 19:53:59 <maxamillion> -1 here as well 19:54:53 <abadger1999> Precedence is that we would classify it as a feature. 19:55:25 <abadger1999> Pretty much anythng that adds UI gets classified as a feature (config is UI) 19:55:34 <maxamillion> done and done 19:56:08 <maxamillion> #agreed https://github.com/ansible/ansible/pull/41171 should not be backported to stable-2.6 because it is a feature 19:56:16 <mattclay> Thanks 19:56:25 <maxamillion> #topic https://github.com/ansible/ansible/pull/41058 19:56:31 <maxamillion> agaffney: the floor is yours 19:57:21 <agaffney> the intention of the PR is to remove some code duplication and allow for mixing/matching features in the default-py-derived callback plugins 19:57:44 <agaffney> right now, you can't use YAML output and hide "skipped" tasks 19:58:09 <agaffney> bcoca has some minor misgivings, but I wanted to get additional feedback 19:59:24 <willthames> other than the short deprecation cycle it seems like a good idea to me 19:59:31 <abadger1999> I lean towards accepting. 19:59:57 <agaffney> the deprecation version can easily be changed. I just didn't know if it should be something like 2.10 or 2.11, or if those versions will even exist 20:00:36 <willthames> it should be 2.11. If that doesn't exist, then we'll change those to whatever is 3.x 4 versions from 2.7 20:00:50 <maxamillion> yeah, I'm leaning towards accepting as well 20:00:59 <agaffney> the 'debug' callback plugin can also probably be deprecated, as the 'yaml' callback plugin overlaps by nature 20:01:18 <abadger1999> (Yeah, deprecation should be +4 versions from the release it first shows up in) 20:01:51 <agaffney> I had just copied the deprecation section from another file and didn't change the version because I wasn't sure what to change it to 20:02:52 <agaffney> so...change the removal version to 2.11 and also deprecate 'debug'. anything else to add? 20:05:33 <abadger1999> Hmm... If bcoca is thinking that yaml shold remain in a separate file... I can sorta see that point. 20:05:54 <abadger1999> If yaml wasn't already needed by ansible, it would definitely be in a separate plugin. 20:06:01 <agaffney> yeah, as do I. I have no strong feelings about that 20:06:16 <agaffney> I can back out the 'yaml' changes 20:06:19 <abadger1999> Maybe we can add some sort of encode and decode hooks 20:06:36 <abadger1999> (well. serialize hooks to be clear it isn't about charset encoding) 20:06:53 <abadger1999> and then the yaml plugin can be separate but have less duplicate code? 20:08:08 <agaffney> you mean have _dump_results() call a separate function just for formatting the dict, with the current JSON implementation as the default in callback/__init__.py? 20:08:32 <abadger1999> Yeah. And then the function is either passed in or it's an attribute of the callback class. 20:08:54 <abadger1999> That way the yaml plugin can override the default 20:09:36 <agaffney> the 'yaml' callback plugin (for example) can just define its own version of the function, just like it does for _dump_results() now 20:10:15 <agaffney> although, I think that could/should be in a separate PR, after I back out the 'yaml' changes in this PR 20:10:45 <agaffney> I'm really more after the ability to mix/match functionality. the removal of some duplicate code was just a bonus 20:11:29 <abadger1999> <nod> 20:11:35 <abadger1999> yeah, that sounds like a plan. 20:11:51 <abadger1999> Also... I now notive the yaml callback plugin returns json sometimes. 20:12:01 <agaffney> okay, so I'll change the removed_in to 2.11 and back out the 'yaml' changes 20:12:11 <abadger1999> Cool. 20:12:12 <abadger1999> +1 20:13:16 <agaffney> and I guess also add a changelog snippet while I'm at it 20:16:12 <abadger1999> Thank you, yes :-) 20:21:15 <maxamillion> alright, I'll consider that one resolved ... any action items or anything? 20:21:26 <maxamillion> (sorry, I got ping'd offline for a couple minutes) 20:22:15 <agaffney> there are a few minor action items for me on my PR, which are already in progress 20:22:25 <jdelaros1> maxamillion, one question, what happens after a PR review? What is next step? 20:23:32 <maxamillion> jdelaros1: once it's reviewed and signed off, it gets merged and if you get merged will likely land in Ansible 2.7 (unless for something the review/iterate process takes forever) 20:23:50 <maxamillion> jdelaros1: https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md#when-will-your-pull-request-be-merged 20:24:04 <jdelaros1> Thanks 20:24:16 <maxamillion> jdelaros1: certainly 20:25:02 <agaffney> I wish there was a good way to write tests for a stdout callback plugin 20:26:20 <maxamillion> on of the ad-hoc command line things and pattern match? 20:27:41 <maxamillion> errr in the scripts that are run in integration tests ... I can never remember the name of them 20:28:04 <agaffney> any code that does matching on the output from ansible-playbook would be....unpleasant to write and read 20:33:10 <maxamillion> #topic Open Floor 20:33:21 <maxamillion> agaffney: agreed :/ 20:33:36 <maxamillion> alright, anything for open floor? I know this has been a crazy long meeting 20:46:35 <maxamillion> #endmeeting