19:00:28 <jillr> #startmeeting ansible core irc public meeting
19:00:28 <zodbot> Meeting started Tue Feb 11 19:00:28 2020 UTC.
19:00:28 <zodbot> This meeting is logged and archived in a public location.
19:00:28 <zodbot> The chair is jillr. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:28 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:00:28 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
19:00:40 <jillr> hola jtanner
19:01:09 <felixfontein> hey!
19:01:14 <felixfontein> :)
19:01:25 <jillr> hey there! today is almost all your PRs felixfontein :)
19:01:36 <felixfontein> yep ;)
19:01:42 <jillr> #topic https://github.com/ansible/ansible/pull/66920/files
19:01:54 <jtanner> didn't we give you commit?
19:02:16 <felixfontein> jtanner: yes, but reviews and other input are still required
19:02:23 <jillr> jtanner: these are proposals to be discussed
19:02:25 <jtanner> was kidding
19:02:41 <felixfontein> for this one the main question is how deprecation sanity checks should work in the future, mainly with collections
19:02:58 <felixfontein> background: there is a sanity check for module.depreate(), but none for removed_in_version and deprecated_aliases
19:03:24 <felixfontein> the module.deprecate() sanity check is usually run once after a major release by the RM, who then creates issues and disables the test again (until the next release)
19:03:25 <jtanner> does this work for collections?
19:03:26 <jillr> it looks like it assumes collection versioning will track with ansible versioning, which I'm not sure is going to be a good assumption going forward, but still reading
19:04:06 <felixfontein> jillr: as long as Ansible itself evaluates removed_in_version etc. w.r.t. its own version (and informs the user accordingly), I guess that's what the checks should care about
19:05:09 <felixfontein> at least I'm not aware of plans to make AnsibleModule aware of collection versions and refer to the versions of the collections when generating warnings for deprecated options
19:06:03 <jtanner> it's a good check for core, but the number of modules in core is about to get very very small
19:07:21 <felixfontein> all deprecation warnings in current modules still refer to Ansible versions, so it would also help a lot for their new home (assuming they should keep Ansible's deprecation cycle)
19:08:00 <agaffney> ansible's version cycles aren't very meaningful in collections for deprecation purposes
19:08:44 <felixfontein> agaffney: true. but there's still ACD which bundles a lot of modules, which users would expect to stick to currently announced deprecation cycles
19:09:22 <jtanner> we don't actually know what ACD's makeup will be
19:10:00 <jtanner> it could be just a synthetic bundling of a bunch of disparate things all with their own versions, like an operating system distro
19:10:17 <jtanner> it could be a generated monolith from disparate things with a bunch of meta rewritten
19:10:20 <jtanner> we have no idea
19:10:50 <felixfontein> ok, so maybe it's better to postpone this resp. let whoever thinks about that stuff also keep this aspect in mind :)
19:11:12 <jtanner> like i said though, this is a good core-only check for the small number of modules we'll end up with
19:11:46 <jtanner> i believe we have hooks in sanity somewhere to short circuit if collection?
19:13:24 <felixfontein> there are checks like `if not self.collection:` in validate_modules/main.py, so I think yes
19:13:43 <jtanner> sounds like a winner
19:14:20 <felixfontein> ok, then I'll change the PR accordingly, and also make sure that the pylint module.deprecate() sanity check is always run (except for collections)
19:14:28 <jtanner> wfm
19:15:15 <jillr> #topic https://github.com/ansible/ansible/pull/66961
19:16:02 <felixfontein> that PR adds basic validations for mutually_exclusive, required_if, required_by, required_together, required_one_of
19:16:24 <jillr> I like the idea, reading the pr
19:16:47 <felixfontein> i.e. make sure that the options listed in there are actually options, that there are no repeats, that values for required_if are of the correct type
19:16:57 <jtanner> i believe samdoran was hoping to pull some of that code apart one day, maybe request his review too
19:17:28 <jtanner> pull the code inside module_utils at least, maybe not the sanity checks
19:17:55 <sdoran> Yes, but those changes should be transparent.
19:18:06 <sdoran> I'll do it in such a way as to maintain backwards compatability.
19:20:12 <jillr> sdoran: do you want to review this then, or is what you're doing totally separate?
19:21:27 <sdoran> Seems unrelated but I could review it.
19:21:33 <jillr> it seems like the change is uncontroversial at least?
19:21:43 <sdoran> Yeah, seems like a good thing to check.
19:21:48 <jillr> cool
19:22:10 <jillr> #topic https://github.com/ansible/ansible/pull/61659
19:22:17 <felixfontein> cool, thanks!
19:22:22 <tremble> We've seen enough typos and general mistakes with parameters I'd say sanity tests are worth it
19:23:20 <jillr> this pr is familiar, I dont recall what we've previously discussed about it though
19:23:40 <felixfontein> that PR was already on the agenda earlier; I was afraid that it could cause behavior change (because of selinux), but recently I realized it can't change any behavior
19:25:16 <felixfontein> the thing is that in all places where I used the new functionality, it was modifying module.params[] before, so the behavior is now exactly the same
19:25:28 <felixfontein> the only difference can happen in ecs_certificate, where it didn't do that
19:26:00 <felixfontein> (it uses this indirectly via lib/ansible/module_utils/crypto.py's write_file)
19:26:05 <jillr> and it looks like the maintainers there are in favor of the change
19:28:57 <jillr> this seems sane to me then
19:29:37 <felixfontein> I think this will improve module code, since directly changing module.params[] is really ugly :)
19:29:58 <jillr> does anyone disagree with this change?
19:31:03 <jtanner> distracted, looking now
19:31:09 <jillr> ack
19:31:44 <jtanner> eh, over my head
19:31:46 <jtanner> no vote from me
19:31:49 <felixfontein> last time nobody disagreed either, the main concern was to make sure that behavior doesn't change in a bad way
19:32:39 <jillr> we'll stick with that then, thanks felixfontein
19:33:07 <jillr> and I skipped one of your items since I was just going off links;
19:33:17 <felixfontein> i.e. ok to merge?
19:33:18 <jillr> #topic  Question: should new sanity tests be backported to stable-2.9?
19:33:21 <felixfontein> in fact you skipped two ;)
19:33:22 <jillr> felixfontein: shipit
19:33:26 <jillr> oops :)
19:33:36 <felixfontein> that question was asked by jborean93 here: https://github.com/ansible/ansible/pull/66965#discussion_r373190109
19:33:50 <felixfontein> (about a specific sanity check)
19:34:11 <tremble> What's the advantage of backporting sanity tests when code should hit devel first?
19:34:14 * nitzmahone doesn't have strong feelings as 1/2 RMs for 2.9 ;)
19:34:31 <jborean93> The more I think about it the more I think it shouldn't be backported
19:34:44 <jborean93> because if we do now a dot release will break sanity checks when running ansible-test
19:35:19 <jborean93> before ansible-test was made public I would say backport, now that people cna use it on their collections it should follow the same bugfix only backport process we have for ansible/ansible
19:35:32 <felixfontein> sounds reasonable to me
19:35:33 <nitzmahone> Most things that would trip a new sanity test probably shouldn't be getting backported anyway, so unless the sanity test change is catching/correcting an existing egregious problem or is needed to allow a backport to succeed, I'm meh for backporting them
19:36:01 <jborean93> yea I asked that question on the PR because I assumed the sanity check that was failing was already in 2.9
19:36:01 * nitzmahone wonders if mattclay has thoughts
19:36:13 <felixfontein> sanity checks should never be needed to backport something, since you can always update ignore.txt in the backport PR
19:36:14 * mattclay reads
19:36:32 <tremble> If I think of all the net new tests we've been adding (and the related fixes) we're going to make ignore.txt explode in the stable branches
19:36:50 <felixfontein> (though I guess bugs should be fixed and probably backported, but that can be checked on a one-by-one basis)
19:37:12 <jillr> or have to backport half of 2.10, yeah I don't think we want to do that
19:37:49 <nitzmahone> and since the applicability of direct backports between 2.10 and 2.9 is likely to go way down once the content diaspora occurs...
19:37:49 <felixfontein> jillr: yep, especially since a lot of PRs which fix sanity checks also do other changes which partially shouldn't be backported
19:37:56 <jillr> felixfontein: exactly
19:38:01 <mattclay> Generally we shouldn't backport new sanity tests.
19:38:17 <jillr> sounds like we're all agreed on that then?
19:38:19 <mattclay> Bug fixes in many cases, yes, but not new tests.
19:38:35 <nitzmahone> Yeah, sounds like as a general rule, don't backport new sanity tests
19:38:45 <felixfontein> sounds good!
19:38:52 <nitzmahone> (one-off special cases can be discussed as needed)
19:39:04 <jillr> cool
19:39:16 <tremble> Nothing would stop collections from running the devel sanity tests against their stable branches...
19:40:00 <nitzmahone> Yeah, once collections and base start diverging, there may be some things we'll have to revisit (esp if folks are maintaining multiple branches of the collections)
19:40:08 <jillr> #topic https://github.com/ansible/ansible/pull/67243
19:40:32 <felixfontein> another sanity check PR :)
19:40:36 <jillr> sorry felixfontein, just realized I had swapped the order of two of your items  :) I think thats your last one?
19:40:51 <felixfontein> jillr: yep, I also just saw that... yes, this is the last one I think ;)
19:40:55 <jillr> ok cool
19:43:00 <felixfontein> this PR cleans up module validation; currently, all options in FILE_COMMON_ARGUMENTS are essentially ignored if add_file_common_arguments=True is passed to AnsibleModule
19:44:24 <felixfontein> which exhibited at least one bug: copy documents `mode` as being of type `path` (https://docs.ansible.com/ansible/latest/modules/copy_module.html#parameter-mode)
19:47:01 <jillr> the idea seems sound, a light skim of the pr nothing obvious stands out to me
19:48:53 <mattclay> Looks good.
19:49:02 <jillr> thanks mattclay
19:49:27 <jillr> felixfontein: I think you're good for that one then
19:49:35 <shertel> +1
19:49:39 <felixfontein> cool, thanks!
19:49:48 <jillr> hi shertel o/
19:49:52 <shertel> Hi
19:49:56 <jillr> #topic https://github.com/ansible/ansible/pull/67281
19:49:58 <felixfontein> I'll do a rebuild_merge then (and once its merged create a PR to fix copy's docs)
19:50:06 <jillr> tremble: youre up
19:50:37 <jillr> I think shertel and I were both on board with this idea
19:50:50 <shertel> I'd prefer we removed the regex and modules use catch_extra_error_codes
19:51:03 <jillr> exactly that^
19:51:29 <tremble> I have a slight preference for having the Reg Ex there but making it disabled by default
19:51:47 <tremble> but not enough that I'd fight shertel over it
19:51:50 <shertel> I'd rather handle the error codes explicitly rather than wildcard
19:51:57 <shertel> Heh
19:52:26 <tremble> The issue is that by disabling this we may introduce sporadic failures into various modules.
19:52:44 <felixfontein> I almost always prefer explicit > implicit
19:53:16 <felixfontein> how about adjusting all callers accordingly? and then start changing them one by one (in new PRs)?
19:53:36 <jillr> it's hard to guess which services have some variant of *NotFound* and will have intermittent issues, but I'd still rather be explicit
19:53:46 <shertel> I just think it will be better in the long run than relying on implicit behavior
19:54:23 <tremble> That's basically the approach I *was* going to take, but I think just accepting the pain might be worth it given people have possibly been seeing weird performance issues from it catching unexpected things
19:54:38 <shertel> Ec2 has their exceptions well documenting, so that will help
19:55:49 <jillr> it sounds like we all agree, lose the regex and add back via catch_extra_error_codes as we find the need?
19:56:32 <tremble> +1
19:56:40 <shertel> I'd like like to try to mitigate some of it before, if possible
19:57:15 <tremble> My intention was to go through the 60-odd modules and attempt to clean up a little once the change landed
19:57:29 <felixfontein> I agree with shertel (but then I have no idea how hard that is to do, and I won't do it :) )
19:58:05 <shertel> I'm not at a computer now, but can go through our modules later today or tomorrow and compile a list of easy ones to fix beforehand
19:58:12 <tremble> felixfontein, 60 modules attempt to try and follow some of the logic and see where NotFound exceptions would be *expected*
19:58:40 * shertel is on PTO and has time to kill
19:59:12 <tremble> The thing with AWS is that "NotFound" often means "I didn't finish creating it yet, but returned anyway"
19:59:16 <sivel> that's not how you are supposed to do PTO!
19:59:18 <felixfontein> don't say that too loud, otherwise you'll end up with tons of review requests :D
19:59:19 <jillr> tremble: not a freeze date or anything, but collections and things are imminent - do you think it's reasonable to do that and land both changes by let's say end of month?
19:59:32 <shertel> :-)
19:59:33 <jillr> I can prioritize the reviews
20:00:01 <tremble> if you too are willing to prioritise reviews I can spend some time cleaning up
20:00:06 <tremble> s/too/two/
20:00:09 <jillr> ok cool, thanks
20:00:49 <jillr> I dont want to leave PRs we deem important in weird a lurch when aws migrates out
20:01:02 <jillr> *in a weird
20:01:28 <jillr> when 67281 is ready to go give us a ping
20:01:36 <felixfontein> btw, does aws migrate to the huge "most community stuff" community, or does it go somewhere else?
20:01:40 <tremble> Yeah, that's why I'd almost rather push something we know is 'broken', since I can finish the cleanup post migration.
20:01:51 <jillr> felixfontein: it will have it's own repo(s)
20:02:01 <shertel> Tremble, I'll ping you with a list tomorrow
20:02:09 <tremble> shertel sounds good.
20:02:14 <felixfontein> that will make working on it easier I guess :)
20:02:18 <jillr> whether it's one or two is still something I'm working on, this week
20:02:20 <tremble> If I'm not online you know my email, that works too
20:02:27 <shertel> Cool
20:02:39 <jillr> awesome, thanks y'all!
20:02:48 <jillr> #endmeeting