Bug 53824 - Automatic "name" variable value for Listeners
Automatic "name" variable value for Listeners
Status: NEW
Product: UCS
Classification: Unclassified
Component: Listener (univention-directory-listener)
UCS 5.0
All Linux
: P5 enhancement (vote)
: ---
Assigned To: UCS maintainers
UCS maintainers
:
Depends on: 48357
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-22 17:32 CEST by Esteban
Modified: 2021-09-23 16:41 CEST (History)
4 users (show)

See Also:
What kind of report is it?: Development Internal
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): API change, Cleanup
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Esteban univentionstaff 2021-09-22 17:32:28 CEST
Talking about this: https://docs.software-univention.de/developer-reference-5.0.html#listener:handler
If the name of a new listener module is mandatory and it always must be the name of the file without ".py" it would be suggested in the documentation to be set to something like
`name = __file__.split('.')[0]`
or

`name = Path(__file__).stem`
Comment 1 Florian Best univentionstaff 2021-09-22 17:36:24 CEST
I would vote for removing this completely.
I think we can use module.__name__ instead.
Comment 2 Esteban univentionstaff 2021-09-22 17:44:49 CEST
(In reply to Florian Best from comment #1)
> I would vote for removing this completely.
> I think we can use module.__name__ instead.

I agree with you. Sounds like a better option at the design level.

But, wouldn't that force us to change it at every point where a module name is used? 
Otherwise whoever loads and uses the module and expects the "name" variable to exist would fail because it is not defined, right?
Comment 3 Florian Best univentionstaff 2021-09-22 17:47:19 CEST
I think nobody uses the name property. And if we need backwards compatibility, we can simply set it in the UDL via something like `module.__dict__.setdefault('name', module.__name__)`.
Comment 4 Esteban univentionstaff 2021-09-22 17:49:33 CEST
(In reply to Florian Best from comment #3)
> I think nobody uses the name property. And if we need backwards
> compatibility, we can simply set it in the UDL via something like
> `module.__dict__.setdefault('name', module.__name__)`.

Sounds great to me.
Comment 5 Daniel Tröder univentionstaff 2021-09-23 08:42:40 CEST
What's the target of this bug? I cannot find a problem description anywhere.
Comment 6 Esteban univentionstaff 2021-09-23 09:00:11 CEST
Hello Daniel, I'm new to this.
It's just an enhancement suggestion, a cleanup.
If the name of the Listener modules is not used in other parts of the code or needs to always be the name of the file it can be automatically generated or removed.
Comment 7 Philipp Hahn univentionstaff 2021-09-23 09:03:15 CEST
(In reply to Daniel Tröder from comment #5)
> What's the target of this bug? I cannot find a problem description anywhere.

Each UDL module currently must have the required attribute "name". It's unclear why it must be given *explicitly* instead of being derived *implicitly* from the module file name.

[ ] research why it must be given explicitly and document this
[ ] Feature-Request to change the API to make "name" obsolete or optional

PS: Having "name" <> "$(basename $0)" leads to all kind of strange issues as some code prefers "name" and other code prefers "$0". This complicates debugging and confuses people. Therefore it should be simplified. QED
Comment 8 Sönke Schwardt-Krummrich univentionstaff 2021-09-23 09:22:30 CEST
(In reply to Florian Best from comment #3)
> I think nobody uses the name property. And if we need backwards
> compatibility, we can simply set it in the UDL via something like
> `module.__dict__.setdefault('name', module.__name__)`.

The name property is sometimes used in debug output:
→ ud.debug(ud.LISTENER, ud.ERROR, "%s: something is wrong" % (name,))
Comment 9 Daniel Tröder univentionstaff 2021-09-23 12:30:23 CEST
The new listener API (package univention.listener) already does create the module level attribute "name" programatically. If you wish to make it optional, just edit univention.listener.handler_configuration.ListenerModuleConfiguration.get_name() to calculate it in case Config.name is unset.

BUT: there is already a problem with "univention-directory-listener-ctrl status".

Listeners created with the new listener API are always in status 0 / red because univention-directory-listener-ctrl is a shell script that parses the Python file and looks for a "name" entry:

module_name=$(sed -rne "s/^name\s*=\s*['\"]([^'\"]+)['\"]\s*(#.*)?$/\1/p;T;q" "$module_file") || rv="$FAIL"

So if the word "name" is not in the Python module, you'll have the same problem.
Comment 10 Philipp Hahn univentionstaff 2021-09-23 16:41:09 CEST
(In reply to Daniel Tröder from comment #9)
> The new listener API (package univention.listener) already does create the
> module level attribute "name" programatically. If you wish to make it
> optional, just edit
> univention.listener.handler_configuration.ListenerModuleConfiguration.
> get_name() to calculate it in case Config.name is unset.
> 
> BUT: there is already a problem with "univention-directory-listener-ctrl
> status".

That's exactly the kind of problems I was talking about: We must change UDL and "u-d-l-ctrl" first, so that "name" can become optional.
I see no reason to keep that variable "name".

It was Bug #48357