19:01:26 <bcoca> #startmeeting ansible core irc public meeting
19:01:26 <zodbot> Meeting started Tue Mar  5 19:01:26 2019 UTC.
19:01:26 <zodbot> This meeting is logged and archived in a public location.
19:01:26 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:01:26 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:01:26 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
19:01:35 <bcoca> #topic https://github.com/ansible/ansible/pull/52045
19:01:49 * shertel waves
19:02:01 * sdoran sips tea
19:02:08 <bcoca> subject is a bit misleading, yes its adding toggle, but mainly its adding 'global sanitization' and deprecation notices for users of plugins that dont currently sanatize correctly
19:02:47 <bcoca> we had previous vote against a toggle to allow user to customize sanitization rules, but this is diff, it enforces the rules (as per previous vote) and established deprecation
19:03:09 <shertel> was merged, just the parent groups feature. Wrong PR?
19:03:26 <bcoca> ah .. nvmd, yes PR changed
19:03:49 <bcoca> https://github.com/ansible/ansible/pull/52748
19:03:57 * felixfontein waves
19:03:58 <bcoca> #topic https://github.com/ansible/ansible/pull/52748
19:04:04 <bcoca> .chair felixfontein
19:04:04 <zodbot> felixfontein is seated in a chair with a nice view of a placid lake, unsuspecting that another chair is about to be slammed into them.
19:04:06 * alongchamps may need a coffee
19:04:25 <felixfontein> bcoca: thanks for the nice chair :D
19:04:33 * bcoca needs whiskey in his coffee
19:04:41 <alongchamps> bumper chairs! what can go wrong...
19:05:29 <bcoca> no one against? ... then i'll consider this covered under previous vote
19:05:31 * shertel eats paczki for sugar crash
19:05:43 <shertel> +1
19:05:44 <bcoca> just wanted to make sure there was no opposition to strat
19:05:54 <bcoca> #topic https://github.com/ansible/ansible/pull/32214
19:06:03 <bcoca> @akasurde?
19:06:57 <bcoca> not sure i have much to discuss here, can Mac user verify change works? ...
19:07:05 <sdoran> I can test it.
19:07:09 <bcoca> go4it
19:07:21 <bcoca> #topic https://github.com/ansible/ansible/issues/52354
19:07:25 <sdoran> I think it makes sense to have it set all three parameters rather only the hostname.
19:07:30 <bcoca> move ansible to follow XDG spec ...
19:07:42 <sdoran> I think that was the only thing that somewhat up for debate.
19:07:51 <bcoca> core team commented and closed ticket, but @c-edw wants to make a case for it
19:08:27 <bcoca> @sdoran i would add that in ticket, not sure its worth debating here unless consensus cannot be achieved
19:08:34 <sdoran> Ok.
19:08:46 <bcoca> no c-edw, moving on
19:08:53 <bcoca> #topic https://github.com/ansible/community/issues/436#issuecomment-465655663
19:08:55 <bcoca> ^ nitzmahone
19:08:58 <bcoca> @nitzmahone
19:09:22 <nitzmahone> thanks
19:09:56 <nitzmahone> Yeah, just wanted to see if anyone had a problem with making module/action `choices` globally case-insensitive throughout ansible
19:10:03 <bcoca> i think lowerstr or 'case_insenstive_str' might be good solution, i would shy away from doing on str due to 3rd party
19:10:12 <nitzmahone> We have some modules that are doing crazy machinations to support that today
19:10:15 <bcoca> istr
19:10:40 <bcoca> nitzmahone: also, there are non-string choices
19:11:00 <felixfontein> I've used it with integers once
19:11:17 <felixfontein> (for protocol version)
19:11:39 <nitzmahone> yeah, I thought of a couple issues with a new `lowerstr` type, but it's been long enough that I forgot what they were
19:11:49 <bcoca> segment_size_kb=dict(default=128, choices=[8, 16, 32, 64, 128, 256, 512], type='int')
19:11:59 <gundalow> Is their any module that does `choices: ['A', 'a']` today?
19:12:06 <nitzmahone> not legitimately
19:12:23 <bcoca> have you looked at all 3k?
19:12:30 <nitzmahone> (they're doing it because they originally wrote with mixed case choices, then changed to lowercase later)
19:12:36 <gundalow> could hack `validate-modules` to see
19:12:56 <nitzmahone> I haven't looked exhaustively
19:13:03 <sdoran> I think making the choice matching case-insensitive is ok.
19:13:07 <bcoca> i would got 'istr' route if possible (jsut cause 3rd party) and +1 to validate checking for this
19:13:07 <sdoran> It makes sense in this context.
19:13:13 <nitzmahone> but I'm not aware of any legit usages of mixed case choices today
19:13:20 <gundalow> What would happen if a module had `choices: IPv4, IPv6` and someone put in junk, would they get an error saying use `ipv6` (even though they put in IPv6)?
19:14:43 <bcoca> im not saying its common use case, but i can see wanting to diff between A and a
19:15:26 <gundalow> nitzmahone: what's the problem you want to solve by going to case-insensitive?
19:15:39 <bcoca> uniformity with 'windows' for one
19:15:45 <sdoran> Agreed. But I think it makes more sense in a programming context. It's probably a nicer UI in playbooks to have the choices checking be case-insensitive.
19:16:03 <nitzmahone> The only time-sensitive issue here is that Powershell added a deprecation warning in 2.8 that we're switching back to case-sensitive, but ran into a couple other cases on the Python side that maybe we should consider going the other way everywhere
19:16:13 <sdoran> Because the folks inputting the data in playbooks aren't necessary programmers and may not care about case distinctions.
19:16:16 <nitzmahone> I don't want to change the deprecation behavior after it releases
19:16:38 <sdoran> I'd just like it to be unified.
19:16:42 <nitzmahone> same
19:16:45 <bcoca> i would implement via istr and declare 'all windows str == istr'
19:17:14 <bcoca> just cause i dont want to break the few cases that do use case
19:17:51 <bcoca> 'NewestInstance', 'OldestLaunchConfiguration', 'ClosestToNextInstanceHour', 'Default']
19:17:52 <bcoca> aunch', 'Terminate', 'HealthCheck', 'ReplaceUnhealthy', 'AZRebalance', 'AlarmNotification', 'ScheduledActions', 'AddToLoadBalancer'
19:17:58 <bcoca> ^ though some people make me want to shoot them
19:18:26 <felixfontein> choices=['ENABLED', 'DISABLED']
19:18:34 <bcoca> STOP YELLING!
19:18:34 <felixfontein> scream it!
19:19:02 <felixfontein> even more fun: choices=['autoscaling:EC2_INSTANCE_TERMINATING', 'autoscaling:EC2_INSTANCE_LAUNCHING']
19:19:07 <bcoca> t(choices=['http', 'https', 'tcp', 'HTTP', 'HTTPS', 'TCP']  < sample of 'we take all', that istr could cover
19:19:58 <bcoca> nitzmahone: i see many modules that rely on upercase inputs mathing in code
19:20:31 <felixfontein> the code would have to return precisely the value specified in the choices list
19:20:40 <felixfontein> (if it's not opt-in by the module)
19:20:41 <bcoca> most, by far, are lower case .. a very few camelcase and just saw a few that are 'title case'
19:21:32 <bcoca> i see some 'choices' that are dynamic via API call ...
19:22:19 <bcoca> and some passed through directly to api .. so depends if that api is case insenstiive, i don't think we can do this across the board
19:23:32 <gundalow> bcoca: hum, so cloud/amazon/elb_target_group.py has lower case in module docs, though what you pasted in argspec. So adding in tolower would mean that module could be simplified
19:23:42 <gundalow> I *think*, only had quick look at PR, sleepy
19:23:46 <nitzmahone> exactly- that's the most common case
19:23:56 <shertel> It seems like the provided and CamelCase option could be lowercased, compared, and if the same the CamelCase is provided as the choice to keep backwards compatibility with modules. As long as 'a' and 'A' mean the same thing...
19:24:00 <nitzmahone> (mismatches, or doubled-up)
19:24:05 <bcoca> gundalow: i see some modules that way, others that arenot
19:24:22 <bcoca> so i think istr can help simlify some modules, but i woudl NOT do this on choices +str by default
19:24:23 <nitzmahone> ooh shertel, that's a good idea
19:24:26 <bcoca> too many cases deviate
19:24:45 <sivel> I'm here now
19:24:51 * nitzmahone has 6min
19:25:00 <bcoca> shertel: but some don't mean the same as they are passed to an API ... so depends on what that API does (is it insensitive?)
19:25:21 <bcoca> its going to be a per module consideration .... and we are looking at hundreds of them
19:25:39 <bcoca> .. in /cloud/ alone ...
19:25:44 <felixfontein> bcoca: I think he wants to return the original string from choices (at least I would do that)
19:25:52 <nitzmahone> the option shertel proposed takes care of that nicely
19:26:19 <bcoca> felixfontein: but that might be validating case also, so if user now does option=lowercase, but api takes LowerCase ... value will pass choices but error downstream
19:26:50 <bcoca> an error that module relies on current code to catch waaay before api call
19:26:51 <nitzmahone> That's the one we'd display a dep warning for
19:27:05 <bcoca> nitzmahone: how? you have to know for each module how its consumed
19:27:23 <felixfontein> bcoca: if the module has 'LowerCase' in the choices list for that option, the option parser should return LowerCase even if the user specifies lowerCASE or whatever else
19:27:27 <bcoca> what do we do for those that DO want case senstitive? you are restricting options
19:27:53 <bcoca> felixfontein: that changes what we do now, whic his 'uservalue in choiceslist'
19:28:15 <bcoca> and this change is to uservalue.lowercase() in [ x.lower() for x in choiceslist]
19:28:33 <bcoca> why i strongly suggest doing an istr and allow modules to 'opt in' vs force this
19:29:08 * nitzmahone wishes he'd written down the hole he poked in `type=lowerstr`, because there was one- I was initially leaning that way as well
19:29:18 * nitzmahone 1min
19:29:44 <nitzmahone> OK, so sounds like more investigation to be done before we can change this; I may yank the PS case-sensitivity deprecation for 2.8 anyway
19:29:48 <felixfontein> I also prefer istr, but I'm very interested in what that hole is :)
19:30:03 * nitzmahone will wrack brain to see if he can come up with it again
19:30:09 <bcoca> agreed, please inform on hole, but has to be pretty big to remove author choice
19:30:12 * nitzmahone gotta jet
19:30:25 <bcoca> #topic https://github.com/ansible/community/issues/450#issuecomment-468438381
19:30:41 <bcoca> ^ rename _facts modules that don't return facts to `non_facts`
19:30:48 * nitzmahone +1000
19:31:01 <bcoca> name is misleading and against convention, we can deprecate/alias to new name to make easy transition
19:31:05 <felixfontein> maybe not rename all of them yet (that's a *lot* of word), but at least define that this is the goal
19:31:18 <bcoca> also, can we add 'validate' to ensure _facts module MUST return ansible_facts?
19:31:54 <felixfontein> also I'm curious if _info is already fixed, or still up to discussion
19:31:58 <bcoca> that is probably tricky
19:32:10 <agaffney> just make sure that doesn't encourage non-facts modules to start returning anible_facts when they shouldn't
19:32:17 <bcoca> felixfontein: we tried proposal for _info ... but there was no consensus, but there were a few on _info side
19:32:41 <felixfontein> in devel there are currently 3 proper _info modules, and out of the 345 _facts modules, more than half does not return ansible_facts
19:32:46 <sivel> felixfontein: we haven't necessarily decided that should be _info, in fact I've leaned toward a system that didn't require a specific extension.  But in the case of something like ec2_elb that manages an elb, there would be ec2_elb_info
19:33:07 <bcoca> agaffney: "when they shoudln't" .. modules can/shoudl return facts when they are an after effect of something they manage, i.e you can return mount facts after you mount/unmount
19:33:19 <sivel> but where there was no associated module that actually managed something, I've advocated for just allowing no postfix
19:33:54 <bcoca> sivel: that is slightly diff discussion, tring to agree that if _facts you MUST return facts, otherwise rename (to what .. that is more open discussion)
19:34:02 <felixfontein> and if there's a xxx module, should the ex-facts module be called xxx_info? or something else?
19:34:10 <sivel> I think that _facts should return ansible_facts
19:34:11 * jtanner lurks
19:34:21 <sivel> things that aren't _facts should not return ansible_facts
19:34:42 <sivel> And by extension, we should have fewer _facts modules
19:34:48 <bcoca> sivel: i think we have more leeway on that, but we do all seem to agree that if named _facts, you MUSt return facts
19:35:15 <sivel> things that return info (not facts), may be named with _info ext
19:35:16 <agaffney> I just don't like modules to unexpectedly set top level vars...or even expectedly when it doesn't make sense (*cough* getent *cough*)
19:35:23 <sivel> those are relatively all of my thoughts
19:35:24 <bcoca> anyone against making a rule that 'modules named _facts, must return ansible_facts'?
19:35:41 <sivel> not against, I am +1 for that rule
19:35:42 <agaffney> there's going to be user confusion on the difference between _facts and _info
19:35:45 <felixfontein> I'm +1 for the rule
19:35:48 <shertel> also +1 for that rule
19:35:51 <bcoca> 5/0
19:35:53 <alongchamps> +1, makes sense
19:36:22 <sivel> and then non _facts should not return ansible_facts
19:36:24 <bcoca> 6/0 ... i say 'wins in landslide' .. we'll need more discussions on what CAN return facts and how to name 'info' modules .. but at least we all agree on _facts
19:36:35 <felixfontein> yay!
19:36:39 <sdoran> +1
19:36:52 <sdoran> (for making it a rule)
19:36:58 <bcoca> #topic https://github.com/ansible/ansible/pull/53064
19:37:06 <felixfontein> next important topic is how to name 'info' modules. because otherwise I can't rename existing ones (in particular ones which are new for Ansible 2.8, would be nice to rename them *before* Ansible 2.8 is released)
19:37:46 <bcoca> felixfontein: i'll add that topic end of meeting, already moved on
19:37:51 <jborean93> yea I don't see how we can make it a rule if we don't have the proper suffix to use
19:38:04 <bcoca> jborean93: we have proper sufix NOT to use
19:38:08 <sdoran> 53064 is a community member wanting to favor `lscpu` as the main source of truth for gathering CPU information.
19:38:13 <felixfontein> bcoca: ok, thanks!
19:38:38 <bcoca> -1 lscpu at this point, we still need to maintain the 'file based' approach for systems w/o lscpu, but fine to match the output
19:38:41 <felixfontein> jborean93: bcoca: having one not to use means that people can use whatever they want, and we have chaos :)
19:38:48 <bcoca> yes, its reimlpementation, but we need to do it anyways
19:38:52 <sdoran> Since we still have to be able to fall back to `/proc/cpuinfo` in the event `lscpu` isn't available, it seems like double the effort support both.
19:39:06 <bcoca> felixfontein: i dont disagree, but harder agreement there, also offtopic
19:39:17 <bcoca> sdoran: exactly
19:39:25 <sdoran> It would be easier to parse the output of `lscpu`, but since we can't rely solely on it, I don't see much benefit to implementing thins.
19:39:28 <sdoran> *this
19:40:08 <sdoran> Anyone in favor of this?
19:40:39 <sivel> We also have little to go on to make sure we aren't regressing on platforms we don't have access to
19:40:40 <sdoran> I just wanted to get a decision so as not to lead mator on in her/his effort
19:40:56 <sdoran> sivel: Yup. That was the last discussion on that PR.
19:41:14 <sdoran> And we don't have a good way to validate since we don't have easy access to all the platforms.
19:41:18 <bcoca> so if we count dag (which was clear -1 on ticket) ... we are at -3/0
19:42:50 <sdoran> Ok, seems we vote to stick with our existing strategy, just fix the bug I found that started this whole discussion.
19:43:02 <sdoran> We can move on.
19:43:07 <bcoca> k, since no one seems to advocate for it, moving on
19:43:23 <bcoca> #topic https://github.com/ansible/community/issues/450#issuecomment-468971081
19:43:32 * cyberpear waves
19:43:40 <bcoca> not sure i understand that
19:43:58 <cyberpear> not sure if the warning is by design...
19:44:11 <cyberpear> I understand that the behavior of using the string "false" will trigger the warning
19:44:23 <cyberpear> I'm using a native boolean and still getting the warning.
19:45:14 <bcoca> i cannot repro
19:45:24 <bcoca> - hosts: localhost
19:45:26 <bcoca> gather_facts: false
19:45:27 <bcoca> tasks:
19:45:29 <bcoca> - name: get url response
19:45:31 <bcoca> debug: msg=hi
19:45:32 <bcoca> when: true
19:45:46 <bcoca> ok: [localhost] => {
19:45:47 <cyberpear> when: mycondition, mycondition: true
19:45:47 <bcoca> "msg": "hi"
19:45:49 <bcoca> }
19:46:05 <cyberpear> it's being passed a var that is a bool
19:46:30 <bcoca> its a string, that is the problem
19:46:54 <felixfontein> when: "true"  triggers the warning, or  when: varname  when varname is a boolean variable
19:47:17 <felixfontein> when: varname or false   does not trigger the warning
19:47:25 <bcoca> cyberpear: open a bug, but im unsure that this is wrong, try native types
19:47:27 <bcoca> or |bool
19:47:28 <cyberpear> when: rhel_07_010010
19:47:40 <bcoca> felixfontein: its 'bare var'
19:47:46 <cyberpear> rhel_07_010010: true
19:47:54 <bcoca> expressions wont trigger
19:48:11 <felixfontein> bcoca: yes, but why are bare variables disallowed in the first place?
19:48:11 <bcoca> @cyberpear open a bug ticket with reproducer, really not for this forum as is
19:48:15 <cyberpear> ok, I'll open a bug
19:48:25 <bcoca> felixfontein:  'x = "false"' was evaluating 'true'
19:48:42 <bcoca> #topic https://github.com/ansible/ansible/pull/51466
19:48:53 <bcoca> -1 since varnames lookup alreaady does this and they can be 'chained' easily
19:50:00 <cyberpear> is there an easy way to get 'varname: value' dictionary?
19:50:03 <sivel> I am -1, just because I don't like the concept anyway, but varnames is less gut wrenching
19:50:23 <sivel> note: less than 10 minutes remaining here
19:50:57 <bcoca> lookup('vars',  'varname')
19:51:08 <bcoca> chainging them for 'regex'
19:51:38 <bcoca> lookup('vars', lookup('varnames', '^q_.*'))
19:52:08 <bcoca> add that to a 'zip' and you can have dict
19:52:17 <shertel> Also -1 because of the varnames lookup
19:52:25 <cyberpear> ok, seems a little complicated, but as long at it works...
19:53:14 <bcoca> cyberpear: even if we allowed the regex, i would be much against having 'mutable return types' on same lookup
19:53:28 <cyberpear> fair enough
19:53:33 <agaffney> cyberpear: it's a bit more complicated to do exactly what they are wanting, it's "bad" to overload the 'vars' lookup functionality like that
19:53:53 <bcoca> also looking forward to sibel's PR  vars(varnames('^q.*'))
19:53:54 <cyberpear> I haven't looked at varnames lookup, but I assume it supports regex? (or just pipe it to some other filters?)
19:54:02 <bcoca> yep
19:54:38 <bcoca> varnames('^q_.*)|zip(vars(varnames('^q.*')))
19:54:44 <bcoca> ^ there is your dict
19:54:54 * bcoca needs to test that
19:54:58 <cyberpear> sounds good... as long as the functionality of the PR is completely available with the vars + varnames lookups, I'm fine w/o this change
19:55:11 <felixfontein> is the order of the names returned by varnames() determinstic?
19:55:24 <bcoca> for a session, it should be
19:55:25 <cyberpear> I'd hope so, otherwise the above wouldn't work
19:56:00 <felixfontein> if we want to suggest this as an alternative, we must make sure that the order is deterministic in this usage
19:56:00 <bcoca> and you can always add sorted filter
19:56:24 <bcoca> felixfontein: in python, you are not guaranteed dictionary order
19:56:46 <cyberpear> would it be reasonable to have varnames lookup always sort?
19:57:10 <cyberpear> I assume the list returned by vars lookup is sorted according to the order of the args given it
19:57:24 <bcoca> so is varnames, you can pass multiple regexes
19:57:31 <felixfontein> bcoca: yep, though for a fixed dictionary two iterations will give the same order in the same program, if you don't change the dict inbetween
19:57:45 <bcoca> varnames('^q_.*', '^k_.*')
19:58:02 <bcoca> felixfontein: that is why i said 'yes, if in same session'
19:58:04 <cyberpear> would that give two lists, or a catenated list?
19:58:09 <bcoca> one list
19:58:28 <bcoca> lookup plugins are supposed to return a list
19:58:52 <bcoca> aslo makes it easier to 'meld' with other lookups/filters
19:59:09 <cyberpear> list or comma separated, I thought... (list if q() rather than lookup())
19:59:09 <felixfontein> bcoca: I think we should mention this in the docs then, so that  varnames('^q_.*)|zip(vars(varnames('^q.*')))  doesn't work because of an implementation detail (which could change), but because of explicitly documented behavior
19:59:45 <bcoca> i can add that later
20:00:17 <cyberpear> could you auto-sort each sublist prior to cat'ing them?
20:00:28 <cyberpear> that way it's deterministic?
20:00:44 <bcoca> we can add sorted option, i would not do by default
20:01:15 <bcoca> will be hard to match with return of 'vars' at that point unless you feed in exact same sort
20:02:09 <cyberpear> or maybe it's something like {% set myvar=lookup('varnames') %}{{ myvar|zip(vars(myvar))}}
20:02:54 <bcoca> k, on that note, im going to close this topic and lets try to do last one
20:02:54 <cyberpear> that way you only do the varnames lookup once, but I generally don't like to see {%..%} more than necessary
20:03:02 <cyberpear> k, thx
20:03:10 <bcoca> #topic https://github.com/ansible/ansible/pull/51149
20:03:24 <shertel> So, background: I have a new option for AWS modules in #49312 that will help determine the minimum permissions necessary to run tests for new modules to expedite the process of being able to run them in CI. After talking with @mattclay we decided to use module_defaults for the new argument in the test runner so all AWS tests use it. Since AWS tests often set module_defaults for credentials those values overwrite the test runner option.
20:03:51 <cyberpear> +1 on module_defaults merging ability :)
20:04:09 <bcoca> its not as much about adding the feature, but how its added i wanted to discuss
20:04:27 <agaffney> iirc, I implemented it without merging originally because merging made it really slow
20:04:36 <bcoca> its modifying a)default behaviour of an 'aggregate' keyword, to be diff than rest (vars, tags, environment, etc)
20:04:44 <agaffney> so before something like this is merged, I'd request some profiling
20:04:51 <bcoca> b) we should add this option to all agregate keywords
20:04:58 <shertel> Good idea, agaffney, I'll do that
20:05:18 <bcoca> and yep, we should check performance, which i would not stop the feature on, but woudl not make it default either
20:06:11 <bcoca> i'm also not sure we can address all these issues at 2 weeks before freeze
20:06:54 <shertel> Yes, maybe not. I could modify all the AWS tests to set the correct module_defaults as a workaround for the time being.
20:07:16 <bcoca> agaffney: can you provide tests/results?
20:07:39 <jtanner> or something we might add to ansible/ansible-baseline
20:08:24 <mattclay> shertel: Another option (for the test PR) would be to use an env var instead of module defaults, if we're not ready to make the necessary changes there.
20:08:48 <agaffney> bcoca: for what?
20:09:00 <jtanner> the "profiling"
20:09:01 <shertel> Yeah, that seems like a nicer alternative
20:09:37 <agaffney> I only even noticed the issue with the original implementation because some CI tests kept timing out. the added overhead of the merging for each task was enough to cause the tests to run longer than some background async process with a 20m timeout
20:09:44 <agaffney> I don't think I ever did any *real* profiling
20:10:16 <shertel> I like trying to break things :-)
20:10:37 <bcoca> i just break thngs, liking is not part of equation
20:11:23 <jtanner> we good here for now? (we have a wonderful internal meeting to triage the queue)
20:12:13 <shertel> I'll move forward with the environment variable solution for now, and bring this up again at the next meeting to try to get an idea of the right way forward
20:12:16 <bcoca> k, tableing for now, we can resume on thursday
20:12:26 <bcoca> #endmeeting