19:01:30 <bcoca> #startmeeting core public irc meeting
19:01:30 <zodbot> Meeting started Tue Jul  2 19:01:30 2019 UTC.
19:01:30 <zodbot> This meeting is logged and archived in a public location.
19:01:30 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:01:30 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:01:30 <zodbot> The meeting name has been set to 'core_public_irc_meeting'
19:01:34 <jillr> o/
19:02:03 <bcoca> #topic https://github.com/ansible/ansible/issues/58587
19:02:10 <bcoca> @agowa338
19:02:14 <nitzmahone> o/
19:02:18 <agowa338> hi
19:02:59 <agowa338> I'd like to talk about `Support omit within lists #58587`
19:03:59 <bcoca> make your case, as sivel mentioned in ticket this has been requested previouslly (though mostly for dicts)
19:04:01 <cyberpear> +1 to the feature. what happens when [omit] is passed?
19:04:18 <bcoca> cyberpear: nothign, since we dont scan all data values/keys for it
19:04:31 <bcoca> it would require writing it into templating and scanning all data structures
19:04:38 <nitzmahone> Today, it gets passed to the module, which knows nothing about it...
19:04:43 <felixfontein> hi!
19:04:47 <bcoca> while 'avoiding' creating the item/key in the first place is already possible and a lot cheaper
19:05:05 <bcoca> nitzmahone: no, the 'module args handler' takes care of it
19:05:09 <nitzmahone> I'd rather include a filter that zaps omits than have to scan everything for them by default
19:05:12 <bcoca> when module_option=omit_token
19:05:21 <nitzmahone> bcoca: I'm saying if you include one in a place we aren't looking for it
19:05:46 <bcoca> nitzmahone: we only look for it in one place ... the ask has been for 'many other places'
19:05:52 <bcoca> including playbook keywords
19:05:53 <nitzmahone> I know.
19:05:56 <felixfontein> I like the filter idea
19:06:08 <cyberpear> +1 to filter for prune_omit
19:06:10 <nitzmahone> I'm saying let's do it with a filter and let people opt into it rather than do it everywhere by default
19:06:14 <bcoca> filter will work on a list or dict, but not on a keyword
19:06:26 <nitzmahone> Keywords already work
19:06:29 <felixfontein> a filter is very efficient because you only need to filter what the user explicitly asks for, and the user has a good way to achieve this
19:06:35 <nitzmahone> Oh, you mean play keywords
19:06:41 <bcoca> nitzmahone: do we have others?
19:06:59 <nitzmahone> I thought you were talking about module args (the only thing that supports omit today)
19:07:01 <bcoca> afaik it currently only works on 'module options' .. unless you are calling those keywords
19:07:18 <nitzmahone> Nope, just missed what you originally meant
19:07:19 <bcoca> k, sorry for mixup, since 'args keyword' holds 'module options'
19:07:45 <cyberpear> omit would be very useful if supported on keywords in addition to module args but that's different, I think
19:08:00 <nitzmahone> keywords aside, a filter should cover all the other requests- we could implement on play keywords somewhat easier than other places, but is a separate decision IMO
19:08:13 <bcoca> actually, dont need new filter
19:08:17 <cyberpear> (like play and task level items)
19:08:21 <nitzmahone> True
19:08:26 <bcoca> mylist|reject('equals', omit)
19:08:31 <nitzmahone> yep
19:08:35 <bcoca> equals_to?
19:08:42 <nitzmahone> I think it's equal_to
19:08:55 <agowa338> but how would you use a filter in the example within the issue?
19:08:59 <nitzmahone> So if we *did* do another filter, it'd just be syntactic sugar for --^
19:09:10 <bcoca> agowa338: var that creats list, use list with |reject on consumption
19:09:15 <nitzmahone> Put what bcoca said at the end of your chain, it will zap the omits
19:09:38 <bcoca> nitzmahone: his example creates list directly on module option
19:09:42 <cyberpear> does it work at depth?
19:09:55 <nitzmahone> That's where you'd need a custom filter
19:10:21 <nitzmahone> ... to recurse- I don't think any of the default filters will recurse a structure
19:10:24 <bcoca> most filters dont recurse
19:12:19 <bcoca> https://github.com/ansible/ansible/issues/58587#issuecomment-507808422
19:12:51 <felixfontein> lol
19:12:53 <bcoca> felixfontein: slowpoke!
19:12:57 <felixfontein> 25 seconds too slow
19:13:50 <bcoca> so i think we are -1 on feature, but would be fine with a 'recursive' option for map/select/reject filters?
19:14:19 <agowa338> ok, that should do, but can what if that list is empty? But I see, maybe we should just add that to the documentation of omit?
19:14:20 <felixfontein> as long as the default for the option is false, sounds good to me
19:14:36 <bcoca> agowa338: when: mylist <= will skip if empty
19:14:37 <felixfontein> agowa338: then you pass an empty list to the module
19:14:58 <felixfontein> or do as bcoca said to avoid that :)
19:15:01 <nitzmahone> If the module can't handle that, it's arguably a bug in the module that should be easily fixed
19:15:14 <nitzmahone> "hey module, nothing to do!" ;)
19:15:33 <agowa338> I thought about parameters, like in the docker example with ports.
19:16:57 <agowa338> If |list returns an empty list, it should be fine, but if it returns none?
19:17:15 <nitzmahone> jump off that bridge if you come to it
19:17:22 <felixfontein> why should it return none?
19:17:25 <bcoca> https://github.com/ansible/ansible/issues/58587#issuecomment-507809934 <= or just avoid having 'omit' before consuming
19:17:42 <agowa338> felixfontein: That was the question, does it return return an empty list or none?
19:18:05 <bcoca> agowa338: you can control what it returns with |default(None or [] or ''  or somethingelse)
19:18:10 <felixfontein> I would be really disappointed if it returns something wihch is not a list, so it must be empty list
19:18:55 <bcoca> ^ see my example building list with set_fact+ loop
19:19:30 <felixfontein> bcoca: it will do surprising stuff when namestuff was already defined before
19:19:45 <bcoca> felixfontein: it can, so DONT DO THAT
19:20:01 <bcoca> or you can just use vars: namestuff=[] in block
19:20:14 <bcoca> or play/role/etc
19:20:30 <bcoca> why you should use specific var names that are not common e.g 'name'
19:20:37 <bcoca> win_f_names
19:20:51 <bcoca> but that is whole other topic
19:21:00 <agowa338> ok, thanks ;-) , that works for me. And with the recurse option everything that I currently think of should also work.
19:21:33 <bcoca> #topic https://github.com/ansible/community/issues/475#issuecomment-507736121
19:21:50 <bcoca> ^ our rules seem inconsistent about 'module docs'
19:22:13 <bcoca> for required/default we enforce removal if they have default values, but for type we have started enforcing they are required to be present
19:22:37 <bcoca> i would not force either, but that is just me, if we are going to enforce anything i think it should be the same standard, not contradicory ones depending on field
19:22:56 <bcoca> only thing i would enforce (which we already do) is doc type == arg spec type
19:23:00 <nitzmahone> I'd argue for consistency we should only require if it's not the default (eg, `type='str'` should go away)
19:23:19 <nitzmahone> but I also don't have strong feelings
19:23:26 <bcoca> do you want to enforc e'if its default it should not appear'?
19:23:28 <felixfontein> how about deprecating the default for type? then our current rules are consistent :)
19:24:08 <bcoca> felixfontein: that seems to try to pin the donkey on the tail
19:24:22 <nitzmahone> We don't like breaking stuff in the module API, and that'd definitely qualify...
19:24:33 <nitzmahone> We already broke it once with the switch from raw to str
19:24:38 <nitzmahone> (ish)
19:24:41 <cyberpear> +1 to removing type=str, +1 to don't include a default
19:24:55 <nitzmahone> -1 to breaking changes
19:25:06 <felixfontein> deprecating doesn't mean that we should break the API, I just mean that sanity tests will tell you "please specify type" if you don't specify it :)
19:25:27 <nitzmahone> I can live with that, but deprecation usually implies something that will be removed
19:25:39 <bcoca> so i see these options a) require all fields always b) require removal of any field that has default value c) just require fields are correct and match argspec
19:26:00 <sdoran> I would be fine omitting the requirement for `type='str'` from the docstring since it is the default.
19:26:00 <agowa338> +1 to don't include a default, one question, how does one specify multiple types? Or is that bad practice?
19:26:12 <cyberpear> (this is more coding standards)
19:26:13 <bcoca> agowa338: raw
19:26:19 <bcoca> and yes, bad practice
19:26:26 <sdoran> It'd be nice if we displayed the type with `ansible-doc foo`, though. Not sure if we do (me may).
19:26:36 <bcoca> raw == argpsec will ignore validation and module should do it itself
19:26:37 <sdoran> That just saves typing.
19:26:37 <nitzmahone> I think we display it if present
19:26:42 <felixfontein> I like having type always there. as opposed to having required and default always there.
19:26:47 <bcoca> sdoran: if its there, we display it
19:26:59 <cyberpear> there's lots of name=str or name=list
19:26:59 <felixfontein> we currently display "-" if no type is specified
19:27:08 <sdoran> If it's not there, we don't display the type?
19:27:32 <bcoca> we are missing 'insert default value' in many cases, we can add that, we didnt fix when default was 'raw'
19:27:54 <sdoran> Yeah, that's a minor thing we can change later. I would like it displayed, though.
19:27:55 <bcoca> so it made sense back then to not display type, but when we changed default to str, we should have changed display, but that is minor issue i can fix in a few mins
19:28:09 <sdoran> It's helpful info for authors.
19:28:11 <bcoca> sdoran: open issue and i'll probably get it done tmrow
19:28:11 <sdoran> Yup.
19:28:22 <sdoran> 👍
19:28:41 <bcoca> need to fix ansible-doc and plugin_formatter.py
19:28:49 <nitzmahone> We'd still have to *have* a default to avoid making it a breaking change, but can add a sanity test that always requires it to be specified for stuff we ship
19:29:06 <felixfontein> https://github.com/ansible/ansible/blob/devel/docs/bin/plugin_formatter.py#L155
19:29:12 <bcoca> nitzmahone: that is test we have now, why i brought up this issue since it contradicts other policies
19:29:37 <felixfontein> bcoca: we don't really have that test now
19:29:44 <felixfontein> I think
19:29:45 <bcoca> felixfontein: we do
19:29:56 <bcoca> i just hit it and was puzzled by it, hence this topic in meeting
19:30:00 <felixfontein> ah
19:30:01 <bcoca> since we normally had policity the other way around
19:30:14 <bcoca> e.g required: false< = we have tests that forces you to remove this
19:30:32 <sdoran> @nitzmahone  Do you mean a default type for the parameter if not specified in `argument_spec`?
19:30:33 <bcoca> i would actually remove both tests and let users document it even if its default
19:30:49 <sdoran> I'm pretty sure we do default to `str` in the code.
19:30:59 <bcoca> yes, i checked
19:31:03 <nitzmahone> Yeah, I guess we do, it's just ignored for a boatload of nonconformant existing modules.
19:31:05 <bcoca> it would make sense if default was still raw
19:31:10 <bcoca> but since wem oved to str ...
19:31:31 <bcoca> nitzmahone: yeah, would prefer not having a test that most existing modules violage
19:31:39 <felixfontein> bcoca: when you leave type away in argspec, but specify 'str' in docs, the sanity check won't complain
19:31:41 <nitzmahone> bcoca: we already do
19:31:51 <nitzmahone> It's validate-modules rule E338
19:32:16 <bcoca> nitzmahone: i know, that is my point, specially since we have other tests that do the opposite
19:32:19 <felixfontein> it will only complain (for type not in argspec) if you don't specify type in the docs, or when you have another type than str in the docs
19:32:39 <bcoca> felixfontein: it complains if type=str is missing in docs
19:32:49 <felixfontein> bcoca: yes, but not if it is missing in argspec
19:33:19 <bcoca> that would not matter since it should still be same, that seems like a capricious rule
19:33:42 <bcoca> we allow required:false in spec, but not in docs ... which i also find puzzling
19:35:25 <felixfontein> where do we prohibit (by a sanity check) that required:false is writtin in the docs?
19:35:36 <felixfontein> I can't find that in https://github.com/ansible/ansible/blob/devel/test/sanity/validate-modules/main.py
19:38:00 <bcoca> dont tknow its there, i know we did big purges to follow that 'rule'
19:38:50 <nitzmahone> https://github.com/ansible/ansible/blob/devel/test/sanity/validate-modules/main.py#L1193-L1199
19:39:22 <bcoca> having default means no required:true, that is diff issue
19:39:24 <nitzmahone> bcoca: you approved the PR that added it ;)
19:39:51 <nitzmahone> ah, you're right, that's a different one
19:40:10 <bcoca> that is 'conflicting' data and we SHOULD validate and reject that
19:40:17 <felixfontein> yep
19:40:28 <bcoca> vs 'redundant' data, which i really think we should not be policing
19:41:53 <bcoca> felixfontein: why do you want to force users to specify type (remove type=str as default) ?
19:41:59 <bcoca> s/user/authors/
19:42:28 <felixfontein> because I find that easier to read, when looking at an argspec
19:42:46 <bcoca> but knowing the defaults is simple, avoids a lot of extra work/data
19:42:58 <bcoca> why not force EVERY field in argspec then? not just type?
19:43:05 <bcoca> required, default, etc
19:43:14 <felixfontein> IMO type is the most important value (next to name), so being able to see it without having to think is good IMO
19:43:37 <felixfontein> because the others are not that important :) but yes, I see your point, and I'm somewhat torn...
19:43:43 <bcoca> if you are reviewing code and trying 'not to think' .. i dont think you should be reviewing code
19:44:05 <felixfontein> it's more when quickly going back to find out what type something has
19:44:18 <bcoca> its not like default changes per module
19:45:21 <felixfontein> I can also live without it being there, I'll just have to get used to that then ;)
19:46:11 <felixfontein> about forcing every field to be there: that makes it hard to read
19:46:45 <felixfontein> if that's the alternative to not having default values, I prefer not having default values
19:47:50 <nitzmahone> bcoca: so basically, you're proposing reverting https://github.com/ansible/ansible/pull/56534, right?
19:48:54 <bcoca> mostly
19:48:59 <bcoca> also the rule against required: false
19:49:04 <bcoca> and similar
19:49:19 <bcoca> i think our 'display' should take care of defaults, but if they are included in docs, its fine also
19:49:32 <bcoca> seems we are being overtly strict for no real gain
19:49:38 <nitzmahone> That one's already apparently not machine-enforced (though I thought it was)
19:50:20 <nitzmahone> I know dag was pretty vocally against the 56534 changes
19:50:55 <bcoca> well, im happy we dont enforce the others, but we can also just remove the rule (since we've had tons of PRs that just removed those fields)
19:51:15 <bcoca> ^ bcs tons, which are not metric, before you ask
19:52:04 <felixfontein> ah, when I see my last comment in that PR: do we want to allow that if an option has aliases, that the documentation uses an alias instead of the "main" option name?
19:52:33 <nitzmahone> overly nitpicky again IMO
19:52:46 <nitzmahone> (if we didn't allow it I mean)
19:52:55 <felixfontein> the main problem is that consistency for type etc. isn't checked in this case, I think
19:53:15 <bcoca> that would require validate-modules to handle aliases
19:53:17 <felixfontein> i.e. if we really want to allow it, argspec validation does need to be adjusted
19:53:34 <bcoca> i would agree with nitzmahone, but until someone adds that code, it should actually complain 'undocumented option'
19:53:47 <bcoca> and 'documentd option not in argspec'
19:54:26 <bcoca> ^ though we use those on purpose in a couple of cases, e.g use= on service (since action plugin uses it but module does not)
19:55:27 <bcoca> nitzmahone: im not sure i want to remove the PR .. more like tweak it to ignore missing type as long as it matches argspec (including default)
19:55:30 <felixfontein> I'm fine with allowing that, we should just make sure that the documentation uses the correct type etc. in this case
19:56:49 <bcoca> so we all more or less agree on making this less strict (match, but dont complain if extra/less info ?)
19:58:59 <bcoca> going to assume that is a yes, unless we want official vote?
19:59:34 <sdoran> Yes. I'm in favor of tweaking existing behavior to not be so strict.
19:59:51 <felixfontein> so remove the check for 338, and adjust 337 so that it only fires if argspec type is not str?
19:59:54 <sdoran> And I opened an issue about having `ansible-doc` automatically display the type if not specified in the docs.
20:00:12 <bcoca> felixfontein: yep
20:00:45 <bcoca> well, 337 goes away and becomes 325
20:02:31 <felixfontein> why not keep 337 and adjust it to only fire if argspec type is not str?
20:02:54 <felixfontein> 325 is explicit type conflict, where 337 is only "type isn't documented even though it is not default (str)"
20:03:31 <bcoca> k, i misread that, that is also a mismatch im ho
20:04:47 <felixfontein> if we change docs generation to display "str" in this case, yes
20:05:00 <bcoca> basucakkt 335 us sane as 325 but due to default
20:05:11 <bcoca> felixfontein: done already
20:05:13 <bcoca> and its 'string'
20:06:03 <felixfontein> yes, it displays 'string', but the source value is 'str' :)
20:06:21 <bcoca> explicit and implicit, both would work
20:06:38 <bcoca> +    if text == 'str' or isinstance(text, Undefined):
20:06:40 <bcoca> +        text = 'string'
20:06:58 <felixfontein> yes
20:08:02 <bcoca> the validate-modules check is also not right, if type is not set in argspec but set in docs, it wont check mismatch
20:08:19 <felixfontein> nitzmahone: how's work going on the return value deprecation?
20:08:48 <bcoca> felixfontein: its not return value, its 'variable deprecation' as we want to apply at many levels
20:09:02 <bcoca> ansible facts, some 'magic vars', etc
20:09:29 <bcoca> k, we can continue details in devel, for now im going to close meeting
20:09:30 <felixfontein> bcoca: it does check mismatch: if type is not in argspec, it checks whether doctype is None (E338), str (fine), or anything else (E335)
20:09:39 <bcoca> #endmeeting