14:59:28 <bcoca> #startmeeting ansible core public irc meeting
14:59:28 <zodbot> Meeting started Thu Aug 23 14:59:28 2018 UTC.
14:59:28 <zodbot> This meeting is logged and archived in a public location.
14:59:28 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:59:28 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
14:59:28 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting'
14:59:37 <ryansb> me waves
15:00:20 <sdoran> o/
15:00:45 <shertel> \o
15:00:58 <abadger1999> Hello
15:01:10 <jwitko1> Hey all,  jwitko here for https://github.com/ansible/ansible/pull/19229 .  Call me when you need me :)
15:02:33 <bcoca> #topic https://github.com/ansible/community/issues/ansible/ansible#27320
15:02:36 <bcoca> @Pilou?
15:04:02 <bcoca> waiting for 1 min, if no show, i'll skip his issues for today
15:04:19 <abadger1999> Heh
15:04:29 <abadger1999> #topic is wrong ;-)
15:04:38 <jwitko> yea URL does not work
15:04:53 <bcoca> copied from his post, which is also 404
15:04:55 <shertel> This one? https://github.com/ansible/ansible/pull/27320
15:05:04 <jwitko> https://github.com/ansible/community/issues/ansible/ansible#27320
15:05:16 <bcoca> #topic https://github.com/ansible/ansible/pull/27320
15:06:12 <bcoca> #topic https://github.com/ansible/ansible/pull/19229
15:06:18 <bcoca> jwitko: go4it
15:06:24 <bcoca> @jwitko1
15:06:32 <jwitko> Hi !   Just looking to bring this up for merge.
15:06:34 <bcoca> ah, missed rneam
15:06:41 <jwitko> Oh, i can wait
15:06:58 <bcoca> other agenda proposer didnt show, so you are up
15:07:06 <jwitko> First time in an ansible meeting so not sure of the protocol here
15:07:09 <abadger1999> For 27320, I'd like spredzy or one of the other crypto maintainers to review.
15:07:12 <bcoca> no other docker module maintainer was available?
15:07:16 <jwitko> Just wanted to bring attention to this as I think its pretty solid at this point
15:07:18 <abadger1999> But otherwise, I have no problem if it gets into 2.7
15:07:27 * abadger1999 will look for them in #ansible-devel
15:09:05 <bcoca> jwitko:  i see you already have a couple of reviewers, do you know if they have 'shipit' ?
15:09:11 <jwitko> There was a report of issues and a feature request 6 hours ago.  The feature request isn't without its merrit.  The issue request I do not experience the same problems and havent for quite a while.
15:09:26 <jwitko> @bcoca, I do not know.
15:09:42 <bcoca> cove dusdanig felixfontein kassiansun softzilla zfil <= list of github ids of pople that deal with docker
15:09:59 <sdoran> I don't feel qualified to review that. I don't know Swarm at all.
15:10:07 <bcoca> i would try to get them to approve and if you still cannot, come back and one of us will try to find the time
15:10:09 * Pilou wave
15:10:14 <bcoca> same here, never used swarm
15:10:14 <ryansb> I've done some work with it, I can take a look as well
15:10:32 <jwitko> @bcoca thanks I will ping them.   @ryansb that would be great!
15:10:37 <abadger1999> Thanks ryansb!
15:10:40 <bcoca> Pilou: hi, will get back to your issues, moved on to next item while you were absent
15:10:53 <bcoca> ryansb++
15:11:06 <bcoca> #topic https://github.com/ansible/ansible/pull/27320
15:11:25 <bcoca> a) post url is wrong b) abadger1999 already mentioned having crypto expert give a look at it
15:12:55 <bcoca> anything else on this?
15:13:20 <Pilou> spredzy aldready approved #27320 isn'it ?
15:13:29 * abadger1999 checks
15:13:38 <abadger1999> If he did, then it's ready
15:14:17 <abadger1999> Thanks Pilou, I'll merge.
15:14:31 <bcoca> #topic https://github.com/ansible/community/issues/ansible/ansible/pull/21215
15:14:41 <abadger1999> bcoca: not a crypto expert necessarily, but the other maintainers of the crypto modules.
15:14:46 <bcoca> ^ i would also start by 'has spredzy checked?'
15:14:51 <bcoca> abadger1999: gtk
15:15:09 <bcoca> Pilou: all your links are bad!
15:15:10 <sdoran> Another 404 on that link.
15:15:12 <shertel> https://github.com/ansible/ansible/pull/21215
15:15:23 <sdoran> 👍
15:15:42 <bcoca> #topic https://github.com/ansible/ansible/pull/21215
15:17:11 <Pilou> (links updated)
15:18:01 <bcoca> so refactoring to shared code seems sensible, but does this change existing interface/functions for either?
15:18:38 <alikins> idea seems reasonable to me
15:19:22 <bcoca> anyone familiar with this specific code?
15:20:25 <sdoran> Any time we mess with crypto I feel we should have a security/crypto expert review it to make sure we're not improperly hashing something.
15:21:22 <bcoca> idem
15:21:23 <alikins> at some point, it may be worthwhile to make crypto impls pluggable (or more specifically, make some of the APIs that uses crypto pluggable and provide multiple implementations of the plugins, and pick the right plugin at run time)
15:21:41 <bcoca> mostly how the hash libs work
15:22:06 <abadger1999> This looks okay.  I probably have touched this code the most.
15:22:19 <abadger1999> I would not name things encrypt whenever possible.
15:22:27 <abadger1999> (since it's hashing, not encrypting)
15:22:42 <bcoca> people confuse them enough as is, since most encryption requires hashing ...
15:22:44 <abadger1999> but there are some existing things that can't be changed for backwards compat (do_encrypt)
15:23:13 <abadger1999> I think the other uses of encrypt are private, though, so they can be renamed sometime in the future.
15:23:42 <bcoca> #topic https://github.com/ansible/ansible/pull/44008
15:24:29 <abadger1999> I don't like some of the style in there like passing **settings around.
15:24:39 <abadger1999> but I think the program logic is good.
15:24:53 <abadger1999> II guess at this point, I'll review it.
15:25:41 <bcoca> k, i was waiting for alikins review, but he did and i missed it, more eyes better
15:25:43 <abadger1999> alikins: Looks like the only question before merging is to you.
15:26:30 <abadger1999> Or perhaps both of you are waiting for someone like bcoca to explain what to do.
15:27:44 <Pilou> is a change required ?
15:28:07 <bcoca> im unsure here, i gave response to alikins's question, was waiting for him to agree/disagree
15:28:46 <bcoca> if no one objects, we can just merge
15:30:02 <alikins> Pilou: I don't know if it is a problem.
15:30:34 <Pilou> note that meanwhile _gather_subset was changed from "barelist" to "list"
15:30:49 <alikins> I suspect it (playbook type being string while config default is a list) doesn't matter because everything assumes it is a list, but haven't tested.
15:31:25 <sivel> Pilou: yes, barelist was scheduled to be removed for the 2.7 release, so it just got nuked
15:31:32 <bcoca> since module is 'list' it doesnt matter if it gets comma string or actual list, the module argspec will make sure its a list
15:33:26 <bcoca> anything else on this one?
15:34:52 <abadger1999> No objections from me
15:34:55 <bcoca> so i'll merge after meeting
15:35:03 <bcoca> #topic https://github.com/ansible/ansible/pull/44008
15:36:42 <bcoca> playbook as a module
15:36:55 <sdoran> My head just exploded.
15:37:03 <bcoca> #topic https://github.com/ansible/ansible/pull/42163
15:38:16 <bcoca> should i wait for shock to wear off?
15:41:11 <abadger1999> Does it do something that can't be easily achieved another way?  I see some back and forth between you and the reporter abut whether delegate_to can substitute.
15:41:48 <sdoran> Using Ansible to run Ansible from another host...
15:42:02 <bcoca> not for the use case he wants, i still dont think this is something we really want .. but not going to strongly oppose it, -0.5 from me
15:42:13 <nitzmahone> -1
15:42:25 <bcoca> a solution based on runner or a strategy or mitogen like would be better
15:42:36 <sdoran> I mean, I see what they're going for, but that seems fraught with peril.
15:42:44 <bcoca> but im also not dead set against this
15:42:50 <abadger1999> If there's no other way to do this, I could see adding it.
15:42:51 <bcoca> sdoran: agreed
15:42:54 <sdoran> @bcoca My thoughts exactly.
15:43:00 <nitzmahone> They're free to use it themselves, but this just seems like a source of problems
15:43:03 <abadger1999> jkeating was doing things like this.... maybe I'd ask him for input.
15:43:04 * sdoran waits for someone to submit ansible_runner module
15:43:04 <bcoca> abadger1999: that is why im 'fence adjacent'
15:43:32 <abadger1999> If he likes it and thinks it would have made his life easier back in the day, it leans me more towards accepting it.
15:44:07 <nitzmahone> This kind of thing belongs on Galaxy, not in the box IMO
15:44:07 <bcoca> i would just like a flashing red 'COMMUNITY' label on this anytime someone looks at it
15:44:19 <sdoran> Or we could ask ryanpetrello since he did the work for Tower remote nodes.
15:44:30 <bcoca> nitzmahone: yes and no, eventually we have to deal with proxy execution .. though awx already does
15:44:52 <nitzmahone> Yeah, but this isn't the right way, so let's not get people used to the wrong way
15:45:00 <bcoca> that is one avenue, 'ansible should not deal with this, other tools do so already'
15:45:10 <abadger1999> @nitzmahone 3/4 of modules belong in galaxy. and all of the strategies except for linear and free (and free just because it keeps us honest).
15:45:13 <sdoran> I'm worried about the bug reports we'd get from that.
15:45:28 <sdoran> Even though it's community, there is not a big enough flashing red banner.
15:45:48 <abadger1999> nitzmahone: If there's a right way, then yeah, I'm against it.... but that's my question, is there currrently a right way?
15:45:52 <bcoca> sdoran: agreed, some leds affixed into inside of user eyelids might be enough
15:46:03 <nitzmahone> @abadger1999: sure, but most of those aren't "meta Ansible"
15:46:07 <abadger1999> If not, it does satisfy our criteria....
15:46:14 <bcoca> abadger1999: currently awx supports this kind of thing, also ansilbe-runner
15:46:46 <abadger1999> It's a command execution on a remote machine.  the module adds support for idempotence and check_mode.
15:46:48 <bcoca> this just makes it so you dont require anythng outside 'ansible core'
15:47:08 <abadger1999> <nod>  So would a module basred on ansible-runner be better?
15:47:13 <abadger1999> and acceptable?
15:47:16 <bcoca> possibly
15:47:24 <bcoca> i would almost make that a 'connection'
15:47:36 <bcoca> but this is 'concrete solution now' vs 'future possible thingy'
15:47:40 <abadger1999> I'm fine saying, rewrite this based on ansible-runner if that's the idea.
15:47:46 <bcoca> which makes me stay on fence area
15:48:08 <bcoca> k, quick vote
15:48:10 <bcoca> -0.5
15:48:27 <abadger1999> +1  (but I could easily change to -1 once I know more)
15:48:54 <bcoca> sdoran, nitzmahone, anyone else?
15:48:55 <abadger1999> I think we currently don't bar modules for what they do, just how they do it.
15:49:08 <bcoca> abadger1999: rarely, but agreed
15:49:23 <abadger1999> So if there's a better way to do it, I'm okay with barring this one and saying, implement it this other way instead.
15:49:37 <sdoran> I don't think this adds much more value that `command` other than some arg validation. You still have to copy all the bits over yourself, e.g., inventory, playbook, with other tasks.
15:49:49 <abadger1999> But if there's not and it's genuinely useful then....
15:49:54 <bcoca> hmm action plugin?
15:49:58 <abadger1999> sdoran: It adds the calue that we usually demand:
15:50:03 <abadger1999> idempotence and check mode.
15:50:12 <abadger1999> *value
15:50:30 <bcoca> yes, but we might wnt 'more' for something called ansible_playbook
15:50:51 <bcoca> community or not, most people will assume this is official/core/supported
15:51:05 <nitzmahone> --^ exactly
15:51:16 <abadger1999> I also wouldn't want to bar it based o nthe fact that it doesn't also copy inventory, playbook, etc over... because it seems like it would be a worse module if it did all that ;-)
15:51:21 <nitzmahone> This doesn't solve enough of the problem to be significantly more useful than command/shell IMO
15:51:40 <bcoca> nitzmahone:  it passes the min we normally ask for
15:51:41 <abadger1999> yeah, the comunity support but it's confusing aspect does get to me.
15:51:51 <nitzmahone> It's a trip hazard IMO
15:51:54 <abadger1999> But that could be a rename instead of a road block.
15:52:02 <abadger1999> and maybe documentation.
15:52:04 <bcoca> he, why i brougth it up here, this is a hard one to decide on
15:52:21 <abadger1999> "The ansible core team recommends anbile-runner/ansible-pull/etc instead of this module"
15:52:34 <bcoca> all caps and flashing RED
15:52:44 <alikins> do we? if so, why?
15:52:48 * bcoca looks for siren .wav
15:53:06 <nitzmahone> We need :actual_rotating_light: here
15:53:08 <bcoca> pull can deal with remote plays (it fetches)
15:53:19 <bcoca> runner can also take payloads
15:53:26 <bcoca> awx will distribute the play
15:53:35 <abadger1999> unofficial_remote_playbook
15:53:43 <bcoca> this is 'everything is on remote, let me verify that cli args are correct'
15:53:46 <sdoran> I don't see how that actually supports check mode. Does it pass in a flag if in check mode?
15:54:13 <bcoca> sdoran: yes, but  .. it doesnt 'check itself' but the intended 2nd execution ...
15:54:17 <sdoran> And it looks like `changed=True` is hard coded.
15:54:19 <bcoca> which is kind of 'not check'
15:54:29 <bcoca> sdoran: good catch, missed taht
15:54:30 <abadger1999> sdoran: Heh.  You are correct, it doesn't currently do the right thing.  It could pass the check mode flag, though.
15:54:36 <bcoca> it shoudl depend on pb results
15:54:42 <abadger1999> sdoran: so it's basically atthe level of our tower modules ;-)
15:54:53 <sdoran> Exactly, which it's not doing.
15:55:01 <abadger1999> "I support checkmode.... except I don't really" ;-)
15:55:06 <sdoran> So this is really "command with the safetfy off" since it always run, even in check mode.
15:55:14 * abadger1999 needs to check if those modules got fixed.
15:55:24 <abadger1999> @sdoran<nod> but that's a bug.
15:55:36 <sdoran> Yes.
15:55:38 <abadger1999> We can ask that it be fixed..
15:55:46 <abadger1999> But would we accept it after it is fixed?
15:55:48 <bcoca> also teh changed=true
15:56:03 <bcoca> he, yeah, no sense in making him fix if we are going to reject
15:56:06 <abadger1999> <nod>
15:56:12 <bcoca> that is just being mean
15:56:29 <abadger1999> yeah, so if he did make this so that changed and checkmode functined, would we accept it?
15:56:43 <bcoca> im still -0.5
15:56:47 <nitzmahone> Still -1
15:56:57 <bcoca> -1.5/1 ? anyone else?
15:57:17 <sdoran> So _if_ it actually supported check mode and was properly idempotent — reported changed/failed based on output of tasks in the called playbook — I'd be +0.5.
15:57:23 <sivel> -1
15:57:31 <bcoca> -2.5/+1.5
15:57:45 <bcoca> ^ i would still call that 'inconclusive'
15:57:52 <abadger1999> I'd like to have jkeating look at it.
15:58:14 <bcoca> k, lets postopne final decision until that and get more people voting
15:58:15 <abadger1999> If he says, this wouldn't be useful or there's another way to do this, then I would definitely switch my vote.
15:58:54 <sdoran> And have ryanpetrello look at it too.
15:58:58 <bcoca> i'll also ping author for next meeting to let him make his case
15:59:03 <sivel> we really need to define what a vote is.  I've been advocating for a minimum 5 core members to be a valid vote, any result is final.
15:59:11 <abadger1999> Should we tell the guy, we aren't sure if we'll take this but if we did it should (1) rename (2) fix idempotence (changed=) and (3) support checkMode properly?
15:59:13 <sivel> regardless of how close
15:59:23 <nitzmahone> This is basically just a static arg mapping from YAML to ansible-playbook
15:59:39 <nitzmahone> sivel: totally agree
15:59:44 <bcoca> nitzmahone: and with connection/become options, that is already a problem
16:00:16 <sivel> also, just outputting the normal stdout of ansible-playbook isn't super useful, it should at minimum use the json callback to give usable output
16:00:18 <abadger1999> sivel: We definitely have to define that.  I'd ask for a different algorithm but yeah.
16:00:19 <sivel> but still -1 anyway
16:00:29 <bcoca> sivel: considering 2 of us are in fence ... if it were 3/2 i would be happier, though i like larger margins
16:00:31 <abadger1999> sivel: +1  json callback.
16:00:42 <nitzmahone> We generally have rejected "low value add" modules that are just a wrapper over a config file and such, this falls into that category IMO
16:01:14 <sivel> Yeah, I didn't see much functionality in there that justified a module
16:01:28 <abadger1999> nitzmahone: not if idempotence and checkmode were fixed (even one of those have  been our bar for low value add in the past)
16:01:29 <bcoca> ok, so let me rephrase the votes, 2 strongly against, 1 meh, another meh for and 1 more for but open to change
16:01:45 <sivel> literally just build a command and execute it
16:01:53 <abadger1999> anyhow... I do think more info will make this clearer.
16:01:54 <sivel> no additional functionality
16:02:03 <bcoca> sivel: what if action plugin that also pushed a payload of playbook + resources?
16:02:20 <bcoca> what would make you accept this kind of feature?
16:03:03 <bcoca> running out of time, i'll add this to next agenda giving time for more feedback/info and author to be able to defend
16:03:04 <abadger1999> So I don't think it's time yet to make a decision... but if we can figure out more alternatives, that would be productive.
16:04:16 <bcoca> agreed, and with that
16:04:19 <bcoca> #endmeeting