19:09:00 <shertel> #startmeeting ansible core public irc meeting
19:09:00 <zodbot> Meeting started Tue Jun  2 19:09:00 2020 UTC.
19:09:00 <zodbot> This meeting is logged and archived in a public location.
19:09:00 <zodbot> The chair is shertel. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:09:00 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:09:00 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting'
19:09:06 <sdoran> o/
19:09:08 <felixfontein> :)
19:09:11 <shertel> hi, sorry about that
19:09:20 <shertel> #chair sdoran felixfontein cyberpear
19:09:20 <zodbot> Current chairs: cyberpear felixfontein sdoran shertel
19:09:22 <felixfontein> no problem :)
19:09:35 <felixfontein> I was afraid core team was double booked again ;)
19:09:45 <bcoca> some of us are ... or x4 booked ...
19:09:52 <shertel> #topic https://github.com/ansible/ansible/pull/69313
19:10:18 <shertel> what's the status of this one?
19:10:24 <felixfontein> oh right, it's still on the agenda
19:10:33 <shertel> oh, already been covered?
19:10:49 <felixfontein> I'm not sure, I hope mattclay or relrod can say
19:10:53 <bcoca> not sure that is core meeeting, but for RMs to deal with
19:10:59 <felixfontein> it was covered last thursday
19:11:21 <mattclay> felixfontein: I haven't had time to look at it again, but it's on my list to get to it.
19:11:42 <felixfontein> mattclay: ok, thanks!
19:11:43 <shertel> mattclay++
19:11:57 <shertel> #topic https://github.com/ansible/ansible/pull/69795
19:12:36 <felixfontein> I noticed yesterday that ansible-doc doesn't really know about suboptions
19:13:05 <cyberpear> yes! looks great!
19:13:07 <felixfontein> so I decided to try out to add basic support for it
19:13:09 <shertel> this looks good
19:13:17 <shertel> just waiting on a review?
19:13:47 <felixfontein> yes, and for general feedback whether this is wanted
19:14:06 <felixfontein> I'm also wondering what `spec` and `options` are. these were handled before. are these plugin specific?
19:14:11 <bcoca> felixfontein: i added to my list, yes, its something i've wanted but never added due to not wainting to deal with the recursion
19:14:25 <bcoca> felixfontein: not sure where spec came from
19:14:35 <bcoca> options are supposed to be only ones for plugins/modules
19:14:41 <cyberpear> looks like yours drops the required part, though?
19:14:47 <felixfontein> options is used in module argspec, and suboptions is used in module documentation
19:14:47 <shertel> #chair bcoca
19:14:47 <zodbot> Current chairs: bcoca cyberpear felixfontein sdoran shertel
19:15:06 <felixfontein> cyberpear: which required part?
19:15:06 <bcoca> felixfontein: options and suboptions should be used in both
19:15:30 * bcoca remembers meeting on the 'suboptions' name since it had 4 diff 'in docs implementations' before we added it to modules
19:15:40 <felixfontein> I think suboptions doesn't work for argspec, and options doesn't work for module docs
19:15:42 <cyberpear> felixfontein: in your example output, the "before" has "required" but the "after" doesnt' have "required"
19:15:47 <felixfontein> (at least HTML module docs)
19:15:53 <bcoca> felixfontein: options is 'required' in docs
19:16:10 <felixfontein> cyberpear: `=` is required, `-` non-required
19:16:13 <bcoca> suboptions was not, but if we are going to displaythem, it should be
19:16:18 <cyberpear> ah, sorry
19:16:22 <felixfontein> cyberpear: same as top-level options
19:16:25 <felixfontein> cyberpear: np :)
19:16:28 <bcoca> cyberpear: not required field but 'key'
19:16:34 <bcoca> felixfontein: confused now ...
19:17:04 <bcoca> there should be 2 keys. options: which is top level, and suboptions, which is possible only in an 'option'
19:17:15 <cyberpear> ignore me, felixfontein sufficiently clarified for me
19:17:25 <felixfontein> bcoca: https://github.com/ansible/ansible/blob/devel/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py#L417 vs https://github.com/ansible/ansible/blob/devel/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py#L266
19:17:38 <felixfontein> one is docs, the other argspec
19:17:57 <bcoca> felixfontein: dont care about argspec for asnible-doc, im ONLY discussing docs
19:18:01 <felixfontein> and for return values its called `contains`
19:18:02 <bcoca> argspec does not show in asnible-doc
19:18:20 <felixfontein> yes. but for module docs, it's always `suboptions`, never `options`
19:18:23 <bcoca> well .. that is my fault .. contains and 'complex' ... fine with sunsetting those
19:18:36 <bcoca> felixfontein: modules always have OPTIONS, its required, ansible-doc wont parse w/o it
19:18:39 <felixfontein> because validation will scream at you if you use `options` in module docs.
19:18:56 <bcoca> ... yet every module .. has options ....
19:19:12 <bcoca> so do all plugins
19:19:14 <felixfontein> ah, you mean the top-level options key
19:19:22 <bcoca> there is no other
19:19:22 <felixfontein> but that is not handled in that function
19:19:33 <bcoca> it is, its part you modify
19:19:38 <bcoca> its what calls add_fields
19:19:39 <felixfontein> argspec also `options`  where docs uses `suboptions`, that's what I'm saying
19:19:46 <bcoca> and you made recursive , which i have to check
19:19:52 <bcoca> forget argspec
19:20:02 <bcoca> it doesnt exist for this discussion, since its about asnible-doc
19:20:07 <bcoca> which cannot know about argsspec
19:20:20 <felixfontein> bcoca: top-level `options` is handled here: https://github.com/ansible/ansible/blob/2b4e08b53b3963e8f2cbfb4c0068071b8a32cb81/lib/ansible/cli/doc.py#L682
19:20:34 <felixfontein> add_fields never gets the top-level `options`, it only gets `suboptions`
19:20:50 <bcoca> if 'options' .. add_fields ...
19:20:56 <bcoca> how is that 'never gets it' ?
19:21:31 <felixfontein> it gets a dict which doesn't contain the key `options`, and none of the sub-dicts contained somewhere in it contain the key `options`
19:21:50 <felixfontein> so the `options` handling part of add_fields is never used (for correct module documentation)
19:22:30 <felixfontein> but doesn't matter in the end, I'm not proposing to remove `options` from https://github.com/ansible/ansible/blob/2b4e08b53b3963e8f2cbfb4c0068071b8a32cb81/lib/ansible/cli/doc.py#L582
19:22:56 <bcoca> ah, those never worked .. were place holders for suboptions .. but once implemented we never went back
19:23:02 <bcoca> i get what you are saying now
19:23:22 <felixfontein> ok :)
19:23:33 <bcoca> that was added to give 'examples' on how it would work on doc side, but nothing should be using, remove both, 'onl;y suboptions' should exist
19:24:03 <felixfontein> I'd like to leave `contains` in there for #69796 :)
19:24:04 <bcoca> .. or contains/complex now that you are adding returndocs
19:24:10 <felixfontein> but I can remove `spec` and `options`
19:24:35 <felixfontein> `contains` is already in that list. about `complex` I'm not sure, since it is pretty unstructured IIRC
19:24:46 <bcoca> they are the same thing
19:25:15 <bcoca> both were pretty unstructured .. i wass making it as i was going on documenting ...
19:25:19 <bcoca> making it up ...
19:25:47 <bcoca> was my first pass at implementing 'returned documenetation' .. back in the 1.x days ...
19:26:00 <felixfontein> `contains` is processed by HTML docs and I think its structure is enforced by validate-modules
19:26:23 <bcoca> someone ran with that .. and that is good
19:26:49 <bcoca> contains was initially only for 'lists' and complex for dicts with subdicts/lists
19:28:04 <felixfontein> ok. so should I remove `options` and `spec` from the list?
19:28:18 <felixfontein> or keep them for now?
19:29:18 <bcoca> i would survey usage in existing, im fine with removing, but if in use .. you might end up with for x in ('suboptions', 'options', 'spec', 'contains'): ....
19:29:50 <bcoca> though 'options' will be tricky since its tld adn required in all modules
19:30:03 <felixfontein> maybe we should just keep it then
19:30:18 <bcoca> afaik, networking is 90% of modules that use suboptions, vmware, docker and a few cloud ones to roudn it out
19:30:30 <bcoca> felixfontein: im fine, we can even just add deprecation msg
19:30:34 <shertel> I'd be fine with just keeping it
19:31:01 * shertel doesn't really have a preference
19:31:06 <felixfontein> bcoca: it would be better if `ansible-test sanity` flags such usage than ansible-doc
19:31:30 <bcoca> well, 1/2 of that predates validate-modules .. probably why it never made it it
19:31:32 <bcoca> it in
19:31:41 <bcoca> im suprised 'contains' is in there
19:31:46 <felixfontein> also validate-modules doesn't validate plugins
19:31:51 <felixfontein> I added contains (next to suboptions) :)
19:31:52 <shertel> it wasn't, previously
19:32:04 <bcoca> that is on my list, need to nag sivel about it a bit more
19:32:25 <bcoca> basically 'ansible-doc' is our 'plugin doc validtor' .. but it is pretty lax
19:32:27 <felixfontein> abadger1999 is producing schema validation for all plugin types, that will probably end up in ansible-test eventually
19:32:56 <bcoca> tried using jsonschema in past, but we end up with 'ANY' for 1/2 the stuff
19:33:09 <felixfontein> :)
19:33:19 <bcoca> adn it does not handle recursion well (or did, have not tried in 1yr)
19:34:08 <bcoca> in any case ansible-doc should be permissive when possible (unkown stuff) to allow for 3rd party to have custom doc sections/additional docs
19:34:43 <felixfontein> so +1 for keeping `options` and `spec`?
19:34:50 <bcoca> +0
19:35:07 <bcoca> +1 if they are actually 'in use' in the migrated content
19:35:49 <felixfontein> I'm +1 for getting plugin docs validation into ansible-test first and then deprecating these
19:36:00 <shertel> +1
19:36:02 <bcoca> yep
19:36:25 <bcoca> but want to get 'templatable plugin config' and 'docs for tests/filters' done first
19:36:36 * bcoca has long list
19:38:50 <shertel> felixfontein: is there anything else to address for that one?
19:39:04 <felixfontein> shertel: I think not
19:39:11 <shertel> #topic https://github.com/ansible/ansible/pull/69796
19:39:58 <felixfontein> this one is a bit more complex.
19:40:30 <felixfontein> I wanted to add version tagging for return values, and noticed that the RETURN docs are not parsed (unlike DOCUMENTATION).
19:41:13 <felixfontein> ansible-doc and plugin_formatter decode the YAML when needed, or just ignore it when it cannot be decoded
19:41:23 <bcoca> felixfontein: they are but not in same stages
19:41:46 <bcoca> and not in same way, since for long time 'return docs' was pretty undefined, but that seems to have changed a lot
19:42:34 <bcoca> i took quick scan on your PR and it looked ok, there are some upcomming changes that will probalby make some of the code obsolete though
19:42:36 <felixfontein> so how about trying to parse return docs as well at the same place where documentation is parsed?
19:42:45 <felixfontein> yes, removal of plugin_formatter
19:42:45 <bcoca> mostly the ast parsing will be 'fallback'
19:42:53 <felixfontein> ah, so more
19:43:04 <bcoca> plugin formatter is just for the docs building
19:43:13 <bcoca> my changes have more to do with how docs get read
19:43:24 <bcoca> also adding test/filter plugins if i can
19:43:48 <felixfontein> ah, that would be awesome!
19:46:14 <shertel> bcoca: are the upcoming changes in a open PR?
19:46:42 <bcoca> some, not all
19:47:33 <bcoca> actually .. i think i forgot to post .... mostly the 'big change' is something we already do in plugin_loader to get configs, but move that to plugdocs
19:47:53 <bcoca> read 'loaded_module.DOCUMENTATION' vs doing ast parse
19:47:56 <felixfontein> yep, that's one thing I noticed, that plugin_loader is apparently getting the docs from somewhere else
19:47:57 * shertel stops looking for it
19:48:18 <felixfontein> which is ok if you load the python module anyway
19:48:21 <bcoca> felixfontein: same 'source' but diff method, much faster/clearner .. and what i want to implement as 'main way' of reading all docs
19:48:33 <bcoca> felixfontein: which we always do
19:48:51 <bcoca> we dont execute modules, but we still read the whole thing into memory as a python lib
19:48:55 <felixfontein> bcoca: it is not that clean, as it executes code
19:49:10 <felixfontein> and that introduced bugs
19:49:19 <felixfontein> at least when the code was executed
19:49:36 <felixfontein> I think it no longer does, since that was fixed in nitzmahone's routing PR
19:49:41 <bcoca> any execute on load, was alwasy happening
19:49:54 <felixfontein> not for modules
19:49:55 <bcoca> dont think that has changed, at least not for plugin_loader
19:50:24 <felixfontein> it changed for the collection loader/finder
19:50:57 <bcoca> that would have broken plugins
19:51:10 <bcoca> as we rely on that to initiallyze config .. which is how they take options
19:52:19 <felixfontein> the problem happened with modules, when you first use pip to upgrade cryptography, and then try to run the module locally in the same ansible-playbook run IIRC
19:52:32 <felixfontein> it didn't happen in ansible/ansible, only in collection
19:52:39 <felixfontein> but that's fixed now
19:52:42 <bcoca> modules are not read as imports, they are located
19:52:50 <bcoca> modules are read as 'plain files'
19:53:10 <felixfontein> that wasn't the case, at least until the routing PR was merged
19:53:20 <felixfontein> then modules from collections were more than lodaed as plain files
19:53:22 <bcoca> afaik, it was always the case
19:53:42 <bcoca> ah, collections might have created that issue, but not the plugin_loader
19:53:46 <felixfontein> ansible 2.9's collection loader still does it
19:53:49 <felixfontein> yep
19:53:55 <felixfontein> it's the collection loader/finder
19:53:55 <bcoca> its always been the case there, modules and module_utils 'get located' but not imported
19:54:19 <bcoca> k, not counting that , specially 2.9/2.8 versions, that was 'pre alpha' imho
19:54:44 <felixfontein> collections in 2.9 work generally pretty good, I think
19:55:15 <bcoca> minimal funcions are there and seem good, but i would not call that 'working' as a lot is missing
19:55:33 <bcoca> still stuff will be missing from 2.10
19:55:55 <bcoca> but i would consider the 'expected' stuff to be at beta stage
19:55:59 <felixfontein> true. but with 2.9 you can already do quite some productive work
19:56:11 <bcoca> even in 2.8 ... just a lot more lmited
19:56:30 <shertel> yeah, < 2.10 is sort of preview in my mind
19:56:33 <bcoca> i prefer small incremental and USABLE updates, but more frequent
19:56:33 <felixfontein> I've never tried 2.8's collections
19:57:03 <bcoca> i think 2.9 - 2.10 should have had 2-3 releases adding features to collections .. before the split .. but that is  just MHO
19:57:20 <bcoca> all this docs stuff should have been finalized well before the exodus
19:57:22 <felixfontein> I agree on that... would have made the split less messy
19:57:49 <felixfontein> but it's too late now :) the split happened, so we have to try to make the best out of it
19:58:16 <bcoca> only reason it  is bearable was cause webknjaz and mkrizek did a great job in migration project .. .othewise  .. i expect we woudl still have import errors in 1/2 the migrated plugins
19:59:25 <shertel> felixfontein: noticed you had a specific question for that topic - "Q: make invalid return YAML also an error, or soft-fail?"
19:59:56 <felixfontein> shertel: good point. during the discussion I started assuming that being more conservative (i.e. don't error on something that wasn't an error before) is probably better
20:00:08 <felixfontein> I don't really like the idea of returning string or parsed YAML though
20:00:11 <shertel> yeah, that's what I'd think too
20:02:13 <felixfontein> what does core team think about erroring when the YAML cannot be parsed? or at least spitting out a warning?
20:02:41 <felixfontein> this code is "only" used in three cases: a) ansible-doc, b) help in ansible-console, c) plugin_formatter
20:03:04 <felixfontein> (and thus indirectly in `ansible-test sanity`, which has a ansible-doc test)
20:03:40 <bcoca> which yaml?
20:03:55 <shertel> I'd be fine with spitting out a warning
20:03:56 <bcoca> some things, like examples is NOT required to be yaml, it can be 'free form text'
20:03:59 <felixfontein> if we make it fail, help will stop working for broken custom plugins, but "nothing else" - and ansible-test will scream if plugin return docs are not valid yaml
20:04:03 <felixfontein> bcoca: return docs
20:04:22 <bcoca> that shoudl be yaml, same as documentation , so those are fine to get fatal error
20:04:33 <bcoca> same with metadata .. but im removing that ...
20:05:49 <felixfontein> I don't want to modify the example parsing
20:06:12 <bcoca> that should stay 'optimistic' parse yaml if possible, use string otherwise
20:06:16 <felixfontein> shertel: disadvantage of a warning is that ansible-test won't complain for invalid YAML in return docs for plugins
20:06:24 <sdoran> I don't think it should fail. A warning may be ok, but could be really noisy. Especially since we haven't historically enforce much on their content.
20:06:26 <bcoca> cause some things, like ini inventory shows a 'ini file' as example
20:06:34 <felixfontein> return docs currently are not parsed anywhere I think
20:06:40 <felixfontein> ah sorry
20:06:41 <felixfontein> examples
20:06:42 <bcoca> they are parsed in ansible-doc
20:06:44 <felixfontein> not return docs
20:06:48 <bcoca> both are
20:07:05 <felixfontein> sdoran: we enforce it for modules, but not for plugins
20:07:09 <bcoca> well, theya re 'read' in utils/plugin_docs, then ansible-docs tries to use yaml or string to display
20:07:30 <bcoca> plugins are not required to have return docs (mostly only lookups actually 'return stuff')
20:07:36 <felixfontein> though I think the docs build might have caught unparsable return docs YAML for plugins in the past - but that isn't running for collections
20:10:29 <felixfontein> ok. so a) warning on invalid return docs YAML, b) failure on invalid return docs YAML, c) silently ignore errors
20:10:39 <felixfontein> right now it looks like most people tend to a)?
20:11:00 <felixfontein> (so all code further down the chain has to be able to deal both with parsed YAML and a string)
20:11:13 <shertel> Yeah, though we might want to wait to revisit when we have more of a quorum
20:12:07 <felixfontein> ok. I can adjust the PR to print a warning, then we can discuss it next time.
20:12:17 <shertel> Okay, sounds good to me.
20:12:25 <felixfontein> if #69795 gets merged I have to adjust it anyway, since it changes the output for #69796's test
20:12:49 * shertel nods
20:14:01 <shertel> ok, I guess that's it for today
20:14:08 <felixfontein> cool. thanks everyone!
20:14:16 <shertel> thanks for coming!
20:14:19 <shertel> #endmeeting