15:00:52 <bcoca> #startmeeting ansible core irc public meeting
15:00:52 <zodbot> Meeting started Thu May 23 15:00:52 2019 UTC.
15:00:52 <zodbot> This meeting is logged and archived in a public location.
15:00:52 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:00:52 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
15:00:52 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
15:01:01 * jtyr would like to go first as he has to pick up child from school in 20 mins ;o)
15:02:20 <jtyr> bcoca: https://github.com/ansible/ansible/pull/56817
15:02:42 <bcoca> anyone else having github issues?
15:02:56 <dmellado> hey bcoca, not like 15' ago
15:03:02 <dmellado> what's the issue?
15:03:09 <bcoca> pages not loading
15:03:39 <dmellado> works for me, like right now
15:03:53 <bcoca> hrmphf
15:03:53 <dmellado> maybe some ISP issue?
15:04:30 <dmellado> has the meeting been cancelled, btw?
15:04:35 <sdoran> GH seems ok for me.
15:04:58 <bcoca> k, so its jsut me
15:05:02 <sdoran> Must be the jackhammers. ;)
15:05:13 <bcoca> weird thing, git is working just fine against github repos, its only browser that is issue
15:05:30 <dmellado> now is when you tell me that you use Edge as a browser xD
15:05:33 <bcoca> sdoran: nah, none today
15:05:49 <bcoca> dmellado: if you knew me, you'ld know how silly that is
15:06:01 <nitzmahone> o/
15:06:05 <bcoca> ^ he knows
15:06:21 <dmellado> bcoca I was basically teasing you xD
15:06:22 <bcoca> #topic https://github.com/ansible/ansible/issues/56345
15:06:33 <bcoca> ^ w3m loads fine, problem seems firefox specific
15:06:43 <sivel> I added this item to the agenda
15:07:15 <sivel> the problem is that when `ignore_errors` is a template, and the cause of the task failure is an undefined var, `ignore_errors` is never templated
15:07:50 <bcoca> would not matter, ignore_errors wont skip undefined task errors  either
15:07:57 <bcoca> it ONLY affects 'task results'
15:08:00 <nitzmahone> Failure inception
15:08:03 <bcoca> i.e task == failed
15:08:23 <sivel> making it always truthy
15:08:27 <bcoca> task definition MUST be correct, those are fatal errors 'ignore_errors' would more aptly named ignore_task_failed
15:09:06 <bcoca> ah, this can only really happen with debug, since it traps undefined
15:09:12 <bcoca> it would not happen with other tasks
15:09:29 <nitzmahone> Seems like post validate should be able to stop before the task runs
15:09:32 <sivel> sorry, I lost internet
15:09:51 <bcoca> nitzmahone: well, unsure ticket shows output that is not matching 'test case'
15:09:56 <bcoca> so unclear
15:10:01 <sivel> so basically if an undefined var causes the task to fail, a template for `ignore_errors` is truthy, and ignores the error
15:10:14 <nitzmahone> sivel: What, did a bird fly by or something ;)
15:10:22 <sivel> nitzmahone: lol
15:10:34 <bcoca> sivel: only if task is doing templating itself, a 'templating fail' while in task def should not be capturable
15:11:16 <sivel> bcoca: it does seem to be.  I verified it
15:11:20 <bcoca> debug does its own templating/capture so it is not a good example, other tasks, i.e ping: data={{undefined}} would not be task failures
15:11:41 <sivel> a failure templating the task, will pull out `ignore_errors` and check if it should be ignored
15:11:49 <sivel> but ignore_errors couldn't be templated
15:11:59 <sivel> so a string value is truthy
15:12:15 <bcoca> still, it should not apply to the task
15:12:21 <sivel> hrm, I deleted my test case
15:13:03 <sivel> one sec
15:13:16 <bcoca> hrm, seems im wrong, we changed 'undefined' to be 'task failed' vs 'task definition error'
15:13:23 <bcoca> which is incorrent, imho
15:13:29 <sivel> yes, I just confirmed again
15:13:34 <sivel> with your ping example
15:13:49 <sivel> fatal: [localhost]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'undef' is undefined\n\nThe error appears to be in '/Users/matt/projects/ansibledev/playbooks/56345/56345.yml': line 8, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n        var: ansible_check_mode\n    - ping:\n
15:13:51 <sivel> ^ here\n"}
15:13:53 <sivel> ...ignoring
15:14:24 <bcoca> so i tink the error is wrong, not about ignore_errors not being templated, but 'undeinfed var' being a task failure
15:14:36 <bcoca> this should be a syntax failure instead
15:14:49 <sivel> this is largely why I brought this issue up
15:15:11 <nitzmahone> Agreed on the "desired behavior"
15:16:10 <sivel> nitzmahone: can you explain that more. I might be dense this morning, and don't understand exactly what you are stating
15:16:21 <nitzmahone> Is that applicable to all FieldAttributes?
15:16:32 <bcoca> That this should be an AnsibleError instead of a task failure?
15:16:56 <nitzmahone> (sorry, just that I agree the current behavior is probably wrong and that a syntax error is the right thing)
15:17:07 <nitzmahone> Yeah
15:17:10 <bcoca> ok, that is what i was going for
15:17:21 <bcoca> but seems to be long standing behaviour, i just went down to 2.5 ...
15:17:40 <sivel> I have no opinion really, but yes as bcoca just pointed out, it's long standing behavior
15:17:48 <bcoca> 2.3 ...
15:17:49 <sivel> so it *could* cause issues if we change it
15:18:02 <nitzmahone> The question is should that apply to all FieldAttribute template failures? I would kinda lean yes...
15:18:09 <bcoca> nitzmahone: yep
15:18:25 <nitzmahone> Rather than a smaller subset
15:18:26 <bcoca> its actually happening on 'task.args'  templatting
15:18:43 <bcoca> which prevents rest of fieldattributes being tempalted
15:19:42 <sivel> I mean, a failure to template ignore_errors, could itself cause ignore_errors to be evaluated as truthy as is
15:19:43 <nitzmahone> Well, shall we just mark it for 2.9 and move on?
15:20:13 <nitzmahone> (don't necessarily need to design/debate the fix now)
15:20:14 <bcoca> sivel: yes, but at that point it should be a AnsibleParserError and ignore_errors is irrelevant
15:20:25 <cyberpear> with this break block rescue usage?
15:20:30 <sivel> bcoca: I think I am re-enforcing what you and nitzmahone have said
15:20:43 <bcoca> cyberpear: yes, since it would be syntax issue and block/rescue is based on task status
15:20:56 <sivel> a failure to template ignore_errors shouldn't cause ignore_errors to be true.  So a parser error, instead of a task error would seem to make more sense
15:21:00 <bcoca> sivel: ah, ok, misunderstood your last statement
15:21:08 * jtyr has to pop out to pickup a child from school. Will be back in 20 mins to discuss PR #56817.
15:21:27 <sivel> bcoca: I think I clarified more in that last statement
15:21:35 <bcoca> well, its 'true' now cause we are capturing previous error and makign it a 'task fail' vs actual parser error, if it were, we would not have evaluated ignore_errors at all
15:22:06 <bcoca> k, so p2 imho, with note about 'fix is making undeifned task def into parser error instead of task fail'
15:22:12 <sdoran> We should fix this for 2.9 and add a porting guide note.
15:22:16 <sivel> Ok, so it sounds like we should indicate on that ticket that the current behavior of an undefined var is wrong
15:22:16 <bcoca> jtyr: noted
15:22:35 <sivel> sdoran: yes, I think that is what we are saying
15:22:39 <bcoca> sdoran: agreed
15:22:39 <sdoran> Yes, I agree that the current (longstanding) behavior is wrong.
15:22:49 <bcoca> k, so that makes 4/0 .. anyone against?
15:22:54 <sivel> I'll update the issue
15:23:01 <bcoca> roger, thankx
15:23:22 <bcoca> #topic https://github.com/ansible/ansible/pull/53438
15:23:30 <Sajna_Shetty> Hi
15:23:49 <bcoca> this was gpl exception for NEW module_utils file?
15:24:09 <Sajna_Shetty> Can we get core team approval for using GPLv3+ license for module_utils
15:24:20 <bcoca> and the reason for it was?
15:24:21 <Sajna_Shetty> last time bcoca was not there in meeting
15:24:58 <sivel> Sajna_Shetty: can you state why this module_utils file should be GPLv3+ instead of BSD?
15:25:04 <Sajna_Shetty> "Our module utility -  module_utils/remote_management/dellemc/ome.py is only meant for dellemc modules, and our legal team insists GPLv3+"
15:25:42 <Sajna_Shetty> it will be hard to get approval for BSD
15:25:46 <bcoca> well, the reason we want BSD is not for using for 'other remote management appliances' but so 3rd parties are free to add modules under other licenses
15:26:13 <nitzmahone> If it's just two modules, why not inline it to GPL modules and be done?
15:26:47 <bcoca> soem of that shared code already exists in other forms or is 'very generic' and not specific to the dell ome
15:27:01 <bcoca> i.e openurlresponse
15:27:28 <bcoca> buildurl/get_base_url .. i really see no reason for these
15:27:53 <Sajna_Shetty> yes. and for session handling
15:28:00 <bcoca> i would argue that most of that common code is not 'specific'
15:28:15 <cyberpear> those all sound super generic
15:28:57 <bcoca> they are, i only see 2 specific things with sesison and that can easily be refactored to be passed in
15:29:18 <bcoca> so as is, im -1 for this exemption
15:29:23 <Sajna_Shetty> okay
15:30:01 <bcoca> anyone else want to weigh in?
15:30:10 <cyberpear> -1
15:30:24 <rajeevarakkal> bcoca: we are planning to have more dell emc ome mdules..which uses this ome.py..
15:30:27 <bcoca> well, was asking for arguments, but if we want to do a vote, im also ready
15:30:46 <bcoca> rajeevarakkal: then we can asses, but im remiss to grant an exception on the current file just cause 'future ...'
15:30:52 <Sajna_Shetty> last time got +1 from jillr
15:32:04 <bcoca> so if no more arguments against/for, i'll call avote in 1min
15:32:10 <rajeevarakkal> becoca: okay..
15:32:21 <nitzmahone> Maybe it should wait to be published as a collection then- you should have much more flexible licensing there.
15:32:42 <bcoca> ^ that is another option, then you have full control over license and release
15:32:57 * cyberpear needs to look at the docs for collections
15:33:15 <nitzmahone> Somebody needs to write them first ;)
15:33:27 <bcoca> ^ someone ...
15:33:39 <cyberpear> heh, makes sense why I haven't seen them, then.
15:33:53 <bcoca> https://github.com/bcoca/collection <= a skeleton
15:33:56 * nitzmahone hides
15:34:14 <rajeevarakkal> what is collection?
15:34:33 <bcoca> new way to distribute ansible content, 2.8 has preview support
15:34:45 <bcoca> allows for better distribution than roles for plugins in general
15:35:04 <nitzmahone> The rest of the tooling comes during 2.9, at which point it will also have docs
15:35:25 <bcoca> we have tools in 'working' stages right now and people are in middle of writing docs both for author/user
15:35:30 <bcoca> so its a bit early
15:35:44 <bcoca> but it does bypass needing all this 'conformance' to core
15:35:53 <rajeevarakkal> becoca: hmm, now we are looking for GPLv3+ license as dellemc want to be sole owner of this and its getting consumed by our multiple modules
15:36:07 <rajeevarakkal> https://github.com/dell/dellemc-openmanage-ansible-modules/tree/devel/library/dellemc
15:36:14 <nitzmahone> (and will likely be the way all new stuff will go in the not too distant future)
15:36:50 <rajeevarakkal> and we have followed the singe vendor .. module utils guide lines
15:37:06 <nitzmahone> Sounds like collections is the way you should go, that way you're also not dependent on our release cycle
15:37:10 <bcoca> rajeevarakkal: and that makes sense for certain code, but i dont think the file in the PR meets the theshold as those are generic functions
15:37:13 <rajeevarakkal> is that possible to get an approval for us to use GPLv3+
15:37:50 <bcoca> rajeevarakkal: its possible, but that depends on vote, and right now im strongly against
15:38:01 <rajeevarakkal> becoca: okay..
15:38:30 <bcoca> i would need to hear a good argument for making generic code gpl in module_utils
15:39:09 <sivel> I think to proceed with GPLv3+ we would need an explanation likely from your legal team why BSD is unacceptable
15:39:26 <sivel> and not that it would necessarily change the vote
15:40:18 <rajeevarakkal> sivel: sure, we will check with our legal once again and will check collection as well like becoca suggested
15:40:47 <sivel> The reason we have this process, is that a GPLv3 license can prevent others from utilize that code in 3rd party modules, that may not be able to license their code as GPLv3, and need a less restrictive license
15:41:47 <nitzmahone> Basically if you go the collection route, keep writing the modules and utils as you are now, and the tools and docs will come during 2.9, so they could be available at the same time as if they went in 2.9 core, but you'd also have full control over the release cycle, changes, and new modules
15:42:17 <rajeevarakkal> sivel: ome.py is meant only remote_mangent/dellemc modules to interact with dell devices
15:42:34 <nitzmahone> If you ship in core, you can't add new modules/features until 2.9++, and bugfixes go out on our schedule
15:42:36 <bcoca> i would also advise looking at module_utils/urls.py as a lot of the common code already does what you want
15:42:40 <rajeevarakkal> anyways we will check with legal team once and try to join next meeting
15:42:47 <sivel> rajeevarakkal: yes, but anyone else, such as some 3rd party, wanting to write a dellemc module, and utilize that code, would cause issues for that 3rd party
15:43:09 <rajeevarakkal> sivel: i got it :)
15:43:26 <bcoca> #topic https://github.com/ansible/ansible/pull/55402
15:43:34 <rajeevarakkal> thanks all :)
15:43:40 <Sajna_Shetty> thanks all
15:44:02 <bcoca> cyberpear: ^ i already have PR that is superset of that, we also need to fix other things before that 'works'tm, that PR only covers part of it
15:44:33 <cyberpear> I'd like to get either this or your equivalent equivalent emerged in time for 2.8.1
15:45:03 <bcoca> well, -1 on this since it doesnt cover all the issues, as for my PR .. we'll see
15:45:22 * cyberpear was disappointed it missed 2.8.0
15:45:25 * jtyr is back
15:46:11 <bcoca> cyberpear: so was i
15:46:48 <bcoca> anyone else want to weigh in?
15:47:31 <nitzmahone> TBH, they seem kind of orthogonal
15:47:59 <bcoca> my pr includes his changes, but we need to validate choices or its still meaningless since 'any value' would trigger the option
15:48:05 <nitzmahone> (ie why does the group behavior need to be in that PR, which is ostensibly about renaming envvars)
15:48:12 <bcoca> also we are not displaying them correctly, which is another thing peopel need to get the choices
15:48:33 <bcoca> renaming envvars?
15:48:40 <bcoca> dont think eitehr PR does that
15:49:08 <nitzmahone> fair enough
15:49:47 <cyberpear> my change is very narrow. The other one includes docs tooling improvements
15:50:15 <bcoca> which are needed for that change to make sense, otherwise 'any value' goes
15:50:16 <cyberpear> both are good and satisfy my requirement, I just want folks to have the option to squash the warning.
15:50:29 <bcoca> which already works, but is not documented
15:50:45 <cyberpear> it does not work. You still get a deprecation warning
15:50:58 <cyberpear> your change also fixes it to avoid the deprecation warning.
15:51:54 <bcoca> as i said, yours is a subset of mine
15:52:19 <cyberpear> so let's just merge yours
15:52:38 <bcoca> can you just wait for people to ok it?
15:52:38 <cyberpear> I'm mostly concerned about it being an access point release not whose PR is merged
15:52:47 <nitzmahone> His would probably just be 2.9 though
15:52:51 <bcoca> my pr is going through normal process
15:52:51 <cyberpear> next Point release
15:53:16 <bcoca> cyberpear: yours would not be backported as it woudl be seen as simple 'new feature'
15:53:45 <cyberpear> a new feature to avoid an erroneous deprecation warning?!
15:53:53 <bcoca> not erroneous
15:53:59 <nitzmahone> bcoca's would probably also be scrutinized heavily for backport
15:54:04 <bcoca> you jsut dont want to see it
15:54:08 <cyberpear> it was already agreed and previous meeting to have an option to avoid the deprecation warning
15:54:11 <sivel> I'm just curious why you would want to opt in to allowing a feature that will be removed
15:54:13 <bcoca> nitzmahone: yes, but as a general fix it has better chances
15:54:23 <cyberpear> a feature is not being removed, a default is being changed.
15:54:32 <bcoca> sivel: no, the deprecation is on defualt value, not on feature itself
15:55:03 <sivel> Ah, I may have misunderstood the original feature. I assumed we were removing the ability for invalid chars in group names
15:55:10 <nitzmahone> We did agree to have a no warning option
15:55:31 <sivel> otherwise, I'm unsure why we have the warning at all, if we aren't going to prevent users in the future from using them
15:55:32 <bcoca> and you remove the warning from the substitution, but not from using the default value
15:55:54 <bcoca> sivel: cause we'll change the default and they will 'automatically' get transformed names
15:56:03 <bcoca> ^ that is not something you want to suprise peopel with
15:56:05 <nitzmahone> It's a problem for Tower if we remove it entirely (among other things)
15:56:11 <bcoca> yes
15:56:15 <cyberpear> which is why we're adding an option for them to explicitly opt into the old Behavior
15:56:34 <bcoca> no, you are adding a option to not get ANY notice, even on deprecated default
15:57:00 <sivel> ¯\_(ツ)_/¯ I'd just give a warning for a deprecation period, and then remove the ability to use invalid chars. I assumed that was the plan
15:57:03 <cyberpear> yes but in that case the user has chosen a non default value
15:57:19 <bcoca> sivel: it was at one point, but got soo much push back we had to revise it
15:57:34 <nitzmahone> There should be a method to silence the warning for explicit opt in
15:57:43 <cyberpear> that's exactly what this PR does
15:58:29 <sivel> I suppose my comment is that if we aren't going to remove it, we should just remove all the warnings, and let people do what they did previously without any workarounds
15:58:47 <bcoca> sivel: then when we change the default, they will get 'different groups'
15:58:53 <sivel> otherwise, there is no incentive for people to change
15:58:56 <bcoca> i.e x-this before, x_this after
15:59:02 <bcoca> ^ that is why there is a warning
15:59:04 <sivel> bcoca: I'm saying remove that feature completely
15:59:24 <sivel> remove the config, remove the group manipulation
15:59:41 <sivel> if there is no deprecation, users are incentivized to do anything worthwhile
15:59:42 <bcoca> then we are back on people opening tickets cause groups.x-this breaks in jinaj2
15:59:46 <nitzmahone> We already went circles on that, we can't remove the feature, but we're trying to nerf it for most people
16:00:03 <sivel> bcoca: we get plenty of those issues for other modules and such
16:00:07 <bcoca> ^ if we remove, it will be in 2.13
16:00:10 <sivel> anyway, you have my opinion
16:00:18 <bcoca> since it already shipped in 2.8
16:00:28 <nitzmahone> If you want to opt into obnoxious group names and the pitfalls it brings, ok
16:00:41 <cyberpear> 👍
16:00:48 <nitzmahone> The warning is just about the impending nerfing
16:01:00 <bcoca> cyberpear: in any case, your PR wont be backported even if we merge now
16:01:37 <bcoca> mine 'might' be
16:01:39 <cyberpear> bcoca: I doubt everyone feels that way... vote?
16:01:54 <cyberpear> my pr is trivial
16:02:13 <sivel> it's a new feature right?  ignore is now a new option
16:02:17 <bcoca> backports are not an issue of voting
16:02:36 <cyberpear> it's in the code, just not documented, but there is a big
16:02:37 <bcoca> sivel: exactly why it wont be backported
16:02:40 <cyberpear> bug
16:03:00 <cyberpear> the ignore option exists today. It squashes the normal warning, but not the deprecation wanting.
16:03:32 <bcoca> a) it does not exist, you can use any 'non choice' to achieve that b) adding it as an option is a 'new feature'
16:03:41 <sivel> it appears to me that ignore is being added to choices
16:03:49 <sivel> where it did not exist before
16:04:26 <cyberpear> line 39
16:04:27 <sivel> I would claim that the bug is that the word `ignore` is listed in the code, when it was never implemented
16:04:30 <cyberpear> its there
16:05:08 <cyberpear> ignore and never are the only options that avoid group munging
16:05:18 <cyberpear> today
16:05:27 <nitzmahone> The fact that it works the way it does makes it "work" in 2.8.0, so if we fix it in 2.8.1 and document it, it would still work as documented in 2.8.0 (which is usually the reason we don't allow backport of new UI)
16:06:13 <nitzmahone> So I wouldn't be opposed to a backport in this specific case, but I also don't have strong feelings either way
16:06:38 <bcoca> but also not 'us' that need convincing and i see our prospects as 'not likely'
16:07:11 <cyberpear> who needs convincing?
16:07:17 <nitzmahone> Exactly- unless a majority wants to override the 2.8 RM, it's his call whether to accept the backport
16:07:30 <nitzmahone> abadger1999: you around?
16:07:41 <abadger1999> Yep
16:07:47 <nitzmahone> Opinion?
16:08:02 * abadger1999 trying to find context
16:08:37 <cyberpear> okay... what is the argument in favor of forcing a deprecation warning for functionality that is not going away, that a user has explicit opted into the old behavior?
16:08:54 <bcoca> deprecation is not for functionality, its for the default of the toggle
16:09:02 <cyberpear> the old behavior is not going away, only default changing
16:09:55 <cyberpear> yes, but if user explicitly opts in it old behavior, he should be spared the depreciation warning for 2 releases
16:10:21 <bcoca> which is diff issue than introducing new option
16:10:47 <bcoca> that is another fix that needs to go into my pr, detect default vs explicit
16:10:48 <abadger1999> Are we moving to a world where those chars are illegal?
16:11:06 <bcoca> abadger1999: by default, yes, but users can force old behaviour if they insist
16:11:12 <cyberpear> no, but by default, they will be translated
16:11:25 <abadger1999> But I mean, is the deprecation valid.
16:11:40 <abadger1999> ie: "this behaviour is deprecated, it will go away in the future" ?
16:12:04 <abadger1999> If so, then I agree with sivel that ignore being listed in the code is the bug.
16:12:10 <bcoca> no, thre is a bug with default detection being same as explicit
16:12:13 <cyberpear> by choosing "ignore" explicitly, he has acknowledged the default change, and opted out. the behavior is not going away, only a default change
16:12:28 <abadger1999> If that behaviour is actually going to exist forever, then the deprecation is wrong and should be removed.
16:12:47 <bcoca> default behavior is going to change
16:12:57 <nitzmahone> The confusion is that the deprecation warning is about the default changing
16:12:57 <bcoca> that is what the deprecation is about
16:13:15 <cyberpear> default today is 'never' so but choosing 'ignore' the user has acknowledged the change
16:13:30 <bcoca> ^ that is a new feature
16:13:35 <nitzmahone> We agreed when designing that that there should be a way to disable that warning, it just didn't make it into the code
16:13:49 <bcoca> nitzmahone: but the warning we talked about wasa diff than the deprecation
16:13:58 <cyberpear> the feature exists, but it only squashed the non dep warning
16:13:59 <bcoca> it was warning on group transform, which you DO avoid
16:15:05 <nitzmahone> The details escape me, but we should never have an undisable-able warning for something like that, otherwise it's just noise
16:15:21 <cyberpear> https://meetbot.fedoraproject.org/ansible-meeting/2019-04-16/ansible_core_public_irc_meeting.2019-04-16-19.01.log.html#1-49
16:15:21 <bcoca> which is the bug, default detection, not 'new option'
16:15:39 <cyberpear> this was already hashed out and agreed to, just the PR was not merged in time for 2.8.0
16:15:45 <bcoca> the issue is that if you set to default explicitly, its not clear
16:16:19 <nitzmahone> Which bcoca's PR addresses, but IMO is unlikely to be eligible for backport
16:16:19 <bcoca> cyberpear: the agreed PR WAS merged, that is why there is an option at all
16:16:30 <cyberpear> which PR was merged after that meeting?
16:16:46 <cyberpear> my PR was created immediately following that meeting in direct response to the agreement there
16:16:54 <cyberpear> same day
16:17:02 <abadger1999> Okay, I think that something cyberpear's change can probable go in.
16:17:10 <abadger1999> But I'm not sure if cyberpear's exact change can.
16:17:16 <abadger1999> I'll have to look at that closer.
16:17:28 <abadger1999> *something like
16:17:48 <cyberpear> I don't care about the particulars, only that I can squash the deprecation warning for a feature that isn't going away
16:17:49 <bcoca> i have alternative PR that solves more of the underlying issues with choices and defaults
16:17:57 <abadger1999> Would you folks want this to land in devel and then be backported or only land in 2.8?
16:18:00 <cyberpear> I'm fine w/ bcoca's PR but it is more involved
16:18:25 <cyberpear> (my PR is a tiny subset of bcoca's)
16:18:27 <bcoca> yes, cause it also deals with incorrect display of options, validation and defaults
16:18:29 <abadger1999> It sounds like bcoca's PR would not be eligible for backporting if we're already concerned that this more minimal change would not be eligible for backporting, though?
16:18:36 <nitzmahone> We need a way to completely silence the warnings- I agree that an unsilenceable warning on that is a bug.
16:18:56 <abadger1999> How long do you guys want to wait for me to look at this in the meeting?
16:18:57 <bcoca> but bugfix should be 'correct explicit detection' not new option
16:19:20 <cyberpear> I'm happy to reword it however you'd like
16:19:31 <nitzmahone> Beyond that, I don't have strong feelings. If the behaviors are logically equivalent, merge something like cyberpear's and backport, then bcoca's can subsume for 2.9
16:20:14 <nitzmahone> So long as the warning can be silenced in some fashion that works the same way in 2.8 as in 2.8++
16:20:46 <nitzmahone> abadger1999: just knowing you're open to a backport of some form is enough for now
16:20:49 <bcoca> depends on which warning, teh original, yes, it can, the deprecation, that would be a change
16:20:56 <abadger1999> yeah... so it looks like cyberpear's change is not quite right... we need a sentinel value for the default which the user can never select.
16:21:07 <bcoca> ^ that is what im adding to my PR
16:21:20 <bcoca> as that fixes the underlying issue 'explicit' vs 'default'
16:21:43 <bcoca> but not using sentiel, but 'origin'
16:21:45 <abadger1999> Then the warning will only trigger if TRANSFORM_INVALID_GROUP_CHARS is the default
16:21:46 <cyberpear> the sentinel value is currently 'never'.  undocumented option 'ignore' is the non-default value the user has explicitly chosen
16:21:48 <nitzmahone> If that change is isolated and fixed for the one case, I'd argue it's backport able
16:21:49 <bcoca> as we keep 'origin' in config
16:22:19 <nitzmahone> But there are a lot of other things in bcoca's PR right now that probably aren't backport worthy
16:22:38 <bcoca> well, they fix a) lack of 'choices' validation, b) incorect display of docs
16:22:42 <cyberpear> but I don't care how the problem is tackled... I meant mine to be the minimum to fix the problem
16:22:49 <bcoca> and d) (workign on) origin detection
16:23:03 <bcoca> cyberpear: but yours does nto fix the problem, it creates a diff bypass
16:23:14 <bcoca> the 'fix' is detecting when 'never' is explicit vs default
16:23:45 <cyberpear> bcoca: if you have a better fix, I'd be thrilled to have it merged... I only care that some fix is merged, not which one
16:24:37 <bcoca> PRs still need to go thorugh review process, i cannot just merge it
16:24:51 <cyberpear> that's fine with me
16:25:12 <cyberpear> I originally closed my PR because yours is a superset.  I re-opened it since yours didn't seem to be making progress towards 2.8.1
16:25:23 <cyberpear> (and realizing that no fix had made 2.8.0)
16:25:26 <abadger1999> yeah, I don't think that bcoca's PR uses origin either?  (And it does have too much other stuff to be backported)
16:25:31 <bcoca> k, ending this now since we are waaay over time and this is just rehashing at this point.
16:25:41 <bcoca> abadger1999: no, as i said, its not in my PR yet, its something im workign on still
16:26:09 <bcoca> abadger1999: and the other stuff is also 'bugs' that exist and shoudl be fixed in 2.8
16:26:17 <abadger1999> So either cyberpear or bcoca would need to implement just the correct changes making use of origin to both fix this and get it backported.
16:26:58 <cyberpear> is there an existing example of using origin someone can point me towards?
16:26:58 <abadger1999> cyberpear, bcoca : You guys probably should #action which of you are going to do which actions.
16:27:13 * cyberpear not clear what more is needed of his PR
16:27:36 <bcoca> your pr does not fix the issue, it bypasses it by changing and makaing 'ignore' explicit
16:28:00 <bcoca> 'ignore' was for the substitution warning, had nothing to do with deprecation
16:28:03 <bcoca> so your pr is invalid as is
16:28:20 <cyberpear> I don't see how it's any different than that part of your PR
16:28:21 <bcoca> the deprecation should be skipped when ANY explicit value is set
16:28:35 <cyberpear> I agree, I just don't know how to do that
16:28:43 <bcoca> cyberpear: its not, currently, i just said my PR is not correct yet
16:28:59 <bcoca> neither is ready
16:29:05 <nitzmahone> So if we can get origin detection standalone and applied to this case, then it's backportable IMO
16:29:18 <bcoca> well, all 4 bugs are backportable
16:29:28 <bcoca> the only thing that is not is adding the new option
16:29:48 <cyberpear> "new option" is already there just not documented
16:29:51 <bcoca> if what you are saying i need to do this in 4 prs
16:29:52 <nitzmahone> There does need to be a fix for the noisy warning in 2.8.x, and I'd also prefer it to be origin based
16:30:22 <bcoca> well, that is why i started with 'validate choices' which was diff, but related bug
16:31:45 <abadger1999> cyberpear: Each setting has an origin attribute.   https://github.com/ansible/ansible/blob/devel/lib/ansible/config/manager.py#L36 )  and ( https://github.com/ansible/ansible/blob/devel/lib/ansible/config/manager.py#L522 to EOF )
16:32:24 * cyberpear looks
16:32:55 <bcoca> ^ which is what im trying to use in my patch, but 1/2 written, not ready for even commit yet
16:32:56 <abadger1999> cyberpear: I'm not 100% sure how that is exposed but you'd need to get the Setting and then look at the origin attribute to tell whether "never" was explicitly set or a default value.
16:33:35 <bcoca> there is api for it, which is what im using
16:34:01 <cyberpear> bcoca: once you've got it done, would you be okay splitting out just that patch into a separate PR? -- if so, I'm happy to close my PR, assuming yours could be backported
16:34:10 <abadger1999> cyberpear, bcoca :  Correct me if I'm wrong but i don't think that "ignore" needs to be implemented in the backport either.
16:34:37 <abadger1999> It just needs to check whether never is from default or explicit.
16:34:37 <nitzmahone> and IIUC ignore works now by accident- would work the same as `yourmom` or anything else, right?
16:34:43 <bcoca> abadger1999: it does not, once we have origin we can bypass deprecation notice on any explicit set
16:34:48 <abadger1999> <nod>
16:34:53 <cyberpear> abadger1999: "ignore" already exists, but I agree it doesn't need to.  Currently, it squashes the "groups not munged" warning, but not the "defaults will change" warning
16:35:17 <bcoca> cyberpear: it was never meant to squash that as it is unrelated to the value
16:35:30 <bcoca> its related to it being set explicit or using defaults
16:35:46 <cyberpear> it does squash it today whether it was intended or not
16:35:59 <cyberpear> (the first warning, but not the second)
16:36:02 <abadger1999> yeah, so please, for backporting, get me a patch that just includes the never-origin change and I think that should definitely be acceptable.
16:36:04 <bcoca> no, it does not, otherwise you would not need change in your PR
16:36:20 <abadger1999> Other things can go into a separate backport PR if we want to discuss whether those are also allowable or not.
16:36:28 <bcoca> well, yes, as i said, 2 diff things, warning vs deprecation warning
16:36:56 * cyberpear will wait for bcoca's origin fix
16:37:05 <bcoca> #endmeeting