19:26:03 <bcoca> #startmeeting ansible core irc public meeting
19:26:03 <zodbot> Meeting started Tue May 28 19:26:03 2019 UTC.
19:26:03 <zodbot> This meeting is logged and archived in a public location.
19:26:03 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:26:03 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:26:03 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
19:26:17 <bcoca> #topic https://github.com/ansible/ansible/pull/56817
19:26:17 <felixfontein> hi!
19:26:23 * bcoca waves
19:26:30 <bcoca> i appoligize, we had runover for post mortem
19:26:35 <shertel> Hi
19:26:37 <sdoran> MOAR MEETINGS
19:26:38 <jillr> o/
19:26:44 <nitzmahone> our grave overfloweth
19:26:44 <bcoca> sdoran: ssssh!~
19:27:00 <bcoca> jtyr?
19:27:35 <bcoca> ^ pr above adds loop_control toggel to how we notify handlers in loops, right now we notify all handlers if 'task' is changed, toggle allows for doing it 'per item'
19:27:43 <jtanner> brb
19:27:46 <bcoca> i.e  notify:  {{ handlers[item] }}
19:28:05 <bcoca> i dislike name, but im good with feature
19:28:08 <bcoca> any comments?
19:28:51 <shertel> it seems useful
19:28:59 <cyberpear> +1 feature
19:29:53 <nitzmahone> yeah, looks ok to me- +1 for having it under loop_control instead of a new top-level "knob"
19:30:16 <felixfontein> +1 for the feature
19:30:28 <bcoca> k, i have some notes on implementation, but as long as we are all happy with feature, will give jtyr green light
19:30:45 <bcoca> #topic https://github.com/ansible/community/issues/466#issuecomment-496615437
19:31:15 <sdoran> +1 for the feature
19:31:19 <nitzmahone> shipit
19:31:28 <mattclay> Adding to my original comment on the agenda, abadger1999 has suggested that we allow unit tests to allow satisfy the test requirement.
19:31:39 <nitzmahone> (also +1)
19:31:40 <bcoca> + 1to test requirements
19:31:57 <jillr> +1 to moar tests
19:32:30 <felixfontein> do I understand 3. correctly that if you even create a really small bugfix PR (maybe just a typo in the docs) for a module which has no tests, that sanity tests will fail?
19:32:41 <mattclay> felixfontein: Yes
19:32:56 <bcoca> that will be fun ... people will avoid fixing those ?
19:33:23 <mattclay> Now's the time to change that if that is considered undesirable behavior.
19:33:30 <bcoca> i would make exception to #3 on 'small patch'
19:33:34 <jtanner> without tests, in many cases, they're just breaking things they didn't know about
19:33:41 <felixfontein> this is probably discouraging some people to do (or complete) little bugfixes / typo corrections
19:33:51 <bcoca> for x in (y) changed to for x in (y,) is obvious fix
19:33:53 <sdoran> What's the reasoning for that? That seems a bit confusing to invoke a failure if a module is on the ignore list.
19:33:55 <nitzmahone> that's one where I wish we had a non-blocking result from sanity that could say "you really oughtta add some tests" but not prevent it from being automerged
19:34:02 <shertel> also +1 for requiring tests. AWS modules have been following those rules for a while, but have had an exception for urgent small patches.
19:34:17 <mattclay> nitzmahone: I can do that with ansible-test -- we just need to get the bot to pick up those messages.
19:34:35 <nitzmahone> Yeah, we'd talked about it previously, just assumed it was probably never gonna happen
19:35:19 <mattclay> So two variations on what I originally proposed: 1) allow unit tests to satisfy the requirement 2) make changes to ignored modules a warning instead of an error
19:35:33 <bcoca> works4me
19:35:38 <nitzmahone> me too
19:35:57 <bcoca> updated comment to reflect
19:36:00 <shertel> sounds good
19:36:00 <sdoran> I just worry that item three could have the effect of discouraging obvious and maybe good bugfixes from getting in if we throw up the "please write tests" barrier.
19:36:12 <bcoca> sdoran: reread now
19:36:51 <bcoca> k, lets vote on 'ammended' text
19:36:52 <bcoca> +1
19:37:01 <sdoran> +1
19:37:04 <jillr> +1
19:37:18 <shertel> +1
19:37:20 <nitzmahone> +1
19:37:44 * bcoca should ask Bercow on palamentary proceedures
19:37:52 <bcoca> k, seems like it passes
19:38:05 <bcoca> #topic https://github.com/ansible/community/issues/466#issuecomment-494938855
19:38:09 <bcoca> piou?
19:38:13 <bcoca> pilou?
19:38:41 <felixfontein> ah +1 for previous (sorry was busy in kitchen)
19:38:44 <bcoca> hmm, default always in 'choices' even when 'None'? which we use as indicator of 'unset'
19:39:05 <bcoca> i would say 'no' , cause if user sets to None .. its not 'unset' ...
19:39:41 <cyberpear> sounds like omit?
19:39:42 <bcoca> users already have 'omit' magic to 'unset' options
19:39:48 <bcoca> cyberpear: jinx!
19:39:49 <nitzmahone> Yeah, seems like some undefined behavior with the intersection of `required: true` among other things
19:40:25 <bcoca> im going to table, i really dont understand the question, as phrased it seems really wrong
19:40:36 <bcoca> since pilou is not here, pushing till next meeting
19:40:50 <felixfontein> is some module using required:no and default:None with choices?
19:41:03 <bcoca> probably plenty
19:41:03 <nitzmahone> default: None is implied
19:41:09 <bcoca> default: none is 'the default'
19:41:16 <shertel> I think this was related to the PR we saw in triage https://github.com/ansible/ansible/pull/56892
19:41:26 <nitzmahone> so most all with choices and required: no are effectively doing that today
19:42:53 <shertel> I don't like modules that specify string 'null' as a choice but it seems like this could alter modules doing that
19:43:00 <bcoca> yeah, that is wrong
19:44:00 <bcoca> k, i have another item i added, but didnt notify interested party, for botmeta, so going to skip and 'open floor'
19:44:04 <bcoca> #topic open floor
19:44:11 <cyberpear> (the limitation I've seen with omit is that you can't set_fact the value)
19:44:27 <bcoca> cyberpear: its for use on the option itself
19:44:33 <bcoca> which you can always do
19:44:58 <jtanner> botmeta?
19:45:09 <bcoca> jtanner: skipped since i didnt notify user yet
19:45:30 <jtanner> is it on the issue? i might be able to resolve out of band
19:45:48 <bcoca> https://github.com/ansible/ansible/pull/56858
19:46:09 <jtanner> oh okay
19:46:22 <bcoca> https://github.com/ansible/ansible/pull/55473 <= also wanted to tell him tha tupdating 40 modules at a time is not good idea
19:47:15 <jtanner> https://github.com/QijunPan is a ghost
19:47:43 <bcoca> probably huawei employee that created account just to add the modules
19:48:14 <jtanner> we've not been picky about setting folks as maintainers so far, so honestly i'd just merge it
19:48:24 <jtanner> i can do so if desired
19:48:42 <bcoca> go4it then
19:48:50 <jtanner> and multi-module PRs are sufficiently controlled by the bot with a message and needs_revision
19:49:07 <bcoca> 50 is normal thershold
19:49:13 <bcoca> 40 .. too close for my comfort
19:49:19 <jtanner> heh
19:49:30 <nitzmahone> looks like just lots of unrelated bugfixes
19:49:53 <bcoca> which is why i paused, if it was 'same fix and repeat' .. diff issue
19:50:12 <bcoca> modifying 40 files whlie replacing same code, is one thing ... doing tons of unrelated fixes ...
19:50:25 <jtanner> ah, i see. not 40+ new modules
19:50:31 <jtanner> just touching 40+ modules
19:50:47 <bcoca> yep
19:50:57 <jtanner> it won't get automerged anyway =)
19:53:37 <zer0_her0> hello bcoca and the rest, could we see https://github.com/ansible/ansible/pull/49432 about check mode markers
19:53:40 <jtanner> i noted the issue in the PR
19:53:44 <jtanner> and merged it
19:54:40 * nitzmahone not opposed to such a thing, but would need to be (at least) per-task, since tasks can opt in/out
19:54:53 <bcoca> it is, thought i reviewed this morning
19:55:45 <bcoca> ah, didnt check in comment
19:56:16 <bcoca> zer0_her0: anything else on that or you were just looking for review?
19:56:44 <zer0_her0> actually i see the support:core label and I am wondering if this means that we should discuss this here
19:57:02 <bcoca> you can, but really means core team member must review/merge
19:57:35 <zer0_her0> I think reviews are accepted for this PR. Is anything else to be done for merge ?
19:57:38 <bcoca> anyone against idea of 'dry run' captions in check mode?
19:57:48 <bcoca> zer0_her0: i had changes
19:57:50 <sdoran> Tests?
19:58:05 <sdoran> Since this is adding a Core feature.
19:58:11 <jtanner> callbacks/default.py
19:58:31 <zer0_her0> bcoca, I did the changes (version_added)
19:58:46 <bcoca> ^ you just got asked for tests also
19:59:04 <bcoca> and i also said changelog was missing
19:59:05 <zer0_her0> what should we add for this ?
19:59:30 <bcoca> ... i lost the other comments ...
19:59:40 <zer0_her0> (changelog is ok), I mean for test, what kind of tests ?
19:59:59 <bcoca> that toggle works as intended
20:00:29 <bcoca> current output unchagned and 'expected' results in diff scenarios x) --check-mode x) check_mode:true on task/block/role
20:00:46 <sdoran> zer0_her0: Look at test/integration/targets/callback_default/runme.sh
20:00:55 <jtanner> new folder like tests/integration/targets/dryrun, with a runme.sh file in there that executes the playbook with the new feature and checks that it worked
20:01:06 <zer0_her0> ok, I'll c heck this
20:01:18 <sdoran> That should give you an idea of what to do. You can ask for further help in #ansible-devel if you get stuck.
20:01:36 <jtanner> and on the the ansible-devel mailing list
20:01:40 <bcoca> #endmeeting