18:59:58 <jborean93> #startmeeting Ansible Core Public IRC Meeting
18:59:58 <zodbot> Meeting started Tue Oct 20 18:59:58 2020 UTC.
18:59:58 <zodbot> This meeting is logged and archived in a public location.
18:59:58 <zodbot> The chair is jborean93. Information about MeetBot at http://wiki.debian.org/MeetBot.
18:59:58 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
18:59:58 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting'
19:00:03 <nitzmahone> boo
19:00:05 <nitzmahone> o/
19:00:07 <jborean93> darn
19:00:10 <felixfontein> hi :)
19:00:13 <jborean93> #chair nitzmahone
19:00:13 <zodbot> Current chairs: jborean93 nitzmahone
19:00:17 <jborean93> hey felix
19:00:39 <felixfontein> hi jordan!
19:01:08 <jborean93> give it a minute to see if anyone else is joining before we get started
19:01:32 <felixfontein> sure :) just made it in time myself, gotta open up some tabs
19:02:44 <sdoran> \o
19:02:54 <jborean93> #topic https://github.com/ansible/ansible/pull/72248
19:02:58 <jborean93> hey
19:03:05 <jborean93> let's get started
19:04:07 <jborean93> so this PR changes whether an option that is set to `None` still meets the `requires_*` checks or not
19:04:11 <felixfontein> that PR fixes an issue tremble brought up this spring
19:04:40 <jborean93> does it also affect when an option is `mandatory=True` and the value is set to `None`?
19:04:46 <felixfontein> right now, if a user explicitly sets something to `None`, it counts as set, even though no module I know of expects that
19:05:10 <felixfontein> do you mean required=True?
19:05:40 <felixfontein> if an option is say type=str, required=True, and the user explicitly sets it to None in a task, the module will no longer accept it with this PR
19:05:41 <jborean93> yes sorry, was thinking of PowerShell parameters
19:05:56 <felixfontein> no problem, I did a quick grep and didn't find anything, so I assumed you mean required :)
19:06:19 <jborean93> unless they also set `allow_none_value=True` right?
19:06:23 <felixfontein> yes
19:06:34 <sdoran> I've been meaning to review this one.
19:06:41 <felixfontein> there might be some modules which actually want this behavior, but I think the vast majority does not
19:06:42 <sdoran> Trying to get my thoughts together.
19:06:57 <sdoran> First, we don't want to create a new private method.
19:07:00 <felixfontein> hi sam!
19:07:08 <sdoran> `_check_required_none()` should be a function.
19:07:11 <felixfontein> I assumed that you'd sooner or later will have to look at it ;)
19:07:41 <sdoran> I've been moving those out of `AnsibleModule`.
19:08:20 <nitzmahone> Off the top of my head, I'm thinking rather than making a potentially breaking semantic change, would it make more sense to use a sentinel? We've done this in many other places...
19:08:22 <felixfontein> I assume the function should raise an exception instead of calling fail_json?
19:08:22 <sdoran> I agree in principal with the bug fix: setting `None` as in input should be an error in almost all cases.
19:08:32 <sdoran> But I am worried about the breaking change.
19:08:39 <sdoran> And the interface.
19:09:07 <jborean93> nitzmahone: the breaking change is the fact that `None` is now not considered set, so to avoid the breaking change we would have to inverse the deault of `allow_none_value`
19:09:13 <nitzmahone> I thought we'd already done that internally anyway on the module utils validator, but sounds like I'm wrong
19:09:13 <sdoran> Yes. And handle the exception by calling `fail_json()` until we have a better solution to that.
19:09:59 <felixfontein> so should I wrap the _check_required_none() call (or however it will be called) every time with `try`/`except`?
19:10:10 <nitzmahone> Well, and we already have `omit` for explicitly making something "unset"
19:10:41 <nitzmahone> But I'm pretty skittish about breaking explicit `None` for existing code
19:10:44 <felixfontein> nitzmahone: omit works on playbook level, it makes sure that the option is not passed to the module at all. but the user can also explicitly pass none (and some people have been doing that in tests)
19:11:00 <jborean93> that could be done to not knowing about omit in general
19:11:17 <nitzmahone> I assume you mean `~` for integration tests, `None` for units
19:11:24 <felixfontein> there's the new allow_none_value which allows module argspecs to explicitly request the old behavior, and older Ansible versions will simply ignore it
19:11:32 <jborean93> but making a breaking change, regardless of how obscure it is, isn't really something we usually do. Especially without a deprecation period
19:11:36 <nitzmahone> Yeah
19:11:44 <felixfontein> nitzmahone: yes, or `{{ none }}`, or passing a `null` in from some JSON input
19:11:49 <nitzmahone> *especially* to the module API
19:11:59 <jborean93> 3rd party people do crazy stuff sometimes
19:12:17 <jborean93> I would be ok with the changes but would probably err to the side of changing the default to be the existing behaviour
19:12:23 <nitzmahone> I'd be -1 to hard break, and even -0.5 to deprecation
19:12:26 <jborean93> and people can opt into `None` being treated as unset
19:12:42 <felixfontein> jborean93: so essentially EVERY module has to be adjusted?
19:12:53 <jborean93> it doesn't really affect every module though
19:13:01 <jborean93> just a tiny subset who might be dealing with `None`
19:13:07 <sdoran> I would also rather do this in such a way that doesn't required changing the signature and call of every single method. But I would have to dig into it more to see how that could be done.
19:13:07 <felixfontein> it doesn't affect modules which have no required parameters at all
19:13:14 <felixfontein> every other module is affected
19:13:31 <jborean93> but you could argue that's due to a bad task setup
19:13:34 <tremble> jborean93 The problem is that anyone using required who assumes that they'll actually get a value will be affected.
19:13:38 <jborean93> why would someone do `None` over `omit`
19:13:42 <felixfontein> sdoran: I tried that first, but I somehow need to know which fields are required, without having to replicate the logic which looks at all the required_* things
19:13:56 <felixfontein> jborean93: it was done in some aws tests for example
19:14:15 <jborean93> that just sounds like the tests were done incorrectly, is there a reason why it had to be None vs omit
19:14:46 <felixfontein> it was a bug in the tests. which didn't surface until a required_if or something like that was changed/added and suddenly something broke
19:15:06 <jborean93> yep but this PR then tries to avoid that bug by making a breaking change in the interface
19:15:31 <tremble> It was a bug in the test, but it surfaces behaviour that users will try to use instead of "{{ omit }}"
19:15:34 <jborean93> people who may be relying on `None` being a valid setting are now broken. Granted it's a edge edge case but I wouldn't say they don't exist
19:15:54 <felixfontein> this bug was already discussed at an earlier meeting: https://meetbot.fedoraproject.org/ansible-meeting/2020-05-05/ansible_core_public_irc_meeting.2020-05-05-19.05.log.html
19:16:27 <felixfontein> jborean93: that's true, but that happens to everyone who relies on a bug :)
19:17:54 <tremble> as soon a people start using module_defaults it's not obvious that {{ omit }} will override the module default.
19:18:13 <jborean93> just trying to bring in some others to get a solid consensus
19:18:48 <jborean93> does it?
19:19:09 <felixfontein> in any case, in all modules I've seen so far, 'required' for a modlue option always meant 'will never be None'
19:19:55 <jborean93> I don't deal with Python modules too much but wouldn't the checks be checking if the value is None or not anyway. It's just the requires_* validations that really are affected here
19:20:00 <bcoca> i would not start by forbidding it, also there is the case wehre 'None' is a valid value and/or the 'default value'
19:20:15 <nitzmahone> That goes back to the need for an internal sentinel
19:20:17 <jborean93> because modules would be doing `if module.params['option'] is not None:` or just `if module.params['option']:`
19:20:18 <felixfontein> jborean93: all modules I know use required / required_* to let AnsibleModule check that something is not None
19:20:34 <felixfontein> jborean93: they do that check if the option can be None
19:20:45 <sdoran> Yeah, most modules assume the arg spec validation gives the correct type already.
19:20:48 <jborean93> but to control the actual behaviour in the cost when it goes to use them and select the use case
19:20:53 <felixfontein> there's no way to distinguish between "not specified" and "user specified None"
19:20:58 <sdoran> This allows an errant `None` to sneak in.
19:21:36 <bcoca> its also a way for user to force 'unspecified' .. but 'omit' is a better option for that
19:21:59 <felixfontein> bcoca: usually if a module requires something, something bad will happen if None is specified
19:22:28 <felixfontein> like if `path` is required, and the user sets `path: ~`, and the module tries to open that path (or call os.path.exists), it will crash
19:22:39 <bcoca> no disagreeing, but a change that checks value supplied is none AND the default into the required_ function might be a better way to handle this w/o backwards incompat
19:22:54 <jborean93> so for me, it's a -1 on the breaking change. I would be ok for a warning message to appear if a value is explicitly `None` to say, did you mean to use `omit`
19:23:03 <bcoca> ^ agreed
19:23:19 <felixfontein> if the module crashes, the warning won't be displayed, right?
19:23:20 <nitzmahone> (for things that don't explicitly opt in to allowing it)
19:23:24 <jborean93> yep
19:23:50 <nitzmahone> Warnings would currently be lost for unhandled exceptions, but not fail_json IIRC
19:23:51 <bcoca> that is fine, if 'new author' opts into behaviour, its fine
19:24:02 <felixfontein> nitzmahone: crashing == fail_json is not called
19:24:03 <bcoca> nitzmahone: correct
19:24:22 <sdoran> I agree with jborean93. Warning would be better.
19:24:59 <bcoca> felixfontein: if its crashing we can point all to this bug and have module author fix it as they see fit, otherwise we are forcing a break on all authors
19:25:03 <jborean93> completely side track, would be good for an unhandled exception to be handled in AnsiballZ that then calls fail_json to preserve this info
19:25:14 <jborean93> that's what happens with PowerShell
19:25:38 <nitzmahone> Heh, we played with that in the past, but IIRC requires AnsiballZ to look more like the PS wrapper ;)
19:25:47 <bcoca> jborean93: we have 'crashed module handler' on controller side, but yeah, that wont preserve warnigns/deprecations lists
19:25:51 <jborean93> and that would be a bad thing how :)
19:26:00 <nitzmahone> (been a long time, there were issues with it in the current design of AnsiballZ that I don't remember)
19:26:16 <bcoca> jborean93, nitzmahone i would put that in 'execution wrapper' for future
19:26:23 <shertel> the code makes sense to me in terms of how I would have expected it to work, but I agree with avoiding the breaking change and making it a warning
19:26:26 <bcoca> that will handle async/normal/mitgoenishlike features
19:26:27 <felixfontein> should the default for allow_none_value change in a later version (i.e. deprecation warning instead of warning)?
19:26:34 <nitzmahone> TBD
19:26:40 <jborean93> but not wanting to linger on this topic for too much longer. It sounds like the general consensus is the option is fine but it shouldn't be a breaking change
19:26:43 <felixfontein> if not, 99.9% of all modules need quite some changes to their argspec
19:26:46 <nitzmahone> That might be an acceptable "3.0" breaking change ;)
19:27:06 <jborean93> honestly I personally would be fine with a deprecation
19:27:15 <jborean93> it's not a massive change that I think warrants 3.0 to me
19:27:50 <nitzmahone> (the changing of the default to "fail", I meant)
19:27:54 <bcoca> i would add facility for 2.11, then talk about deprecating existing in 2.12
19:27:54 <felixfontein> the problem with not using a deprecation is that module authors won't see the warnings, except if they use their own module wrong
19:28:13 <jborean93> yep, +1 to bcoca's suggestion
19:28:16 <felixfontein> bcoca: i.e. a very short deprecation cycle?
19:28:21 <sdoran> And if there is a way to distinguish between "user provided None" vs "nothing specified, so it was set no None automatically" without so many changes to so many functions, that'd be nice too.
19:28:26 <felixfontein> ah sorry, misunderstood
19:28:38 <bcoca> deprecation should have wording 'thsi module will stop working, contact author to ...'
19:28:41 <jborean93> sdoran: not without a sentinal setup I believe
19:28:49 <nitzmahone> sdoran: yeah, a sentinel at least in the early stages of validation would be required to do this regardless
19:28:57 <sdoran> Right. I'd rather use a Sentinel.
19:29:10 <bcoca> sdoran, jborean93 yes there is, but requires pulling the args directly also
19:29:12 <jborean93> but that shouldn't be required until we add the deprecation notice
19:29:13 <felixfontein> sdoran: the problem is that it detecting it (which is easy) needs to be combined with detecting whether an option is required or not
19:29:20 <felixfontein> if the option is not required, None is a perfectly valid value
19:29:38 <nitzmahone> Unfortunately user code *must* opt into getting a sentinel, since you can't override `is` in Python :(
19:29:48 <sdoran> Yeah, it's tricky. Order of operations and context matter a lot.
19:30:07 <nitzmahone> (so we can use it internally during early validation, but user code wouldn't be able to see it unless it said it understood it)
19:31:08 <bcoca> felixfontein: so we just need the detection inside required_X functions
19:31:15 <nitzmahone> I think we'll need the sentinel to implement the warning anyway
19:31:35 <jborean93> well you can check if the option name is in the raw args couldn't you
19:31:49 <felixfontein> bcoca: then these functions need to know the argspec to know whether allow_none_value is allowed for an option or not
19:31:50 <bcoca> nitzmahone: not really, we just need to look at 'args' before they were shoved into params from the required_ functions
19:32:05 <bcoca> felixfontein: and you already ahve that info
19:32:13 <felixfontein> bcoca: also it would repeat the none check logic over many functions, instead of having it in one place
19:32:21 <felixfontein> bcoca: in these functions, no
19:33:19 <felixfontein> bcoca: https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/validation.py#L71-L98 for example receives a list of required_one_of conditions and a dictionary of module arguments, but not the argument spec for them
19:33:33 <bcoca> felixfontein: we dont have them in the 'lower function' but we do in AnsibleModule callers
19:34:06 <felixfontein> bcoca: so you want to pass the argument spec on into these lower functions?
19:34:16 <felixfontein> that looks like a much larger and more complicated change to me
19:34:21 <bcoca> _check_required_if
19:34:28 <felixfontein> since then these functions also need to be able to issue warnings
19:34:42 <bcoca> that is one way, the other is to check the params ^ in the caller function which already has direct access to spec
19:35:18 <bcoca> that is just implementatino detail, we HAVE the data, how we choose to verify .. that is on us, pass the data, do precheck  or wait and do post verification
19:35:19 <felixfontein> but then the caller function has to replicate all the logic that is now done in the loewr functions, because it has to find out *which* of the arguments are actually required
19:35:26 <felixfontein> which is right now only known to the lower functions
19:35:41 <felixfontein> (that's why I'm adding the parameter which allows to retrieve that information)
19:36:04 <bcoca> felixfontein: yes .. but again that is implementation detail, dont think it has anything to do with this being a possible solution
19:36:21 <jborean93> yep, so sounds like we have a consensus
19:36:22 <felixfontein> sdoran wanted a smaller code change, not a larger one
19:36:33 <bcoca> well, i want a pony
19:36:39 <bcoca> s/pony/ferrari/
19:36:40 <sdoran> 😁
19:36:42 <felixfontein> I can offer a cat :P
19:37:03 <jborean93> the new option is fine, just don't want to make the breaking change right now. It could be deprecated in 2.12 but just add the option for authors to opt out of and keep the existing behaviour for now
19:37:04 <felixfontein> (though I'm hoping you don't want it, because I won't give it anyway)
19:37:06 <bcoca> we all want smaller code changes, not always possible
19:37:07 <jborean93> any objections
19:37:25 <bcoca> ^ agreed with jborean93
19:37:38 <felixfontein> ok
19:37:43 <felixfontein> sounds good
19:37:50 <bcoca> specially cause 'smaller code change' is not good reason to break backwards compat
19:37:57 <sdoran> Having to pass around that much context seems like things are leaking. I'm just curious if there is way to solve it differently.
19:38:03 <jborean93> #action PR is good, no breaking changes should be made until it is deprecated at a later point
19:38:05 <bcoca> 'right change' might be more involved, but totally worth it imho
19:38:29 <jborean93> #topic https://github.com/ansible/ansible/pull/71824
19:38:34 <bcoca> sdoran: the main reason it was all in AnsibleModule was to avoid passing the context so every function could use self.moduledata
19:38:46 <felixfontein> sdoran: another solution would be to interpret `None` as "option is not specified" for required, which is a lot easier to implement. I tried that first: https://github.com/ansible/ansible/pull/72243/files
19:38:46 <jborean93> another from felix :)
19:38:56 <bcoca> when we decided 'everythign must go' the cost was 'state must be passed'
19:39:09 <felixfontein> jborean93: sorry, I flooded the agenda :P
19:39:19 <jborean93> that's ok, we wouldn't be here if you didn't :)
19:39:24 <sdoran> Then we probably need another object to hold state. Anyway, discussion for another time.
19:39:47 <felixfontein> :)
19:40:14 <felixfontein> this PR fixes some of the problems which arise then ansible.builtin. is used with certain modules/action plugins, like include_vars or set_fact
19:40:40 <bcoca> felixfontein: https://github.com/ansible/ansible/pull/71824/files#diff-d5c098faf82f2cef39f812689cfc457f65716056af06a0505dba6eca0c731038R25 <= thought we talked about adding constants in non centralized manner
19:40:42 <felixfontein> it doesn't solve all problems, but it fixes all problems I encountered so far, also problems reported by other people in #ansible-devel
19:41:25 <felixfontein> bcoca: collecting the constants for one file in the file itself satisfies non-centralized for me
19:41:34 <bcoca> lib/ansible/parsing/mod_args.py <= see for RAW params ... another constant we should have moved .. but lets not add more
19:41:52 <jborean93> I'm good with the bugfixes, seems like bcoca seems to be onto the reviews. Was there any questions or things you wanted to discuss there in general?
19:41:55 <bcoca> felixfontein: i think you took the opposite from my point
19:42:22 <cyberpear> looks like a good fix to me
19:42:38 <felixfontein> bcoca: then you have to tell me what exactly you want. because I clearly didn't understood it
19:42:50 <sdoran> I am not a fan of FQCNs for builtins, especially given many of those things are not actually modules.
19:43:10 <jborean93> I don't see why it couldn't work in both cases
19:43:12 <bcoca> less per file constants, we have MUCH duplication
19:43:36 <sdoran> This seems to be a change to Core behavior due to a need that arose from documentation. That seems like the tail wagging the dog, and is backwards IMHO.
19:44:01 <bcoca> i suspect this 'fix' would be simpler by updating ansilbe_runtime.yml to point to the short names instead of litering our code with longer lists
19:44:16 <felixfontein> sdoran: core allowed the ansible.builtlin. prefix for everything in core, so it should better also work for all special-cases :)
19:44:38 <bcoca> felixfontein: actually we didnt, we voted against but were overriden by docs
19:44:50 <jborean93> yea agreed, we shouldn't be making special edge cases, would be nice for things to be uniform
19:44:59 <felixfontein> bcoca: that was the question whether it should be mentioned in the docs, and not the question whether it works at all
19:45:28 <bcoca> i dont disagree, why i thought all thsi was too rushed ... but that didnt stop everyone from pushign forward
19:46:36 <nitzmahone> We also have the problem of introducing new reserved actions (or psuedo-actions ala meta) into the playbook syntax into the collections world; it was arguably a problem even before, but seems more likely to be hit now... We'd need to decide if we're reserving new shortnames or not.
19:47:04 <bcoca> felixfontein: im not agains the change to try to make this work, but i think this is not the final/right fix,  but might be the fix we need to live with
19:47:13 <nitzmahone> Ideally we wouldn't special-case the parsing at all, but that's not the way it works today
19:47:32 <felixfontein> bcoca: I'd be happy if this would have been fixed months ago, preferrably before the ansible-base 2.10.0 release :)
19:47:38 <bcoca> nitzmahone: well, i've alwys been saying non tasks looking like tasks leads to trouble, why i initially pushed for include to have diff syntax
19:47:40 <shertel> I have similar thoughts about not promoting FQCN for these builtins (mostly because to me, FQCN suggests another namespace can implement an equivalent, whereas these things behave more like keywords). But I think FQCN should work so for people are using overlapping names in collections and the collections list.
19:48:11 <sdoran> I do think we should fix this. But that it needs to be fixed at all stems from the fact that we started putting FQCNs for builtins in the docs.
19:48:14 <nitzmahone> I'm pretty sure we can never change that for existing stuff and maintain backwards compat
19:48:15 <jborean93> yea I don't think we should document it explicitly but I think it shoudl still work for people wanting to use FQCN in all places
19:48:17 <felixfontein> shertel: the problem is that with the `collection:` keyword, the only way to make sure you actually get the builtin thing is to preprend ansible.builtin. :)
19:48:29 <sdoran> If we didn't put FQCNs in the docs, I don't think this would be a problem that needs fixing.
19:48:41 * felixfontein is waiting for collection foo.bar to rename their foo.bar.bar_meta to foo.bar.meta
19:48:45 <nitzmahone> But eg the new implicit action for argspec validation; IMO it shouldn't be special-cased at all
19:48:55 <felixfontein> (and then someone using `collections: foo.bar`...)
19:48:57 <sdoran> Good example.
19:48:58 <nitzmahone> (and any future new keyword pseudo-action)
19:48:59 <bcoca> sdoran: it would still be a problem, just not as apparent
19:49:01 <shertel> felixfontein: and that's the only reason I am +1 to the change
19:49:06 <sdoran> (the role arg spect action plugin)
19:50:30 <bcoca> felixfontein: to clarify:  namespaces shoudl work with ALL actions/plugins
19:50:31 <felixfontein> shertel: I think that's also the main reason why docs prefers the ansible.builtin. prefix, because it makes sure you actually get what you want when copy'n'pasting that into your playbook/role
19:50:54 <bcoca> felixfontein: yes and no, more about uniformity
19:51:15 * sdoran would also like to nix the `collections` keyword
19:51:16 <jborean93> but if our docs are doing one thing we should at least try to be uniform with it
19:51:44 <shertel> sdoran++
19:51:46 * nitzmahone looks wistfully at horse running from barn in distance @sdoran ;)
19:51:52 <felixfontein> sdoran++
19:51:53 <bcoca> nitzmahone, felixfontein i was thinking that a better fix is to change the task.action to reflect the 'resolved' action, which for these would be the 'short name'
19:51:54 <sdoran> 😁
19:52:15 <sdoran> Anywho, no trying to be a total downer. I just wanted to air my grievances.
19:52:17 <bcoca> sdoran: collections was not part of initial design
19:52:22 <nitzmahone> @bcoca: I tried that once, it has HUGE implications
19:52:31 <nitzmahone> It breaks ALL OVER THE PLACE
19:52:34 <bcoca> nitzmahone: at what scope?
19:52:39 <nitzmahone> everywhere
19:52:41 <sdoran> Given the horse is now long in the distance, this is something we should fix.
19:52:55 <bcoca> nitzmahone: i mean, where did you apply?
19:53:05 <nitzmahone> during task resolution
19:53:06 <felixfontein> also it would allow collection action plugins to get the same behavior as the builtin actions of the same name, like include_vars, set_fact, ...
19:53:25 <bcoca> nitzmahone: that would not work, since pre inspection would fail
19:53:34 <felixfontein> if it would simply be the short name without namespace, and not some special-case "only remove namespace if it is from ansible.bulitin."
19:53:52 <bcoca> felixfontein: that is alreayd the case if they conflict with short name for action plugin
19:54:00 <nitzmahone> I tried in several places; too many things "not ours" use task.action for us to change the semantics at this point
19:54:15 <bcoca> nitzmahone: :-(
19:54:24 <bcoca> then i dont see other way than this 'hardcoded list'
19:54:25 <felixfontein> bcoca: yeah, that could well be
19:54:41 <bcoca> felixfontein: not could, we have open issue about it
19:54:52 <felixfontein> bcoca: I remember modifying `self._task.action` in an action plugin :)
19:55:31 <nitzmahone> What we need to do is a more comprehensive look at the metadata we need on `task` around what it actually is; we've been adding things as they're needed, but there are several pretty clear needs now
19:56:03 <nitzmahone> felixfontein: we're going to start clamping down on that sort of thing pretty soon with runtime warnings (and eventually errors) :D
19:56:24 <bcoca> nitzmahone:  http://paste.debian.net/1168027/
19:56:32 <felixfontein> nitzmahone: don't worry (for my case), I removed it again
19:56:57 <bcoca> ^ wrong file
19:57:49 <felixfontein> ok, so about #71824: what exactly should I change?
19:57:51 <bcoca> http://paste.debian.net/1168028/ <= @nitzmahone
19:57:54 <nitzmahone> If the task had the right metadata from the parse/resolution phase available, we could just change the conditional to use it instead of hacking the source lists.
19:58:31 <bcoca> felixfontein: i would avoid moving all in file constants, consolidate where you can, avoid calling add_builtin_fqcn always .. many of the paths are rerely hit
19:58:38 <nitzmahone> I think that also gets us closer to the place we want to be for future changes as well
19:59:23 * nitzmahone ducks over to start WWG
20:00:19 <jborean93> felix: do you know what the next steps are, I had to step away for a few minutes sorry
20:00:54 <bcoca> k, so 'real' fix is stop dealing with action names directly soo much ...
20:01:12 <felixfontein> bcoca: should I try to make them class constants instead? I'm not sure which ones really are rarely called
20:01:24 <felixfontein> jborean93: not really
20:01:25 <jborean93> but can we use anything in 71824 or does it require some more work
20:01:39 <bcoca> also you repeat a lot of work, example included_file, you can define teh 1st constant using the 2nd
20:02:19 <nitzmahone> Maybe just open a bug and tie it as a 2.11 "must do", because we'll be adding the task metadata... Backport-wise, something like this might be better if we want it to work, but I think it's not the right solution going forward.
20:02:32 <felixfontein> bcoca: that would't really make it much shorter
20:02:54 <bcoca> felixfontein, nitzmahone ... fun thing to figure out ... ansible.legacy.meta ?
20:03:16 <bcoca> felixfontein: it would in central location since you basically define the same constant in 5 diff files
20:03:40 <bcoca> was just example, this could be 7 constants in single file instead of 30 of em in 20 files
20:03:41 <felixfontein> what's ansible.legacy.?
20:03:47 <jborean93> older behaviour
20:03:50 <bcoca> felixfontein: ansible.buildin + overrides
20:03:54 <bcoca> aka library/
20:03:57 <jborean93> so instead of `ansible.builtin` it allows the shortname override
20:04:16 <bcoca> xcept these actions never allowed override ...
20:04:25 <felixfontein> so ansible.legacy. also needs to be added by add_builtin_fqcn()?
20:04:26 <nitzmahone> which is why they shouldn't be ansible.legacy
20:04:45 <bcoca> nitzmahone: yet that is the other namespace we touted that 'everything works'
20:04:49 <nitzmahone> But that's also why we need a metadata-backed approach rather than a string comparison approach
20:05:14 <nitzmahone> I meant internally we shouldn't be using the `ansible.legacy` resolution behavior
20:05:23 <bcoca> nitzmahone: dont disagree there, been saying that since before collections were named ansible playbook bundles
20:05:30 <nitzmahone> (at least for the existing reserved shortnames)
20:05:36 * bcoca misses apb acronym
20:06:55 <felixfontein> ok. so: also add `ansible.legacy.` in add_builtin_fqcn(), or not?
20:07:11 <jborean93> cool, so to me it sounds like we need to fix this but to do it properly we need to implement some metadata and avoid doing the string checks that are currently in place
20:07:21 <felixfontein> or phrased differently: should `ansible.legacy.set_fact` work as `set_fact` or `ansible.builtin.set_fact`?
20:07:46 <bcoca> jborean93: but for things to work before 2024 .. i do think we need to accep thtis PR or variant of it
20:07:59 <bcoca> felixfontein: all 3 should work the same
20:08:03 <cyberpear> is something special about 2024?
20:08:12 <felixfontein> bcoca: ok, so `ansible.legacy.` needs to be added, too.
20:08:14 <bcoca> cyberpear: u right 2034
20:08:15 <jborean93> bcoca is just being optimistic :)
20:08:31 <bcoca> jborean93: yes, yes i was
20:08:46 <felixfontein> basic rule of project management: if you think it takes N time units, add 4 years :P
20:08:48 <jborean93> to me, I'm not objecting to getting it fixed for now, it sounds like bcoca is onto the review and getting things moving
20:09:30 <relrod> bcoca: my Haskell rewrite will be well under way by then. That will solve everything. ;)
20:09:31 * relrod hides
20:09:55 <bcoca> relrod: then we'll have to introduce all these problems for backwards compatibility
20:10:16 * sdoran throws something soft and non-lethal at relrod
20:10:19 <bcoca> ^ i kid, but in M$ that was actual mandate to OWA team so it would not work better than outlook
20:11:40 <felixfontein> bcoca: about the file-level constants: should I make them class constants (when applicable)?
20:12:32 <bcoca> felixfontein: no, thinking we should move to single file, tempted to say constants.py since most of these use it, but that is huge file to include for those that dont
20:12:50 <bcoca> lib/ansible/task_constants.py ?
20:13:02 <bcoca> torn on that
20:13:20 <bcoca> might as well bite bullet and put all  in constants.py
20:13:26 <felixfontein> or lib/ansible/_task_constants.py so that nobody else starts depending on it?
20:14:07 <bcoca> he, like the _ has been respected as private ...
20:14:22 <felixfontein> bcoca: at least it is a warning "if you use this, your code really might break"
20:14:38 <bcoca> yeah, they complain anyways ... but im just jaded on that topic
20:14:59 <bcoca> also we made many 'needed plublic things' use the _ prefix when we should not have to
20:15:10 <bcoca> /me points at _text.py
20:15:13 <felixfontein> so who wants to decide this? I don't want to move everything more than once :)
20:15:30 <felixfontein> yeah, _text is ... funny
20:16:01 <sdoran> We moved those to a public and more appropriate place. But that was long overdue.
20:16:06 <jborean93> well that is behind a proper public place now
20:16:24 <sdoran> I think `constants.py` is where constants should go.
20:16:53 <bcoca> only took a few years, was just glaring example on us 'over privitizing' and people having a justification to ignore other files/methods marked private
20:16:57 <jborean93> I'm +0 on the location, I don't know whether the size of constants.py is really a concern
20:17:26 <bcoca> jborean93: consider the size at runtime once it 'consumes' base.yml
20:17:52 <jborean93> yea I'm sure it's large, I just don't know whether it's actually going to affect import times or not
20:17:53 <bcoca> current size of constants in file is tiny, my worry is when we tack rest of config  and plugin config on top
20:17:53 <nitzmahone> felixfontein: depends on if we want to backport
20:18:18 <felixfontein> nitzmahone: I think it should be backported to 2.9, since this also affects them
20:18:23 <bcoca> jborean93: depends on path, most cases its loade don cli .. which should be fine
20:18:29 <felixfontein> (2.9 users I mean)
20:18:37 <bcoca> felixfontein: backport to 2.10 first ...
20:18:44 <felixfontein> bcoca: definitely!
20:19:17 <felixfontein> so lib/ansible/constants.py?
20:19:36 <felixfontein> I would keep the _ prefix for the constants if they end up in there
20:19:43 <bcoca> yeah ... dont think there is good option here and that is 'known bad' vs 'unkown bad'
20:20:03 <jborean93> I would say constants.py for now as we would be forever stuck bikeshedding it all
20:20:04 <bcoca> he, feel free, but also check, i think there are already dupes in there
20:20:12 <bcoca> jborean93: yep
20:21:00 * relrod tosses out `constants/_task.py`, but would require moving current constants to __init__.py to still work I think.
20:21:04 <felixfontein> should I keep add_builtin_fqcn in add_builtin_fqcn?
20:21:27 <bcoca> add_internal_fqcns ?
20:21:31 <felixfontein> relrod: I think so too
20:21:33 <bcoca> ^ pure bikeshed
20:21:49 <felixfontein> should I keep add_builtin_fqcn in ansible.utils.fqcn?
20:21:53 <felixfontein> <-- that's what I wanted to write
20:22:00 <bcoca> works4me
20:22:11 <felixfontein> I can rename to add_internal_fqcns, since there are now two prefixes
20:24:28 <jborean93> cool, any last questions for that PR?
20:24:33 <jborean93> We have gone over the time a bit
20:24:36 <bcoca> add_this_cause_we_didnt_fix_the_underlying_issues_before_adding_feautres_on_top
20:24:45 <sdoran> +!
20:24:53 <bcoca> /end sarcasm
20:24:57 <sdoran> 😜
20:26:02 <agaffney> bcoca: I don't think you implement that command
20:26:10 <felixfontein> I'll move all constants to lib/ansible/constants.py now
20:26:17 <felixfontein> (resp. now = when I have time :) )
20:26:17 <bcoca> agaffney: its my epitaph
20:26:28 <felixfontein> and rename that function from add_builtin_fqcn to add_internal_fqcns
20:27:14 <jborean93> cool
20:27:37 <jborean93> #action felixfontein to move constants to central location and rename add_fqdn function for 71824
20:27:38 <felixfontein> I hope that's enough to get it merged soon, would be really nice if it could get included in the next ansible-base 2.10.x release
20:28:01 <nitzmahone> Probably not the next one, but the one after that
20:28:21 <jborean93> there's one more item on the list but we are approaching 30 minutes over. UNless you really wish to discuss it quickly I'm going to call it a day
20:28:24 <nitzmahone> Backports have to be merged by Friday EOD
20:28:43 <nitzmahone> +1 to done, some of us are already double-meeting ;)
20:29:03 <nitzmahone> This isn't one I want to rush review on
20:29:19 <felixfontein> jborean93: no, let's discuss it next week
20:29:26 <felixfontein> (or thursday, no idea whether I'm around though)
20:29:32 <jborean93> awesome sauce
20:29:54 <jborean93> have a good one all
20:30:14 <jborean93> #endmeeting