15:01:26 <bcoca> #startmeeting ansible core public irc meeting
15:01:26 <zodbot> Meeting started Thu Jun 11 15:01:26 2020 UTC.
15:01:26 <zodbot> This meeting is logged and archived in a public location.
15:01:26 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:01:26 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
15:01:26 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting'
15:01:57 <bcoca> #topic https://github.com/ansible/ansible/pull/69997
15:02:33 <cyberpear> so this makes the changes discussed previously -- specifically making it a separate to be consistent return type all the time
15:03:09 <cyberpear> (leaving untouched existing filter, including its return-type discrepancies)
15:03:22 <shertel> \o
15:03:24 <bcoca> in any case, new filter should go into collection
15:03:25 <jtanner> hi
15:03:27 <sdoran> o/
15:03:34 <felixfontein> hi everyone :)
15:04:27 <mkrizek> o/
15:06:06 <felixfontein> that PR looks good to me. no idea whether ansible-base wants to have new filters though
15:06:33 <cyberpear> shouldn't the existing filter from which this was copied also be moved to relevant collection?
15:06:55 <cyberpear> it's an exact copy/paste with a tiny change so that a dict is returned instead of a string or list
15:07:11 <cyberpear> if someone changes one, will they know to go over to xyz collection and change the other the same way?
15:07:58 <bcoca> we are routing new plugins to collecitons,  unsure if having similarities woudl be a reason to move those that we chose to keep in base
15:08:14 <bcoca> nor accept new ones in base on same criteria
15:08:28 <cyberpear> previous PR for referenc: https://github.com/ansible/ansible/pull/64474
15:09:19 <bcoca> that policy didnt exist in nov
15:09:22 <cyberpear> discussion from last time was to make it a newly-named filter vs modifying the existing one due to return type considerations
15:09:47 <cyberpear> previous discussion was a week ago... I'll find the link if it's helpful
15:09:51 <bcoca> yes, sorry if it was not clear that new plugins shoudl go into collections
15:10:04 <bcoca> i had assumed everyone was aware of that by now
15:11:15 <cyberpear> can we move the existing `regex_{search,findall}` to the same collection so it's all in the same place?
15:11:29 <cyberpear> and which collection is appropriate?
15:12:23 <bcoca> i would not move out existing as it presents a minimal set already
15:12:44 <bcoca> as for 'which one' ...community.general is the dumping ground if you dont find a better fit
15:13:10 <bcoca> core is not handling the collections, just base at this point
15:13:19 <cyberpear> why is core an appropriate place for the existing versions but not for the enhanced version?
15:13:42 <felixfontein> because someone decided that the existing ones are part of a minimal set of things ansible should have
15:13:47 <bcoca> as i said, the policiy is to route plugins to collections
15:13:50 <felixfontein> (or that are Supported with capital s)
15:13:56 <bcoca> 'new' plugins
15:14:18 <bcoca> and yes, it has to do with what 'core' team will support, among other things
15:14:49 <cyberpear> it was never meant to be a new filter as-such, only an extra option to the existing ones, but due to return type, it has to go to a collection?
15:15:51 <bcoca> basicaly, we are not accepting the change to the existing filter, so yes, it has to be a new one and new ones would go to a collection instead of core
15:17:37 <sdoran> Adding a parameter to a function that changes its return type is generally a bad interface, which is why this needs to be a separate function, which makes it a new filter, which means it should go into a collection since we are not accepting new filters at this time.
15:18:10 <sdoran> That's an unfortunate chain of events since this is, on the surface, a minor change to an existing function.
15:18:36 <sdoran> And it _could_ be implemented as a toggle that changes the output. But that is a bad interface since it can cause unexpected things for the caller.
15:19:36 <cyberpear> how about solving the return type by making it return a string or a list of strings, like the existing ones, but json-embedded in the string?
15:19:55 <cyberpear> so it returns the expected  type but folks who want to can `from_json` on it
15:20:07 <felixfontein> that sounds like an even worse interface
15:20:13 <sdoran> Yeah...
15:20:40 <bcoca> at this point i want to deprecate the old filter and create a more consistent substitute, so any modifications onn it are probably not going far
15:20:54 <abadger1999> sdoran: you could do the opposite.
15:21:10 <abadger1999> Maintain the new filter (if it's a better interface) and kick the old one out to community.general.
15:21:17 <cyberpear> bcoca: which collection will hold the new version?
15:21:24 <bcoca> tbd
15:21:38 <felixfontein> I guess depends on whether it's Supported or not
15:22:03 <abadger1999> If they're both equally useful, though, then that switcheroo probably isn't the way to go.
15:23:48 <cyberpear> they're probably equally useful, but the interface for the more advanced features of the existing filters is awkward
15:24:23 <sdoran> I would say it's less common to have group names to return, but useful when you need it.
15:24:35 <sdoran> They seem both equally usefuly.
15:24:46 <sdoran> s/usefuly/useful
15:24:58 <cyberpear> yes, the new version is most helpful anytime you need to parse multiple items from a single command output
15:25:19 <bcoca> i woudl still not add new one to base
15:25:45 <cyberpear> just used it today to parse output of `net -S example.com ads lookup` command
15:27:52 <cyberpear> supposing it's added to community.general, would folks getting `pip install ansible` be able to use it as 'regex_search_groupdict` or would they have to use fqcn?
15:28:04 <sdoran> fqcn
15:28:05 <felixfontein> they would have to use FQCN
15:28:13 <sdoran> Is that a bad thing?
15:28:20 <cyberpear> yes
15:28:22 <sdoran> Being more aware of where the plugin lives?
15:28:43 <bcoca> no, more typing .. most people dont like the fqcn cause its more typing
15:28:52 <felixfontein> it's more to type, but I guess it's used in situation where things are more complex anyway, so I guess it would be acceptable
15:29:56 <bcoca> one reason of routing plugins to collections is to avoid people trying to push to core to avoid 'needing fqcn'
15:30:03 <cyberpear> I'd be fine moving to community.general if we can also move the existing versions so they can live in the same place
15:30:03 <bcoca> otherwise all pugins end up back in core
15:30:09 <jtanner> not everything is going to end up in runtime/routing.yml ... people are going to have to learn FQCNs at some point.
15:30:18 <bcoca> cyberpear: except existing versions are suported and dont belong in c.general
15:34:26 <felixfontein> ok. should we maybe continue with other topics?
15:35:03 <cyberpear> it's technically possible to make the thing work today, but takes about 25 lines of regex_replace mucking: https://github.com/MindPointGroup/RHEL7-STIG/blob/d0b128cef334374257a7f85d1fcb25b0653f64e0/tasks/parse_etc_passwd.yml#L10-L28
15:35:41 <bcoca> how does that affect the existing PR?
15:36:11 <cyberpear> anyone willing to make an exception for `regex_search_groupdict` into core?
15:37:08 <bcoca> -1
15:38:09 <bcoca> anyone else?
15:38:11 <sdoran> Not at this time. Sorry. -1
15:38:23 <cyberpear> okay, let's move onto felixfontein topics
15:38:40 <bcoca> #topic https://github.com/ansible/ansible/pull/69796
15:38:55 <felixfontein> ok. this is about whether FQCNs should be used in DOCUMENTATION of plugins
15:39:26 <felixfontein> validate-modules doesn't allow it for modules, and the migration script didn't add it for plugins, so most (all?) modules and plugins in collections don't use it
15:39:31 <bcoca> yes, they shoudl use FQCN for 2 reasons, a) some plugin types require name match b) that is name users are going to use
15:39:56 <felixfontein> bcoca: where is that name match required? the only code I could find that actually uses the field is ansible-doc, when formatting the man page
15:39:59 <bcoca> felixfontein: x2 check, migration did have to add to many
15:40:11 <bcoca> ah, but not in docs, in 'name' property
15:40:22 <bcoca> ro 'transport' in case of connection plugins
15:40:22 <felixfontein> that's a completely different property
15:40:36 <bcoca> yep, but you should use fqcn in  name, just casue that is what people need to use
15:40:40 <felixfontein> there it must be FQCN to work
15:40:50 * cyberpear I'd not use it in the docs source, but maybe have the generator fill in the FQCN based on which it's actually in?
15:41:09 <felixfontein> but it is not what currently is there, and there's no sanity check which will enforce it (and for modules a sanity check explicitly disallows it)
15:41:29 <bcoca> for modules in core, makes sense, for those in collections .. not much sense
15:41:38 <bcoca> i woudl argue the check needs to be updated
15:42:57 <felixfontein> I don't see why it should be, because it's just another mention of the collection name that is not required because the code handling the documentation already knows exactly of which collection this is part of
15:43:24 <felixfontein> (too many becauses in one sentence... sorry)
15:44:48 <bcoca> then require the code to always display the 'composed name'
15:44:50 <felixfontein> I don't care about whether I remove the FQCN in that PR or not (changing it back is not much work), but it looks like this is really not defined and core team needs to decide what they want
15:46:10 <bcoca> well, user needs to see FQCN for those things not in base (2.9 compat moved to collections can go either way asnible_builtin_runtime function is still being built/designed)
15:47:07 <felixfontein> true. but that's a display thing. if you require plugins in collections to use it, but there's no sanity check, probably nobody will, and the situation is a lot worse for users because in most cases it won't be shown in ansible-doc because the plugin uses the shortname in its docs
15:47:11 <abadger1999> cyberpear: That's the strategy I've adopted.  It's going to be hard to determine the allowed shortnames otherwise.
15:47:49 <bcoca> felixfontein: i dont disagree, my opinino is a) add sanity check b) use fqcn
15:48:00 <bcoca> makes display simpler and less error prone to have to compose
15:48:11 <cyberpear> when a plugin inevitably moves or a collection is renamed, those FQCNs in the docs will have to be updated manually if they aren't auto-generated
15:48:27 <felixfontein> ok, does anyone plan to add a sanity check for plugin docs before freeze?
15:48:34 <bcoca> moving them requires more manual work than just docs
15:48:49 <bcoca> felixfontein: i would not say 'before freeze' but yes before release
15:48:53 <bcoca> i would not block beta on this
15:49:38 <cyberpear> I think docs changes have historically been not beta-blocking, but I guess now we're talking about more complicated docs features than it had been in the past...
15:49:42 * acozine starts reading the backscroll
15:49:50 <bcoca> not even doc change, sanity test ...
15:50:10 <felixfontein> well, it would be a change that probably ALL collections have to make for all their plugins
15:50:19 <abadger1999> cyberpear: yeah, that's another good point.
15:50:24 <felixfontein> that's a bit much for after freeze
15:51:42 <felixfontein> is it possible to get a decision from core team about this?
15:52:01 <bcoca> waiting for rest to weigh in, i already given my view
15:53:48 <felixfontein> shertel: jtanner: sdoran: mkrizek:
15:53:59 <bcoca> jimi|ansible nitzmahone sivel
15:54:11 <bcoca> webknjaz
15:54:38 <acozine> I'm not sure I've followed the discussion .. . are we waiting to merge PR 69796 because of the cleanup in collections it would require?
15:54:52 <bcoca> no, diff issue
15:55:15 <felixfontein> I can undo that part of the PR if it leads to a quicker merge
15:55:21 <bcoca> its changing names in tests, i asked to not do so, brought up what policy are we following for names
15:55:36 <felixfontein> but the FQCN topic is I think important for the whole of NWO
15:55:38 <jimi|ansible> i don't think it needs to block beta, things won't move that much so adding the sanity check should just be done before release
15:55:46 <nitzmahone> ditto
15:56:07 <felixfontein> but should FQCNs be there after all? right now, they aren't (at least outside of ansible/ansible)
15:56:30 <bcoca> felixfontein: the tests are from 'external collections' so that is why i asked for fqcn
15:56:31 <acozine> we don't want to merge it now because it changes the way tests refer to modules?
15:56:52 <acozine> we are still talking about 69796, right?
15:57:37 <bcoca> acozine: that part of the PR is not a big issue, it just brought up a deeper issue into question
15:58:26 <acozine> bcoca: thanks, I'm trying to understand what the deeper issue is, and what our chances are of addressing it between the beta and the full release
15:58:28 <felixfontein> bcoca: regarding that PR, can we merge it if I undo that part of the PR?
15:58:34 <bcoca> felixfontein: we did intend the docs to be udpated in migration, but ran out of time and made notes that manual updates were needed, this seems to be something that was ignored
15:58:45 <bcoca> felixfontein: yep
15:59:21 <bcoca> we can update that again in PR that does compose (or not, depending on what we decide in the end)
15:59:55 <samccann> can someone summarize the 'bigger issue' here?  Is it that validate-modules requires shortname, but plugins 'should' use FQCN... but they don't today?
16:00:06 <bcoca> consider that ansible-doc -l is based on 'invocation name' while the ansible-doc <name> shows the 'name' inside the file
16:00:16 <bcoca> samccann: basicaly
16:00:16 <felixfontein> samccann: pretty much yes
16:00:17 <shertel> The question is whether we should require/have a sanity test for the DOCUMENTATION to contain the FQCN of the resource instead, if I understand correctly
16:00:45 <bcoca> or should we 'autocompose' the fqcn on display (ansible-doc/website generation)
16:00:46 <felixfontein> shertel: would be nice of the rest of the docs are also validated, but that's probably the most pressing issue :)
16:01:11 <bcoca> im fine either way (do slightly prefer fqcn in docs to avoid compose step)
16:01:14 <felixfontein> abadger1999 mentioned that his code already assumes that it is the shortname
16:01:43 <shertel> I'd prefer autocomposing
16:01:43 <abadger1999> jimi|ansible: Uh... a sanity check should not be added after beta.
16:01:47 <bcoca> that might not be correct, specially for plugins not in ansible_builtin_runtime.yml (as i said, that is grey area)
16:01:55 <abadger1999> jimi|ansible: because suddenly new things start breaking.
16:01:56 <sdoran> I think from the user perspective, always presenting FQCN in the docs removes ambiguity.
16:02:17 <abadger1999> (and mattclay has said that no new tests will go in post-beta)
16:02:19 <sdoran> But we need to take into account what @abadger1999's need for shortnames is.
16:02:35 * abadger1999 is in two meetings... sorry /me catching up here
16:02:37 <bcoca> time is up, current vote favors updating sanity check but not blocking beta for it
16:02:39 <jimi|ansible> abadger1999: this is a sanity check for moving things between collections and making sure the docs are updated, that's not something that's going to be hit often and if it is it's a docs issue
16:03:24 <felixfontein> bcoca: btw, the vars plugin didn't had a FQCN before my PR. I've now added it. should I keep or not keep that?
16:03:41 <samccann> bcoca: updating sanity check to what?
16:03:47 <bcoca> felixfontein: i'll look, probably they should
16:03:54 <bcoca> samccann: to ensure collection plugins use fqcn in docs
16:04:36 <felixfontein> jimi|ansible: it is one that would affect every single plugin out there though (and every single module, if the same is required for modules)
16:05:10 <abadger1999> jimi|ansible: but if you push that in just before ansible-base releases, it will break all collections just prior to ansible-2.10 release which includes those collections.
16:05:13 <samccann> bcoca: and by plugins, do you mean all plugin types, or all plugins except for modules?
16:05:21 <bcoca> all 'documentable' plugins
16:05:30 <abadger1999> So it definitely has to go in before beta.
16:05:38 <bcoca> including modules, but i think this is putting cart before horse
16:05:52 <bcoca> i would want to resolve how we deal with 'builtin runtime' names first
16:05:54 <samccann> bcoca: so then we would be changing modules as well from shortname to FQCN?
16:06:12 <felixfontein> for modules it should be noted that currently, the sanity tests explicitly FORBID a FQCN there. also the sanity tests for 2.9 do that.
16:06:16 <bcoca> samccann: that is already what we require from users, presenting short name would be confusing
16:06:52 <bcoca> felixfontein: again, tests were made for core .. they need to be udpated to 'collection world' even if we dont force fqcn .. that test does not seem wise to apply to collections
16:07:25 <felixfontein> bcoca: true, but there are some collections who also support 2.9, and also 2.9's sanity checks
16:08:17 <bcoca> and the more features we add to collections, the harder it is for same code to support multiplle versions
16:08:47 <bcoca> ^ i have been pointing this out very vocally , but things are becomeing more incompatible eveyr day, why i think being lenient is better approach
16:09:04 <felixfontein> the 2.9 check could be adjusted to accept both FQCN and shortname
16:09:10 <bcoca> yep
16:09:34 <bcoca> we have already been 'backporting' a lot of colleciton related compat to 2.9
16:09:53 <bcoca> at this point i question if 2.9.x wont be 2.10 in disguise
16:10:17 <felixfontein> as long as routing isn't backported, definitely not :)
16:10:22 <acozine> heh
16:10:34 <cyberpear> I think in the real world you're going to have 2 behaviors: 1) folks will stay on 2.9 "forever" because of collections problems and 2) collections will only work very well with 2.10 and newer
16:10:39 <bcoca> felixfontein: 2 days ago that came up ....
16:10:48 <nitzmahone> I can't figure it out even after reading the backscroll- what exactly are we trying to require FQCNs for in docs?
16:10:54 <bcoca> cyberpear: yes, that has been my fear since start
16:11:15 <felixfontein> nitzmahone: `<plugin_type>: <plugin_name>` in DOCUMENTATION of plugins/modules
16:11:31 <felixfontein> right now <plugin_name> is a shortname, except for some plugins in ansible/ansible collection tests
16:11:32 <bcoca> nitzmahone: cause if doc says 'stuff' is name for plugin, it trips people when they have to use ns.coll.stuff instead of 'stuff'
16:11:40 <felixfontein> (and maybe some in the wild, but all I checked were using shortname)
16:11:47 <sivel> collections in 2.8 were alpha, 2.9 were beta, 2.10 ????
16:11:53 <felixfontein> sivel: beta2 ;)
16:11:55 <bcoca> tech previuew
16:11:57 <nitzmahone> lol
16:12:11 <sivel> I fundamentally believe that people shouldn't be trying to support multiple ansible versions in a single collection yet
16:12:20 <sivel> should just be the most recent
16:12:22 <bcoca> agreed
16:12:24 <felixfontein> most people don't look at DOCUMENTATION in the plugin/module's source code, but via ansible-doc of the HTML docs
16:12:32 <felixfontein> so IMO it's enough if there the correct complete name is shown
16:12:45 <felixfontein> sivel: right now the most recent is 2.9
16:12:55 <samccann> sivel: unfortunately a batch of collections are releasing, I think end of next week, that will support 2.9 and 2.10
16:13:09 <bcoca> felixfontein: why im fine with either beingh in source or always composed, i favor source cause not everyone uses ansible-doc/docsite to view (but tehy are on their own) also, composing is more fragil ethan presenting label
16:13:12 <nitzmahone> I'd tend to agree- I dislike adding things that require collection content to know its own name, and this is a case where at runtime/from code, we know it and can display it any way we like
16:13:16 <bcoca> so i have 'SLIGHT' pref for it that way
16:13:28 <mattclay> nitzmahone: +1
16:13:55 * shertel also was a +1 for that
16:14:08 <abadger1999> ansible-doc --json  already provides FQCN.  So it's  redundant to have the name field with fqcn as well.  In addition, the shortname is a thing that's needed in some circumstances.  but calculating it from the fqcn is not necessarily easy.  community.general.databases.mysql.mysql_user => shortname is `databases.mysql.mysql_user`    ;; `databases.mysql.mysql_user` => shortname is still `databases.mysql.mysql_user`
16:14:12 <bcoca> nitzmahone: the issue is that we are NOT presenting fqcn (when not in the doc entry)
16:14:16 <nitzmahone> So making a huge last-minute shuffle to change something everywhere (even if it wasn't last minute, it's DRY principle IMO)
16:15:04 <mattclay> bcoca: Can you clarify what you mean by "when not in the doc entry"?
16:15:08 <bcoca> abadger1999: we use fqcn on list, not on presenting the document itself (cause it comes from file not doc)
16:15:12 <nitzmahone> bcoca: I'm not worried about people who are reading the code, and any tool that's pre-chewing the doc entry can/should include the collection context
16:15:19 <bcoca> anssible-doc -l vs ansilbe-doc <stuff>
16:15:29 <felixfontein> I'd say that's a bug in ansible-doc man output
16:15:33 <nitzmahone> ditto
16:15:34 <bcoca> nitzmahone: im worried about people reading our docs
16:15:51 <bcoca> felixfontein: its also an issue with the docsite
16:16:00 <felixfontein> and fixing that one is a lot easier than a) adding a sanity check for validating plugin docs, and b) making the change in all collections
16:16:09 <bcoca> why im saying there are TWO solutions a) require fqcn in name field b) compose name on output
16:16:11 <nitzmahone> but those are both something we can fix without changing every piece of source everywhere to include a redundant piece of info
16:16:22 <mattclay> bcoca: But can't both of those be fixed by how we present/generate the docs -- without changing the source of those docs to include the FQCN?
16:16:24 <bcoca> if a) .. that requires sannity check
16:16:34 <nitzmahone> (and changing everything)
16:16:36 <felixfontein> bcoca: I think the docs pipeline is already showing the FQCN without needing the FQCN in that field
16:16:38 <bcoca> mattclay: read solution b)
16:16:47 <abadger1999> bcoca: that's incorrect... let me pastebin.
16:17:07 <bcoca> felixfontein, abadger1999 i went from comment above that said otherwise, if that is wrong, then allisgood
16:17:07 <nitzmahone> bcoca: I think we're seeing a strong preference for some form of b)
16:17:27 <nitzmahone> (and not blocking beta for it)
16:17:32 <abadger1999> https://gist.github.com/abadger/5780c6ecbe840cd16bbe3b4fc0e44f99
16:17:45 <bcoca> well, at least we seem to agree on display fqcn .. which was in doubt at first also
16:18:00 <acozine> +1 to display FQCN, users will need it
16:18:06 <felixfontein> bcoca: I don't think it was in doubt
16:18:13 <felixfontein> at least not by me
16:18:19 <acozine> not by the docs team
16:18:41 <samccann> yes, users need to see FQCN
16:19:26 <bcoca> ^ I was only going from mention above taht current docs build didnt behave that way, which seems was not the case
16:19:27 <cyberpear> show FQCN is fine.  Don't require it in the docs source... it's duplicative there... generate it upon output based on the source of the docs
16:19:53 <cyberpear> abadger1999's example shows that ansible-doc already knows how to do this, to some extent
16:19:55 <felixfontein> I guess ansible-doc wouldn't even need to compose the name, it could simply use the name it used to retrieve the docs from. in that case the field in DOCUMENTATION is not used at all.
16:19:58 <abadger1999> Yeah, the current POC for the docs pipeline shows fqcn: https://toshio.fedorapeople.org/ansible/docsite/collections/community/crypto/acme_certificate_module.html#ansible-collections-community-crypto-acme-certificate-module
16:20:11 <bcoca> cyberpear: ansible-doc lists 'load name'
16:20:38 <bcoca> it has to be updated to show fqcn name in all cases for hte name field
16:21:09 <bcoca> ^ then it will match docsite, still not a beta blocker
16:21:28 <abadger1999> felixfontein: Yeah... the canonical source for the name is actually encoded on the filesystem.  all of what's in the DOCUMENTATION is redundant.
16:21:49 <bcoca> FS + runtime.yml
16:22:07 <bcoca> though that is not working (yet)
16:22:23 <abadger1999> (if you had plugins/modules/foobar.py   and it's DOCUMENTATION said `name: quux` .... the actual name needs to be foobar, not quux.)
16:22:30 <nitzmahone> The pluginloader will also give you the "on disk" name if you ask for it, I think the current form just doesn't.
16:22:33 <bcoca> k, seems we are all in agreement at this point, going to end meeting
16:22:38 <acozine> wait
16:22:38 <felixfontein> bcoca: i.e. up for beta 3 aka ansible-base 2.11? ;)
16:22:38 <bcoca> #endmeeting