19:02:07 <jillr> #startmeeting ansible core public irc meeting
19:02:08 <zodbot> Meeting started Tue Aug  6 19:02:07 2019 UTC.
19:02:08 <zodbot> This meeting is logged and archived in a public location.
19:02:08 <zodbot> The chair is jillr. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:02:08 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:02:08 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting'
19:02:17 <jillr> #topic  https://github.com/ansible/ansible/pull/59823
19:02:25 <sdoran> o/
19:02:39 <jhawkesworth> hey
19:02:49 <cyberpear> is this something that could reasonably be backported? -- currently, there's no way to have ansible create files on vfat filesystems w/o the change
19:03:13 <cyberpear> (even if it requires backporting w/o the new env var option)
19:03:44 <cyberpear> here's an example of an idempotent workaround for the issue: https://github.com/MindPointGroup/RHEL7-STIG/pull/250/files
19:04:12 <jillr> I'd agree with that
19:04:47 <agaffney> I wouldn't consider it a regression (unless it worked before 2.8), which generally doesn't qualify it for backporting
19:05:09 <agaffney> but it seems "safe" to backport
19:05:30 <bcoca> +1
19:05:36 <felixfontein> I'd probably consider it a bugfix, or at least the default value (not the new env variable)
19:05:37 <cyberpear> at least a bugfix, though? -- seems like ansible should be able to create files on UEFI filesystems, which exist on modern bare metal hardware
19:06:00 <jillr> yeah it seems sufficiently user-breaking IMO
19:06:21 <tchernomax> +1
19:06:32 <agaffney> huh, that's the first time I've seen an ansible config option without a matching env var
19:06:34 <sivel> solutions: don't use vfat, don't use selinux ;)
19:06:37 <bcoca> i would actually prefer an 'ignore selinux' toggle
19:06:53 <felixfontein> sivel: +1 ;)
19:06:59 <sivel> I already ignore it
19:07:07 <bcoca> but ansible cannot
19:07:07 <jillr> sivel: always be enforcing  :)
19:07:16 <sivel> I enforce that it is disabled
19:07:18 <cyberpear> bcoca: on all files modules, for example?
19:07:19 <bcoca> and fs detection is not perfect
19:07:26 <sivel> / /trolling
19:07:29 <bcoca> sivel: even if disabled, if present, ansible MUST use it
19:07:42 <jillr> ok so, we agree it's a good candidate for backport then?
19:07:49 <bcoca> cyberpear: they all use same code
19:07:57 <bcoca> or at least 'should'
19:07:58 <cyberpear> fair enough
19:08:10 <cyberpear> so the outstanding question is whether the backport should include the env var
19:08:17 <felixfontein> jillr: I think so
19:08:48 <bcoca> idc about env var, but you might want to remove to make backport more acceptable to others
19:08:49 <agaffney> cyberpear: certainly not as it stands in the original PR, since it shows that it was added in 2.9
19:09:00 <sivel> I'm on the side of not backporting the env var
19:09:10 <agaffney> I'd personally vote for just backporting the modified config default
19:09:26 <sivel> also, seeing as though this is technically already configurable, I'd not backport at all
19:09:36 <agaffney> that's an excellent point
19:09:49 <felixfontein> have there been support requests for this to be configurable by env variable? if not, I would not backport that
19:09:58 <felixfontein> (if yes, then *maybe*)
19:09:59 <sdoran> This is changing the default value of a config. It should not be backported.
19:10:06 <sdoran> It's a good fix, but doesn't need to be backported.
19:10:23 <sivel> ok, I have to leave, you have my vote. +1 for 2.9, -1 for backport
19:10:43 <agaffney> yeah, with it already being configurable in 2.8, there's probably no need for a backport
19:11:16 <sdoran> Changing default config values in a point release could be bad.
19:11:35 <bcoca> hmm 0 to bakcport then
19:11:38 <jillr> so that's now, backport: +4, -3 ?
19:11:45 <agaffney> in this case, it's probably not a big deal, since anyone that cares either has an override value in ansible.cfg or is currently broken
19:11:59 <shertel> -1 to backport
19:12:03 <felixfontein> +1 for 2.9, 0 for backport
19:12:15 <bcoca> i thought it already landed in 2.9
19:12:20 <agaffney> -0.5 to backport
19:12:30 <felixfontein> bcoca: it's not merged yet
19:12:33 <jillr> bcoca: it's not merged yet
19:12:35 <cyberpear> in general, I'd agree not changing default.  In this case, I don't see what it could break...
19:12:35 <bcoca> even if not, i thinkonly question is backport, clearly fine for 2.9
19:12:40 <jillr> felixfontein: jinx!
19:12:41 <felixfontein> it needs a changelog fragment before merging
19:12:52 <cyberpear> #action cyberpear to add changeglog
19:12:58 <felixfontein> (maybe even a porting guide entry, though I'd argue that it is minor enough not to need a mention there)
19:13:22 <agaffney> porting guide entry is probably unnecessary
19:13:48 <cyberpear> obv, I'm +1 for backporting (since I brought it up) -- I'd be more okay w/o a backport if there were an env var I could tell folks to set instead of having to edit ini
19:14:25 <cyberpear> (given that ini can't cascade, and good site policy dictates a corporate ini and project-specific env vars)
19:14:49 <cyberpear> Am I missing a way to set config at runtime w/in a playbook or role?
19:15:09 <cyberpear> (that could be a cleaner workaround)
19:16:34 <agaffney> most config options are "global", so they couldn't be set in a playbook
19:16:40 <jhawkesworth> other than another ansible.cfg I can't think of a workaround.  It feels like a sort of policy type configuration setting though (i.e. true for all playbooks, or am i mistaken)?
19:17:24 <cyberpear> Seems like it'd better to have a whitelist of filesystems known to support SELinux, not the other way around, but I'm not here to make that case today
19:18:10 <jillr> so on backporting I think we're at: +2, -5.5
19:18:17 <agaffney> I'd think that whitelists and blacklists are equally difficult to maintain in this case, and it's (probably) possible for a filesystem that generally has selinux support to have that support disabled
19:19:09 <cyberpear> fair enough.  Let's move on.
19:19:12 <bcoca> cyberpear: there are those that support it, those that dont and those that claim either incorrectly, afaik more do than do not (need extended attribute support)
19:19:16 <felixfontein> +1 to move on :)
19:19:16 <cyberpear> #agreed don't backport vfat selinux fixes
19:19:26 <jillr> #topic  https://github.com/ansible/ansible/issues/57958
19:19:59 <cyberpear> so I opened a PR for this one and I think it should be backported to 2.6, since it's a regression introduced in 2.4; worked in 2.3
19:20:37 <cyberpear> in the use case where I ran into it, the debug with changed_when was telling me about non-secure files, but that output was getting lost because of callback=actionable
19:20:52 <bcoca> fix lgtm, needs someone to ensure there are no accidental disclosures, also tests for output to be same
19:21:18 <bcoca> not sure 2.6 qualifies, but +1 to backport rest if this is merged
19:21:20 <cyberpear> I only noticed it because in a playbook w/ apparently no changes per the task output, had a changed=xx count where changed!=0 but no changed output was shown
19:21:25 * jborean93 lurks while waking up
19:22:15 * bcoca really hates the 100 special cases of debug
19:22:21 * cyberpear too
19:22:27 <jborean93> now it's 101 special cases
19:22:36 <bcoca> 103 by changes
19:22:40 <cyberpear> this actually fixes one of the previous special cases
19:22:50 <jborean93> so 99 then :)
19:23:00 <bcoca> no, keeps it constant, still special case
19:23:04 <cyberpear> #info PR https://github.com/ansible/ansible/pull/59958
19:23:10 <bcoca> fixes, does not remove
19:23:12 <cyberpear> that's the PR that fixes the issue
19:23:20 <cyberpear> correct
19:23:45 <cyberpear> I think it was to make debug output less verbose, but I'm not here to argue why we do that, just that it should be done correctly if at all
19:24:21 <cyberpear> originally introduced here: https://github.com/ansible/ansible/commit/5ffb40f
19:25:57 <cyberpear> clean_results strips out "changed" "failed" "skipped" etc from only debug output; no-op for every other module output
19:26:31 <jhawkesworth> fix looks benign.  Only possible improvement I can think is to add a test, although I'm not sure how you would test that.
19:26:32 <cyberpear> the bug came about because "changed" was being stripped prior to the callback evaluating whether it should output it according to "changed" color/status
19:26:50 <cyberpear> it'd have to be a runme.sh, and I'm pretty bad at those, unless I'm modifying an existing one
19:27:54 <cyberpear> sounds like folks are okay with this back to 2.7, but not 2.6?
19:28:07 <jillr> ok so, generally this lg but we want tests before backporting?
19:28:36 <felixfontein> 2.6 is security fixes only, I'm not sure whether this qualifies as really security relevant
19:28:39 <jhawkesworth> a test would aid 'merge confidence' of course
19:28:47 <sdoran> This is not a critical bug fix, so it would only be backport to 2.8
19:28:52 <sdoran> *ed
19:29:08 <sdoran> I would prefer we have tests to prevent future regressions.
19:29:16 <felixfontein> +1 for tests
19:29:23 * cyberpear was hoping to avoid calling include_tasks in with_items for a debug...
19:29:27 <jillr> I will never not want tests
19:29:46 <cyberpear> I'll add a test
19:29:49 <bcoca> we have other callback tests, normally require screen redirect and diff against existing file
19:29:52 <jillr> thanks cyberpear
19:30:03 <jhawkesworth> yeah thanks, nice catch
19:30:04 <sdoran> cyberpear: If you have a simple reproducer, use that for the test.
19:30:15 <jillr> #topic  https://github.com/ansible/ansible/pull/59957
19:30:20 <bcoca> sdoran: issues is reproducer is visible on screen
19:30:37 <bcoca> screen output tests are not normally good
19:30:42 <cyberpear> each of the referenced bugs has a reproducer
19:30:52 <jborean93> cool, so this is less about the PR but more about the verbosity handling with `ansible-galaxy` and `ansible-vault`
19:31:39 <jborean93> the move to argparse has changed how args are parsed where currently `-v*` only works if it comes after the sub command, i.e. `ansible-galaxy init -v` works but `ansible-galaxy -v init` does  nothing
19:31:47 <sdoran> bcoca: Yeah, parsing stdout is less than ideal but it is a somewhat good test.
19:31:54 <bcoca> jborean93: i would argue, this is a change to all ansible cli
19:32:02 <bcoca> sdoran: i said 'not good' but still better than nothing
19:32:08 <jborean93> bcoca: other commands don't use sub parsers so they are not affected
19:32:14 <bcoca> ansible-vault
19:32:36 * jborean93 was mentinoed, I'm prtty sure it was just the 2
19:33:02 <bcoca> yep, avoided subparsers in rest by using -t option
19:33:21 <jborean93> basically right now `-v` works only after the sub command and silently ignores it if it comes before the sub command
19:33:37 <jborean93> the alternative is to do the opposite but it would mean `-v` after a sub command fails the command
19:33:56 <jborean93> sivel had a look to see if it is possible to add it back in but found it wouldn't be viable as we need ot effectively parse it ourselves
19:34:15 <bcoca> jborean93: or create a '-v' subcommand that rewrites the args
19:34:19 <jhawkesworth> as long as failing the command also spits out usage, I think its probably fine to change behaviour in 2.9
19:34:25 <bcoca> but that is probably same issue with other 'global switches'
19:35:15 * jhawkesworth with my 'ansible user' hat on I can say I have never run `ansible-vault` or `ansible-galaxy` with -v so would likely check the syntax if I needed.
19:38:34 <jillr> I feel like 'cmd sub-cmd -foo -opts' is much more common and would say let's go with that, plus printing usage.
19:39:56 <jborean93> I do prefer that way as I typically put `-v` at the end but there are definitely people who do it at the beginning
19:40:06 <bcoca> its still regression
19:40:07 <jborean93> e.g. https://github.com/ansible/ansible/issues/59202
19:40:31 <jillr> well TIL
19:41:58 <jborean93> bcoca: yep but it sounds like there isn't a solution which isn't stop using argparse
19:42:07 * jhawkesworth usually puts -v on the end but won't be upset if this has to change for ansible-vault and ansible-galaxy.  I can't really see this biting many folks as long as its in the porting guide
19:42:09 <bcoca> sivel: unmigrate!
19:42:21 <bcoca> jborean93: what about making -v a subcommand?
19:42:37 <bcoca> then have it 'rewrite to correct args'
19:42:40 <jillr> so our options aiui are: 1) yes it's a regression but here is the new hotness, please adapt,
19:42:43 <jillr> 2) write something to catch specifically the -v case and rewrite the args
19:42:50 <jborean93> what happens if we add more than just `-v` in the future
19:42:59 <bcoca> another subcommand
19:43:03 <jborean93> also `ansible-galaxy` is already quite weird with it's sub commands
19:43:05 <bcoca> also, i think that problem already exists
19:43:28 <jillr> can we handle -v now, and treat it as a deprecation?  and not add any future options like that?
19:44:00 <bcoca> or 3) add -v as subcommand and tell user he has to put it after
19:44:12 <bcoca> ^ and remove in 4 versions (deprecated)
19:44:14 <jborean93> bcoca: 3 is already how it is
19:44:26 <jborean93> can you even do that, sounds like a massive hack
19:44:26 <bcoca> ah, then works4me
19:44:50 <bcoca> not disputing that, but we should give users a hint we are ignoring them
19:45:06 <jborean93> that's the toruble we can't tell if `-v` was used before the sub command
19:45:28 <jborean93> because we add it to each sub parser which replaces the original instance (before sub command)
19:45:40 <agaffney> you can by (ab)using os.argv
19:45:48 <bcoca> why i said to add it as its own subparser, would be captured if first
19:45:57 <bcoca> ^ theoretically
19:46:15 <jborean93> I'm struggling to see how that would work without making it more brittle
19:46:44 <bcoca> not sure how it is more brittle, if action = -v fail and tell user that options are 'after' command
19:47:07 <bcoca> only thing unacceptable is ignoring silently
19:47:15 <jillr> bcoca: so in this case, you're not suggesting to rewrite the args when -v is first - just the warn the user and fail?
19:47:19 <bcoca> im fine if we rewrite it to 'just work'tm or emit a message
19:47:27 <bcoca> jillr: i've given both options
19:47:30 <jillr> ack
19:47:55 <jillr> so 3) catch -v and yell at the user; 4) catch -v and rewrite the args while also warning the user
19:48:09 <jborean93> I'm going to have to try it out and see what's possible
19:48:22 <bcoca> 4) == 2)
19:48:50 * jillr rereads ... ok true, thanks
19:50:05 <jillr> anything else to discuss on this topic?
19:50:39 <jhawkesworth> I'd rather have command fail 'cos I got syntax wrong over silently ignore -v  and I'd say be bold, new goodness here so don't be afraid to have new syntax
19:51:21 <jhawkesworth> i can't imagine people are going to have update lots of things to accommodate -v in a different place on these commands.
19:51:27 <jillr> we clearly have users doing it the other way if we're already getting bug reports, so if it ends up being feasible I'm in favor of a deprecation cycle
19:51:37 <jillr> we should at least explore the option
19:52:27 <jborean93> will have to play around with the code more
19:52:43 <bcoca> from a user perspective, its preferable to 'magically work' but if its too hacky/bad code to maintain, i think its fine to warn them
19:52:54 <bcoca> only 'unacceptable' optoin to me is the 'silent ignore'
19:53:16 <jillr> we're running near time so I'm going to move on and we can discuss more after jborean93 has a chance to look into it
19:53:25 <jborean93> +1
19:53:25 <jillr> #topic  facts modules https://gist.github.com/felixfontein/a6a1be999698da0ab3bd8868de6f16e1
19:53:36 <jillr> felixfontein: youre up
19:53:39 <felixfontein> https://github.com/ansible/community/issues/482#issuecomment-517933946
19:54:08 <felixfontein> some time ago we discusse dwhat to do with _facts modules which return ansible_facts but shouldn't; back then we decided to rename + deprecate facts return with a new feature nitzmahone wanted to add
19:54:19 <felixfontein> unfortunately it looks like that feature won't be there for 2.9
19:55:59 <felixfontein> there are three other possibilities (I know of): a) deprecate _facts module, create new _info module; b) rename, but change return value depending on name used for module; c) rename, make both return facts and non-facts, and mark facts as deprecated via changelog/porting guide (i.e. it will simply go away in four versions)
19:56:28 <felixfontein> I'd like to know which of these options (or another one?) is preferred by the core team
19:57:01 <felixfontein> a) looks clean, but has a big drawback: either we have to duplicate tests, or we just test the _info modules and remove tests for the _facts modules
19:57:50 <felixfontein> b) feels somewhat hacky, and c) could surprise people who didn't find out that they were still using facts when they are removed
19:58:36 <felixfontein> so: any opinions? :)
19:58:57 <jhawkesworth> option (c) seems to allow warnings to come later if implmented in 2.10 but still eventually get to the point where `ansible_facts`are gone.
19:59:35 <bcoca> felixfontein: .. i remit you to original conversation on relying on that feature ...
19:59:53 <felixfontein> yes, if warnings will be implemented say for 2.10, then we could enable them then. there won't be a 4-version cycle for warnings then, though
20:00:16 <bcoca> cannot start deprecation cycle w/o warnings
20:00:24 <bcoca> that got us very burnt in 2.1-2.3
20:01:42 <felixfontein> so how about b): rename, and make return value dependent on which name the module was called with? then people get the warning (that the module and its usage changed), and when they switch to the new name, the behavior changes, so they'll know when something break. they won't know *where exactly* it will break because there's no warning on using the facts, but that's the same problem for a)
20:03:39 <shertel> I would vote for b
20:03:41 <jillr> a) seems like it could be a real pain - these modules will quickly get out of sync and likely cause confusion
20:03:58 <shertel> Not relevant to the question at hand, but I think ec2_metadata_facts makes sense as a _facts module.
20:04:00 <jillr> b) is likely IMO the best option
20:04:33 <felixfontein> jillr: a) is painful from several points of view ;)
20:04:42 <jillr> felixfontein: agreed  :)
20:05:54 <jillr> we're running over, so: a) 0 b) +2  c) 0  ?
20:05:56 <felixfontein> shertel: yes, that makes sense. I haven't checked the cloud modules in detail yet
20:07:02 <felixfontein> more opinions? anyone else likes b)?
20:07:40 * jillr gives it 1 more min
20:08:22 <jillr> felixfontein: please proceed with b) and we can discuss more in the PR if necessary
20:08:26 <felixfontein> if time runs out: I won't be here on Thursday, but I can try to get a test PR for one (or more) modules done until then for b)
20:08:38 <jillr> thanks
20:08:40 <jillr> #endmeeting