19:05:17 <nitzmahone> #startmeeting Core Public IRC Meeting
19:05:17 <zodbot> Meeting started Tue Jun  9 19:05:17 2020 UTC.
19:05:17 <zodbot> This meeting is logged and archived in a public location.
19:05:17 <zodbot> The chair is nitzmahone. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:05:17 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:05:17 <zodbot> The meeting name has been set to 'core_public_irc_meeting'
19:05:47 <felixfontein> zodbot is back!
19:06:09 <nitzmahone> #info agenda https://github.com/ansible/community/issues/536
19:06:44 <felixfontein> I think the agenda needs to change to June eventually... except if you want to wait for July :D
19:06:50 <nitzmahone> lol
19:06:59 <nitzmahone> Going out of order:
19:07:01 <nitzmahone> #topic https://github.com/ansible/ansible/pull/69926
19:07:23 <felixfontein> ok, so let's start with that one!
19:07:40 <nitzmahone> This is the rollback of tagged versions with composite metadata in favor of discrete values everywhere
19:07:54 <felixfontein> I just wanted to call it upgrade, not rollback :)
19:08:15 <nitzmahone> Looks good to me- only question was about if we're going to kill the composite format support in display.deprecated in this PR or later
19:08:34 <felixfontein> and it also affects tagged dates, which are back to regular dates (with collection info in adjacent field)
19:09:58 <felixfontein> I don't think it makes much a difference. there are some collections which already started using it, but it shouldn't make a big difference to them whether we remove it from the PR, or in a follow-up PR
19:10:18 <nitzmahone> If it's unlikely to break things, I'd vote for killing it in this PR then
19:10:40 * nitzmahone wasn't sure how much people had started using it, it's all strings anyway internally in that case
19:11:24 <felixfontein> and new sanity errors will pop up with this PR anyway, which is the main reason they changed it I guess
19:11:41 <bcoca> afaik jborean93 was using it
19:12:19 * jborean93 reads history
19:12:48 <nitzmahone> I'm guessing collection owners that were on the bleeding edge with that will figure it out quickly once sanity starts failing ;)
19:12:53 <felixfontein> (history fortunately is short :) )
19:13:04 <nitzmahone> OK, anything else about that one?
19:13:43 <felixfontein> nitzmahone: yes, they will... I guess we need to publish something on https://github.com/ansible-collections/overview/issues/45 about deprecation / versions soon, and something hopefully final
19:13:52 <felixfontein> if nobody complains I can push a commit which removes tag extraction
19:13:53 <jborean93> I'm doing a quick review on 69926 on the C# stuff
19:14:01 <felixfontein> jborean93: that would be awesome!
19:14:34 <nitzmahone> the C# stuff looked good to me, but I was mostly focused on the Python side this time
19:14:40 <nitzmahone> OK, next one:
19:14:41 <nitzmahone> #topic https://github.com/ansible/ansible/pull/69796
19:14:42 <felixfontein> btw, is the dict order in C#/PowerShell "random", but fixed?
19:14:54 <jborean93> depends on what you use
19:14:54 <felixfontein> why not https://github.com/ansible/ansible/pull/69935 first?
19:15:05 <felixfontein> it is more closely related to https://github.com/ansible/ansible/pull/69926
19:15:23 <jborean93> I had both PRs open and 26 comes before 35 :)
19:15:25 <nitzmahone> felixfontein: that one's good to go- I haven't marked it yet, but I was just about to merge it
19:15:59 <felixfontein> ah. I had a question about 69935 first :) or in fact, two questions
19:16:01 <nitzmahone> I'm also good for backporting the actual warnings to 2.9
19:16:14 <felixfontein> (three in that case)
19:16:18 <nitzmahone> #topic https://github.com/ansible/ansible/pull/69935
19:16:49 <felixfontein> 1) is there a potential incompatibility if a PS/C# module passes null as version to Deprecate(msg, version), now that there's an overload Deprecate(msg, date)?
19:17:08 <felixfontein> (I guess null shouldn't be passed, but AFAIK it worked fine so far)
19:17:24 <nitzmahone> It's not `Date?`, so you can't
19:17:31 <nitzmahone> (ValueType)
19:17:58 <nitzmahone> IIRC if the overload definition/resolution is ambiguous, you'll get a compile error in that case
19:18:04 <felixfontein> 2) should removed_at_date for module options (on the Python side) result in a warning? because I added that warning for removed_at_date in C# (because I had to treat removed_at_date anyway), but not for Python
19:18:14 <nitzmahone> (so if you changed to Date? it would probably fail to compile)
19:18:29 <jborean93> felixfontein: 1. no you can't $null can be used for both a string and DateTime so it can't determine which one to use
19:18:58 <felixfontein> jborean93: ok, so $null worked before (when there was no overload), but now it will lead to a crash (compile error)
19:19:24 <nitzmahone> DateTime is a ValueType, so I don't think there's an amiguity
19:19:42 <jborean93> nitzmahone: it could also depend on how powershell marshals $null to DateTime
19:19:52 <jborean93> it converts it to an empty string for a String type
19:19:53 <nitzmahone> IIRC you get "default value"
19:20:09 <nitzmahone> So `DateTime.default`
19:20:47 <jborean93> in any case, a `$null` value for `$module.Deprecate("msg", $null)` shouldn't have happened anyway
19:20:48 <nitzmahone> IIRC the compiler's pretty good about catching those ambiguities at definition, not at call
19:21:54 <felixfontein> jborean93: "shouldn't have happened" doesn't mean it didn't ;)
19:23:01 <jborean93> ok nitzmahone is right
19:23:25 <jborean93> because `$null` cannot be cast to DateTime (because it's a ValueType) the `(string msg, string version)` is used instead
19:23:35 <nitzmahone> Glad to know my C# hasn't rotted completely away ;)
19:23:39 <jborean93> so if someone did `$module.Deprecate("msg", $null)` it will continue to use the existing path
19:23:54 <jborean93> it wasn't c# I doubted but PowerShell's binder :)
19:23:59 <nitzmahone> heh
19:24:24 <nitzmahone> felixfontein: re 2) yeah, I think the Python one should behave the same
19:24:39 <jborean93> https://gist.github.com/jborean93/a599d375c8d088f295b2f24a5c6d3999, used to test it
19:24:49 <nitzmahone> felixfontein: and was there a 3) ?
19:25:27 <nitzmahone> @jborean93: just for giggles, is it an ambiguous overload compile error on the Type if you change that to `DateTime?`
19:25:49 <jborean93> nope
19:25:52 <felixfontein> 3) ah yes, "backporting the actual warnings to 2.9" - do you mean that they actually mention the date and/or collection name? or just that they print a warning, but ignore dates and collection names?
19:26:02 <nitzmahone> the former
19:26:02 <jborean93> funnily enough, powershell prefers the DateTime one
19:26:08 * cyberpear overall +1 to ease 2.9 compat (even if it's a "feature"), but I'd expect most collections to only work well w/ 2.10 or newer and wouldn't worry too much as long as they're functional in 2.9, even w/o all the bells & whistles
19:26:40 <nitzmahone> cyberpear: definitely- it's a slippery slope for sure ;)
19:27:00 <nitzmahone> (backporting features, I mean)
19:27:01 <felixfontein> indeed. that's why I tried to only include "print the warning, but ignore date / collection_name" :)
19:27:02 <cyberpear> (also, +1 to not converting symlinks to files)
19:27:45 <nitzmahone> felixfontein: I'm fine either way- this covers the part I was mostly worried about, but I'm willing to backport the warnings in this case
19:28:55 <felixfontein> maybe let's do it in two steps, first avoid breakage, and then in a follow-up PR, improve the warnings? I'm not sure when the next 2.9.x version will get released, maybe it's better to do only a smaller (but important) part first
19:29:17 <nitzmahone> 2.9.10 will probably be Monday/Tuesday at this point, since Friday is a companywide-holiday
19:30:05 <felixfontein> so same timeframe as 2.10 beta?
19:30:09 <nitzmahone> So there's time to get it in- I definitely want to include the "it doesn't blow up" part- if you want to do a follow-on PR for the actual backport, we can try to get it in but that way we're definitely not blocked for 2.9.10
19:30:10 <felixfontein> (if that isn't moved again?)
19:30:11 <nitzmahone> yeah
19:30:42 <felixfontein> ok. then I'll make the adjustment that removed_at_date on the Python side also triggers a deprecation warning, and then it should be ready
19:30:49 <nitzmahone> Right now IMO the beta's looking pretty good- if we slip at all, it'd probably just be another day or two, not another week or two
19:30:56 <nitzmahone> 👍
19:31:18 <felixfontein> jborean93: default arguments don't work with overloads for Windows 2012
19:31:24 <nitzmahone> OK, on to the next
19:31:29 <nitzmahone> #topic https://github.com/ansible/ansible/pull/69796
19:31:37 <felixfontein> (since you wrote that for both PRs :) )
19:31:44 <jborean93> hmm really
19:31:58 <felixfontein> yes, that was my first try, and it failed miserably :)
19:32:02 <jborean93> well in any case you can just have it call the 2nd function with the value you want instead of defining the body twice
19:32:16 <felixfontein> yes, I'll change it that way
19:32:50 <felixfontein> ok. that PR is about RETURN documentation
19:33:10 <felixfontein> I propose to parse it at the same time as DOCUMENTATION is parsed (when needed for ansible-doc), and fail if it cannot be parsed
19:33:21 <felixfontein> currently it is only tried to be parsed, and only much later
19:33:30 <nitzmahone> +1 in concept
19:33:33 <felixfontein> (and if it fails to parse, it is just dumped)
19:33:54 <nitzmahone> return value docs are plenty important
19:34:41 <felixfontein> `ansible-test sanity` has been making sure that plugins and modules have valid RETURN yaml, so it could only affect third-party plugins/modules which never saw `ansible-test sanity`
19:35:16 <felixfontein> parts of that PR are the same / very much related to #69926, and whatever gets merged first will cause the other to get a bit simpler
19:36:44 <nitzmahone> I'm personally good to merge 69926 as soon as jborean93 signs off on the C# changes
19:37:08 <felixfontein> should I include the removal of tag extraction?
19:37:16 <nitzmahone> I think so
19:37:33 <nitzmahone> (doesn't seem like a compelling reason to leave it)
19:37:47 <jborean93> the overall changes are good, just want to use the overloaded function and a docs entry
19:37:54 <jborean93> but apart from that 69926 is fine with me
19:39:08 <nitzmahone> felixfontein: anything else you want to discuss on 69796? I agree in concept, but I'm going to have to leave the review to someone else.
19:40:43 <felixfontein> nitzmahone: I would be happy to get a decision on whether making bad YAML for RETURN fail ansible-doc is ok
19:40:55 <nitzmahone> +1 from me
19:40:55 <felixfontein> the last meeting(s) there wasn't quorum
19:41:06 <jborean93> what happens now?
19:41:15 <nitzmahone> silent ignore IIRC
19:41:31 <jborean93> and that's only when running `ansible-doc` right?
19:42:07 <felixfontein> jborean93: and `help` in `ansible-console`
19:42:31 <felixfontein> it's used by these two, and by plugin_formatter (which will go away; the new docs pipeline needs valid YAML too though)
19:42:42 <felixfontein> I've updated 69926
19:42:46 <jborean93> I would be ok with it either failing or it at least displaying the error for those fields saying the RETURN docs are invalid and to bug the module maintainers
19:43:18 <nitzmahone> and TBC: that's only for invalid `RETURN`, not missing, yeah?
19:43:20 <felixfontein> jborean93: it's doing that (in fact same error handling as for invalid YAML in DOCUMENTATION)
19:43:32 <felixfontein> nitzmahone: yes, only invalid RETURN
19:43:36 <nitzmahone> +1
19:43:45 <jborean93> yep that's fine with me then
19:44:07 <felixfontein> bcoca seems also to be fine with it (in fact he also suggested it)
19:44:30 <felixfontein> and acozine, samccann and abadger1999 also seem fine with it
19:44:34 <nitzmahone> Cool- like I said, I'll have to leave the actual review on that one to others, but sounds like we're agreed in principle on how it should work
19:44:37 <shertel> jborean93: I think the choices were fail or warn. I had a slight preference for warning, but don't really care.
19:45:18 <felixfontein> I implemented a warning first (after shertel suggested it), and then added a last commit which replaces it by failure
19:45:23 <nitzmahone> So long as we can easily detect a warning in tests without involving grep (eg nonzero return value) I could live with that
19:45:27 <felixfontein> if someone is interested in the code difference :)
19:45:38 <felixfontein> there's also a test for it ;)
19:45:49 <shertel> I'm happy with it as it is
19:46:00 <nitzmahone> ditto
19:46:20 <felixfontein> great!
19:46:34 <nitzmahone> #topic open floor
19:46:39 <nitzmahone> (mind the gap)
19:46:42 <felixfontein> then I'll update it once 69926 has been merged, it should be even more straightforward then
19:46:51 <jborean93> probably just want a consensus on the last 2 points in https://github.com/ansible/ansible/pull/69959
19:46:59 <jborean93> 1. how to deal with dir symlinks
19:47:23 <jborean93> and 2. check the target link path is inside the collection on install (in addition to the hash check)
19:47:42 <felixfontein> I prefer a check for 2.
19:47:56 <jborean93> 1. right now will skip dir symlinks that point outside the collection and I believe creates a bad tarfile for ones inside a symlink
19:48:05 <felixfontein> for 1. I don't care, I haven't seen that used in collections yet
19:48:09 <jborean93> I need to check the existing behaviour for that as I can't remember if it did work or not
19:48:37 <jborean93> the only reason why I never did 2 was because https://github.com/ansible/ansible/pull/69959#discussion_r437672969
19:48:52 <nitzmahone> On the second one, I think out of an abundance of caution it's not a terrible idea to block symlinks outside the collection at install time too
19:49:46 <nitzmahone> It's definitely a case of "already on the other side of the air-tight hatchway", but I don't see a good reason to allow it to happen
19:49:50 <jborean93> so if there was a link pointing to `/etc/passwd` it will only be created if `FILE.json` has the checksum of the real `/etc/passwd` which you can only really get if you know the value first
19:50:05 <felixfontein> not necessarily :)
19:50:20 <felixfontein> maybe someone published their hash of /etc/passwd, feeling confidend that nobody could possibly abuse that
19:50:32 <nitzmahone> *gauntlet thrown*
19:50:42 <jborean93> true, I guess it doesn't hurt, especially if the code moves around in the future
19:50:43 <jborean93> thanks
19:51:45 <nitzmahone> jborean93: what's the other option for the dir symlink behavior?
19:51:58 <nitzmahone> block install, or somehow make it work?
19:52:07 <jborean93> get it working somehow, i.e. preserve dir symlinks that point to somewhere inside the collection
19:52:19 <jborean93> ones that point outside are skipped and I'm happy to continue that behaviour
19:52:25 <nitzmahone> ditto
19:53:08 <jborean93> but for the former, I think build actually creates it but you cannot extract them properly with tar
19:53:13 <nitzmahone> the other might be nice to have, but if it doesn't work, should probably warn/error on creation of the artifact and leave current behavior for install?
19:53:21 <jborean93> I just wanted to get a consensus on the behaviour we want before moving forward on that
19:54:13 <nitzmahone> #chair sdoran bcoca jborean93 felixfontein
19:54:13 <zodbot> Current chairs: bcoca felixfontein jborean93 nitzmahone sdoran
19:54:18 <nitzmahone> (sorry, forgot to do that)
19:54:50 <nitzmahone> #chair shertel Shrews
19:54:50 <zodbot> Current chairs: Shrews bcoca felixfontein jborean93 nitzmahone sdoran shertel
19:55:44 <jborean93> I'll have a look at the existing behaviour for dir symlinks and just make sure that this doesn't break that
19:55:44 <nitzmahone> jborean93: put me down for "nice to have", but so long as we're clear about what does/doesn't work when creating the tarball, whatever...
19:55:54 <jborean93> yep
19:56:04 <nitzmahone> OK, anything else?
19:56:10 <jborean93> I'm good
19:56:16 * nitzmahone closing in 1min if not...
19:56:21 <jborean93> remember, an arm is not lunch :)
19:56:22 <felixfontein> I'm good too
19:56:30 <nitzmahone> jborean93 :)
19:56:31 <nitzmahone> :P
19:57:29 <nitzmahone> OK then, til next time! Thanks all for your participation!
19:57:31 <nitzmahone> #endmeeting